[PATCH] D142871: [clangd] Semantic highlighting for constrained-parameter

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:720
+      // Pick up the name via VisitNamedDecl
+      Base::VisitTemplateTypeParmDecl(D);
+    }
----------------
kadircet wrote:
> nridge wrote:
> > Am I using the API correctly here? It's a bit weird that `ConstDeclVisitor` would work differently from `RecursiveASTVisitor` in that VisitXXX methods for super-classes are not automatically called
> right, non-recursive ast visitors are mere helpers for visiting most specific type of an ast node which is interesting to the visitor (i.e. visit method is overridden), and only that. they also don't traverse children of an entitie's entries.
> 
> which usually hints towards this being the wrong place to handle such constructs. we actually have an outer recursiveastvisitor, that tries to visit each children. but concept references seem to be missing from there.
> taking a look at the RecursiveASTVisitor pieces around this piece, we actually do visit the children appropriately:
> - https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/RecursiveASTVisitor.h#L1931 calls traverse on the constraint
> - https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/RecursiveASTVisitor.h#L1923 jumps into type constraint
> - https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/RecursiveASTVisitor.h#L510 some more indirection
> - https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/RecursiveASTVisitor.h#L547 calls visit for the decl name
> - https://github.com/llvm/llvm-project/blob/clang/include/clang/AST/RecursiveASTVisitor.h#L830 unfortunately we stop here, because all we store is an identifier.
> 
> We should probably figure out how to properly visit type constraints inside the RecursiveASTVisitor (i guess we should actually be visiting TypeConstraints). @usaxena95 who's looking at C++20 features and their interactions with source tooling.
> 
> In the meanwhile, i think we should have this specialization at the outer layer, in `ExplicitReferenceCollector`, with something like:
> ```
> bool TraverseTemplateTypeParamDeclConstraints(TemplateTypeParmDecl *TTPD) {
>   if (auto *TC = TTPD->getTypeConstraint()) {
>     reportReference(ReferenceLoc{TC->getNestedNameSpecifierLoc(),
>                                  TC->getConceptNameLoc(),
>                                  /*IsDecl=*/false,
>                                  {TC->getNamedConcept()}},
>                     DynTypedNode::create(*TTPD));
>     return RecursiveASTVisitor::TraverseTypeConstraint(TC);
>   }
>   return true;
> }
> ```
> https://github.com/llvm/llvm-project/blob/clang/include/clang/AST/RecursiveASTVisitor.h#L830 unfortunately we stop here, because all we store is an identifier.

This isn't unfortunate, this is where we would handle things internal to the name (e.g. if the name was `operator vector<int>()`) rather than what the name is used for.

The right level to handle this seems to be a missing `TraverseConceptReference()` which observes the lexical reference to a concept (we don't care here that it's a type constraint specifically).
Unfortunately this doesn't exist: we have `TraverseConceptReferenceHelper()` which is private and called nonpolymorphically.

Renaming this to `TraverseConceptReference()` and making it CRTP-overridable seems like the principled solution at the RAV level. Maybe there's some reason it wasn't done this way to start with though.

Failing that, overriding `TraverseTypeConstraint()` in our RAV subclass seems like it fits the pattern more neatly (there's no VisitTypeConstraint, so we have to override Traverse and call Base::Traverse). We're going to have to handle the other ways concepts can be referenced of course, but changing RAV vs working around its limitations is always a tradeoff of practicality.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142871