[clang] Do not hide base member using-decls with different template head.

[usaxena95]

Summary

Fixes: https://github.com/llvm/llvm-project/issues/50886

Adding requires clause to template head or constraining the template parameter type is ineffective because, even though it creates a non-equivalent template head temp.over.link#6 and hence eligible for overload resolution, Derived::foo still hides any previous using decl.
Clang diverges from gcc here and can be seen more clearly in this example:

struct base {
  template <int N, int M>
  int foo() { return 1; };
};

struct bar : public base {
  using base::foo;
  template <int N> 
  int foo() { return 2; };
};

int main() {
  bar f;
  f.foo<10, 10>(); // clang previously errored while GCC does not.
}

https://godbolt.org/z/v5hnh6czq. We see that bar::foo hides base::foo because it only differs in the head.

Adding a trailing requires to the definition was a nice find. In this case, clang considers them overloads because of mismatching requires clause.. So both of them make it to the overload resolution (where constrained Derived::foo is rejected then).

---

In this patch, we do not ignore matching the template head (template parameters, type contraints and trailing requires) while considering whether the using decl of base member should be hidden. The return type of a templated function is still not considered as different return types would create ambiguous candidates.

The changed tests looks reasonable and also matches GCC behaviour: https://godbolt.org/z/8KqPEThrY

Note: We are now able to create an ambiguity in case where both base member and derived member specialisations satisfy the constraints (when the constraints are not same). Ideally using-decl should not create ambiguity. I plan to fix this later if it gathers more attention.

[usaxena95]

Added more tests.

[ilya-biryukov]

Thanks for the change. All the behavior changes look reasonable to me, but I'm struggling to understand whether we follow the standard closely here.
Left a comment to discuss this in depth.

clang/lib/Sema/SemaOverload.cpp

1238

UseMemberUsingDeclRules is used everywhere in the code and in the comments for calls to IsOverload .

Let's maybe change the name in the header instead to avoid chasing all call sites?

1298

Lambda is only used once. Could you inline it to make the code simpler?

bool IsConstructorOrAssignment = false;
if (auto *CMD = dyn_cast<CXXMethodDecl>(FD); CMD && (...)) {
  IsConsutrctorOrAssignment = true;
}

1307

How is this related to checking whether the template parameter lists check?
I seems to be missing the logical connection that goes from the standard wording to the code. Could you explain?

An example that I have in mind is:

struct A {
    template <class T, class U = int> A(T);
    template <class T, class U = int> int foo(T);
};
struct B : A {
    using A::A;
    template <class T> B(T);

    using A::foo;
    template <class T> int foo(T);
};

namespace detail {
    template <class T> int bar(T);
}
using detail::bar;
template <class T, class U = int> int bar(T);

int a = bar(10); // this is definitely ambiguous.

B b(10); // should this be ambiguous?
int c = b.foo(10); // should this be ambiguous?
// Should constructors and functions behave differently? Why?

I see that both Clang and GCC treat member declarations differently, but I don't see why they should given that they are supposed to use the same "corresponds" terminology from the standard. What am I missing?

clang/test/SemaTemplate/concepts-using-decl.cpp

111

Why should they be dropped? Could you provide pointers from the standard?

[usaxena95]

I have switched to a version where we matched template heads only when they are constraints. This is for the moment to unblock the existing bugs and not break existing code prior to C++20.
Let us continue the discussion over the possibility of introducing an ambiguity in https://github.com/llvm/llvm-project/issues/58571

clang/lib/Sema/SemaOverload.cpp

1307

I have filed https://github.com/llvm/llvm-project/issues/58571 to discuss the interpretation of the standard.

clang/test/SemaTemplate/concepts-using-decl.cpp

111

I think I misunderstood that using decls should not be introducing ambiguities.

[usaxena95]

Addressed comments.

[ilya-biryukov]

My only ask is to add a few more tests (see the relevant comment), otherwise LG.

clang/lib/Sema/SemaOverload.cpp

1293

Hopefully this does not regress performance too much now that we keep matching template even if we don't need the result.
No need to take any action (the code looks simpler this way), just something to keep in mind in case it backfires.

clang/test/SemaTemplate/concepts-using-decl.cpp

4

NIT: maybe move this to the end of the file?
I would expect concept-specific tests to go first given the name of the file.
It's still useful to capture the discovered corner-cases, obviously, just not as the first thing, maybe.

113

Could you also check that requires clauses and constraints in template parameters do not hide each other?

