This is an archive of the discontinued LLVM Phabricator instance.

Properly print unnamed TagDecl objects in diagnostics
ClosedPublic

Authored by aaron.ballman on Sep 28 2022, 9:21 AM.

Details

Summary

The diagnostics engine is very smart about being passed a NamedDecl to print as part of a diagnostic; it gets the "right" form of the name, quotes it properly, etc. However, the result of using an unnamed tag declaration was to print '' instead of anything useful.

This patch causes us to print the same information we'd have gotten if we had printed the type of the declaration rather than the name of it, as that's the most relevant information we can display.

Diff Detail

Event Timeline

aaron.ballman created this revision.Sep 28 2022, 9:21 AM
Herald added a reviewer: ributzka. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
aaron.ballman requested review of this revision.Sep 28 2022, 9:21 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 28 2022, 9:21 AM
aaron.ballman added inline comments.Sep 28 2022, 9:28 AM
clang/test/ExtractAPI/enum.c
3

This test's use of diff makes the diagnostic changes rather challenging. We can't diff the file path that's included when printing an unnamed object, because that path may be different from machine to machine. diff doesn't have any wildcard matching functionality to help here either. So we use sed to mutate the test file.. but the output uses the path of %t with escapes and we don't have a sed-compatible way to do that on Windows.

e.g., the output we want to match will contain F:\\users\\aballman\\desktop\\test.c on Windows but %t gives F:\users\aballman\desktop\test.c which sed turns into F:sers ballman esktop est.c or some other garbled form.

I'm hoping someone has a better way to approach this. I added %/et (for escaped %t) to get the behavior I needed, but I'd prefer not to modify lit for this.

erichkeane accepted this revision.Sep 28 2022, 9:45 AM

LGTM!

clang/lib/AST/Decl.cpp
4481

Do we want to assert that this is either an EnumDecl, or a RecordDecl AND RD.isAnonymousStructOrUnion?

This revision is now accepted and ready to land.Sep 28 2022, 9:45 AM

The clangd test failure found by precommit CI is stumping me as to how to resolve it. @sammccall -- can you help me out? The issue is that the test is expecting no name but now we're printing more information, but that information includes a file path which may differ between machines.

The clangd test failure found by precommit CI is stumping me as to how to resolve it. @sammccall -- can you help me out? The issue is that the test is expecting no name but now we're printing more information, but that information includes a file path which may differ between machines.

Ping @sammccall. FWIW, I found that the failing part of this test was change somewhat recently in https://github.com/llvm/llvm-project/commit/15f3cd6bfc670ba6106184a903eb04be059e5977 to add the markings to print the name of this class, so also pinging @mizvekov to see if this test coverage is necessary (another way to solve the issue is to stop trying to read the name of the unnamed class in the unit test).

Sorry, Monday was a holiday here...

