[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.
Ilya Biryukov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 30 03:11:43 PDT 2019
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
----------------
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?)
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:25
+/// Gets an underlying type in a type hierarchy. Should be used to find the type
+/// in a hieracrchy when the HighlightingTokenCollector's RecursiveASTVisitor
----------------
The comment is a bit complicated, would it be fair to say this is trying to
`find the type that should be used to highlight a typedef`?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:31
+ : public RecursiveASTVisitor<UnderlyingTypeVisitor> {
+ const Type *Underlying;
+
----------------
We need to initialize `Underlying` to `nullptr`
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:38
+ /// If neither are found returns the type ptr.
+ const Type *getUnderlyingType(QualType T) {
+ Underlying = &(*T);
----------------
Could you make this function static or define it out of class?
That would allow the clients to avoid writing the boilerplate of creating the visitor
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:39
+ const Type *getUnderlyingType(QualType T) {
+ Underlying = &(*T);
+ TraverseType(T);
----------------
NIT: maybe simplify to `T.getTypePtr()`?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:58
+ bool TraverseMemberPointerType(MemberPointerType *T) {
+ return RecursiveASTVisitor<UnderlyingTypeVisitor>::TraverseType(
+ T->getPointeeType());
----------------
NIT: no need to specify template arguments to the base class, just use `RecursiveASTVisitor::TraverseType()`
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:64
+ bool TraverseDecltypeType(DecltypeType *T) {
+ return RecursiveASTVisitor<UnderlyingTypeVisitor>::TraverseType(
+ T->getUnderlyingType());
----------------
Despite our best efforts, `RecursiveASTVisitor` still traverses extra stuff, e.g. array sizes
```
template <class T>
struct X{
using foo = int a[sizeof(T)]; // will probably get 'T' now.
}
```
Using the recursive ast visitor will lead to hard-to-detect bugs because it visits too much. It's much simpler to write a function that will possibly visit too little, but won't visit the things we don't want to visit:
```
const Type* getTypeForHighlighting(QualType T) {
while (true) {
if (T.isAnyPointerType())
T = T.getPointeeType();
else if (T.isReferenceType())
T = T.getReferencedType();
else if (T.isArrayType())
T = T.getArrayElementType();
else if (T.isFunctionType())
T = T.getReturnType();
else if (T.isTypedefType())
T = T.getUnderlyingTypedefType();
else if (T.isDecltypeType())
T = T.getAs<DecltypeType>().getExpression().getType(); // have to careful here, can it loop forever?
else
break;
}
}
```
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:193
if (const auto *TSI = TD->getTypeSourceInfo())
- addType(TD->getLocation(), TSI->getTypeLoc().getTypePtr());
- return true;
- }
-
- bool VisitTemplateTypeParmTypeLoc(TemplateTypeParmTypeLoc &TL) {
- // TemplateTypeParmTypeLoc does not have a TagDecl in its type ptr.
- addToken(TL.getBeginLoc(), TL.getDecl());
+ addType(TD->getLocation(), UnderlyingTypeVisitor().getUnderlyingType(
+ TSI->getTypeLoc().getType()));
----------------
Could we signal when `addType` cannot produce a highlighting?
E.g. with a `vlog()`? So that it's easy to detect if we missed something in practice.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:252
+ if (const TemplateTypeParmType *TD = dyn_cast<TemplateTypeParmType>(TP))
+ // TemplateTypeParmType also do not have a TagDecl.
+ addToken(Loc, TD->getDecl());
----------------
Why do we consider all non-tag-decl types as corner-cases?
The non-tag-decl cases are actually much more common: only classes, structs unions and enums are tag decls, all other non-composite types are not (including builtins, template types, many dependent types, etc)
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:19
namespace clangd {
+void PrintTo(const HighlightingToken &T, ::std::ostream *OS) {
+ *OS << "(" << T.R.start.line << ", " << T.R.start.character << ") -> (" << T.R.end.line << ", " << T.R.end.character << "): " << (int)T.Kind;
----------------
Just define a stream output operator for `HighlightingToken`.
And we should do this inside the file where `HighlightingToken` is declared to avoid potential ODR violations (e.g. if two tests define the same function, we are in trouble)
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:458
+ using $TemplateParameter[[MemberT]] =
+ $TemplateParameter[[T]]*& (T::*)($Class[[A]]);
+ };
----------------
Could you also add checks that usages of these typedefs are highlighted in the same way as their declarations?
Given that we do not call `getUnderlyingType()` for those, I suspect they won't be. Perhaps I'm missing something
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66516/new/
https://reviews.llvm.org/D66516
More information about the cfe-commits
mailing list