[AST] Accept identical TypeConstraint referring to other template parameters.

[ChuanqiXu]

Summary

The current implementation to judge the similarity of TypeConstraint in ASTContext::isSameTemplateParameter is problematic, it couldn't handle the following case:

C++
template <__integer_like _Tp, C<_Tp> Sentinel>
constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
    return __t;
}

When we see 2 such declarations from different modules, we would judge their similarity by ASTContext::isSame* methods. But problems come for the TypeConstraint. Originally, we would profile each argument one by one. But it is not right. Since the profiling result of _Tp would refer to two different template type declarations. So it would get different results. It is right since the _Tp in different modules refers to different declarations indeed. So the same declaration in different modules would meet incorrect our-checking results.

It is not the thing we want. We want to know if the TypeConstraint have the same expression.

[ChuanqiXu]

Remove unnecessary changes.

[ChuanqiXu]

Add comments.

[vsapsai]

I don't know enough to say if it is a good approach or not, I'll need to check what we can achieve by modifying Profile in ArgLoc.getArgument().Profile. Specifically, I'm interested to see if we can use the same ASTContext for profile. Also I have a few stylistic comments but I want to figure out high-level stuff first.

And I have some ideas about the tests. It might be covered elsewhere but I'm curious if there are any not-isSameTemplateParameter test cases? Also it can be useless but will it work with the deeper template nesting? Something like C<Level1<Level2>>.

[ChuanqiXu]

Add tests for nested template parameters.

[ChuanqiXu]

In D129068#3629135, @vsapsai wrote:

I don't know enough to say if it is a good approach or not, I'll need to check what we can achieve by modifying Profile in ArgLoc.getArgument().Profile.

I tried but I don't find good solution. If you could find something workable, it should be great.

Specifically, I'm interested to see if we can use the same ASTContext for profile.

I tried this but it doesn't work. They refer to the same ASTContext.

Also I have a few stylistic comments but I want to figure out high-level stuff first.

Of course.

And I have some ideas about the tests. It might be covered elsewhere but I'm curious if there are any not-isSameTemplateParameter test cases?

I am not sure what you mean about "not-isSameTemplateParameter test case".

Also it can be useless but will it work with the deeper template nesting? Something like C<Level1<Level2>>.

Yeah, added.

[ChuanqiXu]

Minor changes.

[vsapsai]

After looking at this change more, I was thinking about changing the title from how to what you are doing. For example, something like "[AST] Accept identical TypeConstraint referring to other template parameters." You can tweak it as you know better what's going on but that is my general understanding of the change.

Instead of my previous ideas with Profile I'm looking into how we are handling constraints from different modules when they are specified in requires clause. Because I wasn't able to cause errors with

template <__integer_like _Tp, typename Sentinel>
constexpr _Tp funcA(_Tp &&__t, Sentinel &&last) requires C<Sentinel, _Tp> {
  return __t;
}

And it seems beneficial to handle constraints across modules in the same way. Similar story with template parameters referring to other template parameters.

In D129068#3629217, @ChuanqiXu wrote:

And I have some ideas about the tests. It might be covered elsewhere but I'm curious if there are any not-isSameTemplateParameter test cases?

I am not sure what you mean about "not-isSameTemplateParameter test case".

For example, when I make the change

+#if 0
         if (XID != YID)
           return false;
+#endif

in ASTContext::isSameTemplateParameter, only "PCH/cxx2a-constraints.cpp" is failing. So it looks like there aren't many tests verifying various similar-but-slightly-different constrained templates are rejected. I agree that some of such tests should have existed before your changes and I'm not asking to test all possible kinds of mismatches. But we can still bolster the test suite, I hope. Though specific tests might depend on the implementation, so you don't have to rush with extra tests (unless you want to).

[ChuanqiXu]

Add more tests.

[ChuanqiXu]

In D129068#3637716, @vsapsai wrote:

After looking at this change more, I was thinking about changing the title from how to what you are doing. For example, something like "[AST] Accept identical TypeConstraint referring to other template parameters." You can tweak it as you know better what's going on but that is my general understanding of the change.