I don't think unconditionally including the filename in printQualifiedName() is great for tools, it can be unreasonably long and is generally just noise when shown in the context of that file. I'm surprised that USR generation + that clangd test are the only things that broke! Poor test coverage in the tools, I think :-(
If the intent is to change this for diagnostics only, can it be behind a PrintingPolicy flag?

The reason USR generation broke is it was deliberately testing/handling, see bool USRGenerator::EmitDeclName() in USRGeneration.cpp. (I'm not very familiar with the implementation, a bit more so with how its results are used). This isn't a good reason to keep the output as "" forever - we should probably change it to detect empty names explicitly instead.

clang/test/Index/usrs.m
121

This changes the semantics of the USRs: previously if an anonymous enum moved (e.g. because a file was edited) it retains its identity as long as the first enumerator keeps its name.

The equivalence classes of decls under USR mapping is important for some tools: I don't know all the ways Apple/XCode stuff uses it, but refactoring tools, clangd indexing etc will be affected by this.

(Occasional changes to the concrete USR values are manageable, basically we need to invalidate indexes that are otherwise usable across versions)

Sorry, Monday was a holiday here...

No worries, I hope you had a good holiday!

I don't think unconditionally including the filename in printQualifiedName() is great for tools, it can be unreasonably long and is generally just noise when shown in the context of that file. I'm surprised that USR generation + that clangd test are the only things that broke! Poor test coverage in the tools, I think :-(
If the intent is to change this for diagnostics only, can it be behind a PrintingPolicy flag?

Diagnostics are largely the intent for this and I could probably put a printing policy flag, but I'm not yet convinced that printing nothing is actually better for tools in general. I think it really boils down to whether the name is user-facing or not. e.g., for USRs, it seems like we don't want to print this sort of thing, which is fine because users don't stare at those. But tools like clang-query output various names of things based on user queries, and it seems like it's less useful for that output to print nothing for the name.

That said, it still sounds like we should have a printing policy for it, but what should the default be? (I lean towards "print more information instead of less".)

The reason USR generation broke is it was deliberately testing/handling, see bool USRGenerator::EmitDeclName() in USRGeneration.cpp. (I'm not very familiar with the implementation, a bit more so with how its results are used). This isn't a good reason to keep the output as "" forever - we should probably change it to detect empty names explicitly instead.

Ah, thank you for the details!

clang/test/Index/usrs.m
121

Thank you for the extra set of eyes on this test, that's really good information!

Sorry, Monday was a holiday here...

No worries, I hope you had a good holiday!

Thanks :-)

I don't think unconditionally including the filename in printQualifiedName() is great for tools, it can be unreasonably long and is generally just noise when shown in the context of that file. I'm surprised that USR generation + that clangd test are the only things that broke! Poor test coverage in the tools, I think :-(
If the intent is to change this for diagnostics only, can it be behind a PrintingPolicy flag?

Diagnostics are largely the intent for this and I could probably put a printing policy flag, but I'm not yet convinced that printing nothing is actually better for tools in general.

Agreed - I think in my perfect world we'd switch between (unnammed struct) and (unnamed struct at ...).
... wait, we already have PrintingPolicy::AnonymousTagLocations, looks like that does what I want. I've set this for clangd in e212a4f838f17e2d37b1d572d8c1b49c50d7fe17, so this patch should be fine with just updating the string in the test to (unnamed class).

I think it really boils down to whether the name is user-facing or not. e.g., for USRs, it seems like we don't want to print this sort of thing, which is fine because users don't stare at those. But tools like clang-query output various names of things based on user queries, and it seems like it's less useful for that output to print nothing for the name.

Yeah, I can't see a case where silently printing *nothing* is clearly what we want.
Even USRs mostly don't do that: they try to print the name, detect nothing got printed, and print something else instead!

That said, it still sounds like we should have a printing policy for it, but what should the default be? (I lean towards "print more information instead of less".)

Given that we have a way to suppress the filename, I don't think we should add another printing policy (until we see a reason to).
Changing USR generation to not rely on this detail seems easier, I can take a stab at this.

Sorry, Monday was a holiday here...

No worries, I hope you had a good holiday!

Thanks :-)

I don't think unconditionally including the filename in printQualifiedName() is great for tools, it can be unreasonably long and is generally just noise when shown in the context of that file. I'm surprised that USR generation + that clangd test are the only things that broke! Poor test coverage in the tools, I think :-(
If the intent is to change this for diagnostics only, can it be behind a PrintingPolicy flag?

Diagnostics are largely the intent for this and I could probably put a printing policy flag, but I'm not yet convinced that printing nothing is actually better for tools in general.

Agreed - I think in my perfect world we'd switch between (unnammed struct) and (unnamed struct at ...).
... wait, we already have PrintingPolicy::AnonymousTagLocations, looks like that does what I want. I've set this for clangd in e212a4f838f17e2d37b1d572d8c1b49c50d7fe17, so this patch should be fine with just updating the string in the test to (unnamed class).

Hehe, I was getting a start on your idea when I discovered that as well. :-D

I think it really boils down to whether the name is user-facing or not. e.g., for USRs, it seems like we don't want to print this sort of thing, which is fine because users don't stare at those. But tools like clang-query output various names of things based on user queries, and it seems like it's less useful for that output to print nothing for the name.

Yeah, I can't see a case where silently printing *nothing* is clearly what we want.
Even USRs mostly don't do that: they try to print the name, detect nothing got printed, and print something else instead!

That said, it still sounds like we should have a printing policy for it, but what should the default be? (I lean towards "print more information instead of less".)

Given that we have a way to suppress the filename, I don't think we should add another printing policy (until we see a reason to).
Changing USR generation to not rely on this detail seems easier, I can take a stab at this.

Thank you, I think that's a good idea. I'll rebase on top of those changes.

Changing USR generation to not rely on this detail seems easier, I can take a stab at this.

Seems trivial: https://reviews.llvm.org/D135191
If there are still diffs in USR tests after rebasing on that, I'm happy to take a look at them.

Changing USR generation to not rely on this detail seems easier, I can take a stab at this.

Seems trivial: https://reviews.llvm.org/D135191
If there are still diffs in USR tests after rebasing on that, I'm happy to take a look at them.

Thank you for this! I applied 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:

F:\source\llvm-project>python llvm\out\build\x64-Debug\bin\llvm-lit.py -sv -j61 clang\test\
llvm-lit.py: F:\source\llvm-project\llvm\utils\lit\lit\llvm\config.py:46: note: using lit tools: C:\GnuWin32\bin
llvm-lit.py: F:\source\llvm-project\llvm\utils\lit\lit\llvm\config.py:456: note: using clang: f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe
llvm-lit.py: F:\source\llvm-project\llvm\utils\lit\lit\discovery.py:247: warning: test suite 'Clang-Unit' contained no tests
-- Testing: 15539 tests, 61 workers --
Testing:
FAIL: Clang :: Index/usrs.m (1 of 15539)
******************** TEST 'Clang :: Index/usrs.m' FAILED ********************
Script:
--
: 'RUN: at line 103';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\c-index-test.exe -test-load-source-usrs all -target x86_64-apple-macosx10.7 F:\source\llvm-project\clang\test\Index\usrs.m -isystem F:\source\llvm-project\clang\test\Index/Inputs | f:\source\llvm-project\llvm\out\build\x64-debug\bin\filecheck.exe F:\source\llvm-project\clang\test\Index\usrs.m
: 'RUN: at line 173';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\c-index-test.exe -test-load-source all F:\source\llvm-project\clang\test\Index\usrs.m -isystem F:\source\llvm-project\clang\test\Index/Inputs | 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
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 103"
$ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\c-index-test.exe" "-test-load-source-usrs" "all" "-target" "x86_64-apple-macosx10.7" "F:\source\llvm-project\clang\test\Index\usrs.m" "-isystem" "F:\source\llvm-project\clang\test\Index/Inputs"
# command stderr:
F:\source\llvm-project\clang\test\Index\usrs.m:25:12: warning: class 'Foo' defined without specifying a base class [-Wobjc-root-class]
Number FIX-ITs = 0
F:\source\llvm-project\clang\test\Index\usrs.m:25:15: note: add a super class to fix this problem
Number FIX-ITs = 0
F:\source\llvm-project\clang\test\Index\usrs.m:51:12: warning: class 'CWithExt' defined without specifying a base class [-Wobjc-root-class]
Number FIX-ITs = 0
F:\source\llvm-project\clang\test\Index\usrs.m:51:20: note: add a super class to fix this problem
Number FIX-ITs = 0
F:\source\llvm-project\clang\test\Index\usrs.m:101:9: warning: 'MACRO3' macro redefined [-Wmacro-redefined]
Number FIX-ITs = 0
F:\source\llvm-project\clang\test\Index\usrs.m:100:9: note: previous definition is here
Number FIX-ITs = 0

$ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\filecheck.exe" "F:\source\llvm-project\clang\test\Index\usrs.m"
$ ":" "RUN: at line 173"
$ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\c-index-test.exe" "-test-load-source" "all" "F:\source\llvm-project\clang\test\Index\usrs.m" "-isystem" "F:\source\llvm-project\clang\test\Index/Inputs"
# command stderr:
F:\source\llvm-project\clang\test\Index\usrs.m:44:13: error: synthesized property 'd1' must either be named the same as a compatible instance variable or must explicitly name an instance variable
F:\source\llvm-project\clang\test\Index\usrs.m:25:12: warning: class 'Foo' defined without specifying a base class [-Wobjc-root-class]
F:\source\llvm-project\clang\test\Index\usrs.m:25:15: note: add a super class to fix this problem
F:\source\llvm-project\clang\test\Index\usrs.m:51:12: warning: class 'CWithExt' defined without specifying a base class [-Wobjc-root-class]
F:\source\llvm-project\clang\test\Index\usrs.m:51:20: note: add a super class to fix this problem
F:\source\llvm-project\clang\test\Index\usrs.m:86:6: error: instance variables may not be placed in class extension
F:\source\llvm-project\clang\test\Index\usrs.m:101:9: warning: 'MACRO3' macro redefined [-Wmacro-redefined]
F:\source\llvm-project\clang\test\Index\usrs.m:100:9: note: previous definition is here
F:\source\llvm-project\clang\test\Index\usrs.m:44:13: error: synthesized property 'd1' must either be named the same as a compatible instance variable or must explicitly name an instance variable
Number FIX-ITs = 0
F:\source\llvm-project\clang\test\Index\usrs.m:25:12: warning: class 'Foo' defined without specifying a base class [-Wobjc-root-class]
Number FIX-ITs = 0
F:\source\llvm-project\clang\test\Index\usrs.m:25:15: note: add a super class to fix this problem
Number FIX-ITs = 0
F:\source\llvm-project\clang\test\Index\usrs.m:51:12: warning: class 'CWithExt' defined without specifying a base class [-Wobjc-root-class]
Number FIX-ITs = 0
F:\source\llvm-project\clang\test\Index\usrs.m:51:20: note: add a super class to fix this problem
Number FIX-ITs = 0
F:\source\llvm-project\clang\test\Index\usrs.m:86:6: error: instance variables may not be placed in class extension
Number FIX-ITs = 0
F:\source\llvm-project\clang\test\Index\usrs.m:101:9: warning: 'MACRO3' macro redefined [-Wmacro-redefined]
Number FIX-ITs = 0
F:\source\llvm-project\clang\test\Index\usrs.m:100:9: note: previous definition is here
Number FIX-ITs = 0

$ "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]
                                                                                        ^

Input file: <stdin>
Check file: F:\source\llvm-project\clang\test\Index\usrs.m

-dump-input=help explains the following input dump.

Input was:
<<<<<<
             .
             .
             .
           381: // CHECK: usrs.m:3:45: ReturnStmt= Extent=[3:45 - 3:57]
           382: // CHECK: usrs.m:3:52: BinaryOperator= Extent=[3:52 - 3:57]
           383: // CHECK: usrs.m:3:52: UnexposedExpr=x:3:33 Extent=[3:52 - 3:53]
           384: // CHECK: usrs.m:3:52: DeclRefExpr=x:3:33 Extent=[3:52 - 3:53]
           385: // CHECK: usrs.m:3:56: UnexposedExpr=y:3:40 Extent=[3:56 - 3:57]
           386: // CHECK: usrs.m:3:56: DeclRefExpr=y:3:40 Extent=[3:56 - 3:57]
check:188'0                                                                   X error: no match found
           387: // 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]
check:188'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:188'1                                                                                             ?                                                possible intended match
           388: // CHECK: usrs.m:6:3: EnumConstantDecl=ABA:6:3 (Definition) Extent=[6:3 - 6:6]
check:188'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           389: // CHECK: usrs.m:7:3: EnumConstantDecl=CADABA:7:3 (Definition) Extent=[7:3 - 7:9]
check:188'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           390: // CHECK: usrs.m:10:1: EnumDecl=enum (unnamed at F:\source\llvm-project\clang\test\Index\usrs.m:10:1):10:1 (Definition) Extent=[10:1 - 13:2]
check:188'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           391: // CHECK: usrs.m:11:3: EnumConstantDecl=FOO:11:3 (Definition) Extent=[11:3 - 11:6]
check:188'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           392: // CHECK: usrs.m:12:3: EnumConstantDecl=BAR:12:3 (Definition) Extent=[12:3 - 12:6]
check:188'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             .
             .
             .
>>>>>>

error: command failed with exit status: 1

--

********************
Testing:
FAIL: Clang :: ExtractAPI/enum.c (3 of 15539)
******************** TEST 'Clang :: ExtractAPI/enum.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   rm -rf F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp
: 'RUN: at line 2';   split-file F:\source\llvm-project\clang\test\ExtractAPI\enum.c F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp
: 'RUN: at line 3';   sed -e "s@INPUT_DIR@F:/source/llvm-project/llvm/out/build/x64-Debug/tools/clang/test/ExtractAPI/Output/enum.c.tmp@g"  F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/reference.output.json.in >> F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/reference.output.json
: 'RUN: at line 5';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\16.0.0\include -nostdsysteminc -extract-api -triple arm64-apple-macosx    -x c-header F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/input.h -o F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/output.json -verify
: 'RUN: at line 9';   sed -e "s@\"generator\": \".*\"@\"generator\": \"?\"@g"  F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/output.json >> F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/output-normalized.json
: 'RUN: at line 11';   diff 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
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "rm" "-rf" "F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp"
$ ":" "RUN: at line 2"
$ "split-file" "F:\source\llvm-project\clang\test\ExtractAPI\enum.c" "F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp"
$ ":" "RUN: at line 3"
$ "sed" "-e" "s@INPUT_DIR@F:/source/llvm-project/llvm/out/build/x64-Debug/tools/clang/test/ExtractAPI/Output/enum.c.tmp@g" "F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/reference.output.json.in"
$ ":" "RUN: at line 5"
$ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe" "-cc1" "-internal-isystem" "f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\16.0.0\include" "-nostdsysteminc" "-extract-api" "-triple" "arm64-apple-macosx" "-x" "c-header" "F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/input.h" "-o" "F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/output.json" "-verify"
$ ":" "RUN: at line 9"
$ "sed" "-e" "s@"generator": ".*"@"generator": "?"@g" "F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/output.json"
$ ":" "RUN: at line 11"
$ "diff" "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"
# 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)"
        ]
      },
      {
***************
*** 703,709 ****
          "title": "Constant"
        },
        "pathComponents": [
!         "(anonymous)",
          "Constant"
        ]
      },
--- 703,709 ----
          "title": "Constant"
        },
        "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)",
          "Constant"
        ]
      },
