[Sema] ADL: Associated namespaces for class types and enumeration types (CWG 1691)

[riccibruno]

Summary

CWG 1691 changed the definition of the namespaces associated with a class type or enumeration type.

For a class type, the associated namespaces are the innermost enclosing namespaces of the associated classes.
For an enumeration type, the associated namespace is the innermost enclosing namespace of its declaration.

This also fixes CWG 1690 and CWG 1692.

I have applied the fix to all language versions, even though I think that the DR only applies to C++14. The reason for this is that it is possible to return a local type from a lambda in C++11, and I don't think that this will affect anyone (although this can totally be a failure of my imagination...)

[riccibruno]

Removed the call to isTransparentContext() in the loop in CollectEnclosingNamespace. It is not actually needed since we only care about finding the innermost enclosing namespace.

[rjmccall]

Hmm. Does this never impact code that's just using a locally-defined type within its scope? I guess if ADL is involved, unqualified lookup must have reached the scope of the innermost namespace, and so it would be searching that namespace anyway.

In that case, I think I'm mollified that the source-compatibility risk is low and we should just unconditionally apply the new rule. LGTM.

[rsmith]

I have applied the fix to all language versions, even though I think that the DR only applies to C++14

DRs don't have a specific version that they are intended to apply to; that's up to us to determine, and generally we apply them retroactively as far back as they make sense -- they are supposed to be bug fixes, after all. (From ISO's perspective, C++11 was retracted and replaced by C++14 when the latter was published, so it's simply not meaningful for a DR to officially target anything older than that, and the only version that DRs can meaningfully list is whatever version was most recently published when they were approved.)

This looks good to me. Thanks!

[riccibruno]

In D60573#1463569, @rjmccall wrote:

Hmm. Does this never impact code that's just using a locally-defined type within its scope? I guess if ADL is involved, unqualified lookup must have reached the scope of the innermost namespace, and so it would be searching that namespace anyway.

In that case, I think I'm mollified that the source-compatibility risk is low and we should just unconditionally apply the new rule. LGTM.

I am not sure about what you mean. It is certainly possible to construct a piece of C++11 code which breaks with this patch. A possible example is:

constexpr int f(void *) { return 1; }

static auto lambda = []() { struct S {} s; return s; };
using S = decltype(lambda());

template <typename T> void test() {
    constexpr T *p = nullptr;
    static_assert(f(p) == 1, "");
}

constexpr int f(S *) { return 2; }

template void test<S>();

Clang currently accept this because ADL does not currently find the second better matching f. After this patch it will be found and the static_assert will fail (which is the behavior of gcc, and as far as I can tell the specified behavior).

[riccibruno]

In D60573#1463611, @rsmith wrote:

I have applied the fix to all language versions, even though I think that the DR only applies to C++14

DRs don't have a specific version that they are intended to apply to; that's up to us to determine, and generally we apply them retroactively as far back as they make sense -- they are supposed to be bug fixes, after all. (From ISO's perspective, C++11 was retracted and replaced by C++14 when the latter was published, so it's simply not meaningful for a DR to officially target anything older than that, and the only version that DRs can meaningfully list is whatever version was most recently published when they were approved.)

This looks good to me. Thanks!

Good to know, thanks for the explanation and review !

[rjmccall]

In D60573#1463641, @riccibruno wrote:

In D60573#1463569, @rjmccall wrote:

Hmm. Does this never impact code that's just using a locally-defined type within its scope? I guess if ADL is involved, unqualified lookup must have reached the scope of the innermost namespace, and so it would be searching that namespace anyway.

In that case, I think I'm mollified that the source-compatibility risk is low and we should just unconditionally apply the new rule. LGTM.

I am not sure about what you mean. It is certainly possible to construct a piece of C++11 code which breaks with this patch.

Yes, but these examples are contrived, so it's easy to rationalize that the source impact is low. The typical use-pattern of a local type is that you only use it locally, so the most important question would be whether it is possible to change the semantics of, say,

void test() {
  struct A { ... };
  foo(A{});
}

But I think the answer is "no", for the reasons I explained.

[riccibruno]

In D60573#1463777, @rjmccall wrote:

In D60573#1463641, @riccibruno wrote:

In D60573#1463569, @rjmccall wrote:

Hmm. Does this never impact code that's just using a locally-defined type within its scope? I guess if ADL is involved, unqualified lookup must have reached the scope of the innermost namespace, and so it would be searching that namespace anyway.

In that case, I think I'm mollified that the source-compatibility risk is low and we should just unconditionally apply the new rule. LGTM.

I am not sure about what you mean. It is certainly possible to construct a piece of C++11 code which breaks with this patch.

Yes, but these examples are contrived, so it's easy to rationalize that the source impact is low. The typical use-pattern of a local type is that you only use it locally, so the most important question would be whether it is possible to change the semantics of, say,

void test() {
  struct A { ... };
  foo(A{});
}

But I think the answer is "no", for the reasons I explained.

I can't imagine a way to change the result of the lookup for foo in an example like this without escaping the local type from the function.

Thanks for the review !

lib/Sema/SemaLookup.cpp

