[PATCH] D134453: Disambiguate type names when printing NTTP types

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D134453#3877127 <https://reviews.llvm.org/D134453#3877127>, @mizvekov wrote:

> I think the type printer's purpose is for printing types for diagnostics, not for generating a lossless representation of types as strings, as the ast dumper does.
> So packing so much information in them that hurts readability would be against that purpose.
> I do agree with everyone here that the provided example is one where we are unhelpful and provide too little information.
>
> When printing types for diagnostics we roughly want to:
>
> 1. Point the user to the relevant type which is being diagnosed, as in the source code. When we have a TypeLoc, and that would otherwise be more presentable for the given diagnostic, we can point to the type with a caret. Otherwise, we try to print the type as closely as we can to what was written, so that the user can connect the diagnostic with something that was done in source code.
> 2. Give some sort of explanatory analysis, revealing implicit / context sensitive information which might be relevant.
>
> I think that for the motivating example, we completely fail to provide 1), we don't have a TypeLoc, and we only have a canonical type.
> I think fixing 1) is the most pressing issue, and is not addressed by this patch.
> Ideally, long term, we can remove all instances where we try to print canonical types for diagnostic purposes, and not have the type printer deal with that, simplifying things.
>
> For the short term, we have canonical types, and we need the type printer to deal with them reasonably.
> I agree that the way MSVC is presenting these canonicalized NTTPs of class type looks the most sensible here.

Could you describe this in more detail? I find that to be more verbose than is necessary/helpful and probably the variable name based option to be more informative than the type name based option (if I have `Shape{3, 5}` (because I have two int members) that's less helpful than `Shape{.length = 3, .width = 5}` and all the type information seems pretty large/verbose.

& given the example:

  NDArray<float, Height{DimensionImpl<Height>{Dimension{0}}}, Width{DimensionImpl<Width>{Dimension{0}}}, Channels{DimensionImpl<Channels>{Dimension{0}}}>

V

  NDArray<float, Height{{{0}}}, Width{{{0}}}, Channels{{{0}}}>

The former seems to not add a lot of value for the user - in this case due to the CRTP and other shenanigans. Admittedly other types might have nested aggregates that are more interesting/less obvious - but variable names still seem to me like they'd be more informative than types. (what I'm suggesting is the outermost has a type (since there's nothing else to use there, and C++ requires a type there) and nested things could use variable names (because they're more contextually relevant - differentiating a `width` and `height`, for instance, even though they might be the same type))

> But by fixing 1), we have more options. We can do something like:
>
>   no known conversion from 'NDArray<[...], W (aka 'Width{}')>' to 'const NDArray<[...], H (aka 'Height{}')>' for 1st argument
>
> Or perhaps in addition to the aka, we can generate notes which place a caret in source code for each of W and H.

That feels liable to be /really/ verbose & make it harder to find the signal in all that noise, but maybe there's ways to make that work. Certainly having the type as written gives us more choices about how to do it/see what can be made to work, for sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453