[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:177
       return;
+    if (TP->isPointerType() || TP->isLValueReferenceType())
+      // When highlighting dependant template types the type can be a pointer or
----------------
ilya-biryukov wrote:
> jvikstrom wrote:
> > ilya-biryukov wrote:
> > > ilya-biryukov wrote:
> > > > jvikstrom wrote:
> > > > > ilya-biryukov wrote:
> > > > > > jvikstrom wrote:
> > > > > > > ilya-biryukov wrote:
> > > > > > > > jvikstrom wrote:
> > > > > > > > > ilya-biryukov wrote:
> > > > > > > > > > `RecursiveASTVisitor` also traverses the pointer and reference types, why does it not reach the inner `TemplateTypeParmType` in the cases you describe?
> > > > > > > > > The D in `using D = ...` `typedef ... D` does not have a TypeLoc (at least not one that is visited). Therefore we use the VisitTypedefNameDecl (line 121) to get the location of `D` to be able to highlight it. And we just send the typeLocs typeptr to addType (which is a Pointer for `using D = T*;`)...
> > > > > > > > > 
> > > > > > > > > But maybe we should get the underlying type before we call addType with TypePtr? Just a while loop on line 123 basically (can we have multiple PointerTypes nested in each other actually?)
> > > > > > > > > 
> > > > > > > > > Even if we keep it in addType the comment is actually wrong, because it obviously works when for the actual "type occurrences" for `D` (so will fix that no matter what). This recursion will just make us add more duplicate tokens...
> > > > > > > > Could we investigate why `RecursiveASTVisitor` does not visit the `TypeLoc` of a corresponding decl?
> > > > > > > > Here's the code from `RecursiveASTVisitor.h` that should do the trick:
> > > > > > > > ```
> > > > > > > > DEF_TRAVERSE_DECL(TypeAliasDecl, {
> > > > > > > >   TRY_TO(TraverseTypeLoc(D->getTypeSourceInfo()->getTypeLoc()));
> > > > > > > >   // We shouldn't traverse D->getTypeForDecl(); it's a result of
> > > > > > > >   // declaring the type alias, not something that was written in the
> > > > > > > >   // source.
> > > > > > > > })
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > If it doesn't, we are probably holding it wrong.
> > > > > > > There just doesn't seem to be a TypeLoc for the typedef'ed Decl.  We can get the `T*` TypeLoc (with `D->getTypeSourceInfo()->getTypeLoc()`). But there isn't one for `D`. Even the `D->getTypeForDecl` returns null.
> > > > > > > 
> > > > > > > And I have no idea where I'd even start debugging that. Or if it's even a bug
> > > > > > > 
> > > > > > I may have misinterpreted the patch. Are we trying to add highlightings for the names of using aliases here? E.g. for the following range:
> > > > > > ```
> > > > > > template <class T>
> > > > > > struct Foo {
> > > > > >   using [[D]] = T**;
> > > > > > };
> > > > > > ```
> > > > > > 
> > > > > > Why isn't this handled in `VisitNamedDecl`?
> > > > > > We don't seem to call this function for `TypedefNameDecl` at all and it actually weird. Is this because we attempt to highlight typedefs as their underlying types?
> > > > > So currently using aliases and typedefs are highlighted the same as the underlying type (in most cases). One case where they aren't is when the underlying type is a template parameter (which is what this patch is trying to solve).
> > > > > 
> > > > > 
> > > > > > Why isn't this handled in VisitNamedDecl?
> > > > > 
> > > > > The Decl is actually visited in `VisitNamedDecl`, however as it is a `TypeAliasDecl` which we do not have a check for in the addToken function it will not get highlighted in that visit.
> > > > > 
> > > > > Actually, could add a check for `TypeAliasDecl` in `addToken` (should probably be a check for `TypedefNameDecl` to cover both `using ...` and `typedef ...`) and move the code from the `VisitTypedefNameDecl` to the `addToken` function inside that check instead.
> > > > > 
> > > > > 
> > > > > 
> > > > > > We don't seem to call this function for TypedefNameDecl at all and it actually weird. Is this because we attempt to highlight typedefs as their underlying types?
> > > > > 
> > > > > 
> > > > > Don't understand what you mean. What function? 
> > > > > So currently using aliases and typedefs are highlighted the same as the underlying type (in most cases). 
> > > > Thanks for clarifying this. This is where my confusion is coming from.
> > > > A few question to try understanding the approach taken (sorry if that's too detailed, I am probably missing the context here)
> > > > - What do we fallback to? From my reading of the code, we do not highlight them at all if the underlying type is not one of the predefined cases.
> > > > - Why are pointers and **l-value** references special? What about arrays, r-value references, function types, pack expansions, etc.?
> > > > 
> > > > > Don't understand what you mean. What function?
> > > > We don't call `VisitNamedDecl` from `VisitTypedefNameDecl`. I guess that's intentional if we try to highlight them as underlying types.
> > > Summarizing the offline discussion:
> > > - we chose to highlight typedefs same as their underlying type (e.g. if it's a class we highlight the typedef as class too)
> > > - we need to ensure all typdefs are highlighted in **some** color. That means we need to handle all composite types in this function and recurse into those. That involves handling at least functions, arrays, all reference types (not just l-value references).
> > > - we should add tests for this.
> > > 
> > > @jvikstrom, please let me know if I missed something.
> > > we chose to highlight typedefs same as their underlying type (e.g. if it's a class we highlight the typedef as class too)
> > 
> > So this version should do that. For the typedef part.
> > 
> > When there are references with typedefs `SomeTypedefTypeThatIsNotBuiltinAndHasNoTagDecl R = ...` this will not highlight the `SomeTypedefTypeThatIsNotBuiltinAndHasNoTagDecl` because RecursiveASTVisitor does not traverse into typedeftypelocs, so we'd need to add the getUnderlyingType thing in VisitTypeLoc as well.
> > Also need to do this for auto.
> > 
> > And, does it make sense to highlight function typedefs as their return type?
> > 
> > 
> > But I'm a bit worried this current approach might not be really sensible.
> > The 'short circuit'  when we find a TagDecl is because otherwise we get weird results at times. (for example TemplateInstantiations, if instantiated with a builtin we'd highlight that as a Primitive while it should be a class because of traversal order)
> > 
> > I guess it would be possible to create a TypeVisitor kind of like how you get deduced types from a typeptr, but that would be a lot of boilerplate.
> > 
> > Maybe what we could do instead of stopping the traversal early when finding a TagDecl is add one field for each Leaf type in UnderlyingTypeVisitor. When returning we'd then return the Record if there is one or Enum if there is one, ... , or a Builtin if none of the others exist.
> > 
> > Is there maybe some other way of finding the "correct" type for a type hierarchy or maybe doing the very basic ranking would be enough? (Record > Enum > TemplateTypeParm > ObjC stuff > Builtin)
> > 
> > And, does it make sense to highlight function typedefs as their return type?
> I also think it's confusing. But we need to fallback to something if we see a function type (or some other unhandled type) and I believe we 
> currently just don't highlight those typedefs at all, which seems bad.
> 
> I would just have a separate highlighting kind for typedefs (regardless of what the underlying type is).
> This would allow to avoid both technical problems (simplifying the implementation) and logical questions (e.g. 'should we highlight typedefs to functions as their return type?)
> I would just have a separate highlighting kind for typedefs (regardless of what the underlying type is).

FWIW, this is what Eclipse CDT does.

(Although clangd's current choice to highlight typedefs based on their target type is certainly an interesting one. From a user's point of view I can see arguments in either direction. On the one hand, revealing information about the target type is useful. On the other hand, knowing that a type //is// an alias for something else can also be useful.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66516