template <IsEmpty T> ...
// vs
template <class T> requires IsEmpty<T> ...
// vs
template <class T> void foo() requires IsEmpty<T> ...

Should not hide each other.

Another interesting case (that probably does not yet work):

struct base { template <class T> void foo(); };
struct derived : base { 
  using base::foo; 
  template <IsEmpty T> void foo();
};

^^ derived().foo<Empty>() will probably produce an ambiguity now (as we don't have an explicit requires clause here). I don't think it's worth fixing now, but keeping a test for it with a FIXME seems reasonable.

[usaxena95]

Added more tests.

clang/test/SemaTemplate/concepts-using-decl.cpp

113

The last test case would work though since we perform template head check if any template head is contrained. This is not ambiguous as overloading chooses the "most contrained" version.

[ilya-biryukov]

clang/test/SemaTemplate/concepts-using-decl.cpp

98

NIT: add newline?

113

Ah, right, this case is not ambiguous to start with. I think I made a mistake in the description.
The call with Empty would work either way and does not check anything:

• we only see a constrained overload => it works,
• we see both overloads, but the one with IsEmpty is more specialized => it works.

I suggest also checking that derived().foo<int>() works. In that case the outcomes are different:

• we only see a constrained overload => compiler error,
• we see both overloads => constrained version does not match and the overload succeeds.

PS I was definitely under the impression that we are checking for the requires clause, but not constrained template parameters. However, I now see that I was wrong and we should also be fine with that case.

[usaxena95]

Addressed comments. Added release notes.

clang/lib/Sema/SemaOverload.cpp

if (New->getFriendObjectKind() && New->getQualifier() &&
     Match = TemplateSpecResult.getAsSingle<FunctionDecl>();
     return Ovl_Match;
   }
 
   return Ovl_Overload;
 }
 
 bool Sema::IsOverload(FunctionDecl *New, FunctionDecl *Old,
-                      bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs,
+                      bool IsForUsingDecl, bool ConsiderCudaAttrs,
                       bool ConsiderRequiresClauses) {
   // C++ [basic.start.main]p2: This function shall not be overloaded.
   if (New->isMain())
     return false;
 
   // MSVCRT user defined entry points cannot be overloaded.
   if (New->isMSVCRTEntryPoint())
     return false;
bool Sema::IsOverload(FunctionDecl *New, FunctionDecl *Old,
   // parameters (C++ 1.3.10), which includes the presence or absence
   // of the ellipsis; see C++ DR 357).
   if (OldQType != NewQType &&
       (OldType->getNumParams() != NewType->getNumParams() ||
        OldType->isVariadic() != NewType->isVariadic() ||
        !FunctionParamTypesAreEqual(OldType, NewType)))
     return true;
 
+  if (NewTemplate) {
     // C++ [temp.over.link]p4:
     //   The signature of a function template consists of its function
     //   signature, its return type and its template parameter list. The names
     //   of the template parameters are significant only for establishing the
     //   relationship between the template parameters and the rest of the
     //   signature.
     //
     // We check the return type and template parameter lists for function
     // templates first; the remaining checks follow.
-  //
-  // However, we don't consider either of these when deciding whether
-  // a member introduced by a shadow declaration is hidden.
-  if (!UseMemberUsingDeclRules && NewTemplate &&
-      (!TemplateParameterListsAreEqual(NewTemplate->getTemplateParameters(),
-                                       OldTemplate->getTemplateParameters(),
-                                       false, TPL_TemplateMatch) ||
-       !Context.hasSameType(Old->getDeclaredReturnType(),
-                            New->getDeclaredReturnType())))
+    bool SameTemplateParameterList = TemplateParameterListsAreEqual(
+        NewTemplate->getTemplateParameters(),
+        OldTemplate->getTemplateParameters(), false, TPL_TemplateMatch);
+    bool SameReturnType = Context.hasSameType(Old->getDeclaredReturnType(),
+                                              New->getDeclaredReturnType());
+    auto IsConstructorOrAssignment = [](FunctionDecl *FD) {
+      auto *CMD = dyn_cast<CXXMethodDecl>(FD);
+      if (!CMD)
+        return false;
+      if (CMD->isCopyAssignmentOperator() || CMD->isMoveAssignmentOperator())
         return true;
-
+      return isa<CXXConstructorDecl>(CMD);
+    };
+    // C++ [namespace.udecl]p4:
+    //   The member from the base class is hidden or overridden by the
+    //   implicitly-declared copy/move constructor or assignment operator of the
+    //   derived class.
+    // C++ [namespace.udecl]p11:
+    //   The set of declarations named by a using-declarator that inhabits a
+    //   class C does not include member functions and member function templates
+    //   of a base class that "correspond" to (and thus would conflict with) a
+    //   declaration of a function or function template in C.
+    // Comparing return types is not required for the "correspond" check to
+    // decide whether a member introduced by a shadow declaration is hidden.
+    if (IsForUsingDecl && !IsConstructorOrAssignment(New) &&
+        !SameTemplateParameterList)
+      return true;
+    if (!IsForUsingDecl && (!SameTemplateParameterList || !SameReturnType))
+      return true;
+  }
   // If the function is a class member, its signature includes the
   // cv-qualifiers (if any) and ref-qualifier (if any) on the function itself.
   //
   // As part of this, also check whether one of the member functions
   // is static, in which case they are not overloads (C++
   // 13.1p2). While not part of the definition of the signature,
   // this check is important to determine whether these functions
   // can be overloaded.
   CXXMethodDecl *OldMethod = dyn_cast<CXXMethodDecl>(Old);
   CXXMethodDecl *NewMethod = dyn_cast<CXXMethodDecl>(New);
   if (OldMethod && NewMethod &&
       !OldMethod->isStatic() && !NewMethod->isStatic()) {
     if (OldMethod->getRefQualifier() != NewMethod->getRefQualifier()) {
-      if (!UseMemberUsingDeclRules &&
-          (OldMethod->getRefQualifier() == RQ_None ||
+      if (!IsForUsingDecl && (OldMethod->getRefQualifier() == RQ_None ||
                               NewMethod->getRefQualifier() == RQ_None)) {
         // C++0x [over.load]p2:
         //   - Member function declarations with the same name and the same
         //     parameter-type-list as well as member function template
         //     declarations with the same name, the same parameter-type-list, and
         //     the same template parameter lists cannot be overloaded if any of
         //     them, but not all, have a ref-qualifier (8.3.5).
         Diag(NewMethod->getLocation(), diag::err_ref_qualifier_overload)
           << NewMethod->getRefQualifier() << OldMethod->getRefQualifier();

clang/test/CXX/dcl.dcl/basic.namespace/namespace.udecl/p12.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++98 %s
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 // C++03 [namespace.udecl]p12:
 //   When a using-declaration brings names from a base class into a
 //   derived class scope, member functions in the derived class
 //   override and/or hide member functions with the same name and
 //   parameter types in a base class (rather than conflicting).
 
 template <unsigned n> struct Opaque {};
-template <unsigned n> void expect(Opaque<n> _) {} // expected-note 4 {{candidate function template not viable}}
+template <unsigned n> void expect(Opaque<n> _) {}
 
 // PR5727
 // This just shouldn't crash.
 namespace test0 {
   template<typename> struct RefPtr { };
   template<typename> struct PtrHash {
     static void f() { }
   };

 namespace test3 {
   struct Base {
     template <class T> Opaque<0> foo() { return Opaque<0>(); }
     template <int n> Opaque<1> foo() { return Opaque<1>(); }
   };
 
   struct Derived1 : Base {
     using Base::foo;
-    template <int n> Opaque<2> foo() { return Opaque<2>(); } // expected-note {{invalid explicitly-specified argument for template parameter 'n'}}
+    template <int n> Opaque<2> foo() { return Opaque<2>(); }
   };
 
   struct Derived2 : Base {
-    template <int n> Opaque<2> foo() { return Opaque<2>(); } // expected-note {{invalid explicitly-specified argument for template parameter 'n'}}
+    template <int n> Opaque<2> foo() { return Opaque<2>(); }
     using Base::foo;
   };
 
   struct Derived3 : Base {
     using Base::foo;
-    template <class T> Opaque<3> foo() { return Opaque<3>(); } // expected-note {{invalid explicitly-specified argument for template parameter 'T'}}
+    template <class T> Opaque<3> foo() { return Opaque<3>(); }
   };
 
   struct Derived4 : Base {
-    template <class T> Opaque<3> foo() { return Opaque<3>(); } // expected-note {{invalid explicitly-specified argument for template parameter 'T'}}
+    template <class T> Opaque<3> foo() { return Opaque<3>(); }
     using Base::foo;
   };
 
   void test() {
     expect<0>(Base().foo<int>());
     expect<1>(Base().foo<0>());
-    expect<0>(Derived1().foo<int>()); // expected-error {{no matching member function for call to 'foo'}} expected-error {{no matching function for call to 'expect'}}
+    expect<0>(Derived1().foo<int>());
     expect<2>(Derived1().foo<0>());
-    expect<0>(Derived2().foo<int>()); // expected-error {{no matching member function for call to 'foo'}} expected-error {{no matching function for call to 'expect'}}
+    expect<0>(Derived2().foo<int>());
     expect<2>(Derived2().foo<0>());
     expect<3>(Derived3().foo<int>());
-    expect<1>(Derived3().foo<0>()); // expected-error {{no matching member function for call to 'foo'}} expected-error {{no matching function for call to 'expect'}}
+    expect<1>(Derived3().foo<0>());
     expect<3>(Derived4().foo<int>());
-    expect<1>(Derived4().foo<0>()); // expected-error {{no matching member function for call to 'foo'}} expected-error {{no matching function for call to 'expect'}}
+    expect<1>(Derived4().foo<0>());
   }
 }
 
 // PR7384: access control for member templates.
 namespace test4 {
   class Base {
   protected:
     template<typename T> void foo(T);

clang/test/CXX/drs/dr2xx.cpp

struct A {
     template<typename> void g(int);
     float &h() const;
   };
   struct B : A {
     using A::f;
     using A::g;
     using A::h;
     int &f(int);
-    template<int> int &g(int); // expected-note {{candidate}}
+    template<int> int &g(int);
     int &h();
   } b;
   int &w = b.f(0);
-  int &x = b.g<int>(0); // expected-error {{no match}}
+  int &x = b.g<int>(0); // expected-error {{non-const lvalue reference to type 'int' cannot bind to a temporary of type 'void'}}
   int &y = b.h();
   float &z = const_cast<const B&>(b).h();
 
   struct C {
     virtual void f(const int) = 0;
   };
   struct D : C {
     void f(int);

clang/test/SemaTemplate/concepts-using-decl.cpp

+// RUN: %clang_cc1 -std=c++20 -verify %s
+// expected-no-diagnostics
+
+namespace heads_without_concepts {
+struct base {
+  template <int N, int M>
+  int foo() { return 1; };
+};
+
+struct bar : public base {
+  using base::foo;
+  template <int N> 
+  int foo() { return 2; };
+};
+
+void func() {
+  bar f;
+  f.foo<10>();
+  f.foo<10, 10>();
+}
+}
+
+namespace static_methods {
+template<class> concept False = false;
+
+struct Base {
+    static void foo(auto);
+};
+struct Derived : public Base {
+    using Base::foo;
+    static void foo(False auto);
+};
+void func() {
+    Derived::foo(42);
+}
+}
+
+namespace members_not_hidden {
+template <unsigned n> struct Opaque {};
+template <unsigned n> void expect(Opaque<n> _) {}
+
+struct Empty{};
+constexpr int EmptySize = sizeof(Empty);
+
+template<typename T> concept IsEmpty = sizeof(T) == EmptySize;
+
+namespace base_members_not_hidden {
+struct base {
+  template <typename T>
+  Opaque<0> foo() { return Opaque<0>(); };
+};
+
+struct bar1 : public base {
+  using base::foo;
+  template <typename T> requires IsEmpty<T> 
+  Opaque<1> foo() { return Opaque<1>(); };
+};
+
+struct bar2 : public base {
+  using base::foo;
+  template <IsEmpty T>
+  Opaque<1> foo() { return Opaque<1>(); };
+};
+
+struct bar3 : public base {
+  using base::foo;
+  template <typename T>
+  Opaque<1> foo() requires IsEmpty<T> { return Opaque<1>(); };
+};
+
+void func() {
+  expect<0>(base{}.foo<Empty>());
+  expect<0>(base{}.foo<int>());
+  expect<1>(bar1{}.foo<Empty>());
+  expect<0>(bar1{}.foo<int>());
+  expect<1>(bar2{}.foo<Empty>());
+  expect<0>(bar2{}.foo<int>());
+  expect<1>(bar3{}.foo<Empty>());
+  expect<0>(bar3{}.foo<int>());
+}
+}
+namespace base_members_hidden {
+struct base {
+  template <typename T> requires IsEmpty<T>
+  Opaque<0> foo() { return Opaque<0>(); };
+};
+struct bar : public base {
+  using base::foo;
+  template <typename T> requires IsEmpty<T>
+  Opaque<1> foo() { return Opaque<1>(); };
+};
+void func() { 
+  expect<0>(base{}.foo<Empty>());
+  expect<1>(bar{}.foo<Empty>());
+}
+}
+}
 No newline at end of file