It looks better. Done.

Instead of my previous ideas with Profile I'm looking into how we are handling constraints from different modules when they are specified in requires clause. Because I wasn't able to cause errors with

template <__integer_like _Tp, typename Sentinel>
constexpr _Tp funcA(_Tp &&__t, Sentinel &&last) requires C<Sentinel, _Tp> {
  return __t;
}

And it seems beneficial to handle constraints across modules in the same way. Similar story with template parameters referring to other template parameters.

For require clause, it is handled by https://github.com/llvm/llvm-project/blob/f6e0c05e3dcb0b6f28424fb3435d547c04c3edd5/clang/lib/AST/ASTContext.cpp#L6453-L6463. But sadly we couldn't follow its style directly. Since requires clause is different with the inplace concept usage in the AST level.

In D129068#3629217, @ChuanqiXu wrote:

And I have some ideas about the tests. It might be covered elsewhere but I'm curious if there are any not-isSameTemplateParameter test cases?

I am not sure what you mean about "not-isSameTemplateParameter test case".

For example, when I make the change

+#if 0
         if (XID != YID)
           return false;
+#endif

in ASTContext::isSameTemplateParameter, only "PCH/cxx2a-constraints.cpp" is failing. So it looks like there aren't many tests verifying various similar-but-slightly-different constrained templates are rejected. I agree that some of such tests should have existed before your changes and I'm not asking to test all possible kinds of mismatches. But we can still bolster the test suite, I hope. Though specific tests might depend on the implementation, so you don't have to rush with extra tests (unless you want to).

Agreed. I added a test in this revision. And other kind of tests I imaged is covered by the checks in line 6230-6247. So I don't expect the current test covers all cases. But I feel it is not too bad.

[vsapsai]

Thanks for the changes, they look good! While I was looking how we handle "requires" constraints in other cases, I've noticed that they are suspiciously similar. That's why I offer the following change to unify comparison of the constraint expressions

diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 92293622cc3d..555669f027a7 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -2664,6 +2664,11 @@ public:
   /// that they may be used in declarations of the same template.
   bool isSameTemplateParameter(const NamedDecl *X, const NamedDecl *Y) const;
 
+  /// Determine whether two 'requires' expressions are similar enough.
+  /// Use of 'requires' isn't mandatory, works with constraints expressed in
+  /// other ways too.
+  bool isSameConstraintExpr(const Expr *XCE, const Expr *YCE) const;
+
   /// Retrieve the "canonical" template argument.
   ///
   /// The canonical template argument is the simplest template argument
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index aced0ab39ace..f3937d6304f9 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -6245,14 +6245,10 @@ bool ASTContext::isSameTemplateParameter(const NamedDecl *X,
         auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
         if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
           return false;
-        llvm::FoldingSetNodeID XID, YID;
-        for (auto &ArgLoc : TXTCArgs->arguments())
-          ArgLoc.getArgument().Profile(XID, X->getASTContext());
-        for (auto &ArgLoc : TYTCArgs->arguments())
-          ArgLoc.getArgument().Profile(YID, Y->getASTContext());
-        if (XID != YID)
-          return false;
       }
+      if (!isSameConstraintExpr(TXTC->getImmediatelyDeclaredConstraint(),
+                                TYTC->getImmediatelyDeclaredConstraint()))
+        return false;
     }
     return true;
   }
