[clang][TypePrinter] Add option to skip over elaborated types

[li.zhe.hua]

Summary

Elaborated types are sugar that represent how the type was spelled in
the original source. When printing a type outside of that original
context, the qualifiers as saved in the elaborated type will be
incorrect. Additionally, their existence also inhibits the use of
PrintingCallbacks::isScopeVisible as a customization point.

[aaron.ballman]

Will this new printing policy be used in tree?

clang/include/clang/AST/PrettyPrinter.h

143–145

The name is somewhat confusing to me, because tag names are also elaborations and those are handled by SuppressTagKeyword -- so what is the interaction between those when the user specifies true for IgnoreElaboratedQualifiers and false for SuppressTagKeyword?

clang/unittests/AST/TypePrinterTest.cpp

116–119

[li.zhe.hua]

Accept suggested edits.

[li.zhe.hua]

In D149677#4319646, @aaron.ballman wrote:

Will this new printing policy be used in tree?

Not initially, no. (I'm understanding "in tree" as "in the LLVM project".)

Some additional context: I'm working on a refactoring tool to, in part, deprecate an alias by replacing it with its definition. That functionality by itself could be provided through something like a clang-tidy, so I say "initially".

clang/include/clang/AST/PrettyPrinter.h

143–145

Ah, I hadn't considered tag names. Let me see...

I tried this out with the template specialization test case I added. If I have

using Alias = struct a::S<struct b::Foo>;

then printing the aliased type of Alias with true for IgnoreElaboratedQualifiers and false for SuppressTagKeyword gives "S<struct shared::b::Foo>". (I'm guessing that printing the template specialization with SuppressTagKeyword disabled is weird or unexpected?) So it would seem that SuppressTagKeyword still functions, as long as the desugared type supports it?

Back to the topic of names, it seems like "qualifiers" is not sufficient, as it also ignores elaborated tag keywords. Perhaps IgnoreElaboration?

---

Note: I initially had the name as IgnoreElaboratedTypes, but there was also the tag definition printing logic that I didn't fully understand, so I thought just saying "qualifiers" would be more correct.

[aaron.ballman]

In D149677#4320178, @li.zhe.hua wrote:

In D149677#4319646, @aaron.ballman wrote:

Will this new printing policy be used in tree?

Not initially, no. (I'm understanding "in tree" as "in the LLVM project".)

You understand correctly. :-)

Some additional context: I'm working on a refactoring tool to, in part, deprecate an alias by replacing it with its definition. That functionality by itself could be provided through something like a clang-tidy, so I say "initially".

We typically don't add new printing policies in tree without some need for them in the project itself (otherwise this becomes a maintenance problem), and I'm not certain I see a strong need for this to live in Clang. I think it might be best to wait until you're closer to having a clang-tidy check that would use this functionality before we continue with this patch. WDYT?

[li.zhe.hua]

In D149677#4349523, @aaron.ballman wrote:

In D149677#4320178, @li.zhe.hua wrote:

Some additional context: I'm working on a refactoring tool to, in part, deprecate an alias by replacing it with its definition. That functionality by itself could be provided through something like a clang-tidy, so I say "initially".

We typically don't add new printing policies in tree without some need for them in the project itself (otherwise this becomes a maintenance problem), and I'm not certain I see a strong need for this to live in Clang. I think it might be best to wait until you're closer to having a clang-tidy check that would use this functionality before we continue with this patch. WDYT?

I believe there is value for having this option, as without it, the PrintingCallbacks::isScopeVisible callback is effectively useless in practice. It won't be triggered in non-canonical types because the ElaboratedType represents a fixed qualification on the name.

Note: there may be an argument for folding this functionality directly into FullyQualifiedName (which currently affects template-ids despite the comment on it suggesting it only is for function names). If the template name was elaborated, FullyQualifiedName does nothing, which seems inconsistent/incorrect.

[aaron.ballman]

In D149677#4350337, @li.zhe.hua wrote:

In D149677#4349523, @aaron.ballman wrote:

In D149677#4320178, @li.zhe.hua wrote:

Some additional context: I'm working on a refactoring tool to, in part, deprecate an alias by replacing it with its definition. That functionality by itself could be provided through something like a clang-tidy, so I say "initially".

We typically don't add new printing policies in tree without some need for them in the project itself (otherwise this becomes a maintenance problem), and I'm not certain I see a strong need for this to live in Clang. I think it might be best to wait until you're closer to having a clang-tidy check that would use this functionality before we continue with this patch. WDYT?

I believe there is value for having this option, as without it, the PrintingCallbacks::isScopeVisible callback is effectively useless in practice. It won't be triggered in non-canonical types because the ElaboratedType represents a fixed qualification on the name.

Note: there may be an argument for folding this functionality directly into FullyQualifiedName (which currently affects template-ids despite the comment on it suggesting it only is for function names). If the template name was elaborated, FullyQualifiedName does nothing, which seems inconsistent/incorrect.

Adding @dblaikie as he might have some opinions here.

I'm not seeing how the changes you've got here interact with isScopeVisible(); However, I also note that the only use of isScopeVisible() in tree is in clangd, so also adding @sammccall to see if they're hitting issues there or not.

[dblaikie]

In D149677#4356863, @aaron.ballman wrote:

In D149677#4350337, @li.zhe.hua wrote:

In D149677#4349523, @aaron.ballman wrote:

In D149677#4320178, @li.zhe.hua wrote:

Some additional context: I'm working on a refactoring tool to, in part, deprecate an alias by replacing it with its definition. That functionality by itself could be provided through something like a clang-tidy, so I say "initially".

We typically don't add new printing policies in tree without some need for them in the project itself (otherwise this becomes a maintenance problem), and I'm not certain I see a strong need for this to live in Clang. I think it might be best to wait until you're closer to having a clang-tidy check that would use this functionality before we continue with this patch. WDYT?

I believe there is value for having this option, as without it, the PrintingCallbacks::isScopeVisible callback is effectively useless in practice. It won't be triggered in non-canonical types because the ElaboratedType represents a fixed qualification on the name.

Note: there may be an argument for folding this functionality directly into FullyQualifiedName (which currently affects template-ids despite the comment on it suggesting it only is for function names). If the template name was elaborated, FullyQualifiedName does nothing, which seems inconsistent/incorrect.

Adding @dblaikie as he might have some opinions here.

Not sure I'm quite understanding everything going on here, but I think it's a reasonable request that the change be observable in some way in addition to/other than a unit test. If not, yeah, maybe wait until it can be/the work where it is used is at least up on phab.

[sammccall]

In D149677#4356863, @aaron.ballman wrote:

I'm not seeing how the changes you've got here interact with isScopeVisible(); However, I also note that the only use of isScopeVisible() in tree is in clangd, so also adding @sammccall to see if they're hitting issues there or not.

I don't think I can offer much experience one way or the other.
clangd's original use of this extension point was in printing deduced types (replace auto with a concrete type) so usually there was no sugar on the types.
In other cases we're e.g. generating function signatures, and happy with *either* type-as-written-somewhere-in-the-code *or* type-relative-to-enclosing-namespace, as long as we're not producing some long qualified form that nobody would ever write.

We typically don't add new printing policies in tree without some need for them in the project itself (otherwise this becomes a maintenance problem), and I'm not certain I see a strong need for this to live in Clang

The maintenance concern makes sense ("why do we have this, and can do we still need it" needs to be an answerable question).

But this paints out-of-tree users into a corner: the designs clang has chosen here only really allow extension in two places:

• customize the printing, but this can *only* be done in TypePrinter (or reimplement the whole thing)
• replace the sugar on the types before printing, but TreeTransform is private, and any other solution requires tightly coupling with the full set of types Clang supports

I think if we want clang to be a generally-useful representation of the source code then we need to make concessions to out-of-tree use-cases either directly or via extensibility.

[aaron.ballman]

In D149677#4365031, @sammccall wrote:

In D149677#4356863, @aaron.ballman wrote:

I'm not seeing how the changes you've got here interact with isScopeVisible(); However, I also note that the only use of isScopeVisible() in tree is in clangd, so also adding @sammccall to see if they're hitting issues there or not.

I don't think I can offer much experience one way or the other.
clangd's original use of this extension point was in printing deduced types (replace auto with a concrete type) so usually there was no sugar on the types.
In other cases we're e.g. generating function signatures, and happy with *either* type-as-written-somewhere-in-the-code *or* type-relative-to-enclosing-namespace, as long as we're not producing some long qualified form that nobody would ever write.

We typically don't add new printing policies in tree without some need for them in the project itself (otherwise this becomes a maintenance problem), and I'm not certain I see a strong need for this to live in Clang

The maintenance concern makes sense ("why do we have this, and can do we still need it" needs to be an answerable question).

But this paints out-of-tree users into a corner: the designs clang has chosen here only really allow extension in two places:

• customize the printing, but this can *only* be done in TypePrinter (or reimplement the whole thing)
• replace the sugar on the types before printing, but TreeTransform is private, and any other solution requires tightly coupling with the full set of types Clang supports

I think if we want clang to be a generally-useful representation of the source code then we need to make concessions to out-of-tree use-cases either directly or via extensibility.

My primary concern with making those concessions is that there's no real metric for reviewers to use to determine whether a change is reasonable or not; anyone can argue "but I need this" and the above two points still hold them back. My secondary concern is that this will lead to TypePrinter being used for purposes which it was not designed to be used. This was an implementation detail class for getting a string-ified form of a QualType primarily for printing diagnostics. But it's morphing into a utility class to handle arbitrary type printing. We see the same sort of things with declaration printing, as well. That suggests to me that we need to change this class (and probably the decl printer too) to be more user-extensible for these out-of-tree needs. But it's a pretty heavy lift to expect @li.zhe.hua to do that work just because they're the latest person with a custom need, so I'm not certain of the best way forward.

That said, I'm not strongly opposed to the changes in this patch, so perhaps we should move forward with this and hope that kicks the can down the road long enough for someone to propose/implement a more extensible approach.

clang/include/clang/AST/PrettyPrinter.h

143–145

I think IgnoreElaboration has roughly the same problem of "which do I use and how do these operate in combination?" I was trying to convince myself that perhaps using a single field with an enumeration of named choices would be better, but I'm not quite able to do it because there are other fields that also interact (like the scope printing fields) and it's not clear we'd end up in a better place at the end.

More thoughts below...

clang/lib/AST/TypePrinter.cpp

1579

So, effectively, the idea here is: you want the ability to skip printing the elaborated keyword and nested name specifier, and you additionally want to skip use of ElaboratedTypePolicyRAII when calling printBefore()/printAfter() because that forces suppression of tags and scopes?

I think what I'm struggling with a bit is that this is doing three things at once; one is the elaboration keyword (so we no longer print typename or struct when IncludeTagDefinition is false), another is the nested name specifier (we no longer print the leading foo::bar), and the third is that we no longer suppress tags and scopes when printing the underlying type. That makes it tricky to figure out how to name this thing, but the best I could come up with is: SuppressElaboration which isn't really different from IgnoreElaboration at all. So I think either of those names is "fine", but I'm still a bit uncomfortable about how complex the interactions are becoming.

[li.zhe.hua]

Switch option name to SuppressElaboration

clang/lib/AST/TypePrinter.cpp

1579

another is the nested name specifier (we no longer print the leading foo::bar), and the third is that we no longer suppress tags and scopes when printing the underlying type

I would say that this is the primary desired behavior of the option. More specifically, it's not that we don't print the nested name specifier and no longer suppress those options, but rather, we print the underlying type with the options as specified by the policy. SuppressTagKeyword, SuppressScope, and FullyQualifiedName work as expected (from my perspective). The interaction with IncludeTagDefinition dropping the keyword wasn't explicitly intended, but it is still consistent with printing the canonical type (which would also drop the keyword).

One last name idea to throw out there, in case it seems worth discussing: DesugarElaboratedTypes. In that case, I'd probably also move the check up above L1559 and we treat the entire ElaboratedType as if it did not exist.

[aaron.ballman]

LGTM -- my concerns about the complexity still stand, but I don't think those concerns are serious enough to warrant rejecting the changes.

[mizvekov]

This was never mentioned in the review, but I have a late question: Why did we need to implement a new flag, instead of using getFullyQualifiedName from QualTypeNames.cpp?

clang/include/clang/AST/PrettyPrinter.h

 /// This type is intended to be small and suitable for passing by value.
 /// It is very frequently copied.
 struct PrintingPolicy {
   /// Create a default printing policy for the specified language.
   PrintingPolicy(const LangOptions &LO)
       : Indentation(2), SuppressSpecifiers(false),
         SuppressTagKeyword(LO.CPlusPlus), IncludeTagDefinition(false),
         SuppressScope(false), SuppressUnwrittenScope(false),
-        SuppressInlineNamespace(true), SuppressInitializers(false),
-        ConstantArraySizeAsWritten(false), AnonymousTagLocations(true),
-        SuppressStrongLifetime(false), SuppressLifetimeQualifiers(false),
+        SuppressInlineNamespace(true), SuppressElaboration(false),
+        SuppressInitializers(false), ConstantArraySizeAsWritten(false),
+        AnonymousTagLocations(true), SuppressStrongLifetime(false),
+        SuppressLifetimeQualifiers(false),
         SuppressTemplateArgsInCXXConstructors(false),
         SuppressDefaultTemplateArgs(true), Bool(LO.Bool),
         Nullptr(LO.CPlusPlus11 || LO.C2x), NullptrTypeInNamespace(LO.CPlusPlus),
-        Restrict(LO.C99), Alignof(LO.CPlusPlus11),
-        UnderscoreAlignof(LO.C11), UseVoidForZeroParams(!LO.CPlusPlus),
+        Restrict(LO.C99), Alignof(LO.CPlusPlus11), UnderscoreAlignof(LO.C11),
+        UseVoidForZeroParams(!LO.CPlusPlus),
         SplitTemplateClosers(!LO.CPlusPlus11), TerseOutput(false),
         PolishForDeclaration(false), Half(LO.Half),
         MSWChar(LO.MicrosoftExt && !LO.WChar), IncludeNewlines(true),
         MSVCFormatting(false), ConstantsAsWritten(false),
         SuppressImplicitBase(false), FullyQualifiedName(false),
         PrintCanonicalTypes(false), PrintInjectedClassNameWithArguments(true),
         UsePreferredNames(true), AlwaysIncludeTypeForTemplateArgument(false),
         CleanUglifiedParameters(false), EntireContentsOfLargeArray(true),
struct PrintingPolicy {
   /// written, e.g., for anonymous namespaces.
   unsigned SuppressUnwrittenScope : 1;
 
   /// Suppress printing parts of scope specifiers that correspond
   /// to inline namespaces, where the name is unambiguous with the specifier
   /// removed.
   unsigned SuppressInlineNamespace : 1;
 
+  /// Ignore qualifiers and tag keywords as specified by elaborated type sugar,
+  /// instead letting the underlying type print as normal.
+  unsigned SuppressElaboration : 1;
+
   /// Suppress printing of variable initializers.
   ///
   /// This flag is used when printing the loop variable in a for-range
   /// statement. For example, given:
   ///
   /// \code
   /// for (auto x : coll)
   /// \endcode

clang/lib/AST/TypePrinter.cpp

assert(OwnedTagDecl->getTypeForDecl() == T->getNamedType().getTypePtr() &&
            "OwnedTagDecl expected to be a declaration for the type");
     PrintingPolicy SubPolicy = Policy;
     SubPolicy.IncludeTagDefinition = false;
     OwnedTagDecl->print(OS, SubPolicy, Indentation);
     spaceBeforePlaceHolder(OS);
     return;
   }
 
+  if (Policy.SuppressElaboration) {
+    printBefore(T->getNamedType(), OS);
+    return;
+  }
+
   // The tag definition will take care of these.
   if (!Policy.IncludeTagDefinition)
   {
     OS << TypeWithKeyword::getKeywordName(T->getKeyword());
     if (T->getKeyword() != ETK_None)
       OS << " ";
     NestedNameSpecifier *Qualifier = T->getQualifier();
     if (Qualifier)
       Qualifier->print(OS, Policy);
   }
 
   ElaboratedTypePolicyRAII PolicyRAII(Policy);
   printBefore(T->getNamedType(), OS);
 }
 
 void TypePrinter::printElaboratedAfter(const ElaboratedType *T,
                                         raw_ostream &OS) {
   if (Policy.IncludeTagDefinition && T->getOwnedTagDecl())
     return;
+
+  if (Policy.SuppressElaboration) {
+    printAfter(T->getNamedType(), OS);
+    return;
+  }
+
   ElaboratedTypePolicyRAII PolicyRAII(Policy);
   printAfter(T->getNamedType(), OS);
 }
 
 void TypePrinter::printParenBefore(const ParenType *T, raw_ostream &OS) {
   if (!HasEmptyPlaceHolder && !isa<FunctionType>(T->getInnerType())) {
     printBefore(T->getInnerType(), OS);
     OS << '(';

clang/unittests/AST/TypePrinterTest.cpp

TEST(TypePrinter, ParamsUglified) {
   ASSERT_TRUE(PrintedTypeMatches(Code, {},
                                  varDecl(hasType(qualType().bind("id"))),
                                  "const __f<_Tp &> *", nullptr));
   ASSERT_TRUE(PrintedTypeMatches(Code, {},
                                  varDecl(hasType(qualType().bind("id"))),
                                  "const f<Tp &> *", Clean));
 }
 
+TEST(TypePrinter, SuppressElaboration) {
+  llvm::StringLiteral Code = R"cpp(
+    namespace shared {
+    namespace a {
+    template <typename T>
+    struct S {};
+    }  // namespace a
+    namespace b {
+    struct Foo {};
+    }  // namespace b
+    using Alias = a::S<b::Foo>;
+    }  // namespace shared
+  )cpp";
+
+  auto Matcher = typedefNameDecl(hasName("::shared::Alias"),
+                                 hasType(qualType().bind("id")));
+  ASSERT_TRUE(PrintedTypeMatches(
+      Code, {}, Matcher, "a::S<b::Foo>",
+      [](PrintingPolicy &Policy) { Policy.FullyQualifiedName = true; }));
+  ASSERT_TRUE(PrintedTypeMatches(Code, {}, Matcher,
+                                 "shared::a::S<shared::b::Foo>",
+                                 [](PrintingPolicy &Policy) {
+                                   Policy.SuppressElaboration = true;
+                                   Policy.FullyQualifiedName = true;
+                                 }));
+}
+
 TEST(TypePrinter, TemplateIdWithNTTP) {
   constexpr char Code[] = R"cpp(
     template <int N>
     struct Str {
       constexpr Str(char const (&s)[N]) { __builtin_memcpy(value, s, N); }
       char value[N];
     };
     template <Str> class ASCII {};