[C++20] [Modules] Fix may-be incorrect ADL for module local entities

[ChuanqiXu9]

Close #123815

See the comments for details. We can't get primary context arbitrarily since the redecl may have different context and information.

There is a TODO for modules specific case, I'd like to make it after this PR.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

Changes

Close #123815

See the comments for details

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaLookup.cpp (+52-2)
• (added) clang/test/Modules/module-local-hidden-friend-2.cppm (+43)

diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index e18e3c197383e2..068f5231a50bd3 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -2913,8 +2913,58 @@ static void CollectEnclosingNamespace(Sema::AssociatedNamespaceSet &Namespaces,
   // replace the entire inline namespace tree with its root.
   while (!Ctx->isFileContext() || Ctx->isInlineNamespace())
     Ctx = Ctx->getParent();
-
-  Namespaces.insert(Ctx->getPrimaryContext());
+  
+  // Actually it is fine to always do `Namespaces.insert(Ctx);` simply. But it
+  // may cause more allocations in Namespaces and more unnecessary lookups. So
+  // we'd like to insert the representative namespace only.
+  DeclContext *PrimaryCtx = Ctx->getPrimaryContext();
+  Decl* PrimaryD = cast<Decl>(PrimaryCtx);
+  Decl *D = cast<Decl>(Ctx);
+  ASTContext &AST = D->getASTContext();
+
+  // TODO: Technically it is better to insert one namespace per module. e.g.,
+  //
+  // ```
+  // //--- first.cppm
+  // export module first;
+  // namespace ns { ... } // first namespace
+  //
+  // //--- m-partA.cppm
+  // export module m:partA;
+  // import first;
+  //
+  // namespace ns { ... }
+  // namespace ns { ... }
+  //
+  // //--- m-partB.cppm
+  // export module m:partB;
+  // import first;
+  // import :partA;
+  //
+  // namespace ns { ... }
+  // namespace ns { ... }
+  //
+  // ...
+  //
+  // //--- m-partN.cppm
+  // export module m:partN;
+  // import first;
+  // import :partA;
+  // ...
+  // import :part$(N-1);
+  //
+  // namespace ns { ... }
+  // namespace ns { ... }
+  //
+  // consume(ns::any_decl); // the lookup
+  // ```
+  //
+  // We should only insert once for all namespaces in module m.
+  if (D->isInNamedModule() && 
+      !AST.isInSameModule(D->getOwningModule(), PrimaryD->getOwningModule()))
+    Namespaces.insert(Ctx);
+  else
+    Namespaces.insert(PrimaryCtx);
 }
 
 // Add the associated classes and namespaces for argument-dependent
diff --git a/clang/test/Modules/module-local-hidden-friend-2.cppm b/clang/test/Modules/module-local-hidden-friend-2.cppm
new file mode 100644
index 00000000000000..d415e495abb213
--- /dev/null
+++ b/clang/test/Modules/module-local-hidden-friend-2.cppm
@@ -0,0 +1,43 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm \
+// RUN:     -fmodule-file=a=%t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/use.cc -fmodule-file=a=%t/a.pcm -fmodule-file=b=%t/b.pcm \
+// RUN:     -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-reduced-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-reduced-module-interface -o %t/b.pcm \
+// RUN:     -fmodule-file=a=%t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/use.cc -fmodule-file=a=%t/a.pcm -fmodule-file=b=%t/b.pcm \
+// RUN:     -fsyntax-only -verify
+
+//--- a.cppm
+export module a;
+
+namespace n {
+}
+
+//--- b.cppm
+export module b;
+import a;
+
+namespace n {
+struct monostate {
+	friend bool operator==(monostate, monostate) = default;
+};
+
+export struct wrapper {
+	friend bool operator==(wrapper const &, wrapper const &) = default;
+
+	monostate m_value;
+};
+}
+
+//--- use.cc
+// expected-no-diagnostics
+import b;
+
+static_assert(n::wrapper() == n::wrapper());

[ChuanqiXu9]

@nikic given the lookup process is on the hot path, it'll be appreciated if you can test the compilation time impact on this.

[ChuanqiXu9]

@nikic given the lookup process is on the hot path, it'll be appreciated if you can test the compilation time impact on this for non module users.

[nikic]

I'm not seeing any impact: https://llvm-compile-time-tracker.com/compare.php?from=ff0f1dd341cb2fdc1281a14413b3aa93bf9a20c7&to=4bfdd091675b9957686ac81457629e124950dc25&stat=instructions:u

[ChuanqiXu9]

I'm not seeing any impact: https://llvm-compile-time-tracker.com/compare.php?from=ff0f1dd341cb2fdc1281a14413b3aa93bf9a20c7&to=4bfdd091675b9957686ac81457629e124950dc25&stat=instructions:u

Thank you!

[ChuanqiXu9]

I'd like to land this given the scale of the patch and I want to fix it before 20.x and I may not be available in the next few days.