[clang] fix assert in ADL finding entity in the implicit global module

[mizvekov]

This adds to the assert the implicit global module case as in module purview.

Fixes #109879

[llvmbot]

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Matheus Izvekov (mizvekov)

Changes

This adds to the assert the implicit global module case as in module purview.

Fixes #109879

---

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

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+5-2)
• (modified) clang/lib/Sema/SemaLookup.cpp (+3-2)
• (added) clang/test/Modules/GH109879-1.cpp (+26)
• (added) clang/test/Modules/GH109879-2.cpp (+29)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e511614fcf2451..5923888383022a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -43,7 +43,7 @@ code bases.
   still supporting SPARC V8 CPUs need to specify ``-mcpu=v8`` with a
   `config file
   <https://clang.llvm.org/docs/UsersManual.html#configuration-files>`_.
-  
+
 - The ``clang-rename`` tool has been removed.
 
 C/C++ Language Potentially Breaking Changes
@@ -115,7 +115,7 @@ C++ Language Changes
 - Allow single element access of GCC vector/ext_vector_type object to be
   constant expression. Supports the `V.xyzw` syntax and other tidbits
   as seen in OpenCL. Selecting multiple elements is left as a future work.
-- Implement `CWG1815 <https://wg21.link/CWG1815>`_. Support lifetime extension 
+- Implement `CWG1815 <https://wg21.link/CWG1815>`_. Support lifetime extension
   of temporary created by aggregate initialization using a default member
   initializer.
 
@@ -452,6 +452,9 @@ Miscellaneous Clang Crashes Fixed
 
 - Fixed ``-ast-dump`` crashes on codes involving ``concept`` with ``-ast-dump-decl-types``. (#GH94928)
 
+- Fixed internal assertion firing when a declaration in the implicit global
+  module is found through ADL. (GH#109879)
+
 OpenACC Specific Changes
 ------------------------
 
diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index ed5d44aa898f4f..f3f62474d06441 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -3850,8 +3850,9 @@ void Sema::ArgumentDependentLookup(DeclarationName Name, SourceLocation Loc,
             // exports are only valid in module purview and outside of any
             // PMF (although a PMF should not even be present in a module
             // with an import).
-            assert(FM && FM->isNamedModule() && !FM->isPrivateModule() &&
-                   "bad export context");
+            assert(FM &&
+                   (FM->isNamedModule() || FM->isImplicitGlobalModule()) &&
+                   !FM->isPrivateModule() && "bad export context");
             // .. are attached to a named module M, do not appear in the
             // translation unit containing the point of the lookup..
             if (D->isInAnotherModuleUnit() &&
diff --git a/clang/test/Modules/GH109879-1.cpp b/clang/test/Modules/GH109879-1.cpp
new file mode 100644
index 00000000000000..a984556b382e20
--- /dev/null
+++ b/clang/test/Modules/GH109879-1.cpp
@@ -0,0 +1,26 @@
+// 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/B.cppm -fprebuilt-module-path=%t -emit-module-interface -o %t/B.pcm
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -fprebuilt-module-path=%t -verify %t/C.cpp
+
+//--- A.cppm
+module;
+export module A;
+export extern "C" void foo(struct Bar);
+
+//--- B.cppm
+module;
+import A;
+export module B;
+
+//--- C.cpp
+import B;
+struct Bar {};
+void test() {
+  foo(Bar());
+  // expected-error@-1 {{declaration of 'foo' must be imported}}
+  // [email protected]:3 {{declaration here is not visible}}
+}
diff --git a/clang/test/Modules/GH109879-2.cpp b/clang/test/Modules/GH109879-2.cpp
new file mode 100644
index 00000000000000..ccec57839898a2
--- /dev/null
+++ b/clang/test/Modules/GH109879-2.cpp
@@ -0,0 +1,29 @@
+// 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/B.cppm -fprebuilt-module-path=%t -emit-module-interface -o %t/B.pcm
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -fprebuilt-module-path=%t -verify %t/C.cpp
+
+//--- foo.h
+struct Bar {};
+extern "C" void foo(struct Bar);
+
+//--- A.cppm
+module;
+#include "foo.h"
+export module A;
+export extern "C" using ::foo;
+//--- B.cppm
+module;
+import A;
+export module B;
+
+//--- C.cpp
+// expected-no-diagnostics
+import B;
+#include "foo.h"
+void test() {
+  foo(Bar());
+}

[ChuanqiXu9]

LGTM

[ChuanqiXu9]

nit: generally we don't like such unrelated changes. We can land them seperately in other NFC patches.

[mizvekov]

This is just stray whitespace cleanup that is automatically performed by most unix editors.

I don't think doing a whole separate commit to fix it is worth it, specially as there aren't a lot of lines changed here.

We do generally just ignore these unless there is so much noise it makes review difficult.

[shafik]

Please don't do this in the future the policy is indeed to keep unrelated changes separate.

There are many practical reasons for this but mainly it reduces churn in the event of revert now we lose multiple changes and if they where unrelated that is just a waste all around.