[C++20] [Modules] Allow Stmt::Profile to treat lambdas as equal conditionally

[ChuanqiXu]

Summary

Close https://github.com/llvm/llvm-project/issues/63544.

Background: We landed std modules in libcxx recently but we haven't landed the corresponding in-tree tests. According to @Mordante, there are only 1% libcxx tests failing with std modules. And the major blocking issue is the lambda expression in the require clauses.

The root cause of the issue is that previously we never consider any lambda expression as the same. Per [temp.over.link]p5:

Two lambda-expressions are never considered equivalent.

I thought this is an oversight at first but @rsmith explains that in the wording, the program is as if there is only a single definition, and a single lambda-expression. So we don't need worry about this in the spec. The explanation makes sense. But it didn't reflect to the implementation directly.

Here is a cycle in the implementation. If we want to merge two definitions, we need to make sure its implementation are the same. But according to the explanation above, we need to judge if two lambda-expression are the same by looking at its parent definitions. So here is the problem.

To solve the problem, I think we have to profile the lambda expressions actually to get the accurate information. But we can't do this universally. So in this patch I tried to modify the interface of Stmt::Profile and only profile the lambda expression during the process of merging the constraint expressions.

[shafik]

clang/lib/AST/StmtProfile.cpp

188–189

nit

[ChuanqiXu]

Address comments.

[ChuanqiXu]

I'd like to land this in the next week if no objections come in. Since clang-17 is going to be branched and this one is small and relevant. Also this one prevents the testing of the std modules.

clang/include/clang/AST/Stmt.h

public:
   ///
   /// \param Context the AST context in which the statement resides
   ///
   /// \param Canonical whether the profile should be based on the canonical
   /// representation of this statement (e.g., where non-type template
   /// parameters are identified by index/level rather than their
   /// declaration pointers) or the exact representation of the statement as
   /// written in the source.
+  /// \param ProfileLambdaExpr whether or not to profile lambda expressions.
+  /// When false, the lambda expressions are never considered to be equal to
+  /// other lambda expressions. When true, the lambda expressions with the same
+  /// implementation will be considered to be the same. ProfileLambdaExpr should
+  /// only be true when we try to merge two declarations within modules.
   void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
-               bool Canonical) const;
+               bool Canonical, bool ProfileLambdaExpr = false) const;
 
   /// Calculate a unique representation for a statement that is
   /// stable across compiler invocations.
   ///
   /// \param ID profile information will be stored in ID.
   ///
   /// \param Hash an ODRHash object which will be called where pointers would
   /// have been used in the Profile function.

