[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D134813#3834776 <https://reviews.llvm.org/D134813#3834776>, @aaron.ballman wrote:

> Thank you for this! I applied D135191 <https://reviews.llvm.org/D135191> over the top of my changes here and ran the tests to get all the new failures with the changes, then I reverted those tests which failed back to their trunk form. When I re-ran the tests, I get the following failures:
>
>   ******************** TEST 'Clang :: Index/usrs.m' FAILED ********************
>   ...
>   $ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\filecheck.exe" "-check-prefix=CHECK-source" "F:\source\llvm-project\clang\test\Index\usrs.m"
>   # command stderr:
>   F:\source\llvm-project\clang\test\Index\usrs.m:188:18: error: CHECK-source: expected string not found in input
>   // CHECK-source: usrs.m:5:1: EnumDecl=:5:1 (Definition) Extent=[5:1 - 8:2]
>                    ^
>   <stdin>:386:63: note: scanning from here
>   // CHECK: usrs.m:3:56: DeclRefExpr=y:3:40 Extent=[3:56 - 3:57]
>                                                                 ^
>   <stdin>:387:89: note: possible intended match here
>   // CHECK: usrs.m:5:1: EnumDecl=enum (unnamed at F:\source\llvm-project\clang\test\Index\usrs.m:5:1):5:1 (Definition) Extent=[5:1 - 8:2]

This isn't a change to USRs, it's a change to libclang's `clang_getCursorSpelling`. I think it's safe to update (unlike the USRs earlier in the file).
Since this is a general-purpose API it makes sense that it would follow the change in printName() defaults.

>   ******************** TEST 'Clang :: ExtractAPI/enum.c' FAILED ********************
>   # command output:
>   *** F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/reference.output.json
>   --- F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/output-normalized.json
>   ***************
>   *** 655,667 ****
>             "navigator": [
>               {
>                 "kind": "identifier",
>   !             "spelling": "(anonymous)"
>   !           }
>   !         ],
>   !         "title": "(anonymous)"
>   !       },
>   !       "pathComponents": [
>   !         "(anonymous)"
>           ]
>         },
>         {
>   --- 655,667 ----
>             "navigator": [
>               {
>                 "kind": "identifier",
>   !             "spelling": "enum (unnamed at F:\\source\\llvm-project\\llvm\\out\\build\\x64-Debug\\tools\\clang\\test\\ExtractAPI\\Output\\enum.c.tmp/input.h:17:1)"
>   !           }
>   !         ],
>   !         "title": "enum (unnamed at F:\\source\\llvm-project\\llvm\\out\\build\\x64-Debug\\tools\\clang\\test\\ExtractAPI\\Output\\enum.c.tmp/input.h:17:1)"
>   !       },
>   !       "pathComponents": [
>   !         "enum (unnamed at F:\\source\\llvm-project\\llvm\\out\\build\\x64-Debug\\tools\\clang\\test\\ExtractAPI\\Output\\enum.c.tmp/input.h:17:1)"
>           ]
>         },
>   ...

ExtractAPI seems to produce a data description of the AST for programmatic consumption (apple swift interop?), this looks like a breaking change to me.
It looks like they have at least one explicit check similar to USRs in clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:361, but I know very little about how this tool is used out-of-tree by Apple: I'm not sure how much the exact strings/lack of strings matters, may need owners of that tool involved here.

>   ******************** TEST 'Clang :: Index/annotate-comments-typedef.m' FAILED ********************

I can't see the actual output of the tool in that log, but I guess this is related to clang/lib/Index/CommentToXML.cpp:908.
I don't know if reporting exactly `<anonymous>` is important behavior that should be preserved, or `(unnamed)` would be fine. I don't even know what CommentToXML is: again I guess it's consumed by something related to XCode out-of-tree.

> and also the clangd unit tests failed:
>
>   [----------] 1 test from FindExplicitReferencesTest
>   [ RUN      ] FindExplicitReferencesTest.All
>   ...
>   With diff:
>   @@ -1,3 +1,3 @@
>   -0: targets = {}
>   +0: targets = {(unnamed)}
>    1: targets = {x}, decl
>    2: targets = {fptr}, decl
>   
>   
>                void foo() {
>                 $0^class {} $1^x;
>                 int (*$2^fptr)(int $3^a, int) = nullptr;
>                }

  Yes, this is expected: the test identifies which target is referenced by name, and we changed the name from "" to "(unnamed)".
  This patch can change the test data on clang-tools-extra/clangd/unittests/FindTargetTests.cpp:1619 from `targets = {}` to `targets={(unnamed)}`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134813/new/

https://reviews.llvm.org/D134813