***************
*** 743,755 ****
          "navigator": [
            {
              "kind": "identifier",
!             "spelling": "(anonymous)"
!           }
!         ],
!         "title": "(anonymous)"
!       },
!       "pathComponents": [
!         "(anonymous)"
        ]
      },
      {
--- 743,755 ----
          "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:21:1)"
!           }
!         ],
!         "title": "enum (unnamed at F:\\source\\llvm-project\\llvm\\out\\build\\x64-Debug\\tools\\clang\\test\\ExtractAPI\\Output\\enum.c.tmp/input.h:21:1)"
!       },
!       "pathComponents": [
!         "enum (unnamed at F:\\source\\llvm-project\\llvm\\out\\build\\x64-Debug\\tools\\clang\\test\\ExtractAPI\\Output\\enum.c.tmp/input.h:21:1)"
        ]
      },
      {
***************
*** 791,797 ****
          "title": "OtherConstant"
        },
        "pathComponents": [
!         "(anonymous)",
          "OtherConstant"
        ]
      }
--- 791,797 ----
          "title": "OtherConstant"
        },
        "pathComponents": [
!         "enum (unnamed at F:\\source\\llvm-project\\llvm\\out\\build\\x64-Debug\\tools\\clang\\test\\ExtractAPI\\Output\\enum.c.tmp/input.h:21:1)",
          "OtherConstant"
        ]
      }

