[clang] [C++20] [Modules] Don't profile UnresolvedLookupExpr in require clause and noexcept clause

[ChuanqiXu9]

Close #190333

See clang/test/Modules/callable-require-clause-merge.cppm and clang/test/Modules/polluted-operator.cppm for motivating case.

In short, the unrelated global operator may pollute the operator in require clause and noexcept clause, which makes clang emits hard to understand false-positive diagnostic message to end users.

This patch tries to avoid such problems for require clause and noexcept clause specifically.

This may not be perfect. I feel we may face other similar problems in other clause due to the design of UnresolvedLookupExpr. But on the one hand, it is better to fix it fundamentally after we saw more cases so that we can have a better understanding, on the other hand, it is better to stop blooding right now as the approach here is simple enough.

[llvmbot]

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Chuanqi Xu (ChuanqiXu9)

Changes

Close #190333

See clang/test/Modules/callable-require-clause-merge.cppm and clang/test/Modules/polluted-operator.cppm for motivating case.

In short, the unrelated global operator may pollute the operator in require clause and noexcept clause, which makes clang emits hard to understand false-positive diagnostic message to end users.

This patch tries to avoid such problems for require clause and noexcept clause specifically.

This may not be perfect. I feel we may face other similar problems in other clause due to the design of UnresolvedLookupExpr. But on the one hand, it is better to fix it fundamentally after we saw more cases so that we can have a better understanding, on the other hand, it is better to stop blooding right now as the approach here is easy and not bad.

---

Full diff: https://github.com/llvm/llvm-project/pull/194283.diff

6 Files Affected:

• (modified) clang/include/clang/AST/Stmt.h (+26-1)
• (modified) clang/lib/AST/ASTContext.cpp (+6-2)
• (modified) clang/lib/AST/StmtProfile.cpp (+21-7)
• (modified) clang/lib/AST/Type.cpp (+9-1)
• (added) clang/test/Modules/callable-require-clause-merge.cppm (+43)
• (modified) clang/test/Modules/polluted-operator.cppm (-5)

diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index d940aa6562c4c..5461e91ba0aa1 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -1617,8 +1617,33 @@ class alignas(void *) Stmt {
   /// 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.
+  /// \param IgnoringUnresolvedLookupExpr whether or not to ignore
+  /// UnresolvedLookupExpr when profiling. When true,
+  /// IgnoringUnresolvedLookupExpr won't be invoked during profiling. This is
+  /// useful in case we don't hope the unresolved lookup expr to pollute the
+  /// profile result. e.g.,
+  ///
+  /// "a.h"
+  ///
+  ///   #pragma once
+  ///   struct F {
+  ///     template <typename... T> requires ((sizeof(T) > 0) && ...)
+  ///     void operator()(T...) {}
+  ///   } f;
+  ///
+  /// and
+  ///
+  /// "c.h"
+  ///
+  ///   void operator&&(struct X, struct X);
+  ///   #include "a.h"
+  ///
+  /// Here the `F::operator()` may produce different profiling results depending
+  /// on whether there is a freestanding `operator&&` declared before it. And
+  /// this affects declaration merging in modules.
   void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
-               bool Canonical, bool ProfileLambdaExpr = false) const;
+               bool Canonical, bool ProfileLambdaExpr = false,
+               bool IgnoringUnresolvedLookupExpr = false) const;
 
   /// Calculate a unique representation for a statement that is
   /// stable across compiler invocations.
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index abf5f8a832043..d2298bd1b30e7 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -7454,8 +7454,12 @@ bool ASTContext::isSameConstraintExpr(const Expr *XCE, const Expr *YCE) const {
     return true;
 
   llvm::FoldingSetNodeID XCEID, YCEID;
-  XCE->Profile(XCEID, *this, /*Canonical=*/true, /*ProfileLambdaExpr=*/true);
-  YCE->Profile(YCEID, *this, /*Canonical=*/true, /*ProfileLambdaExpr=*/true);
+  /// The unresolved lookup expr may misguide the profiling results. See
+  /// clang/test/Modules/callable-require-clause-merge.cppm for an example.
+  XCE->Profile(XCEID, *this, /*Canonical=*/true, /*ProfileLambdaExpr=*/true,
+               /*IgnoringUnresolvedLookupExpr=*/true);
+  YCE->Profile(YCEID, *this, /*Canonical=*/true, /*ProfileLambdaExpr=*/true,
+               /*IgnoringUnresolvedLookupExpr=*/true);
   return XCEID == YCEID;
 }
 
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index 8219e57644be6..a9c81f0aa027a 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -30,11 +30,13 @@ namespace {
     llvm::FoldingSetNodeID &ID;
     bool Canonical;
     bool ProfileLambdaExpr;
+    bool IgnoringUnresolvedLookupExpr;
 
   public:
     StmtProfiler(llvm::FoldingSetNodeID &ID, bool Canonical,
-                 bool ProfileLambdaExpr)
-        : ID(ID), Canonical(Canonical), ProfileLambdaExpr(ProfileLambdaExpr) {}
+                 bool ProfileLambdaExpr, bool IgnoringUnresolvedLookupExpr)
+        : ID(ID), Canonical(Canonical), ProfileLambdaExpr(ProfileLambdaExpr),
+          IgnoringUnresolvedLookupExpr(IgnoringUnresolvedLookupExpr) {}
 
     virtual ~StmtProfiler() {}
 