namespace {
   private:
     Sema::AssociatedClassSet ClassesTransitive;
   };
 } // end anonymous namespace
 
 static void
 addAssociatedClassesAndNamespaces(AssociatedLookup &Result, QualType T);
 
+// Given the declaration context \param Ctx of a class, class template or
+// enumeration, add the associated namespaces to \param Namespaces as described
+// in [basic.lookup.argdep]p2.
 static void CollectEnclosingNamespace(Sema::AssociatedNamespaceSet &Namespaces,
                                       DeclContext *Ctx) {
-  // Add the associated namespace for this class.
-
-  // We don't use DeclContext::getEnclosingNamespaceContext() as this may
-  // be a locally scoped record.
+  // The exact wording has been changed in C++14 as a result of
+  // CWG 1691 (see also CWG 1690 and CWG 1692). We apply it unconditionally
+  // to all language versions since it is possible to return a local type
+  // from a lambda in C++11.
+  //
+  // C++14 [basic.lookup.argdep]p2:
+  //   If T is a class type [...]. Its associated namespaces are the innermost
+  //   enclosing namespaces of its associated classes. [...]
+  //
+  //   If T is an enumeration type, its associated namespace is the innermost
+  //   enclosing namespace of its declaration. [...]
 
-  // We skip out of inline namespaces. The innermost non-inline namespace
+  // We additionally skip inline namespaces. The innermost non-inline namespace
   // contains all names of all its nested inline namespaces anyway, so we can
   // replace the entire inline namespace tree with its root.
-  while (Ctx->isRecord() || Ctx->isTransparentContext() ||
-         Ctx->isInlineNamespace())
+  while (Ctx->isInlineNamespace() || Ctx->isTransparentContext() ||
+         !Ctx->isFileContext())
     Ctx = Ctx->getParent();
 
-  if (Ctx->isFileContext())
-    Namespaces.insert(Ctx->getPrimaryContext());
 }
 
 // Add the associated classes and namespaces for argument-dependent