error: command failed with exit status: 1

--

********************
Testing:
FAIL: Clang :: Index/annotate-comments-typedef.m (4 of 15539)
******************** TEST 'Clang :: Index/annotate-comments-typedef.m' FAILED ********************
Script:
--
: 'RUN: at line 1';   rm -rf F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\Index\Output\annotate-comments-typedef.m.tmp
: 'RUN: at line 2';   mkdir F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\Index\Output\annotate-comments-typedef.m.tmp
: 'RUN: at line 3';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\c-index-test.exe -test-load-source all -comments-xml-schema=F:\source\llvm-project\clang\test\Index/../../bindings/xml/comment-xml-schema.rng F:\source\llvm-project\clang\test\Index\annotate-comments-typedef.m > F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\Index\Output\annotate-comments-typedef.m.tmp/out
: 'RUN: at line 4';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\filecheck.exe F:\source\llvm-project\clang\test\Index\annotate-comments-typedef.m < F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\Index\Output\annotate-comments-typedef.m.tmp/out
: 'RUN: at line 8';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\filecheck.exe F:\source\llvm-project\clang\test\Index\annotate-comments-typedef.m -check-prefix=WRONG < F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\Index\Output\annotate-comments-typedef.m.tmp/out
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "rm" "-rf" "F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\Index\Output\annotate-comments-typedef.m.tmp"
$ ":" "RUN: at line 2"
$ "mkdir" "F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\Index\Output\annotate-comments-typedef.m.tmp"
$ ":" "RUN: at line 3"
$ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\c-index-test.exe" "-test-load-source" "all" "-comments-xml-schema=F:\source\llvm-project\clang\test\Index/../../bindings/xml/comment-xml-schema.rng" "F:\source\llvm-project\clang\test\Index\annotate-comments-typedef.m"
$ ":" "RUN: at line 4"
$ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\filecheck.exe" "F:\source\llvm-project\clang\test\Index\annotate-comments-typedef.m"
# command stderr:
F:\source\llvm-project\clang\test\Index\annotate-comments-typedef.m:39:11: error: CHECK: expected string not found in input
// CHECK: StructDecl=:[[@LINE-4]]:9 (Definition) {{.*}} BriefComment=[Comment about Foo] FullCommentAsHTML=[<p class="para-brief"> Comment about Foo </p>] FullCommentAsXML=[<Class file="{{[^"]+}}annotate-comments-typedef.m" line="[[@LINE-4]]" column="9"><Name>&lt;anonymous&gt;</Name><USR>c:@SA@Foo</USR><Declaration>struct {}</Declaration><Abstract><Para> Comment about Foo </Para></Abstract></Class>]
          ^