@@ -86,8 +88,11 @@ namespace {
   public:
     StmtProfilerWithPointers(llvm::FoldingSetNodeID &ID,
                              const ASTContext &Context, bool Canonical,
-                             bool ProfileLambdaExpr)
-        : StmtProfiler(ID, Canonical, ProfileLambdaExpr), Context(Context) {}
+                             bool ProfileLambdaExpr,
+                             bool IgnoringUnresolvedLookupExpr)
+        : StmtProfiler(ID, Canonical, ProfileLambdaExpr,
+                       IgnoringUnresolvedLookupExpr),
+          Context(Context) {}
 
   private:
     void HandleStmtClass(Stmt::StmtClass SC) override {
@@ -184,8 +189,11 @@ namespace {
   class StmtProfilerWithoutPointers : public StmtProfiler {
     ODRHash &Hash;
   public:
+    // Set IgnoringUnresolvedLookupExpr as we don't want the unresolved lookup
+    // expr affecting the merging results.
     StmtProfilerWithoutPointers(llvm::FoldingSetNodeID &ID, ODRHash &Hash)
-        : StmtProfiler(ID, /*Canonical=*/false, /*ProfileLambdaExpr=*/false),
+        : StmtProfiler(ID, /*Canonical=*/false, /*ProfileLambdaExpr=*/false,
+                       /*IgnoringUnresolvedLookupExpr=*/true),
           Hash(Hash) {}
 
   private:
@@ -2269,6 +2277,10 @@ void StmtProfiler::VisitOverloadExpr(const OverloadExpr *S) {
 
 void
 StmtProfiler::VisitUnresolvedLookupExpr(const UnresolvedLookupExpr *S) {
+  if (IgnoringUnresolvedLookupExpr) {
+    ID.AddInteger(0);
+    return;
+  }
   VisitOverloadExpr(S);
 }
 
@@ -2951,8 +2963,10 @@ void StmtProfiler::VisitHLSLOutArgExpr(const HLSLOutArgExpr *S) {
 }
 
 void Stmt::Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
-                   bool Canonical, bool ProfileLambdaExpr) const {
-  StmtProfilerWithPointers Profiler(ID, Context, Canonical, ProfileLambdaExpr);
+                   bool Canonical, bool ProfileLambdaExpr,
+                   bool IgnoringUnresolvedLookupExpr) const {
+  StmtProfilerWithPointers Profiler(ID, Context, Canonical, ProfileLambdaExpr,
+                                    IgnoringUnresolvedLookupExpr);
   Profiler.Visit(this);
 }
 
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 6c295c1a9c409..87ab7cd3ad185 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -4035,7 +4035,15 @@ void FunctionProtoType::Profile(llvm::FoldingSetNodeID &ID, QualType Result,
     for (QualType Ex : epi.ExceptionSpec.Exceptions)
       ID.AddPointer(Ex.getAsOpaquePtr());
   } else if (isComputedNoexcept(epi.ExceptionSpec.Type)) {
-    epi.ExceptionSpec.NoexceptExpr->Profile(ID, Context, Canonical);
+    // Make sure the profiling result of the noexcept expression
+    // won't be affected by the unresolved lookup expressions.
+    // See clang/test/Modules/polluted-operator.cppm for an example
+    // for it.
+    //
+    // ProfileLambdaExpr=false is the default value.
+    epi.ExceptionSpec.NoexceptExpr->Profile(
+        ID, Context, Canonical, /*ProfileLambdaExpr=*/false,
+        /*IgnoringUnresolvedLookupExpr=*/true);
   } else if (epi.ExceptionSpec.Type == EST_Uninstantiated ||
              epi.ExceptionSpec.Type == EST_Unevaluated) {
     ID.AddPointer(epi.ExceptionSpec.SourceDecl->getCanonicalDecl());
diff --git a/clang/test/Modules/callable-require-clause-merge.cppm b/clang/test/Modules/callable-require-clause-merge.cppm
new file mode 100644
index 0000000000000..6cdb369639a39
--- /dev/null
+++ b/clang/test/Modules/callable-require-clause-merge.cppm
@@ -0,0 +1,43 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/mymod.cppm -emit-module-interface -o %t/mymod.pcm
+// RUN: %clang_cc1 -std=c++20 %t/consumer.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/mymod.cppm -emit-reduced-module-interface -o %t/mymod.pcm
+// RUN: %clang_cc1 -std=c++20 %t/consumer.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+
+// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf %t/mymod.cppm -emit-module-interface -o %t/mymod.pcm
+// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf %t/consumer.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf %t/mymod.cppm -emit-reduced-module-interface -o %t/mymod.pcm
+// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf %t/consumer.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+
+//--- r.h
+struct F {
+  template <typename... T> requires ((sizeof(T) > 0) && ...)
+  void operator()(T...) {}
+} f;
+
+struct G {
+  template <typename T, typename U>
+    requires requires(T t, U u) { t + u; }
+  void operator()(T, U) {}
+} g;
+
+//--- mymod.cppm
+module;
+#include "r.h"
+export module mymod;
+export using ::f;
+export using ::g;
+
+//--- consumer.cpp
+// expected-no-diagnostics
+void operator&&(struct X, struct X);
+void operator+(struct X, struct Y);
+#include "r.h"
+import mymod;
+
+void h() { f(); g(1, 2); }
diff --git a/clang/test/Modules/polluted-operator.cppm b/clang/test/Modules/polluted-operator.cppm
index 45cc5e37d6a64..e6f0cdf092414 100644
--- a/clang/test/Modules/polluted-operator.cppm
+++ b/clang/test/Modules/polluted-operator.cppm
@@ -71,9 +71,4 @@ export namespace std {
   using std::operator&&;
 }
 
-#ifdef SKIP_ODR_CHECK_IN_GMF
 // expected-no-diagnostics
-#else
-// expected-error@* {{has different definitions in different modules; first difference is defined here found data member '_S_copy_ctor' with an initializer}}
-// expected-note@* {{but in 'a.<global>' found data member '_S_copy_ctor' with a different initializer}}
-#endif

[mizvekov]

This problem has a mirror in that the same difficulties are encountered when profiling CXXOperatorCallExpr, and the same solution we have implemented for that applies here.

[mizvekov]

This will make us ignore a bunch of differences in noexcept signatures, even when modules are not concerned at all.

[ChuanqiXu9]

Adding some path to only ignore it when we're merging entities.

[ChuanqiXu9]

This problem has a mirror in that the same difficulties are encountered when profiling CXXOperatorCallExpr, and the same solution we have implemented for that applies here.

Could you elaborate? I can only guess right now.

If you're saying we should extract the operator kind for UnresolvedLookupExpr? If yes, I feel it may not work. As the expression is nullptr in another context. See https://godbolt.org/z/rc5br469M for an example.

If you're saying we should perform more trick in CXXFoldExpr? I feel we've already done that in #190732 . But with CXXOperatorCallExpr, we have https://godbolt.org/z/rc5br469M , I mean it doesn't working right now. Do you think we need to improve in VisitCXXOperatorCallExpr?

[mizvekov]

This problem has a mirror in that the same difficulties are encountered when profiling CXXOperatorCallExpr, and the same solution we have implemented for that applies here.

Could you elaborate? I can only guess right now.

If you're saying we should extract the operator kind for UnresolvedLookupExpr? If yes, I feel it may not work. As the expression is nullptr in another context. See https://godbolt.org/z/rc5br469M for an example.

The StmtProfiler needs to profile the expression syntactically, ignoring implicit differences purely caused by semantic analysis.

So for exemple, VisitCXXOperatorCallExpr deals with that same problem as the issue this is trying to solve, because Sema can produce a CXXOperatorCallExpr expression when an overloaded operator is available, and another kind when it's not.

So we profile both cases the same as if the overloaded operator was not there, by pretending that instead of having a CXXOperatorCallExpr, we had an expression which used the underlying syntactic operator instead (ie UnaryOperator, BinaryOperator, etc).

[ChuanqiXu9]

This problem has a mirror in that the same difficulties are encountered when profiling CXXOperatorCallExpr, and the same solution we have implemented for that applies here.

Could you elaborate? I can only guess right now.
If you're saying we should extract the operator kind for UnresolvedLookupExpr? If yes, I feel it may not work. As the expression is nullptr in another context. See https://godbolt.org/z/rc5br469M for an example.

The StmtProfiler needs to profile the expression syntactically, ignoring implicit differences purely caused by semantic analysis.

So for exemple, VisitCXXOperatorCallExpr deals with that same problem as the issue this is trying to solve, because Sema can produce a CXXOperatorCallExpr expression when an overloaded operator is available, and another kind when it's not.

So we profile both cases the same as if the overloaded operator was not there, by pretending that instead of having a CXXOperatorCallExpr, we had an expression which used the underlying syntactic operator instead (ie UnaryOperator, BinaryOperator, etc).

I guess I got your point. What you want may look like

void VisitCXXFoldExpr(const CXXFoldExpr *S) {
      if (UnresolvedLookupExpr *ULE = S->getCallee()) {
              Expr *Underlying = ULE->getUnderlyingOperator(); // fake API
              Visit(Underlying);
      } else
         ID.AddInteger(0);
      ...
}

Right?

If yes, it can't solve the problem. As it will still produce different results for the fold expression which have no operator&& in the context.

I think if we want to solve the problems, we have to profiling UnresolvedLookupExpr as nullptr in these cases.

[ChuanqiXu9]

FWIW, CXXFoldExpr already have profiled the underlying operator:

llvm-project/clang/lib/AST/StmtProfile.cpp

Lines 2404 to 2405 in f2cb2a1

	ID.AddInteger(S->getOperator());
	}

So it looks like, my previous PR is already what you want?

[mizvekov]

It's not about profiling the underlying operator, it's about profiling an expression kind as if it were a different kind.

So you have to perform a notional rewrite of an expression that involves an operator into the equivalent expression that would have been produced if you had not.

[ChuanqiXu9]

It's not about profiling the underlying operator, it's about profiling an expression kind as if it were a different kind.

So you have to perform a notional rewrite of an expression that involves an operator into the equivalent expression that would have been produced if you had not.

I am not sure if I understood. I feel like what you're saying is the same thing as I had in 4c2e49d

could you elaborate?

[mizvekov]

I am not sure if I understood. I feel like what you're saying is the same thing as I had in 4c2e49d

could you elaborate?

I think its close to what that commit proposes to do, but there are some problems.

For example, it skips a VisitStmtNoChildren call to the FoldExpr, which could introduce ambiguities.

It would have helped to explain in a comment that it intentionally doesn't profile the callee sub-expression, and that's basically the difference between what existed before.

[ChuanqiXu9]

I am not sure if I understood. I feel like what you're saying is the same thing as I had in 4c2e49d
could you elaborate?

I think its close to what that commit proposes to do, but there are some problems.

For example, it skips a VisitStmtNoChildren call to the FoldExpr, which could introduce ambiguities.

It would have helped to explain in a comment that it intentionally doesn't profile the callee sub-expression, and that's basically the difference between what existed before.

Thanks. Sent #195983

[mizvekov]

Closing as superseded by #195983