[clang] Fix visitation of ConceptSpecializationExpr in constrained-parameter

[nridge]

Summary

RecursiveASTVisitor needs to traverse TypeConstraint::ImmediatelyDeclaredConstraint

[kadircet]

just some drive-by comments.

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

457

Flags is preserved between EXPECT_DECLS calls, so either use assignment or set it only once at the beginning of the test.

clang/include/clang/AST/ExprConcepts.h

132 ↗

(On Diff #279135)

i think we should have some tests in clang, at least an ast-dump test in clang/test/AST/ (for both cases) and possibly also in clang/unittests/AST/SourceLocationTest.cpp

[hokein]

as you mentioned in the description, there are two problems, I'd prefer address them in two different patches.

clang/include/clang/AST/ExprConcepts.h

132 ↗

(On Diff #279135)

+1, ast-dump should be enough to test the invalid end loc, I have a D84461 to slightly improve the dump.

clang/include/clang/AST/RecursiveASTVisitor.h

1780

Looks like we may visit some nodes in ConceptReference twice:

• getImmediatelyDeclaredConstraint returns a ConceptSpecializationExpr (most cases?) which is a subclass of ConceptReference;
• TraverseStmt(ConceptSpecializationExpr*) will dispatch to TraverseConceptSpecializationExpr which invokes TraverseConceptReference (see Line 2719);

It is sad that we don't have enough test coverage, could you write some tests in clang/unittests/Tooling/RecursiveASTVisitorTests/?

[nridge]

clang/include/clang/AST/ExprConcepts.h

132 ↗

(On Diff #279135)

Will do. By the way, is there something more tailored than ninja check-clang to run these ast-dump tests? ninja check-clang takes quite a while to run...

[nridge]

Spun off the source-location issue to D84613.

Some other review comments remain to be addressed.

[nridge]

clang/include/clang/AST/RecursiveASTVisitor.h

1780

It is true that there will be two calls to TraverseConceptReference(). However, they are called on two different ConceptReference objects:

• the call in TraverseConceptSpecializationExpr will visit the base subobject of the ConceptSpecializationExpr (which inherits from ConceptReference)
• the call in TraverseTemplateTypeParmDecl will visit the base subobject of the TypeConstraint (which also inherits from ConceptReference).

So, I think this is fine -- there are two distinct ConceptReference objects in the AST, and with this patch we visit both of them.

[nridge]

Finish addressing review comments

[nridge]

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

435

I'd like to keep this clangd-specific test coverage as I think it's valuable to have. However, I can spin it off into a separate patch if you prefer.

[riccibruno]

Will do. By the way, is there something more tailored than ninja check-clang to run these ast-dump tests? ninja check-clang takes quite a while to run...

You can use check-clang-ast for the lit tests in AST/, check-clang-sema for the lit tests in Sema/ and so on. Or you can run an individual test with llvm-lit ~/path/to/the/lit/test.

[nridge]

(This should be ready for another round of review.)

[hokein]

clang/include/clang/AST/RecursiveASTVisitor.h

1780

I understand that they are two different ConceptReference objects, but they have members (FoundDecl, ArgsAsWritten) that may refer to the same AST nodes.

template <typename T, typename U>
concept binary_concept = true;
struct Foo {};

template<binary_concept<Foo> T> // the template argument Foo will be visited twice.
void k2();

I'm not sure what's is the right approach here, I can see two options:

• traverse TC + immediately-declared-constraint expr, this seem to cause some ast nodes visited twice (maybe not a big deal?)
• just traverse immediately-declared-constraint expr, this seems not breaking any tests, but the immediately-declared-constraint expr could be nullptr (e.g. broken code, missing required template arguments); or the immediately-declared-constraint expr could be a CXXFoldExpr, which will make some members in ConceptReference not be visited;

@rsmith, do you have any idea about this?

[nridge]

Rebase on top of D85108

[nridge]

clang/include/clang/AST/RecursiveASTVisitor.h

1780

From clangd's point of view, it would be sufficient to visit the immediately-declared-constraint-expr without visiting any of its descendants. However, I'm not sure how to accomplish this using RecursiveASTVisitor. (I think I'd want to call WalkUpFromXXX(TC->getImmediatelyDeclaredConstraint()), where XXX is the dynamic type of the immediately-delcared-constraint, but I don't know how to dispatch to that dynamic type; RecursiveASTVisitor seems to be designed to do the dispatch on Traverse calls, not WalkUpFrom calls).

[hokein]

clang/include/clang/AST/RecursiveASTVisitor.h

1780

Thinking more about this.

I'm aligned on the idea of only traversing the immediately declared constraint since the TypeContraint is a wrapper of it. The regression is that we'd not perform traversal if the immediately declared constraint is nullptr (this just happens for broken code).

Fix ideas:

• build a recovery expression if we can't not build a immediately declared constraint due to the missing required template arguments etc;
• still traverse the TC if immediately declared constraint is nullptr;

[nridge]

Only visit the TypeConstraint if the immediately-declared-constraint is null

[nridge]

Update test

[nridge]

clang/include/clang/AST/RecursiveASTVisitor.h

1780

Thanks, this suggestion makes sense to me. I went with the second approach.

[hokein]

Thanks!

clang/include/clang/AST/RecursiveASTVisitor.h

1781

nit: just

if (Expr *IDC = ...) {
  ...
} else {
    TRY_TO(TraverseConceptReference(*TC));
}

clang/unittests/Tooling/RecursiveASTVisitorTests/Concept.cpp

3

nit: should be merged in the above line.

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

Code = R"cpp(
     void bar(T t) {
       t.foo();
     }
   )cpp";
   EXPECT_DECLS(
       "ConceptSpecializationExpr",
       {"template <typename T> concept Fooable = requires (T t) { t.foo(); };"});
 
   // trailing requires clause
   Code = R"cpp(
       template <typename T>
       concept Fooable = true;
 
       template <typename T>
       void foo() requires [[Fooable]]<T>;
   )cpp";
   EXPECT_DECLS("ConceptSpecializationExpr",
                {"template <typename T> concept Fooable = true;"});
+
+  // constrained-parameter
+  Code = R"cpp(
+    template <typename T>
+    concept Fooable = true;
+
+    template <[[Fooable]] T>
+    void bar(T t);
+  )cpp";
+  EXPECT_DECLS("ConceptSpecializationExpr",
+               {"template <typename T> concept Fooable = true;"});
+
+  // partial-concept-id
+  Code = R"cpp(
+    template <typename T, typename U>
+    concept Fooable = true;
+
+    template <[[Fooable]]<int> T>
+    void bar(T t);
+  )cpp";
+  EXPECT_DECLS("ConceptSpecializationExpr",
+               {"template <typename T, typename U> concept Fooable = true;"});
 }
 
 TEST_F(TargetDeclTest, FunctionTemplate) {
   Code = R"cpp(
     // Implicit specialization.
     template<typename T> bool foo(T) { return false; };
     bool x = [[foo]](42);
   )cpp";