<stdin>:452:521: note: scanning from here
// CHECK: annotate-comments-typedef.m:37:11: TypedefDecl=Foo:37:11 (Definition) RawComment=[/** Comment about Foo */] RawCommentRange=[34:1 - 34:25] BriefComment=[Comment about Foo] FullCommentAsHTML=[<p class="para-brief"> Comment about Foo </p>] FullCommentAsXML=[<Typedef file="F:\source\llvm-project\clang\test\Index\annotate-comments-typedef.m" line="37" column="11"><Name>Foo</Name><USR>c:@T@Foo</USR><Declaration>typedef struct Foo Foo</Declaration><Abstract><Para> Comment about Foo </Para></Abstract></Typedef>]
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                        ^
<stdin>:452:521: note: with "@LINE-4" equal to "35"
// CHECK: annotate-comments-typedef.m:37:11: TypedefDecl=Foo:37:11 (Definition) RawComment=[/** Comment about Foo */] RawCommentRange=[34:1 - 34:25] BriefComment=[Comment about Foo] FullCommentAsHTML=[<p class="para-brief"> Comment about Foo </p>] FullCommentAsXML=[<Typedef file="F:\source\llvm-project\clang\test\Index\annotate-comments-typedef.m" line="37" column="11"><Name>Foo</Name><USR>c:@T@Foo</USR><Declaration>typedef struct Foo Foo</Declaration><Abstract><Para> Comment about Foo </Para></Abstract></Typedef>]
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                        ^
<stdin>:452:521: note: with "@LINE-4" equal to "35"
// CHECK: annotate-comments-typedef.m:37:11: TypedefDecl=Foo:37:11 (Definition) RawComment=[/** Comment about Foo */] RawCommentRange=[34:1 - 34:25] BriefComment=[Comment about Foo] FullCommentAsHTML=[<p class="para-brief"> Comment about Foo </p>] FullCommentAsXML=[<Typedef file="F:\source\llvm-project\clang\test\Index\annotate-comments-typedef.m" line="37" column="11"><Name>Foo</Name><USR>c:@T@Foo</USR><Declaration>typedef struct Foo Foo</Declaration><Abstract><Para> Comment about Foo </Para></Abstract></Typedef>]
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                        ^