@@ -6279,15 +6275,20 @@ bool ASTContext::isSameTemplateParameterList(
     if (!isSameTemplateParameter(X->getParam(I), Y->getParam(I)))
       return false;
 
-  const Expr *XRC = X->getRequiresClause();
-  const Expr *YRC = Y->getRequiresClause();
-  if (!XRC != !YRC)
+  if (!isSameConstraintExpr(X->getRequiresClause(), Y->getRequiresClause()))
     return false;
-  if (XRC) {
-    llvm::FoldingSetNodeID XRCID, YRCID;
-    XRC->Profile(XRCID, *this, /*Canonical=*/true);
-    YRC->Profile(YRCID, *this, /*Canonical=*/true);
-    if (XRCID != YRCID)
+
+  return true;
+}
+
+bool ASTContext::isSameConstraintExpr(const Expr *XCE, const Expr *YCE) const {
+  if (!XCE != !YCE)
+    return false;
+  if (XCE) {
+    llvm::FoldingSetNodeID XCEID, YCEID;
+    XCE->Profile(XCEID, *this, /*Canonical=*/true);
+    YCE->Profile(YCEID, *this, /*Canonical=*/true);
+    if (XCEID != YCEID)
       return false;
   }
 
@@ -6450,17 +6451,9 @@ bool ASTContext::isSameEntity(const NamedDecl *X, const NamedDecl *Y) const {
         return false;
     }
 
-    const Expr *XRC = FuncX->getTrailingRequiresClause();
-    const Expr *YRC = FuncY->getTrailingRequiresClause();
-    if (!XRC != !YRC)
+    if (!isSameConstraintExpr(FuncX->getTrailingRequiresClause(),
+                              FuncY->getTrailingRequiresClause()))
       return false;
-    if (XRC) {
-      llvm::FoldingSetNodeID XRCID, YRCID;
-      XRC->Profile(XRCID, *this, /*Canonical=*/true);
-      YRC->Profile(YRCID, *this, /*Canonical=*/true);
-      if (XRCID != YRCID)
-        return false;
-    }
 
     auto GetTypeAsWritten = [](const FunctionDecl *FD) {
       // Map to the first declaration that we've already merged into this one.

In ASTContext::isSameTemplateParameter it is exactly the same change as yours modulo comments.

clang/test/Modules/concept.cppm

38

In what cases operator() is critical for the test? I was thinking about replacing with something like "funcA", "funcB", "funcC", so that diagnostic verification is easier because it is tricky to understand which method "__fn::operator()" refers to.

[ChuanqiXu]

Address comments.

[ChuanqiXu]

In D129068#3640249, @vsapsai wrote:

Thanks for the changes, they look good! While I was looking how we handle "requires" constraints in other cases, I've noticed that they are suspiciously similar. That's why I offer the following change to unify comparison of the constraint expressions

diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 92293622cc3d..555669f027a7 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -2664,6 +2664,11 @@ public:
   /// that they may be used in declarations of the same template.
   bool isSameTemplateParameter(const NamedDecl *X, const NamedDecl *Y) const;
 
+  /// Determine whether two 'requires' expressions are similar enough.
+  /// Use of 'requires' isn't mandatory, works with constraints expressed in
+  /// other ways too.
+  bool isSameConstraintExpr(const Expr *XCE, const Expr *YCE) const;
+
   /// Retrieve the "canonical" template argument.
   ///
   /// The canonical template argument is the simplest template argument
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index aced0ab39ace..f3937d6304f9 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -6245,14 +6245,10 @@ bool ASTContext::isSameTemplateParameter(const NamedDecl *X,
         auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
         if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
           return false;
-        llvm::FoldingSetNodeID XID, YID;
-        for (auto &ArgLoc : TXTCArgs->arguments())
-          ArgLoc.getArgument().Profile(XID, X->getASTContext());
-        for (auto &ArgLoc : TYTCArgs->arguments())
-          ArgLoc.getArgument().Profile(YID, Y->getASTContext());
-        if (XID != YID)
-          return false;
       }
+      if (!isSameConstraintExpr(TXTC->getImmediatelyDeclaredConstraint(),
+                                TYTC->getImmediatelyDeclaredConstraint()))
+        return false;
     }
     return true;
   }
@@ -6279,15 +6275,20 @@ bool ASTContext::isSameTemplateParameterList(
     if (!isSameTemplateParameter(X->getParam(I), Y->getParam(I)))
       return false;
 
-  const Expr *XRC = X->getRequiresClause();
-  const Expr *YRC = Y->getRequiresClause();
-  if (!XRC != !YRC)
+  if (!isSameConstraintExpr(X->getRequiresClause(), Y->getRequiresClause()))
     return false;
-  if (XRC) {
-    llvm::FoldingSetNodeID XRCID, YRCID;
-    XRC->Profile(XRCID, *this, /*Canonical=*/true);
-    YRC->Profile(YRCID, *this, /*Canonical=*/true);
-    if (XRCID != YRCID)
+
+  return true;
+}
+
+bool ASTContext::isSameConstraintExpr(const Expr *XCE, const Expr *YCE) const {
+  if (!XCE != !YCE)
+    return false;
+  if (XCE) {
+    llvm::FoldingSetNodeID XCEID, YCEID;
+    XCE->Profile(XCEID, *this, /*Canonical=*/true);
+    YCE->Profile(YCEID, *this, /*Canonical=*/true);
+    if (XCEID != YCEID)
       return false;
   }
 
@@ -6450,17 +6451,9 @@ bool ASTContext::isSameEntity(const NamedDecl *X, const NamedDecl *Y) const {
         return false;
     }
 
-    const Expr *XRC = FuncX->getTrailingRequiresClause();
-    const Expr *YRC = FuncY->getTrailingRequiresClause();
-    if (!XRC != !YRC)
+    if (!isSameConstraintExpr(FuncX->getTrailingRequiresClause(),
+                              FuncY->getTrailingRequiresClause()))
       return false;
-    if (XRC) {
-      llvm::FoldingSetNodeID XRCID, YRCID;
-      XRC->Profile(XRCID, *this, /*Canonical=*/true);
-      YRC->Profile(YRCID, *this, /*Canonical=*/true);
-      if (XRCID != YRCID)
-        return false;
-    }
 
     auto GetTypeAsWritten = [](const FunctionDecl *FD) {
       // Map to the first declaration that we've already merged into this one.

In ASTContext::isSameTemplateParameter it is exactly the same change as yours modulo comments.

Thanks for the detailed suggestion! Done!

clang/test/Modules/concept.cppm

38

The operator() is not critical for the test. It is reduced from the actual testing. But I prefer to remain operator() here. Since tests are rarely read and the current style could improve the diversity. Diversity is good for tests (to me).

[ilya-biryukov]

A few NITs from my side, hopefully they can improve things a bit.

clang/include/clang/AST/ASTContext.h

2676 ↗

(On Diff #443548)

NIT: maybe be more specific here?
It's not clear what "similar enough" means without context, other comments follow the pattern.

clang/lib/AST/ASTContext.cpp

6241

Do we even want to compare the template argument sizes here?
Would comparing only getImmediatelyDeclaredConstraint produce the same results?

If that's a performance optimization, we are probably better off moving it inside isSameConstraintExpr.

[ChuanqiXu]

Address comments.

[ChuanqiXu]

clang/lib/AST/ASTContext.cpp

6241

I've moved them into isSameTypeConstraint. Now the code looks better.

[vsapsai]

My comments are addressed, thanks for making the changes!

Please wait for @ilya-biryukov's approval to confirm he has nothing else to add.

[ilya-biryukov]

LGTM, thanks!

[ChuanqiXu]

Thanks for reviewing!

clang/lib/AST/ASTContext.cpp

if (auto *TX = dyn_cast<TemplateTypeParmDecl>(X)) {
     const TypeConstraint *TYTC = TY->getTypeConstraint();
     if (!TXTC != !TYTC)
       return false;
     if (TXTC && TYTC) {
       auto *NCX = TXTC->getNamedConcept();
       auto *NCY = TYTC->getNamedConcept();
       if (!NCX || !NCY || !isSameEntity(NCX, NCY))
         return false;
       if (TXTC->hasExplicitTemplateArgs() != TYTC->hasExplicitTemplateArgs())
         return false;
       if (TXTC->hasExplicitTemplateArgs()) {
         auto *TXTCArgs = TXTC->getTemplateArgsAsWritten();
         auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
         if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
           return false;
-        llvm::FoldingSetNodeID XID, YID;
-        for (auto &ArgLoc : TXTCArgs->arguments())
-          ArgLoc.getArgument().Profile(XID, X->getASTContext());
-        for (auto &ArgLoc : TYTCArgs->arguments())
-          ArgLoc.getArgument().Profile(YID, Y->getASTContext());
-        if (XID != YID)
-          return false;
-      }
-    }
+        // We couldn't compare the profiling result for the template
+        // args here. Consider the following example in different modules:
+        //
+        // template <__integer_like _Tp, C<_Tp> Sentinel>
+        // constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+        //   return __t;
+        // }
+        //
+        // When we compare the profiling result for `C<_Tp>` in different
+        // modules, it will compare the type of `_Tp` in different modules.
+        // However, the type of `_Tp` in different modules refer to different
+        // types here naturally. So we couldn't compare the profiling result
+        // for the template args directly.
+      }
+      auto *XConstraint = TXTC->getImmediatelyDeclaredConstraint();
+      auto *YConstraint = TYTC->getImmediatelyDeclaredConstraint();
+      if (!XConstraint != !YConstraint)
+        return false;
+      if (XConstraint) {
+        XConstraint->Profile(XID, *this, /*Canonical=*/true);
+        YConstraint->Profile(YID, *this, /*Canonical=*/true);
+      }
     return true;
   }
 
   if (auto *TX = dyn_cast<NonTypeTemplateParmDecl>(X)) {
     auto *TY = cast<NonTypeTemplateParmDecl>(Y);
     return TX->isParameterPack() == TY->isParameterPack() &&
            TX->getASTContext().hasSameType(TX->getType(), TY->getType());
   }

clang/test/Modules/concept.cppm

 // RUN: rm -rf %t
 // RUN: mkdir %t
 // RUN: split-file %s %t
 //
 // RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -I%t -DDIFFERENT %t/B.cppm -verify
 // RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -I%t %t/B.cppm -verify
 
 //--- foo.h
 #ifndef FOO_H
 #define FOO_H
 
 template <class T>
 concept Range = requires(T &t) { t.begin(); };
 
 template<class _Tp>
 concept __integer_like = true;
 
 template <class _Tp>
 concept __member_size = requires(_Tp &&t) { t.size(); };
 
+template <class First, class Second>
+concept C = requires(First x, Second y) { x+y; };
+
 struct A {
 public:
   template <Range T>
   using range_type = T;
 };
 
 struct __fn {
   template <__member_size _Tp>
   constexpr __integer_like auto operator()(_Tp&& __t) const {
     return __t.size();
   }
+
+  template <__integer_like _Tp, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+    return __t;
+  }
+
+  template <template<class> class H, class S, C<H<S>> Sentinel>
+  constexpr H<S> operator()(H<S> &&__s, Sentinel &&last) const {
+    return __s;
+  }
+
+// Tests that we could find different concept definition indeed.
+#ifndef DIFFERENT
+  template <__integer_like _Tp, __integer_like _Up, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, _Up _u, Sentinel &&last) const {
+    return __t;
+  }
+#else
+  template <__integer_like _Tp, __integer_like _Up, C<_Up> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, _Up _u, Sentinel &&last) const {
+    return __t;
+  }
+#endif
 };
 #endif
 
 //--- A.cppm
 module;
 #include "foo.h"
 export module A;
 
 //--- B.cppm
-// expected-no-diagnostics
 module;
 #include "foo.h"
 export module B;
 import A;
 
+#ifdef DIFFERENT
+// expected-error@foo.h:41 {{'__fn::operator()' from module 'A.<global>' is not present in definition of '__fn' provided earlier}}
+// expected-note@* 1+{{declaration of 'operator()' does not match}}
+#else
+// expected-no-diagnostics
+#endif
+
+template <class T>
+struct U {
+  auto operator+(U) { return 0; }
+};
+
 void foo() {
     A a;
     struct S {
         int size() { return 0; }
         auto operator+(S s) { return 0; }
     };
     __fn{}(S());
+    __fn{}(S(), S());
+    __fn{}(S(), S(), S());
+
+    __fn{}(U<int>(), U<int>());
 }