[clangd] Semantic highlighting for constrained-parameter

[nridge]

clang-tools-extra/clangd/FindTarget.cpp

719

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

[kadircet]

clang-tools-extra/clangd/FindTarget.cpp

719

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;
}

[sammccall]

clang-tools-extra/clangd/FindTarget.cpp

719

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.

[nridge]

Address review comment

[nridge]

Also filed https://github.com/llvm/llvm-project/issues/60469 for the seemingly missing RecursiveASTVisitor methods

[kadircet]

clang-tools-extra/clangd/FindTarget.cpp

1047

can you also add a comment like // We actually want to handle all ConceptReferences but RAV doesn't traverse it polymorphically. So handle the ones inside TypeConstraints specially here.

[nridge]

address review comment

clang-tools-extra/clangd/FindTarget.cpp

llvm::SmallVector<ReferenceLoc> refInDecl(const Decl *D,
 
   Visitor V{Resolver};
   V.Visit(D);
   return V.Refs;
 }
 
 llvm::SmallVector<ReferenceLoc> refInStmt(const Stmt *S,
                                           const HeuristicResolver *Resolver) {
   struct Visitor : ConstStmtVisitor<Visitor> {
     Visitor(const HeuristicResolver *Resolver) : Resolver(Resolver) {}
 
     const HeuristicResolver *Resolver;
     // FIXME: handle more complicated cases: more ObjC, designated initializers.
     llvm::SmallVector<ReferenceLoc> Refs;
 
     void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E) {
       Refs.push_back(ReferenceLoc{E->getNestedNameSpecifierLoc(),
bool TraverseObjCProtocolLoc(ObjCProtocolLoc ProtocolLoc) {
     return true;
   }
 
   bool TraverseConstructorInitializer(CXXCtorInitializer *Init) {
     visitNode(DynTypedNode::create(*Init));
     return RecursiveASTVisitor::TraverseConstructorInitializer(Init);
   }
 
+  bool TraverseTypeConstraint(const TypeConstraint *TC) {
+    // We want to handle all ConceptReferences but RAV is missing a
+    // polymorphic Visit or Traverse method for it, so we handle
+    // TypeConstraints specially here.
+    Out(ReferenceLoc{TC->getNestedNameSpecifierLoc(),
+                     TC->getConceptNameLoc(),
+                     /*IsDecl=*/false,
+                     {TC->getNamedConcept()}});
+    return RecursiveASTVisitor::TraverseTypeConstraint(TC);
+  }
+
 private:
   /// Obtain information about a reference directly defined in \p N. Does not
   /// recurse into child nodes, e.g. do not expect references for constructor
   /// initializers
   ///
   /// Any of the fields in the returned structure can be empty, but not all of
   /// them, e.g.
   ///   - for implicitly generated nodes (e.g. MemberExpr from range-based-for),

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

$Bracket[[>]] $LocalVariable_def[[s7]];
       // Brackets support: Concepts
       R"cpp(
         template $Bracket[[<]]typename $TemplateParameter_def[[T]]$Bracket[[>]]
         concept $Concept_decl[[C]] = true;
         template $Bracket[[<]]typename $TemplateParameter_def[[T]]$Bracket[[>]] requires $Concept[[C]]$Bracket[[<]]$TemplateParameter[[T]]$Bracket[[>]]
         class $Class_def[[Z]] {};
         template $Bracket[[<]]typename, typename$Bracket[[>]]
         concept $Concept_decl[[C2]] = true;
-        template $Bracket[[<]]C2$Bracket[[<]]int$Bracket[[>]] $TemplateParameter_def[[A]]$Bracket[[>]]
+        template $Bracket[[<]]$Concept[[C2]]$Bracket[[<]]int$Bracket[[>]] $TemplateParameter_def[[A]]$Bracket[[>]]
         class $Class_def[[B]] {};
       )cpp",
       // no crash
       R"cpp(
         struct $Class_def[[Foo]] {
           void $Method_decl[[foo]]();
         };