Input file: <stdin>
Check file: F:\source\llvm-project\clang\test\Index\annotate-comments-typedef.m

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            .
            .
            .
          447: // CHECK: CommentAST=[
          448: // CHECK: (CXComment_FullComment
          449: // CHECK: (CXComment_Paragraph
          450: // CHECK: (CXComment_Text Text=[ Comment about Foo ])))] Extent=[35:9 - 37:10]
          451: // CHECK: annotate-comments-typedef.m:36:14: FieldDecl=iii:36:14 (Definition) Extent=[36:10 - 36:17] [access=public]
          452: // CHECK: annotate-comments-typedef.m:37:11: TypedefDecl=Foo:37:11 (Definition) RawComment=[/** Comment about Foo */] RawCommentRange=[34:1 - 34:25] BriefComment=[Comment about Foo] FullCommentAsHTML=[<p class="para-brief"> Comment about Foo </p>] FullCommentAsXML=[<Typedef file="F:\source\llvm-project\clang\test\Index\annotate-comments-typedef.m" line="37" column="11"><Name>Foo</Name><USR>c:@T@Foo</USR><Declaration>typedef struct Foo Foo</Declaration><Abstract><Para> Comment about Foo </Para></Abstract></Typedef>]
check:39'0                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                             X error: no match found
check:39'1                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                               with "@LINE-4" equal to "35"
check:39'2                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                               with "@LINE-4" equal to "35"
          453: // CHECK: CommentAST=[
check:39'0     ~~~~~~~~~~~~~~~~~~~~~~~
          454: // CHECK: (CXComment_FullComment
check:39'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          455: // CHECK: (CXComment_Paragraph
check:39'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          456: // CHECK: (CXComment_Text Text=[ Comment about Foo ])))] Extent=[35:1 - 37:14]
check:39'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          457: // CHECK: annotate-comments-typedef.m:35:9: StructDecl=Foo:35:9 (Definition) RawComment=[/** Comment about Foo */] RawCommentRange=[34:1 - 34:25] BriefComment=[Comment about Foo] FullCommentAsHTML=[<p class="para-brief"> Comment about Foo </p>] FullCommentAsXML=[<Class file="F:\source\llvm-project\clang\test\Index\annotate-comments-typedef.m" line="35" column="9"><Name>&lt;anonymous&gt;</Name><USR>c:@SA@Foo</USR><Declaration>struct {}</Declaration><Abstract><Para> Comment about Foo </Para></Abstract></Class>]
check:39'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            .
            .
            .
>>>>>>

error: command failed with exit status: 1

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
********************
Failed Tests (3):
  Clang :: ExtractAPI/enum.c
  Clang :: Index/annotate-comments-typedef.m
  Clang :: Index/usrs.m


Testing Time: 379.02s
  Unsupported      :  1760
  Passed           : 13745
  Expectedly Failed:    31
  Failed           :     3

1 warning(s) in tests

and also the clangd unit tests failed:

[----------] 1 test from FindExplicitReferencesTest
[ RUN      ] FindExplicitReferencesTest.All
Built preamble of size 187020 for file C:\clangd-test\TestTU.cpp version null in 0.06 seconds
Built preamble of size 187020 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187020 for file C:\clangd-test\TestTU.cpp version null in 0.07 seconds
Built preamble of size 187020 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187020 for file C:\clangd-test\TestTU.cpp version null in 0.07 seconds
Built preamble of size 187020 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187020 for file C:\clangd-test\TestTU.cpp version null in 0.06 seconds
Built preamble of size 187020 for file C:\clangd-test\TestTU.cpp version null in 0.09 seconds
Built preamble of size 187020 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.06 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187212 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.06 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.07 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.07 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
F:\source\llvm-project\clang-tools-extra\clangd\unittests\FindTargetTests.cpp(1924): error: Expected equality of these values:
  ExpectedRefs
    Which is: "0: targets = {}\n1: targets = {x}, decl\n2: targets = {fptr}, decl\n3: targets = {a}, decl\n"
  Actual.DumpedReferences
    Which is: "0: targets = {(unnamed)}\n1: targets = {x}, decl\n2: targets = {fptr}, decl\n3: targets = {a}, decl\n"
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;
             }

Built preamble of size 187028 for file C:\clangd-test\TestTU.cpp version null in 0.09 seconds
Built preamble of size 187028 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187028 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.07 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.06 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.06 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.06 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.06 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.06 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.07 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.07 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.07 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.08 seconds
Built preamble of size 187024 for file C:\clangd-test\TestTU.cpp version null in 0.07 seconds
Built preamble of size 187020 for file C:\clangd-test\TestTU.cpp version null in 0.06 seconds
Built preamble of size 187020 for file C:\clangd-test\TestTU.cpp version null in 0.07 seconds
[  FAILED  ] FindExplicitReferencesTest.All (19046 ms)
[----------] 1 test from FindExplicitReferencesTest (19048 ms total)

I'm testing on Windows, FWIW.

