This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by usaxena95 on Oct 21 2022, 4:14 AM.

Details

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.

Diff Detail

Event Timeline

usaxena95 created this revision.Oct 21 2022, 4:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2022, 4:14 AM
usaxena95 requested review of this revision.Oct 21 2022, 4:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2022, 4:14 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
usaxena95 edited the summary of this revision. (Show Details)Oct 21 2022, 4:30 AM
usaxena95 edited the summary of this revision. (Show Details)
usaxena95 edited the summary of this revision. (Show Details)
usaxena95 edited the summary of this revision. (Show Details)
usaxena95 edited the summary of this revision. (Show Details)
usaxena95 edited the summary of this revision. (Show Details)Oct 21 2022, 4:33 AM
usaxena95 added reviewers: ilya-biryukov, Restricted Project.
usaxena95 retitled this revision from Do not hide base member using decls with different template head. to [clang] Do not hide base member using-decls with different template head..
usaxena95 updated this revision to Diff 469552.Oct 21 2022, 4:46 AM

Added more tests.

usaxena95 edited the summary of this revision. (Show Details)Oct 21 2022, 5:02 AM

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 marked 4 inline comments as done.Oct 24 2022, 8:52 AM

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 updated this revision to Diff 470174.Oct 24 2022, 8:52 AM
usaxena95 marked 2 inline comments as done.

Addressed comments.

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 updated this revision to Diff 470505.Oct 25 2022, 8:04 AM
usaxena95 marked 3 inline comments as done.

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.

usaxena95 edited the summary of this revision. (Show Details)Oct 26 2022, 11:52 AM
ilya-biryukov accepted this revision.Oct 27 2022, 1:53 AM
ilya-biryukov added inline comments.
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.

This revision is now accepted and ready to land.Oct 27 2022, 1:53 AM
usaxena95 updated this revision to Diff 471086.Oct 27 2022, 2:33 AM
usaxena95 marked 2 inline comments as done.

Addressed comments. Added release notes.

This revision was landed with ongoing or failed builds.Oct 27 2022, 2:53 AM
This revision was automatically updated to reflect the committed changes.