clang/lib/AST/ASTContext.cpp

 bool ASTContext::isSameConstraintExpr(const Expr *XCE, const Expr *YCE) const {
   if (!XCE != !YCE)
     return false;
 
   if (!XCE)
     return true;
 
   llvm::FoldingSetNodeID XCEID, YCEID;
-  XCE->Profile(XCEID, *this, /*Canonical=*/true);
-  YCE->Profile(YCEID, *this, /*Canonical=*/true);
+  XCE->Profile(XCEID, *this, /*Canonical=*/true, /*ProfileLambdaExpr=*/true);
+  YCE->Profile(YCEID, *this, /*Canonical=*/true, /*ProfileLambdaExpr=*/true);
   return XCEID == YCEID;
 }
 
 bool ASTContext::isSameTypeConstraint(const TypeConstraint *XTC,
                                       const TypeConstraint *YTC) const {
   if (!XTC != !YTC)
     return false;
 
bool ASTContext::isSameEntity(const NamedDecl *X, const NamedDecl *Y) const {
   // Identical template names and kinds match if their template parameter lists
   // and patterns match.
   if (const auto *TemplateX = dyn_cast<TemplateDecl>(X)) {
     const auto *TemplateY = cast<TemplateDecl>(Y);
 
     // ConceptDecl wouldn't be the same if their constraint expression differs.
     if (const auto *ConceptX = dyn_cast<ConceptDecl>(X)) {
       const auto *ConceptY = cast<ConceptDecl>(Y);
-      const Expr *XCE = ConceptX->getConstraintExpr();
-      const Expr *YCE = ConceptY->getConstraintExpr();
-      assert(XCE && YCE && "ConceptDecl without constraint expression?");
-      llvm::FoldingSetNodeID XID, YID;
-      XCE->Profile(XID, *this, /*Canonical=*/true);
-      YCE->Profile(YID, *this, /*Canonical=*/true);
-      if (XID != YID)
+      if (!isSameConstraintExpr(ConceptX->getConstraintExpr(),
+                                ConceptY->getConstraintExpr()))
         return false;
     }
 
     return isSameEntity(TemplateX->getTemplatedDecl(),
                         TemplateY->getTemplatedDecl()) &&
            isSameTemplateParameterList(TemplateX->getTemplateParameters(),
                                        TemplateY->getTemplateParameters());
   }

clang/lib/AST/StmtProfile.cpp

 #include "llvm/ADT/FoldingSet.h"
 using namespace clang;
 
 namespace {
   class StmtProfiler : public ConstStmtVisitor<StmtProfiler> {
   protected:
     llvm::FoldingSetNodeID &ID;
     bool Canonical;
+    bool ProfileLambdaExpr;
 
   public:
-    StmtProfiler(llvm::FoldingSetNodeID &ID, bool Canonical)
-        : ID(ID), Canonical(Canonical) {}
+    StmtProfiler(llvm::FoldingSetNodeID &ID, bool Canonical,
+                 bool ProfileLambdaExpr)
+        : ID(ID), Canonical(Canonical), ProfileLambdaExpr(ProfileLambdaExpr) {}
 
     virtual ~StmtProfiler() {}
 
     void VisitStmt(const Stmt *S);
 
     void VisitStmtNoChildren(const Stmt *S) {
       HandleStmtClass(S->getStmtClass());
     }
#include "clang/AST/StmtNodes.inc"
     void VisitTemplateArgument(const TemplateArgument &Arg);
   };
 
   class StmtProfilerWithPointers : public StmtProfiler {
     const ASTContext &Context;
 
   public:
     StmtProfilerWithPointers(llvm::FoldingSetNodeID &ID,
-                             const ASTContext &Context, bool Canonical)
-        : StmtProfiler(ID, Canonical), Context(Context) {}
+                             const ASTContext &Context, bool Canonical,
+                             bool ProfileLambdaExpr)
+        : StmtProfiler(ID, Canonical, ProfileLambdaExpr), Context(Context) {}
+
   private:
     void HandleStmtClass(Stmt::StmtClass SC) override {
       ID.AddInteger(SC);
     }
 
     void VisitDecl(const Decl *D) override {
       ID.AddInteger(D ? D->getKind() : 0);
 
void VisitTemplateName(TemplateName Name) override {
       Name.Profile(ID);
     }
   };
 
   class StmtProfilerWithoutPointers : public StmtProfiler {
     ODRHash &Hash;
   public:
     StmtProfilerWithoutPointers(llvm::FoldingSetNodeID &ID, ODRHash &Hash)
-        : StmtProfiler(ID, false), Hash(Hash) {}
+        : StmtProfiler(ID, /*Canonical=*/false, /*ProfileLambdaExpr=*/false),
+          Hash(Hash) {}
 
   private:
     void HandleStmtClass(Stmt::StmtClass SC) override {
       if (SC == Stmt::UnresolvedLookupExprClass) {
         // Pretend that the name looked up is a Decl due to how templates
         // handle some Decl lookups.
         ID.AddInteger(Stmt::DeclRefExprClass);
       } else {

 
 void
 StmtProfiler::VisitCXXTemporaryObjectExpr(const CXXTemporaryObjectExpr *S) {
   VisitCXXConstructExpr(S);
 }
 
 void
 StmtProfiler::VisitLambdaExpr(const LambdaExpr *S) {
+  if (!ProfileLambdaExpr) {
     // Do not recursively visit the children of this expression. Profiling the
     // body would result in unnecessary work, and is not safe to do during
     // deserialization.
     VisitStmtNoChildren(S);
 
     // C++20 [temp.over.link]p5:
     //   Two lambda-expressions are never considered equivalent.
     VisitDecl(S->getLambdaClass());
+
+    return;
+  }
+
+  CXXRecordDecl *Lambda = S->getLambdaClass();
+  ID.AddInteger(Lambda->getODRHash());
+
+  for (const auto &Capture : Lambda->captures()) {
+    ID.AddInteger(Capture.getCaptureKind());
+    if (Capture.capturesVariable())
+      VisitDecl(Capture.getCapturedVar());
+  }
 }
 
 void
 StmtProfiler::VisitCXXScalarValueInitExpr(const CXXScalarValueInitExpr *S) {
   VisitExpr(S);
 }
 
 void StmtProfiler::VisitCXXDeleteExpr(const CXXDeleteExpr *S) {
void StmtProfiler::VisitTemplateArgument(const TemplateArgument &Arg) {
   case TemplateArgument::Pack:
     for (const auto &P : Arg.pack_elements())
       VisitTemplateArgument(P);
     break;
   }
 }
 
 void Stmt::Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
-                   bool Canonical) const {
-  StmtProfilerWithPointers Profiler(ID, Context, Canonical);
+                   bool Canonical, bool ProfileLambdaExpr) const {
+  StmtProfilerWithPointers Profiler(ID, Context, Canonical, ProfileLambdaExpr);
   Profiler.Visit(this);
 }
 
 void Stmt::ProcessODRHash(llvm::FoldingSetNodeID &ID,
                           class ODRHash &Hash) const {
   StmtProfilerWithoutPointers Profiler(ID, Hash);
   Profiler.Visit(this);
 }

clang/test/Modules/merge-requires-with-lambdas.cppm

+// Tests that we can merge the concept declarations with lambda well.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %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 %t/A0.cppm -emit-module-interface -o %t/A0.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TestA.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/A1.cppm -emit-module-interface -o %t/A1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TestA1.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/A2.cppm -emit-module-interface -o %t/A2.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TestA2.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/A3.cppm -emit-module-interface -o %t/A3.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TestA3.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+
+//--- A.h
+template <class _Tp>
+concept A = requires(const _Tp& __t) { []<class __Up>(const __Up&) {}(__t); };
+
+//--- A1.h
+template <class _Tp>
+concept A = requires(const _Tp& __t) { []<class __Up>(__Up) {}(__t); };
+
+//--- A2.h
+template <class _Tp>
+concept A = requires(const _Tp& __t) { []<class __Up>(const __Up& __u) {
+    (int)__u;
+}(__t); };
+
+//--- A3.h
+template <class _Tp>
+concept A = requires(const _Tp& __t) { [t = '?']<class __Up>(const __Up&) {
+    (int)t;
+}(__t); };
+
+//--- A.cppm
+module;
+#include "A.h"
+export module A;
+export using ::A;
+
+//--- A0.cppm
+module;
+#include "A.h"
+export module A0;
+export using ::A;
+
+//--- TestA.cpp
+// expected-no-diagnostics
+import A;
+import A0;
+
+template <class C>
+void f(C) requires(A<C>) {}
+
+//--- A1.cppm
+module;
+#include "A1.h"
+export module A1;
+export using ::A;
+
+//--- TestA1.cpp
+import A;
+import A1;
+
+template <class C>
+void f(C) requires(A<C>) {} // expected-error 1+{{reference to 'A' is ambiguous}}
+                            // expected-note@* 1+{{candidate found by name lookup is 'A'}}
+
+//--- A2.cppm
+module;
+#include "A2.h"
+export module A2;
+export using ::A;
+
+//--- TestA2.cpp
+import A;
+import A2;
+
+template <class C>
+void f(C) requires(A<C>) {} // expected-error 1+{{reference to 'A' is ambiguous}}
+                            // expected-note@* 1+{{candidate found by name lookup is 'A'}}
+
+//--- A3.cppm
+module;
+#include "A3.h"
+export module A3;
+export using ::A;
+
+//--- TestA3.cpp
+import A;
+import A3;
+
+template <class C>
+void f(C) requires(A<C>) {} // expected-error 1+{{reference to 'A' is ambiguous}}
+                            // expected-note@* 1+{{candidate found by name lookup is 'A'}}

clang/test/Modules/pr63544.cppm

+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++23 %t/a.cppm -emit-module-interface -o %t/m-a.pcm
+// RUN: %clang_cc1 -std=c++23 %t/b.cppm -emit-module-interface -o %t/m-b.pcm
+// RUN: %clang_cc1 -std=c++23 %t/m.cppm -emit-module-interface -o %t/m.pcm \
+// RUN:     -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++23 %t/pr63544.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+
+//--- foo.h
+
+namespace std {
+struct strong_ordering {
+  int n;
+  constexpr operator int() const { return n; }
+  static const strong_ordering equal, greater, less;
+};
+constexpr strong_ordering strong_ordering::equal = {0};
+constexpr strong_ordering strong_ordering::greater = {1};
+constexpr strong_ordering strong_ordering::less = {-1};
+} // namespace std
+
+namespace std {
+template <typename _Tp>
+class optional {
+private:
+    using value_type = _Tp;
+    value_type __val_;
+    bool __engaged_;
+public:
+    constexpr bool has_value() const noexcept
+    {
+        return this->__engaged_;
+    }
+
+    constexpr const value_type& operator*() const& noexcept
+    {
+        return __val_;
+    }
+
+    optional(_Tp v) : __val_(v) {
+        __engaged_ = true;
+    }
+};
+
+template <class _Tp>
+concept __is_derived_from_optional = requires(const _Tp& __t) { []<class __Up>(const optional<__Up>&) {}(__t); };
+
+template <class _Tp, class _Up>
+    requires(!__is_derived_from_optional<_Up>)
+constexpr strong_ordering
+operator<=>(const optional<_Tp>& __x, const _Up& __v) {
+    return __x.has_value() ? *__x <=> __v : strong_ordering::less;
+}
+} // namespace std
+
+//--- a.cppm
+module;
+#include "foo.h"
+export module m:a;
+export namespace std {
+    using std::optional;
+    using std::operator<=>;
+}
+
+//--- b.cppm
+module;
+#include "foo.h"
+export module m:b;
+export namespace std {
+    using std::optional;
+    using std::operator<=>;
+}
+
+//--- m.cppm
+export module m;
+export import :a;
+export import :b;
+
+//--- pr63544.cpp
+// expected-no-diagnostics
+import m;
+int pr63544() {
+    std::optional<int> a(43);
+    int t{3};
+    return a<=>t;
+}