clang accepts ambiguous function name lookup when using operator()

[rnburn]

Bugzilla Link	22039
Version	trunk
OS	All
CC	@DougGregor,@rnburn

Extended Description

clang accepts this code:

#include <iostream>                                                                                 
                                                                                                    
struct A {                                                                                          
  void operator()(int) {                                                                            
    std::cout << "A::f\n";                                                                          
  }                                                                                                 
};                                                                                                  
                                                                                                    
struct B {                                                                                          
  void operator()() {                                                                               
    std::cout << "B::f\n";                                                                          
  }                                                                                                 
};                                                                                                  
                                                                                                    
struct C                                                                                            
  : A, B                                                                                            
{                                                                                                   
};                                                                                                  
                                                                                                    
int main() {                                                                                        
  C c;                                                                                              
  c();                                                                                              
  return 0;                                                                                         
} 

which should give an ambiguous name lookup error. Both gcc and intel's compiler correctly give the operator() is ambiguous error

[wheatman]

I can confirm that the behaviors are still the same with gcc rejected the code with an ambiguous call and clang accepting it.

And perhaps I don't understand something, but I'm not sure it is supposed to be an ambiguous call, the B struct doesn't take an argument and so if we don't specify an argument it is called, if we specify an argument then the A struct operator is called

The code is

struct A {
    void operator()(int) {}
    };
struct B {
    void operator()() {}
};
struct C : A, B {};

int main() {
    C c;
    c();
    return 0;
} 

https://godbolt.org/z/vWhqWjPM3

However, on the other hand, if I replace the operator() with just a normal function f to get the following code
https://godbolt.org/z/16sKenbsn

struct A {
    void f(int) {}
    };
struct B {
    void f() {}
};
struct C : A, B {};

int main() {
    C c;
    c.f();
    return 0;
} 

Then clang matches gcc and says

<source>:11:7: error: member 'f' found in multiple base classes of different types
    c.f();
      ^
<source>:2:10: note: member found by ambiguous name lookup
    void f(int) {}
         ^
<source>:5:10: note: member found by ambiguous name lookup
    void f() {}
         ^
1 error generated.
Compiler returned: 1

And so perhaps it is wrong and for some reason operators are treated specially

For example if we instead make the same example with operator += to get the following code
https://godbolt.org/z/o7qEnneY5

struct A {
    A& operator+=(int) {
        return *this;
    }
    };
struct B {
    B& operator+=(float) {
        return *this;
    }
};
struct C : A, B {};

int main() {
    C c;
    c+=5;
    return 0;
} 

once again clang accepts and gcc does not

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[shafik]

Note, MSVC agrees w/ clang and EDG agrees w/ gcc: https://godbolt.org/z/x6sT4be17 🤔

[shafik]

I am not sure what the expected result here is.

I have read:

class.member.lookup
D94987
DR 39
n1543

multiple times and I come to a different conclusion each time. It does not help that N1543 has like 15 examples but does not show the desired result for each one after the new wording.

CC @zygoloid

[zygoloid]

Sure looks ambiguous to me. Presumably we're not diagnosing it because of this:

llvm-project/clang/lib/Sema/SemaOverload.cpp

Line 14938 in ef33d6c

R.suppressDiagnostics();

There are calls like that /all over/ SemaOverload.cpp. Presumably the intent here is to suppress access control checking for candidates we don't actually select, but this is also suppressing ambiguity errors.

[shafik]

Sure looks ambiguous to me. Presumably we're not diagnosing it because of this:

I think after looking https://eel.is/c++draft/class.member.lookup#9 more carefully I see that but an example w/ f() and f(int) would have helped to clarify that name lookup and overload resolution are separate steps.

[shafik]

Confirmed that the lack of diagnostics is because of R.suppressDiagnostics();, seeing what the fallout of removing it is now.

[shafik]

Confirmed that the lack of diagnostics is because of R.suppressDiagnostics();, seeing what the fallout of removing it is now.

This test fails, this is the first line:

llvm-project/clang/test/CXX/class.access/p4.cpp

Line 75 in dda3b70

a(pub);

also the next few lines and here:

llvm-project/clang/test/CXX/class.access/p4.cpp

Line 86 in dda3b70

ca(pub);

and the next few lines.

[zygoloid]

This test fails, this is the first line:

Yup, that's in line with my expectation that these calls are here to suppress access diagnostics. Perhaps we should add a LookupResult::suppressAccessDiagnostics() and change all these calls to use that instead?

[shafik]

This test fails, this is the first line:

Yup, that's in line with my expectation that these calls are here to suppress access diagnostics. Perhaps we should add a LookupResult::suppressAccessDiagnostics() and change all these calls to use that instead?

Ok, I did a quick prototype and it looks good, PR will be forthcoming.

[shafik]

PR: https://reviews.llvm.org/D155387

@zygoloid not wedded to my approach if you think we should approach it a bit differently, let me know.