[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

Fri Sep 23 12:22:53 PDT 2022

dblaikie added inline comments.

================
Comment at: clang/include/clang/AST/PrettyPrinter.h:307
+  /// decltype(s) will be printed as "S<Point{1,2}>" if enabled and as "S<{1,2}>" if disabled,
+  /// regardless if PrintCanonicalTypes is enabled.
+  unsigned AlwaysIncludeTypeForNonTypeTemplateArgument : 1;
----------------
aaron.ballman wrote:
> DoDoENT wrote:
> > aaron.ballman wrote:
> > > dblaikie wrote:
> > > > DoDoENT wrote:
> > > > > dblaikie wrote:
> > > > > > What does `PrintCanonicalTypes` have to do with this? Does it overlap with this functionality in some way, but doesn't provide the functionality you want in particular?
> > > > > Thank you for the question. If you set the `PrintCanonicalTypes` to `false`, the `S<Point{1, 2}>` would be printed as `S<Point{1, 2}>` even without this patch. However, if you set it to `true`, it will be printed as `S<{1, 2}>`.
> > > > > 
> > > > > I don't fully understand why it does that, but it's quite annoying.
> > > > > 
> > > > > For a better example, please take a look at the `TemplateIdWithComplexFullTypeNTTP` unit tests that I've added: if `PrintCanonicalTypes` is set to `true`, the original print output of type is `NDArray<float, {{{0}}}, {{{0}}}, {{{0}}}>`, and if set to `false` (which is default), the output is `NDArray<float, Height{{{0}}}, Width{{{0}}}, Channels{{{0}}}>` - so the NTTP type is neither fully written nor fully omitted, which is weird.
> > > > > 
> > > > > As I said, I don't really understand the idea behind `PrintCanonicalTypes`, but when my new `AlwaysIncludeTypeForNonTypeTemplateArgument` is enabled, you will get the full type printed, regardless of value of `PrintCanonicalTypes` setting.
> > > > > 
> > > > Perhaps this might be more of a bug in PrintCanonicalTypes than something to add a separate flag for.
> > > > 
> > > > @aaron.ballman D55552 for context here... 
> > > > 
> > > > Hmm, actually, just adding the top level `Height{{0}}, Width{{0}}, Channels{{0}}` is sufficient to make this code compile (whereas with the `{{{0}}}` it doesn't form a valid identifier.
> > > > 
> > > > So what's your use case for needing more explicitness than that top level? 
> > > > Perhaps this might be more of a bug in PrintCanonicalTypes than something to add a separate flag for.
> > > >
> > > > @aaron.ballman D55552 for context here...
> > > 
> > > I looked over D55552 again and haven't spotted anything with it that seems amiss; the change there is to grab the canonical type before trying to print it which is all the more I'd expect `PrintCanonicalTypes` to impact.
> > > 
> > > This looks like the behavior you'd get when you desugar the type. Check out the AST dump for `s`: https://godbolt.org/z/vxh5j6qWr
> > > ```
> > > `-VarDecl <line:3:1, col:20> col:20 s 'S<Point{1, 2}>':'S<{1, 2}>' callinit
> > > ```
> > > We generate that type information at https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/TextNodeDumper.cpp#L663 for doing the AST dump, note how the second type printed is the desugared type and that matches what we're seeing from the pretty printer.
> > > So what's your use case for needing more explicitness than that top level?
> > 
> > As I described in the [github issue](https://github.com/llvm/llvm-project/issues/57562), I'm trying to write a clang-based tool that will have different behavior if the printed `{{{0}}}` is actually `Width` than if its `Height` or anything else.
> > 
> > You can see the the issue in the AST dump for `bla`: https://godbolt.org/z/fMr4f13o3
> > 
> > The type is
> > ```
> > `-VarDecl <line:20:1, col:21> col:21 bla 'NDArray<float, W>':'NDArray<float, {{{0}}}>' callinit
> >   `-CXXConstructExpr <col:21> 'NDArray<float, W>':'NDArray<float, {{{0}}}>' 'void () noexcept'
> > ```
> > 
> > so it's unknown whether `{{{0}}}` represent the `Width` or `Height`. My patch makes it work exactly like GCC (see the comparison of error message between [clang 15 and GCC 12.1](https://godbolt.org/z/WenWe8caf).
> > 
> > > Perhaps this might be more of a bug in PrintCanonicalTypes than something to add a separate flag for.
> > 
> > This was also my first thought and the first version of my patch (before even submitting it here) was to actually change the behavior of `PrintCanonicalTypes`. However, that change made some tests fail, as I described in the patch description:
> > 
> > - CodeGenCXX/debug-info-template.cpp
> > - SemaCXX/constexpr-printing.cpp
> > - SemaCXX/cxx2a-nttp-printing.cpp
> > - SemaTemplate/temp_arg_string_printing.cpp
> > 
> > Of course, it's possible to simply update the tests, but I actually don't fully understand what is the goal of `PrintCanonicalTypes` and whether its current behavior is actually desired or not, so I played it safe and introduced a new policy that is disabled by default until I get more feedback from more experienced LLVM developers.
> > 
> > The patch does solve my problem and since I'm building LLVM from source anyway, I can have it enabled in my fork.
> > 
> > I just want to see if it would also be beneficial to be introduced into the upstream LLVM.
> > Of course, it's possible to simply update the tests, but I actually don't fully understand what is the goal of PrintCanonicalTypes and whether its current behavior is actually desired or not, so I played it safe and introduced a new policy that is disabled by default until I get more feedback from more experienced LLVM developers.
> 
> `PrintCanonicalTypes` is what controls output like whether we print a typedef name (not the canonical type) or the final underlying type all typedefs involved (the canonical type). It won't have an impact on things like whether we print the name of a structure or not.
> 
> After looking into this, I think we do want changes here. I'm not 100% convinced we need a new policy member. I tried out printing the type names unconditionally and the results were a bit of a mixed bag (some diagnostics got more clear, other diagnostics didn't become more clear but also didn't become more confusing):
> ```
> error: 'note' diagnostics expected but not seen:
>   File F:\source\llvm-project\clang\test\SemaTemplate\temp_arg_nontype_cxx20.cpp Line 189: in instantiation of template class 'Diags::X<{1, 2}>' requested here
> error: 'note' diagnostics seen but not expected:
>   File F:\source\llvm-project\clang\test\SemaTemplate\temp_arg_nontype_cxx20.cpp Line 189: in instantiation of template class 'Diags::X<Diags::A{1, 2}>' requested here
> ```
> seems like a clarifying change, while:
> ```
> error: 'error' diagnostics expected but not seen:
>   File F:\source\llvm-project\clang\test\SemaCXX\cxx2a-nttp-printing.cpp Line 19: no member named 'display' in 'ASCII<{"this nontype template argument is [...]"}>'
>   File F:\source\llvm-project\clang\test\SemaCXX\cxx2a-nttp-printing.cpp Line 25: no member named 'display' in 'ASCII<{{119, 97, 105, 116, 32, 97, 32, 115, 27, 99, ...}}>'
>   File F:\source\llvm-project\clang\test\SemaCXX\cxx2a-nttp-printing.cpp Line 33: no member named 'display' in 'ASCII<{"what??!"}>'
> error: 'error' diagnostics seen but not expected:
>   File F:\source\llvm-project\clang\test\SemaCXX\cxx2a-nttp-printing.cpp Line 19: no member named 'display' in 'ASCII<Str<43>{"this nontype template argument is [...]"}>'
>   File F:\source\llvm-project\clang\test\SemaCXX\cxx2a-nttp-printing.cpp Line 25: no member named 'display' in 'ASCII<Str<14>{{119, 97, 105, 116, 32, 97, 32, 115, 27, 99, ...}}>'
>   File F:\source\llvm-project\clang\test\SemaCXX\cxx2a-nttp-printing.cpp Line 33: no member named 'display' in 'ASCII<Str<8>{"what??!"}>'
> ```
> seems like it's neither here nor there.
> 
> @dblaikie, do you have feelings on how to go with this?
Yeah, I'm inclined to think that `Diags::X<{1, 2}>` is just too simplified - it's unambiguous if the parameter isn't `auto`, but isn't valid syntax (so the language still expects a type name there) & so maybe we should do the same in diagnostics?

@aaron.ballman - though I'm still confused by the behavior-change when `PrintCanonicalTypes = false` that causes the top level names to be printed? Maybe that's just a weird case/red herring/bug in `PrintCanonicalTypes = true` that skips those top level names and we could print them unconditionally?

@DoDoENT - I was/am curious if/why you need more explicitness than would be provided by `PrintCanonicalTypes = false` - if the output was `NDArray<float, Height{{{0}}}, Width{{{0}}}, Channels{{{0}}}>` would that be sufficient for your needs? (I think in that case the name would be valid to use in code, which I think is a reasonable basis to make the decision - but I'm not sure how to justify adding all the intermediate names too)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453