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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov 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:
> > > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66516