[PATCH] D149677: [clang][TypePrinter] Add option to skip over elaborated types

Wed May 24 12:50:09 PDT 2023

aaron.ballman added a comment.

In D149677#4365031 <https://reviews.llvm.org/D149677#4365031>, @sammccall wrote:

> In D149677#4356863 <https://reviews.llvm.org/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.

================
Comment at: clang/include/clang/AST/PrettyPrinter.h:143-145
+  /// Ignore qualifiers as specified by elaborated type sugar, instead letting
+  /// the underlying type handle printing the qualifiers.
+  unsigned IgnoreElaboratedQualifiers : 1;
----------------
li.zhe.hua wrote:
> aaron.ballman wrote:
> > 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`?
> 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 [[ https://github.com/llvm/llvm-project/blob/1b05e74982242da81d257f660302585cee691f3b/clang/lib/AST/TypePrinter.cpp#L1559-L1568 | tag definition printing logic ]] that I didn't fully understand, so I thought just saying "qualifiers" would be more correct.
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...

================
Comment at: clang/lib/AST/TypePrinter.cpp:1570

+  if (Policy.IgnoreElaboratedQualifiers) {
+    printBefore(T->getNamedType(), OS);
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149677