clang/include/clang/AST/RecursiveASTVisitor.h

 DEF_TRAVERSE_DECL(BuiltinTemplateDecl, {
   TRY_TO(TraverseTemplateParameterListHelper(D->getTemplateParameters()));
 })
 
 DEF_TRAVERSE_DECL(TemplateTypeParmDecl, {
   // D is the "T" in something like "template<typename T> class vector;"
   if (D->getTypeForDecl())
     TRY_TO(TraverseType(QualType(D->getTypeForDecl(), 0)));
-  if (const auto *TC = D->getTypeConstraint())
+  if (const auto *TC = D->getTypeConstraint()) {
+    if (Expr *IDC = TC->getImmediatelyDeclaredConstraint()) {
+      TRY_TO(TraverseStmt(IDC));
+    } else {
+      // Avoid traversing the ConceptReference in the TypeCosntraint
+      // if we have an immediately-declared-constraint, otherwise
+      // we'll end up visiting the concept and the arguments in
+      // the TC twice.
       TRY_TO(TraverseConceptReference(*TC));
+    }
+  }
   if (D->hasDefaultArgument() && !D->defaultArgumentWasInherited())
     TRY_TO(TraverseTypeLoc(D->getDefaultArgumentInfo()->getTypeLoc()));
 })
 
 DEF_TRAVERSE_DECL(TypedefDecl, {
   TRY_TO(TraverseTypeLoc(D->getTypeSourceInfo()->getTypeLoc()));
   // We shouldn't traverse D->getTypeForDecl(); it's a result of
   // declaring the typedef, not something that was written in the

clang/unittests/Tooling/CMakeLists.txt

add_clang_unittest(ToolingTests
   HeaderIncludesTest.cpp
   LexicallyOrderedRecursiveASTVisitorTest.cpp
   LookupTest.cpp
   QualTypeNamesTest.cpp
   RangeSelectorTest.cpp
   RecursiveASTVisitorTests/Attr.cpp
   RecursiveASTVisitorTests/Callbacks.cpp
   RecursiveASTVisitorTests/Class.cpp
+  RecursiveASTVisitorTests/Concept.cpp
   RecursiveASTVisitorTests/ConstructExpr.cpp
   RecursiveASTVisitorTests/CXXBoolLiteralExpr.cpp
   RecursiveASTVisitorTests/CXXMemberCall.cpp
   RecursiveASTVisitorTests/CXXMethodDecl.cpp
   RecursiveASTVisitorTests/CXXOperatorCallExprTraverser.cpp
   RecursiveASTVisitorTests/DeclRefExpr.cpp
   RecursiveASTVisitorTests/ImplicitCtor.cpp
   RecursiveASTVisitorTests/ImplicitCtorInitializer.cpp

clang/unittests/Tooling/RecursiveASTVisitorTests/Concept.cpp

+//===- unittest/Tooling/RecursiveASTVisitorTests/Concept.cpp----------------==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TestVisitor.h"
+#include "clang/AST/ExprConcepts.h"
+
+using namespace clang;
+
+namespace {
+
+struct ConceptVisitor : ExpectedLocationVisitor<ConceptVisitor> {
+  bool VisitConceptSpecializationExpr(ConceptSpecializationExpr *E) {
+    ++ConceptSpecializationExprsVisited;
+    return true;
+  }
+  bool TraverseConceptReference(const ConceptReference &R) {
+    ++ConceptReferencesTraversed;
+    return true;
+  }
+
+  int ConceptSpecializationExprsVisited = 0;
+  int ConceptReferencesTraversed = 0;
+};
+
+TEST(RecursiveASTVisitor, ConstrainedParameter) {
+  ConceptVisitor Visitor;
+  EXPECT_TRUE(Visitor.runOver("template <typename T> concept Fooable = true;\n"
+                              "template <Fooable T> void bar(T);",
+                              ConceptVisitor::Lang_CXX2a));
+  // Check that we visit the "Fooable T" template parameter's TypeConstraint's
+  // ImmediatelyDeclaredConstraint, which is a ConceptSpecializationExpr.
+  EXPECT_EQ(1, Visitor.ConceptSpecializationExprsVisited);
+  // There are two ConceptReference objects in the AST: the base subobject
+  // of the ConceptSpecializationExpr, and the base subobject of the
+  // TypeConstraint itself. To avoid traversing the concept and arguments
+  // multiple times, we only traverse one.
+  EXPECT_EQ(1, Visitor.ConceptReferencesTraversed);
+}
+
+} // end anonymous namespace