Page MenuHomePhabricator

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

Authored by ChuanqiXu on Jul 4 2022, 3:05 AM.

Details

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.

Diff Detail

Event Timeline

ChuanqiXu created this revision.Jul 4 2022, 3:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2022, 3:05 AM
ChuanqiXu requested review of this revision.Jul 4 2022, 3:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2022, 3:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ChuanqiXu updated this revision to Diff 442055.Jul 4 2022, 3:07 AM

Remove unnecessary changes.

ChuanqiXu updated this revision to Diff 442165.Jul 4 2022, 7:29 PM

Add comments.

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 updated this revision to Diff 442181.Jul 4 2022, 11:19 PM

Add tests for nested template parameters.

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 updated this revision to Diff 442792.Jul 6 2022, 11:36 PM

Minor changes.

ChuanqiXu added a reviewer: Restricted Project.Jul 6 2022, 11:36 PM

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.

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 updated this revision to Diff 443141.Jul 7 2022, 10:56 PM
ChuanqiXu retitled this revision from [AST] Profiling on constraint expression instead of arguments for TypeConstraint in ASTContext::isSameTemplateParameter to [AST] Accept identical TypeConstraint referring to other template parameters..

Add more tests.

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.

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.

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 updated this revision to Diff 443548.Jul 10 2022, 8:20 PM

Address comments.

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).

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

clang/include/clang/AST/ASTContext.h
2676

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
6254

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 updated this revision to Diff 443823.Jul 11 2022, 8:06 PM

Address comments.

ChuanqiXu marked 2 inline comments as done.Jul 11 2022, 8:06 PM
ChuanqiXu added inline comments.
clang/lib/AST/ASTContext.cpp
6254

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

vsapsai accepted this revision as: vsapsai.Jul 12 2022, 8:05 AM

My comments are addressed, thanks for making the changes!

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

This revision is now accepted and ready to land.Jul 12 2022, 8:05 AM
ilya-biryukov accepted this revision.Jul 12 2022, 8:08 AM

LGTM, thanks!

ChuanqiXu marked an inline comment as done.Jul 12 2022, 8:32 AM

Thanks for reviewing!