[C++20][Modules] Update ADL to handle basic.lookup.argdep p4 [P1815R2 part 1]

[iains]

Summary

This includes the revised provisions of [basic.lookup.argdep] p4

1. ADL is amended to handle p 4.3 where functions in trasitively imported modules may

become visible when they are exported in the same namespace as a visible type.

2. If a function is in a different modular TU, and has internal-linkage, we invalidate

its entry in an overload set.

[basic.lookup.argdep] p5 ex 2 now passes.

[ChuanqiXu]

clang/include/clang/Sema/Overload.h

800

clang/lib/Sema/SemaOverload.cpp

6403

6406

The current implementation may reject following two cases:

module;
static void foo(int); // Ignore header
...
export module A;
void bar() { foo(5); } // Should it be invalid?

and

export module A;
static void foo(int);
...
module :private;
void bar() { foo(5); } // Should it be invalid?

I mean in the above examples, the function of foo(int) is defined in the same TU but the call to it might be rejected.

clang/test/CXX/basic/basic.link/p10-ex2.cpp

10–35 ↗

(On Diff #442459)

I feel like the test is irrelevant with the revision, isn't it? If yes, I think we could land this as a separate NFC patch.

clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p5-ex2.cpp

37–42

This looks more consistent with the example.

[iains]

clang/lib/Sema/SemaOverload.cpp

6406

The current implementation may reject following two cases:

module;
static void foo(int); // Ignore header
...
export module A;
void bar() { foo(5); } // Should it be invalid?

and

export module A;
static void foo(int);
...
module :private;
void bar() { foo(5); } // Should it be invalid?

I mean in the above examples, the function of foo(int) is defined in the same TU but the call to it might be rejected.

6406

The current implementation may reject following two cases:

module;
static void foo(int); // Ignore header
...

Actually, I have a note to check on the global module case, since we have special rules that allow merging of items there,

export module A;
void bar() { foo(5); } // Should it be invalid?

and

export module A;
static void foo(int);
...
module :private;
void bar() { foo(5); } // Should it be invalid?

I mean in the above examples, the function of `foo(int)` is defined in the same TU but the call to it might be rejected.

otherwise, agreed, these cases should be ok - I guess we need a second test case with lookups that should succeed.

clang/test/CXX/basic/basic.link/p10-ex2.cpp

10–35 ↗

(On Diff #442459)

well, it is related to the paper mentioned, but yes we can make this separate.

clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p5-ex2.cpp

37–42

This changes the example so that M is directly included in the implementation rather than transitively (the example does specifically use a different module name for the implementation)

I am not sure what you mean by "more consistent"
(the example will fail to reject some of the lookups with this change).

[iains]

address review comments.

[iains]

TODO: second test-case for cases that should succeed, split out basic-link / p10-ex2 test case.

[ChuanqiXu]

clang/lib/Sema/SemaOverload.cpp

6406

Actually, I have a note to check on the global module case, since we have special rules that allow merging of items there,

Yeah, it is surprising that we couldn't handle the global module fragment case.

clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p5-ex2.cpp

37–42

Oh, I didn't notice it uses a different module name. My bad.

[iains]

updated after clarification of the provisions of basic.lookup.argdep p4

[ChuanqiXu]

clang/lib/Sema/SemaLookup.cpp

3870

nit: Although it should be true due to D is ExportDeclContext, it looks better to add an assertion at the first sight.

3873–3874

3875–3889

nit: how do you think about the suggested style? (not required)

3878

The std says module instead of module unit. So I think we should consider partition unit here. So:

• We need to add a test about partition in the revision.
• We need to add isInSameModule methods in other revisions. I know we've talked this several times before... I'll take a look.

3882–3884

Maybe it is worth to implement getNonInlineEnclosingNamespaceContext and we could simplify the code here:

clang/lib/Sema/SemaOverload.cpp

6407

I introduced isModuleUnitOfCurrentTU to simplify the code a little bit.

[iains]

rebased, addressed review comments.

[iains]

sorry got a little delayed in responding to this (sorting out some testing problems)

clang/lib/Sema/SemaLookup.cpp

3870

OK, Since exports must be in module purview and not in PMF (which we should expect from existing checks)
let's then check we have a correct context for export - which will avoid needing to re-check this below.

3873–3874

OK, we can use this here, although it is probably a little more than necessary, since we already know that FM cannot be a GMF or PMF ( but the function will check those anyway)

3875–3889

I guess it's a little shorter, and roughly the same in readability,

3878

The std says module instead of module unit. So I think we should consider partition unit here.

yes, I guess so - it is a bit unfortunate that we need to use the name comparison again.

So:

• We need to add a test about partition in the revision.

TODO, probably better to add another example rather than modify the text of the standard one.

3882–3884

Perhaps. but I would prefer that to be a separate cleanup - since it would be touching code unrelated to this.
Also that does not actually save any work, it might be better to find a way to cache the computation at the time the associated entities are found.

[ChuanqiXu]

LGTM basically if the following comments addressed.

clang/lib/Sema/SemaLookup.cpp

3875–3889

The current implementation skips a break of the outer loop.

clang/lib/Sema/SemaOverload.cpp

6407

This is not addressed.

[iains]

rebased, addressed review comments

[iains]

it would be good to get this landed before llvm-15 branches..

clang/lib/Sema/SemaLookup.cpp

3875–3889

I'm not sure exactly what you are referring to here (maybe I missed something) -

[ChuanqiXu]

clang/lib/Sema/SemaLookup.cpp

3875–3889

hmmm, sorry, I misread something. It should be fine.

[ChuanqiXu]

FYI

When I implements/tests the modularized STL for libstdc++ (https://github.com/ChuanqiXu9/stdmodules), I find the patch blocks it. Due to the time is limited (I really wish we could have something workable in clang15), I made a pre-review commit in: https://github.com/llvm/llvm-project/commit/15ddc09ef9b05ffd5398165049b5202264fa203a

The problem shows in the tests:

struct A {};

 template<typename Func>
 constexpr bool __call_is_nt(A)
 {
     return true;
 }
 ns::A make();

 template <typename T>
 bool foo(T t) {
     auto func = [](){};
     return __call_is_nt<decltype(func)>(t);
 }

The linkage of __call_is_nt<decltype(func)> in clang is internal linkage. So it is refused by this patch. This is not the fault of the patch. According to [basic.link], only names have linkage. So __call_is_nt<decltype(func)> should have same linkage with its template, which should be external.

The root cause for the problem is that the semantics of internal linkage in clang is a mixed form of internal linkage and TU-Local in C++ Specifications. So here is the slight difference. It is hard to fix the root problem so I choose to give a workaround here.

clang/include/clang/Sema/Overload.h

enum OverloadFailureKind {
     /// This constructor/conversion candidate fail due to an address space
     /// mismatch between the object being constructed and the overload
     /// candidate.
     ovl_fail_object_addrspace_mismatch,
 
     /// This candidate was not viable because its associated constraints were
     /// not satisfied.
     ovl_fail_constraints_not_satisfied,
+
+    /// This candidate was not viable because it has internal linkage and is
+    /// from a different module unit than the use.
+    ovl_fail_module_mismatched,
   };
 
   /// A list of implicit conversion sequences for the arguments of an
   /// OverloadCandidate.
   using ConversionSequenceList =
       llvm::MutableArrayRef<ImplicitConversionSequence>;
 
   /// OverloadCandidate - A single candidate in an overload set (C++ 13.3).

clang/lib/Sema/SemaLookup.cpp

for (auto *NS : AssociatedNamespaces) {
     //
     //     -- Any using-directives in the associated namespace are
     //        ignored.
     //
     //     -- Any namespace-scope friend functions declared in
     //        associated classes are visible within their respective
     //        namespaces even if they are not visible during an ordinary
     //        lookup (11.4).
+    //
+    // C++20 [basic.lookup.argdep] p4.3
+    //     -- are exported, are attached to a named module M, do not appear
+    //        in the translation unit containing the point of the lookup, and
+    //        have the same innermost enclosing non-inline namespace scope as
+    //        a declaration of an associated entity attached to M.
     DeclContext::lookup_result R = NS->lookup(Name);
     for (auto *D : R) {
       auto *Underlying = D;
       if (auto *USD = dyn_cast<UsingShadowDecl>(D))
         Underlying = USD->getTargetDecl();
 
       if (!isa<FunctionDecl>(Underlying) &&
           !isa<FunctionTemplateDecl>(Underlying))
         continue;
 
       // The declaration is visible to argument-dependent lookup if either
       // it's ordinarily visible or declared as a friend in an associated
       // class.
       bool Visible = false;
       for (D = D->getMostRecentDecl(); D;
            D = cast_or_null<NamedDecl>(D->getPreviousDecl())) {
         if (D->getIdentifierNamespace() & Decl::IDNS_Ordinary) {
           if (isVisible(D)) {
             Visible = true;
             break;
+          } else if (getLangOpts().CPlusPlusModules &&
+                     D->isInExportDeclContext()) {
+            // C++20 [basic.lookup.argdep] p4.3 .. are exported ...
+            Module *FM = D->getOwningModule();
+            // 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->isModulePurview() && !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 (!isModuleUnitOfCurrentTU(FM) &&
+                llvm::any_of(AssociatedClasses, [&](auto *E) {
+                  // ... and have the same innermost enclosing non-inline
+                  // namespace scope as a declaration of an associated entity
+                  // attached to M
+                  if (!E->hasOwningModule() ||
+                      E->getOwningModule()->getTopLevelModuleName() !=
+                          FM->getTopLevelModuleName())
+                    return false;
+                  // TODO: maybe this could be cached when generating the
+                  // associated namespaces / entities.
+                  DeclContext *Ctx = E->getDeclContext();
+                  while (!Ctx->isFileContext() || Ctx->isInlineNamespace())
+                    Ctx = Ctx->getParent();
+                  return Ctx == NS;
+                })) {
+              Visible = true;
+              break;
+            }
           }
         } else if (D->getFriendObjectKind()) {
           auto *RD = cast<CXXRecordDecl>(D->getLexicalDeclContext());
           // [basic.lookup.argdep]p4:
           //   Argument-dependent lookup finds all declarations of functions and
           //   function templates that
           //  - ...
           //  - are declared as a friend ([class.friend]) of any class with a

clang/lib/Sema/SemaOverload.cpp

void Sema::AddOverloadCandidate(
   // allowing them in this context, but keep them around so we can point
   // to them in diagnostics.
   if (!AllowExplicit && ExplicitSpecifier::getFromDecl(Function).isExplicit()) {
     Candidate.Viable = false;
     Candidate.FailureKind = ovl_fail_explicit;
     return;
   }
 
+  // Functions with internal linkage are only viable in the same module unit.
+  if (auto *MF = Function->getOwningModule()) {
+    if (getLangOpts().CPlusPlusModules && !MF->isModuleMapModule() &&
+        Function->getFormalLinkage() <= Linkage::InternalLinkage &&
+        !isModuleUnitOfCurrentTU(MF)) {
+      Candidate.Viable = false;
+      Candidate.FailureKind = ovl_fail_module_mismatched;
+      return;
+    }
+  }
+
   if (Function->isMultiVersion() && Function->hasAttr<TargetAttr>() &&
       !Function->getAttr<TargetAttr>()->isDefaultVersion()) {
     Candidate.Viable = false;
     Candidate.FailureKind = ovl_non_default_multiversion_function;
     return;
   }
 
   if (Constructor) {

clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p5-ex2.cpp

+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 M.cpp -emit-module-interface -o M.pcm
+// RUN: %clang_cc1 -std=c++20 N.cpp -emit-module-interface -o N.pcm \
+// RUN:   -fmodule-file=M.pcm
+// RUN: %clang_cc1 -std=c++20 Q.cpp -emit-module-interface -o Q.pcm
+// RUN: %clang_cc1 -std=c++20 Q-impl.cpp -fsyntax-only -fmodule-file=Q.pcm \
+// RUN:   -fmodule-file=N.pcm -verify
+
+//--- M.cpp
+export module M;
+namespace R {
+export struct X {};
+export void f(X);
+} // namespace R
+namespace S {
+export void f(R::X, R::X);
+}
+
+//--- N.cpp
+export module N;
+import M;
+export R::X make();
+namespace R {
+static int g(X);
+}
+export template <typename T, typename U>
+void apply(T t, U u) {
+  f(t, u);
+  g(t);
+}
+
+//--- Q.cpp
+export module Q;
+
+//--- Q-impl.cpp
+module Q;
+import N;
+
+namespace S {
+struct Z {
+  template <typename T> operator T();
+};
+} // namespace S
+void test() {
+  // OK, decltype(x) is R::X in module M
+  auto x = make();
+
+  // error: R and R::f are not visible here
+  R::f(x); // expected-error {{declaration of 'R' must be imported from module 'N' before it is required}}
+  // [e-mail]:4 {{declaration here is not visible}}
+  // expected-error@-2 {{no type named 'f' in namespace 'R'}}
+
+  f(x); // Found by [basic.lookup.argdep] / p4.3
+
+  // error: S::f in module M not considered even though S is an associated
+  // namespace, since the entity Z is in a different module from f.
+  f(x, S::Z()); // expected-error {{no matching function for call to 'f'}}
+  // [e-mail]:4 {{candidate function not viable: requires 1 argument, but 2 were provided}}
+
+  // error: S::f is visible in instantiation context, but  R::g has internal
+  // linkage and cannot be used outside N.cpp
+  apply(x, S::Z()); // [e-mail]:10 {{no matching function for call to 'g'}}
+                    // expected-note@-1 {{in instantiation of function template specialization 'apply<R::X, S::Z>' requested here}}
+}