-// lookup that involves a template argument (C++ [basic.lookup.koenig]p2).
+// lookup that involves a template argument (C++ [basic.lookup.argdep]p2).
 static void
 addAssociatedClassesAndNamespaces(AssociatedLookup &Result,
                                   const TemplateArgument &Arg) {
-  // C++ [basic.lookup.koenig]p2, last bullet:
+  // C++ [basic.lookup.argdep]p2, last bullet:
   //   -- [...] ;
   switch (Arg.getKind()) {
     case TemplateArgument::Null:
       break;
 
     case TemplateArgument::Type:
       // [...] the namespaces and classes associated with the types of the
       // template arguments provided for template type parameters (excluding
switch (Arg.getKind()) {
 
     case TemplateArgument::Pack:
       for (const auto &P : Arg.pack_elements())
         addAssociatedClassesAndNamespaces(Result, P);
       break;
   }
 }
 
-// Add the associated classes and namespaces for
-// argument-dependent lookup with an argument of class type
-// (C++ [basic.lookup.koenig]p2).
+// Add the associated classes and namespaces for argument-dependent lookup
+// with an argument of class type (C++ [basic.lookup.argdep]p2).
 static void
 addAssociatedClassesAndNamespaces(AssociatedLookup &Result,
                                   CXXRecordDecl *Class) {
 
   // Just silently ignore anything whose name is __va_list_tag.
   if (Class->getDeclName() == Result.S.VAListTagName)
     return;
 
-  // C++ [basic.lookup.koenig]p2:
+  // C++ [basic.lookup.argdep]p2:
   //   [...]
   //     -- If T is a class type (including unions), its associated
   //        classes are: the class itself; the class of which it is a
-  //        member, if any; and its direct and indirect base
-  //        classes. Its associated namespaces are the namespaces in
-  //        which its associated classes are defined.
+  //        member, if any; and its direct and indirect base classes.
+  //        Its associated namespaces are the innermost enclosing
+  //        namespaces of its associated classes.
 
   // Add the class of which it is a member, if any.
   DeclContext *Ctx = Class->getDeclContext();
   if (CXXRecordDecl *EnclosingClass = dyn_cast<CXXRecordDecl>(Ctx))
     Result.Classes.insert(EnclosingClass);
+
   // Add the associated namespace for this class.
   CollectEnclosingNamespace(Result.Namespaces, Ctx);
 
   // -- If T is a template-id, its associated namespaces and classes are
   //    the namespace in which the template is defined; for member
   //    templates, the member template's class; the namespaces and classes
   //    associated with the types of the template arguments provided for
   //    template type parameters (excluding template template parameters); the
case Type::VariableArray:
       continue;
 
     //     -- If T is a fundamental type, its associated sets of
     //        namespaces and classes are both empty.
     case Type::Builtin:
       break;
 
     //     -- If T is a class type (including unions), its associated
-    //        classes are: the class itself; the class of which it is a
-    //        member, if any; and its direct and indirect base
-    //        classes. Its associated namespaces are the namespaces in
-    //        which its associated classes are defined.
+    //        classes are: the class itself; the class of which it is
+    //        a member, if any; and its direct and indirect base classes.
+    //        Its associated namespaces are the innermost enclosing
+    //        namespaces of its associated classes.
     case Type::Record: {
       CXXRecordDecl *Class =
           cast<CXXRecordDecl>(cast<RecordType>(T)->getDecl());
       addAssociatedClassesAndNamespaces(Result, Class);
       break;
     }
 
-    //     -- If T is an enumeration type, its associated namespace is
-    //        the namespace in which it is defined. If it is class
-    //        member, its associated class is the member's class; else
-    //        it has no associated class.
+    //     -- If T is an enumeration type, its associated namespace
+    //        is the innermost enclosing namespace of its declaration.
+    //        If it is a class member, its associated class is the
+    //        member’s class; else it has no associated class.
     case Type::Enum: {
       EnumDecl *Enum = cast<EnumType>(T)->getDecl();
 
       DeclContext *Ctx = Enum->getDeclContext();
       if (CXXRecordDecl *EnclosingClass = dyn_cast<CXXRecordDecl>(Ctx))
         Result.Classes.insert(EnclosingClass);
 
-      // Add the associated namespace for this class.
+      // Add the associated namespace for this enumeration.
       CollectEnclosingNamespace(Result.Namespaces, Ctx);
 
       break;
     }
 
     //     -- If T is a function type, its associated namespaces and
     //        classes are those associated with the function parameter
     //        types and those associated with the return type.

test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp

namespace adl_class_type {
 
   // associated class: itself, local type
   namespace X2 {
     auto foo() {
       struct S {} s;
       return s;
     }
     using S = decltype(foo());
-    void f(S); // expected-note {{'X2::f' declared here}}
+    void f(S); // #1
   }
   void test2() {
-    f(X2::S{}); // FIXME: This is well-formed; X2 is the innermost enclosing namespace
-                // of the local struct S.
-                // expected-error@-2 {{use of undeclared identifier 'f'}}
+    f(X2::S{}); // This is well-formed; X2 is the innermost enclosing namespace
+                // of the local struct S. Calls #1.
   }
 
   // associated class: the parent class
   namespace X3 {
     struct S {
       struct T {};
       friend void f(T);
     };
namespace adl_class_type {
   namespace X5 {
     namespace N {
       auto get_lambda() { return [](){}; }
       void f(decltype(get_lambda()));
     }
 
     void test5() {
       auto lambda = N::get_lambda();
-      f(lambda); // FIXME: This is well-formed. expected-error {{use of undeclared}}
+      f(lambda); // ok
     }
   }
 } // namespace adl_class_type
 
 // class template specialization: as for class type plus
 //   for non-type template arguments:
 //    - nothing
 //   for type template arguments:

 namespace adl_enumeration_type {
   namespace N {
     enum E : int;
     void f(E);
     struct S {
       enum F : int;
       friend void g(F);
     };
+    auto foo() {
+      enum G {} g;
+      return g;
+    }
+    using G = decltype(foo());
+    void h(G);
   }
 
   void test() {
     N::E e;
     f(e); // ok
     N::S::F f;
     g(f); // ok
+    N::G g;
+    h(g); // ok
+
   }
 }
 
 // pointer and reference type:
 //  associated namespaces and classes of the pointee type
 // array type:
 //  associated namespaces and classes of the base type
 namespace adl_point_array_reference_type {

test/CXX/drs/dr16xx.cpp

 
 #if __cplusplus > 201703L
   enum E1 {};
   enum E2 {};
   auto c = To<E1>() <=> To<E2>(); // expected-error {{invalid operands to binary expression ('To<dr1687::E1>' and 'To<dr1687::E2>')}}
 #endif
 }
 
+namespace dr1690 { // dr1690: 9
+  // See also the various tests in "CXX/basic/basic.lookup/basic.lookup.argdep".
+#if __cplusplus >= 201103L
+  namespace N {
+    static auto lambda = []() { struct S {} s; return s; };
+    void f(decltype(lambda()));
+  }
+
+  void test() {
+    auto s = N::lambda();
+    f(s); // ok
+  }
+#endif
+}
+
+namespace dr1691 { // dr1691: 9
+#if __cplusplus >= 201103L
+  namespace N {
+    namespace M {
+      enum E : int;
+      void f(E);
+    }
+    enum M::E : int {};
+    void g(M::E); // expected-note {{declared here}}
+  }
+  void test() {
+    N::M::E e;
+    f(e); // ok
+    g(e); // expected-error {{use of undeclared}}
+  }
+#endif
+}
+
+namespace dr1692 { // dr1692: 9
+  namespace N {
+    struct A {
+      struct B {
+        struct C {};
+      };
+    };
+    void f(A::B::C);
+  }
+  void test() {
+    N::A::B::C c;
+    f(c); // ok
+  }
+}
+
 namespace dr1696 { // dr1696: 7
   namespace std_examples {
 #if __cplusplus >= 201402L
     extern struct A a;
     struct A {
       const A &x = { A{a, a} };
       const A &y = { A{} }; // expected-error {{default member initializer for 'y' needed within definition of enclosing class 'A' outside of member functions}} expected-note {{here}}
     };