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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D134453#3828352 <https://reviews.llvm.org/D134453#3828352>, @dblaikie wrote:

> In D134453#3826832 <https://reviews.llvm.org/D134453#3826832>, @aaron.ballman wrote:
>
>> In D134453#3825902 <https://reviews.llvm.org/D134453#3825902>, @dblaikie wrote:
>>
>>>>>>> Again, I'm not advocating for the printing as-is, I think adding the top level name that disambiguates would be a good thing - and I think the GCC and MSVC examples somewhat show why adding all the other layers would be harmful to readability - there's a lot of text in those messages that doesn't directly help the user and gets in the way of identifying the important differences between the type names.
>>>>>>
>>>>>> I think this is a matter of taste. In the example that you've shown, I personally prefer the verbosity of GCC and don't see it as "less readable", but as "more informative". However, I do understand that some people may prefer the output you suggested. What about making this configurable, i.e. behind some clang flag, so that developers that prefer verbosity can enable that?
>>>>>
>>>>> I don't think that's the right choice for clang - though I'm not the only decider here. Be great to hear from @aaron.ballman at some point.
>>>>>
>>>>> My perspective on this issue at the moment is that we should fix any case where we print an ambiguous type (so `template<auto A> struct t1;` should never produce `t1<{}>` and should instnead produce `t1<t2{}>`) - but I don't think extra {} should be added where the language doesn't require it and where it isn't necessary to disambiguate.
>>>>
>>>> My thinking is along a somewhat different line of approach. Clang has `-ast-print` which is intended to pretty print the AST back to the original source code without losing information. It is a `-cc1` option and not a driver option (though we do document it a little bit here https://releases.llvm.org/15.0.0/tools/clang/docs/HowToSetupToolingForLLVM.html), so it's not something we expose trivially to users. This option has always been aspirational -- we'd love it to pretty print back to the original source, but we know there are plenty of cases where we don't manage it and we typically don't require people to maintain it except superficially. Now that we have clang-format, I think its utility is largely theoretical in some ways. Despite its known issues, I am aware of at least some users who make use of the functionality (for various reasons) and I am not 100% convinced we'll get community agreement to drop support for it as a failed experiment despite my personal feelings that the functionality is more trouble than it's worth, especially because I'm reasonably certain some other functionality relies on it (ARC rewriting maybe?) and we'd have to figure out what to do with that.
>>>>
>>>> So my thinking is: if we're going to have `-ast-print`, we should maintain it, and part of maintaining it means having the ability for it to print this code back to compilable source with the same semantics as the original source. Our current behavior with the example is... not particularly fantastic: https://godbolt.org/z/88eK3q869. Alternatively, we could remove `-ast-print` as an option and narrow its focus to only the internal uses we need it for.
>>>
>>> Hmm - that seems to be to be somewhat of a tangent, though? I think the claim that `template<auto> struct t1; struct t2 { }; t1<t2{}> v1;` and then rendering `t1<{}>` in a diagnostic is wrong is solid, and we should fix that for auto parameters to include the type info that'd be necessary in the source code/to disambiguate. (I guess C++20 actually requires the name even in non-ambiguous, non-auto cases - so I'd be OK leaning towards the syntax requirement, even if it's not necessary to disambiguate (maybe there are cases where it is?).
>>>
>>> You're suggesting that -ast-print should respect the type as-written by the user, even? Not just "enough to be valid C++ code/disambiguate" - and that mode might be enough to support @DoDoENT's use case using strings for type information? (more addressing this feature request, less about the incidental diagnostic quality bug report that's a side issue in this discussion)
>>
>> I'm mostly looking at it from the perspective of "is it reasonable to add a new printing policy here and if so, what do we need from it" first and then "when is it reasonable to enable that printing policy". I think the presence of `-ast-print` answers the first question that we probably do need *some* sort of printing policy here because we believe we have at least two scenarios to support: print for a diagnostic and print for the pretty printer.
>
> Ah, OK - I'm not sure that use case motivates a different printing mode, though - skipping to the bottom to continue this thought.
>
>> We might even find we want a verbosity level instead of a boolean switch: print minimal type info, print "disambiguating" type info, print all type info, though it sort of sounds to me like we only need two modes (disambiguating mode for diagnostics and all type info for pretty printing/@DoDoENT).
>>
>> (I think the original goal of `-ast-print` was to respect the code as-written by the user, but I am not convinced we need that strict of a goal. I think "produces code which compiles and has the same semantics as the original source" is a more reasonable goal and once we can do that much we can think about improving fidelity more if we need to.)
>
> OK, so it sounds like the printing behavior change (not necessarily with a policy flag) necessary for diagnostics (make it non-ambiguous/not invalid code which is what is currently emitted) would be a good bug fix both for diagnostics and for ast-print? I'm not sure we need anything other than that.

Agreed, fixing up diagnostics and ast print to not print invalid types would be a good fix to have. It doesn't necessitate a new printing policy.

> So that what is currently printing as `t1<{}>` (ambiguous if the NTTP is of type `auto`, non-ambiguous, but invalid C++ if the NTTP has the specific type already) prints out as `t1<t2{}>` (correct C++ and disambiguates in the case of `auto` ambiguity).
>
> Or would you want the non-auto case to print in diagnostics as `t1<{}>` since there's no ambiguity, despite that not being valid C++? And then have a separate policy for ast-print that still prints it as `t1<t2{}>` so it's valid code?

Personally (and others are welcome to disagree with me here!), I think we would want to print `t1<t2{}>` even in the `auto` case for diagnostics and ast print. For diagnostics, we sometimes print type information reasonably far away from where the declaration of a type is (and use a note to shift the user's gaze to the related type), and so I think having the extra type information is useful for that situation. But even when we print the type information reasonably close to the declaration of the type, it can be helpful because the code context might be easy to lose -- consider using Clang from an IDE where diagnostics are added to a listbox; if you copy the diagnostic from the listbox to paste into an email to a coworker to ask about it, the related code isn't going to come along without extra intervention.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453