sammccall added a comment.EditedOct 4 2022, 2:30 PM

Thank you for this! I applied 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)}.

Thank you for this! I applied 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)}.

Thank you for the extra analysis! In terms of next steps, do you think I should roll your changes into this review and adjust the tests accordingly, or do you think we should land your changes first and then I rebase on top of them?

I just landed that change to USR generation as 20c9ac29250493f5e0a3791dc1e5e9114ff0dc6e, so there should now be no USR changes (just some changes to debug representations that happen to be in some of the USR test files, which can be updated).

I'd guess we need some kind of change to CommentXML and ExtractAPI, but I don't know enough to be sure what it should be.

I just landed that change to USR generation as 20c9ac29250493f5e0a3791dc1e5e9114ff0dc6e, so there should now be no USR changes (just some changes to debug representations that happen to be in some of the USR test files, which can be updated).

Thank you!

I'd guess we need some kind of change to CommentXML and ExtractAPI, but I don't know enough to be sure what it should be.

Ping @dang @zixuw and @dexonsmith for questions about how to handle ExtractAPI changes, and @gribozavr for questions about CommentXML

I'd guess we need some kind of change to CommentXML and ExtractAPI, but I don't know enough to be sure what it should be.

Ping @dang @zixuw and @dexonsmith for questions about how to handle ExtractAPI changes, and @gribozavr for questions about CommentXML

Thanks for the heads up! Looking.

At a glance it seems that ExtractAPI would like to fall back to the original formats to not break downstream tooling. But not sure if it's fine because it's still consistent. As far as I can tell this only affects anonymous TagDecls. @QuietMisdreavus thoughts?

IMO we would still want to keep the ExtractAPI outputs unchanged if it's easy enough to do. We're using getName()/getQualifiedNameAsString() for names and index::generateUSRForDecl() for USRs. Is there a 'policy' or some alternatives to use? Or is the location info now part of the 'correct' format for USRs (seems a bit weird to see that as part of the USR)?

