llvm
[clang][TypePrinter] Unify printing of anonymous/unnamed tag types
In https://github.com/llvm/llvm-project/pull/168534 we made the TypePrinter re-use printNestedNameSpecifier for printing scopes. However, the way that the names of anonymous/unnamed types get printed by the two are slightly inconsistent with each other.
printNestedNameSpecifier calls all TagTypes without an identifer (anonymous). On the other hand, TypePrinter prints them slightly more accurate (it differentiates anonymous vs. unnamed decls) and allows for some additional customization points. E.g., with MSVCFormatting, it will print `unnamed struct' instead of (unnamed struct). printNestedNameSpecifier already accounts for MSVCFormatting for namespaces, but doesn't for TagTypes. This inconsistency means that if an unnamed tag is printed as part of a scope then it's displayed as (anonymous struct), but if it's the entity whose scope is being printed, then it shows as (unnamed struct).
This patch moves the printing of anonymous/unnamed tags into a common helper and re-uses it from TypePrinter and printNestedNameSpecifier. We also do this from the AST matchers because they were aligned with how printNestedNameSpecifier represents the name.
I wasn't sure where to put the helper, so I just put it in TypeBase.h for now. Though I suspect there's a better home for it, possibly DeclBase.h?
@llvm/pr-subscribers-hlsl @llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-static-analyzer-1
@llvm/pr-subscribers-clang
Author: Michael Buch (Michael137)
Changes
In https://github.com/llvm/llvm-project/pull/168534 we made the TypePrinter re-use printNestedNameSpecifier for printing scopes. However, the way that the names of anonymous/unnamed get printed by the two are slightly inconsistent with each other.
printNestedNameSpecifier calls all TagTypes without an identifer (anonymous). On the other hand, TypePrinter prints them slightly more accurate (it differentiates anonymous vs. unnamed decls) and allows for some additional customization points. E.g., with MSVCFormatting, it will print \'instead of(). printNestedNameSpecifieralready accounts forMSVCFormattingfor namespaces, but doesn't forTagTypes. This inconsistency means that if an unnamed tag is printed as part of a scope it's displayed as (anonymous struct), but if it's the entity being whose scope is being printed, then it shows as (unnamed struct)`.
This patch moves the printing of anonymous/unnamed tags into a common helper and re-uses it from TypePrinter and printNestedNameSpecifier. We also do this from the AST matchers because they were aligned with how printNestedNameSpecifier represents the name.
I wasn't sure where to put the helper, so I just put it in TypeBase.h for now. Though I suspect there's a better home for it, possibly DeclBase.h?
Full diff: https://github.com/llvm/llvm-project/pull/169445.diff
7 Files Affected:
- (modified) clang/include/clang/AST/TypeBase.h (+4)
- (modified) clang/lib/AST/Decl.cpp (+4-1)
- (modified) clang/lib/AST/TypePrinter.cpp (+59-44)
- (modified) clang/lib/ASTMatchers/ASTMatchersInternal.cpp (+9-1)
- (modified) clang/unittests/AST/NamedDeclPrinterTest.cpp (+28)
- (modified) clang/unittests/AST/TypePrinterTest.cpp (+1-1)
- (modified) clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp (+28-5)
diff --git a/clang/include/clang/AST/TypeBase.h b/clang/include/clang/AST/TypeBase.h
index f07861f50fe8c..2f27deda2cd66 100644
--- a/clang/include/clang/AST/TypeBase.h
+++ b/clang/include/clang/AST/TypeBase.h
@@ -7374,6 +7374,10 @@ bool isSubstitutedDefaultArgument(ASTContext &Ctx, TemplateArgument Arg,
ArrayRef<TemplateArgument> Args,
unsigned Depth);
+void printAnonymousTagDecl(llvm::raw_ostream &OS, const TagDecl *D,
+ const PrintingPolicy &Policy,
+ bool PrintKindDecoration, bool PrintTagLocations);
+
/// Represents a qualified type name for which the type name is
/// dependent.
///
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 591457b1d66b4..c9bb414a9a1de 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -35,6 +35,7 @@
#include "clang/AST/Stmt.h"
#include "clang/AST/TemplateBase.h"
#include "clang/AST/Type.h"
+#include "clang/AST/TypeBase.h"
#include "clang/AST/TypeLoc.h"
#include "clang/Basic/Builtins.h"
#include "clang/Basic/IdentifierTable.h"
@@ -1793,7 +1794,9 @@ void NamedDecl::printNestedNameSpecifier(raw_ostream &OS,
if (TypedefNameDecl *TD = RD->getTypedefNameForAnonDecl())
OS << *TD;
else if (!RD->getIdentifier())
- OS << "(anonymous " << RD->getKindName() << ')';
+ printAnonymousTagDecl(OS, llvm::cast<TagDecl>(RD), P,
+ /*PrintKindDecoration=*/true,
+ /*AllowSourceLocations=*/false);
else
OS << *RD;
} else if (const auto *FD = dyn_cast<FunctionDecl>(DC)) {
diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index d2881d5ac518a..e9b06cc17ae30 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -23,6 +23,7 @@
#include "clang/AST/TemplateBase.h"
#include "clang/AST/TemplateName.h"
#include "clang/AST/Type.h"
+#include "clang/AST/TypeBase.h"
#include "clang/Basic/AddressSpaces.h"
#include "clang/Basic/AttrKinds.h"
#include "clang/Basic/ExceptionSpecificationType.h"
@@ -1518,11 +1519,11 @@ void TypePrinter::printTagType(const TagType *T, raw_ostream &OS) {
return;
}
- bool HasKindDecoration = false;
+ bool PrintKindDecoration = Policy.AnonymousTagLocations;
if (T->isCanonicalUnqualified()) {
if (!Policy.SuppressTagKeyword && !D->getTypedefNameForAnonDecl()) {
- HasKindDecoration = true;
+ PrintKindDecoration = false;
OS << D->getKindName();
OS << ' ';
}
@@ -1547,49 +1548,12 @@ void TypePrinter::printTagType(const TagType *T, raw_ostream &OS) {
assert(Typedef->getIdentifier() && "Typedef without identifier?");
OS << Typedef->getIdentifier()->getName();
} else {
- // Make an unambiguous representation for anonymous types, e.g.
- // (anonymous enum at /usr/include/string.h:120:9)
- OS << (Policy.MSVCFormatting ? '`' : '(');
-
- if (isa<CXXRecordDecl>(D) && cast<CXXRecordDecl>(D)->isLambda()) {
- OS << "lambda";
- HasKindDecoration = true;
- } else if ((isa<RecordDecl>(D) && cast<RecordDecl>(D)->isAnonymousStructOrUnion())) {
- OS << "anonymous";
- } else {
- OS << "unnamed";
- }
+ if (isa<CXXRecordDecl>(D) && cast<CXXRecordDecl>(D)->isLambda())
+ PrintKindDecoration = false;
- if (Policy.AnonymousTagLocations) {
- // Suppress the redundant tag keyword if we just printed one.
- // We don't have to worry about ElaboratedTypes here because you can't
- // refer to an anonymous type with one.
- if (!HasKindDecoration)
- OS << " " << D->getKindName();
-
- PresumedLoc PLoc = D->getASTContext().getSourceManager().getPresumedLoc(
- D->getLocation());
- if (PLoc.isValid()) {
- OS << " at ";
- StringRef File = PLoc.getFilename();
- llvm::SmallString<1024> WrittenFile(File);
- if (auto *Callbacks = Policy.Callbacks)
- WrittenFile = Callbacks->remapPath(File);
- // Fix inconsistent path separator created by
- // clang::DirectoryLookup::LookupFile when the file path is relative
- // path.
- llvm::sys::path::Style Style =
- llvm::sys::path::is_absolute(WrittenFile)
- ? llvm::sys::path::Style::native
- : (Policy.MSVCFormatting
- ? llvm::sys::path::Style::windows_backslash
- : llvm::sys::path::Style::posix);
- llvm::sys::path::native(WrittenFile, Style);
- OS << WrittenFile << ':' << PLoc.getLine() << ':' << PLoc.getColumn();
- }
- }
-
- OS << (Policy.MSVCFormatting ? '\'' : ')');
+ printAnonymousTagDecl(OS, D, Policy,
+ /*PrintKindDecoration=*/PrintKindDecoration,
+ /*PrintTagLocations=*/Policy.AnonymousTagLocations);
}
// If this is a class template specialization, print the template
@@ -2469,6 +2433,57 @@ static bool isSubstitutedTemplateArgument(ASTContext &Ctx, TemplateArgument Arg,
return false;
}
+void clang::printAnonymousTagDecl(llvm::raw_ostream &OS, const TagDecl *D,
+ const PrintingPolicy &Policy,
+ bool PrintKindDecoration,
+ bool PrintTagLocations) {
+ assert(D);
+
+ // Make an unambiguous representation for anonymous types, e.g.
+ // (anonymous enum at /usr/include/string.h:120:9)
+ OS << (Policy.MSVCFormatting ? '`' : '(');
+
+ if (isa<CXXRecordDecl>(D) && cast<CXXRecordDecl>(D)->isLambda()) {
+ OS << "lambda";
+ } else if ((isa<RecordDecl>(D) &&
+ cast<RecordDecl>(D)->isAnonymousStructOrUnion())) {
+ OS << "anonymous";
+ } else {
+ OS << "unnamed";
+ }
+
+ // Suppress the redundant tag keyword if we just printed one.
+ // We don't have to worry about ElaboratedTypes here because you can't
+ // refer to an anonymous type with one.
+ if (PrintKindDecoration)
+ OS << " " << D->getKindName();
+
+ if (PrintTagLocations) {
+ PresumedLoc PLoc =
+ D->getASTContext().getSourceManager().getPresumedLoc(D->getLocation());
+ if (PLoc.isValid()) {
+ OS << " at ";
+ StringRef File = PLoc.getFilename();
+ llvm::SmallString<1024> WrittenFile(File);
+ if (auto *Callbacks = Policy.Callbacks)
+ WrittenFile = Callbacks->remapPath(File);
+ // Fix inconsistent path separator created by
+ // clang::DirectoryLookup::LookupFile when the file path is relative
+ // path.
+ llvm::sys::path::Style Style =
+ llvm::sys::path::is_absolute(WrittenFile)
+ ? llvm::sys::path::Style::native
+ : (Policy.MSVCFormatting
+ ? llvm::sys::path::Style::windows_backslash
+ : llvm::sys::path::Style::posix);
+ llvm::sys::path::native(WrittenFile, Style);
+ OS << WrittenFile << ':' << PLoc.getLine() << ':' << PLoc.getColumn();
+ }
+ }
+
+ OS << (Policy.MSVCFormatting ? '\'' : ')');
+}
+
bool clang::isSubstitutedDefaultArgument(ASTContext &Ctx, TemplateArgument Arg,
const NamedDecl *Param,
ArrayRef<TemplateArgument> Args,
diff --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 0874b3d0c45f5..e83a6e97ec638 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -18,6 +18,7 @@
#include "clang/AST/ExprConcepts.h"
#include "clang/AST/ParentMapContext.h"
#include "clang/AST/PrettyPrinter.h"
+#include "clang/AST/TypeBase.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Basic/LLVM.h"
#include "clang/Lex/Lexer.h"
@@ -514,7 +515,14 @@ static StringRef getNodeName(const RecordDecl &Node,
return Node.getName();
}
Scratch.clear();
- return ("(anonymous " + Node.getKindName() + ")").toStringRef(Scratch);
+
+ llvm::raw_svector_ostream OS(Scratch);
+
+ printAnonymousTagDecl(
+ OS, llvm::cast<TagDecl>(&Node), Node.getASTContext().getPrintingPolicy(),
+ /*PrintKindDecoration=*/true, /*PrintTagLocations=*/false);
+
+ return OS.str();
}
static StringRef getNodeName(const NamespaceDecl &Node,
diff --git a/clang/unittests/AST/NamedDeclPrinterTest.cpp b/clang/unittests/AST/NamedDeclPrinterTest.cpp
index cd833725b448d..2fdda1929b2a3 100644
--- a/clang/unittests/AST/NamedDeclPrinterTest.cpp
+++ b/clang/unittests/AST/NamedDeclPrinterTest.cpp
@@ -265,3 +265,31 @@ TEST(NamedDeclPrinter, NestedNameSpecifierTemplateArgs) {
ASSERT_TRUE(
PrintedNestedNameSpecifierMatches(Code, "method", "vector<int>::"));
}
+
+TEST(NamedDeclPrinter, NestedNameSpecifierLambda) {
+ const char *Code =
+ R"(
+ auto l = [] {
+ struct Foo {
+ void method();
+ };
+ };
+)";
+ ASSERT_TRUE(PrintedNestedNameSpecifierMatches(
+ Code, "method", "(lambda)::operator()()::Foo::"));
+}
+
+TEST(NamedDeclPrinter, NestedNameSpecifierAnonymousTags) {
+ const char *Code =
+ R"(
+ struct Foo {
+ struct {
+ struct {
+ void method();
+ } i;
+ };
+ };
+)";
+ ASSERT_TRUE(PrintedNestedNameSpecifierMatches(
+ Code, "method", "Foo::(anonymous)::(unnamed)::"));
+}
diff --git a/clang/unittests/AST/TypePrinterTest.cpp b/clang/unittests/AST/TypePrinterTest.cpp
index 3cadf9b265bd1..de4cfa4074eba 100644
--- a/clang/unittests/AST/TypePrinterTest.cpp
+++ b/clang/unittests/AST/TypePrinterTest.cpp
@@ -328,7 +328,7 @@ TEST(TypePrinter, NestedNameSpecifiers) {
// Further levels of nesting print the entire scope.
ASSERT_TRUE(PrintedTypeMatches(
Code, {}, fieldDecl(hasName("u"), hasType(qualType().bind("id"))),
- "union level1()::Inner::Inner(int)::(anonymous struct)::(unnamed)",
+ "union level1()::Inner::Inner(int)::(unnamed struct)::(unnamed)",
[](PrintingPolicy &Policy) {
Policy.FullyQualifiedName = true;
Policy.AnonymousTagLocations = false;
diff --git a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
index 5d452355d0e43..697623c8a48e8 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2709,25 +2709,48 @@ TEST_P(ASTMatchersTest, HasName_MatchesAnonymousNamespaces) {
EXPECT_TRUE(matches(code, recordDecl(hasName("::a::C"))));
}
-TEST_P(ASTMatchersTest, HasName_MatchesAnonymousOuterClasses) {
+TEST_P(ASTMatchersTest, HasName_MatchesUnnamedOuterClasses) {
if (!GetParam().isCXX()) {
return;
}
EXPECT_TRUE(matches("class A { class { class C; } x; };",
- recordDecl(hasName("A::(anonymous class)::C"))));
+ recordDecl(hasName("A::(unnamed class)::C"))));
EXPECT_TRUE(matches("class A { class { class C; } x; };",
- recordDecl(hasName("::A::(anonymous class)::C"))));
+ recordDecl(hasName("::A::(unnamed class)::C"))));
EXPECT_FALSE(matches("class A { class { class C; } x; };",
recordDecl(hasName("::A::C"))));
EXPECT_TRUE(matches("class A { struct { class C; } x; };",
- recordDecl(hasName("A::(anonymous struct)::C"))));
+ recordDecl(hasName("A::(unnamed struct)::C"))));
EXPECT_TRUE(matches("class A { struct { class C; } x; };",
- recordDecl(hasName("::A::(anonymous struct)::C"))));
+ recordDecl(hasName("::A::(unnamed struct)::C"))));
EXPECT_FALSE(matches("class A { struct { class C; } x; };",
recordDecl(hasName("::A::C"))));
}
+TEST_P(ASTMatchersTest, HasName_MatchesAnonymousOuterClasses) {
+ if (!GetParam().isCXX()) {
+ return;
+ }
+
+ EXPECT_TRUE(matches(
+ "class A { struct { struct { class C; } x; }; };",
+ recordDecl(hasName("A::(anonymous struct)::(unnamed struct)::C"))));
+ EXPECT_TRUE(matches(
+ "class A { struct { struct { class C; } x; }; };",
+ recordDecl(hasName("::A::(anonymous struct)::(unnamed struct)::C"))));
+ EXPECT_FALSE(matches("class A { struct { struct { class C; } x; }; };",
+ recordDecl(hasName("A::(unnamed struct)::C"))));
+ EXPECT_TRUE(matches(
+ "class A { class { public: struct { class C; } x; }; };",
+ recordDecl(hasName("A::(anonymous class)::(unnamed struct)::C"))));
+ EXPECT_TRUE(matches(
+ "class A { class { public: struct { class C; } x; }; };",
+ recordDecl(hasName("::A::(anonymous class)::(unnamed struct)::C"))));
+ EXPECT_FALSE(matches("class A { class { public: struct { class C; } x; }; };",
+ recordDecl(hasName("A::(unnamed struct)::C"))));
+}
+
TEST_P(ASTMatchersTest, HasName_MatchesFunctionScope) {
if (!GetParam().isCXX()) {
return;
:penguin: Linux x64 Test Results
- 112149 tests passed
- 4518 tests skipped
:white_check_mark: The build succeeded and all tests passed.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
:white_check_mark: With the latest revision this PR passed the C/C++ code formatter.
I like the direction.
Some comments:
- This would be better declared in Decl.h, since it's about printing declarations, and not related to types.
Agreed
- This could probably get rid of the additional bool flags, and rely on PrintingPolicy entirely.
- Instead of the
printAnonymousTagDeclhelper, we could probably implement a
Neat idea! Trying this out. Might need to make some compromises with when the kind name gets printed, but lets see
I like the direction.
Some comments:
- This would be better declared in Decl.h, since it's about printing declarations, and not related to types.
- This could probably get rid of the additional bool flags, and rely on PrintingPolicy entirely.
- Instead of the
printAnonymousTagDeclhelper, we could probably implement a
@mizvekov Latest commit reworks the patch to do the anonymous type-printing in TagDecl::printName. Getting the tag keyword to print was a bit fiddly. E.g., not a big fan of having to introduce another bit into the PrintingPolicy for this. The resulting name format turned out more consistent now though, which is nice. Is this along the lines of what you had in mind?
- This could probably get rid of the additional bool flags, and rely on PrintingPolicy entirely.
- Instead of the
printAnonymousTagDeclhelper, we could probably implement a
This is mostly implemented now in the latest patchset. My only reservation about moving the printing logic into TagDecl::printName is that we no longer are able to easily determine whether to print the tag keyword. Previously we've done this in: https://github.com/llvm/llvm-project/blob/762a171b3d6f6eb64dc5a844fcb25caa4ece7d00/clang/lib/AST/TypePrinter.cpp#L1523-L1529
But now we have to rely on the PrintingPolicy telling us this, putting the burden on the callers (specifically callers of QualType::print). Maybe that's an OK trade-off? You can see this in the clangd/unittests/HoverTests.cpp changes in the latest revision. We now don't omit the tag keyword in the name itself. For the Hover case specifically we could fix this by setting the new SuppressTagKeywordInAnonNames policy here: https://github.com/llvm/llvm-project/blob/762a171b3d6f6eb64dc5a844fcb25caa4ece7d00/clang-tools-extra/clangd/Hover.cpp#L175-L183
But there's probably other places where this needs to be adjusted (though based on the test changes maybe it's just that clangd case). Wdyt? Is that a reasonable trade-off?