zixuw added inline comments.Oct 5 2022, 10:26 AM
clang/test/ExtractAPI/typedef_anonymous_record.c
74 ↗(On Diff #463582)

Why is the A dropped here? Isn't this USR still referring to an anonymous struct (typedef struct { } MyStruct;)?

zixuw added a comment.Oct 5 2022, 10:45 AM

Ah sorry I just finished reading the discussions. IIUC 20c9ac29250493f5e0a3791dc1e5e9114ff0dc6e should have already fixed the USR generation part, and all of the USR updates in the test cases should be gone now?

For the name part (title) I believe it's less likely to cause any problem, but would still be good to get rid of the location info just for the sake of keeping diff working and not having to change lit 🙃 (yeah probably using diff was not a good idea in the first place.. 😬)

zixuw added a comment.Oct 5 2022, 11:18 AM

Setting PrintingPolicy::AnonymousTagLocations to false in https://reviews.llvm.org/D135295

zixuw added a comment.Oct 5 2022, 12:47 PM

I'm pulling this on top of https://reviews.llvm.org/D135295 to try locally now.

clang/include/clang/AST/Decl.h
3659

nit: missing an override here. (warning: 'printName' overrides a member function but is not marked 'override' [-Winconsistent-missing-override])

zixuw added a comment.Oct 5 2022, 1:08 PM

With the PrintingPolicy fix in https://reviews.llvm.org/D135295 and landed USR fix, the diff within ExtractAPI tests is only the wording with anonymous enums, and we can drop the lit change:

658c658
<             "spelling": "(anonymous)"
---
>             "spelling": "enum (unnamed)"
661c661
<         "title": "(anonymous)"
---
>         "title": "enum (unnamed)"
664c664
<         "(anonymous)"
---
>         "enum (unnamed)"
706c706
<         "(anonymous)",
---
>         "enum (unnamed)",
746c746
<             "spelling": "(anonymous)"
---
>             "spelling": "enum (unnamed)"
749c749
<         "title": "(anonymous)"
---
>         "title": "enum (unnamed)"
752c752
<         "(anonymous)"
---
>         "enum (unnamed)"
794c794
<         "(anonymous)",
---
>         "enum (unnamed)",

@dang @QuietMisdreavus for this change.

dang accepted this revision.Oct 6 2022, 1:15 AM

With the PrintingPolicy fix in https://reviews.llvm.org/D135295 and landed USR fix, the diff within ExtractAPI tests is only the wording with anonymous enums, and we can drop the lit change:

658c658
<             "spelling": "(anonymous)"
---
>             "spelling": "enum (unnamed)"
661c661
<         "title": "(anonymous)"
---
>         "title": "enum (unnamed)"
664c664
<         "(anonymous)"
---
>         "enum (unnamed)"
706c706
<         "(anonymous)",
---
>         "enum (unnamed)",
746c746
<             "spelling": "(anonymous)"
---
>             "spelling": "enum (unnamed)"
749c749
<         "title": "(anonymous)"
---
>         "title": "enum (unnamed)"
752c752
<         "(anonymous)"
---
>         "enum (unnamed)"
794c794
<         "(anonymous)",
---
>         "enum (unnamed)",

@dang @QuietMisdreavus for this change.

As far as I know this should affect downstream consumers like Swift-DocC. I am happy with these changes.

aaron.ballman marked 2 inline comments as done.

Rebased and updated based on review feedback.

Thank you *so much* to @sammccall @zixuw and @dang for the help with indexing and API extraction, I really appreciate it!

clang/include/clang/AST/Decl.h
3659

Good catch, thank you!

clang/lib/AST/Decl.cpp
4481

It's a reasonable suggestion, but I'd rather not because printing is used by the dump methods and dumping is often used from a debugger where things might be in a slightly weird state.

Updated to fix the clangd unit test breakage.

Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2022, 5:52 AM
Herald added a subscriber: kadircet. · View Herald Transcript

Fixes the failing AST unit test in Clang found by precommit CI.

This revision was landed with ongoing or failed builds.Oct 14 2022, 5:18 AM
This revision was automatically updated to reflect the committed changes.

Out of curiosity, not sure if it's worth fixing because it's easy enough to work around: I think after this change, it's not possible anymore to call printName(raw_ostream &OS) on a (statically typed) TagDecl / EnumDecl because it's hidden by TagDecl::printName(raw_ostream &OS, const PrintingPolicy &Policy) while it works perfectly for a NamedDecl. Is this intentional?

Out of curiosity, not sure if it's worth fixing because it's easy enough to work around: I think after this change, it's not possible anymore to call printName(raw_ostream &OS) on a (statically typed) TagDecl / EnumDecl because it's hidden by TagDecl::printName(raw_ostream &OS, const PrintingPolicy &Policy) while it works perfectly for a NamedDecl. Is this intentional?

That's not intentional, thank you for pointing it out! This should be corrected in 0566791ab28b5b842c3befea63d7d47fe9ba1a59

Out of curiosity, not sure if it's worth fixing because it's easy enough to work around: I think after this change, it's not possible anymore to call printName(raw_ostream &OS) on a (statically typed) TagDecl / EnumDecl because it's hidden by TagDecl::printName(raw_ostream &OS, const PrintingPolicy &Policy) while it works perfectly for a NamedDecl. Is this intentional?

That's not intentional, thank you for pointing it out! This should be corrected in 0566791ab28b5b842c3befea63d7d47fe9ba1a59

Thanks! Note that the same probably holds true (but I didn't test) for all other classes that override printName(raw_ostream &OS, const PrintingPolicy &Policy), ie DecompositionDecl, MSGuidDecl, UnnamedGlobalConstantDecl, and TemplateParamObjectDecl.

Out of curiosity, not sure if it's worth fixing because it's easy enough to work around: I think after this change, it's not possible anymore to call printName(raw_ostream &OS) on a (statically typed) TagDecl / EnumDecl because it's hidden by TagDecl::printName(raw_ostream &OS, const PrintingPolicy &Policy) while it works perfectly for a NamedDecl. Is this intentional?

That's not intentional, thank you for pointing it out! This should be corrected in 0566791ab28b5b842c3befea63d7d47fe9ba1a59

Thanks! Note that the same probably holds true (but I didn't test) for all other classes that override printName(raw_ostream &OS, const PrintingPolicy &Policy), ie DecompositionDecl, MSGuidDecl, UnnamedGlobalConstantDecl, and TemplateParamObjectDecl.

You're correct, but I figured those are somewhat uncommon AST nodes, so we can probably expose those APIs as-needed rather than doing all of them. However, if you prefer they all get handled, it's easy enough!

Thanks! Note that the same probably holds true (but I didn't test) for all other classes that override printName(raw_ostream &OS, const PrintingPolicy &Policy), ie DecompositionDecl, MSGuidDecl, UnnamedGlobalConstantDecl, and TemplateParamObjectDecl.

You're correct, but I figured those are somewhat uncommon AST nodes, so we can probably expose those APIs as-needed rather than doing all of them.

Oh, alright.

However, if you prefer they all get handled, it's easy enough!

No, not needed. I was only running into it with an EnumDecl while going to LLVM 16. But as I said, it's easy enough to work around.

Thanks! Note that the same probably holds true (but I didn't test) for all other classes that override printName(raw_ostream &OS, const PrintingPolicy &Policy), ie DecompositionDecl, MSGuidDecl, UnnamedGlobalConstantDecl, and TemplateParamObjectDecl.

You're correct, but I figured those are somewhat uncommon AST nodes, so we can probably expose those APIs as-needed rather than doing all of them.

Oh, alright.

However, if you prefer they all get handled, it's easy enough!

No, not needed. I was only running into it with an EnumDecl while going to LLVM 16. But as I said, it's easy enough to work around.

In general, I think we want to encourage callers to use the variant with a PrintingPolicy so that user-controlled options are properly handled (e.g., spelling it _Bool instead of bool older C modes, that sort of thing), so I figure the extra work is a subtle encouragement. :-)