Bug 42356 - improve list of candidates and error recovery for ambiguous call
Summary: improve list of candidates and error recovery for ambiguous call
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.4.1
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2009-12-12 07:38 UTC by Ivan Godard
Modified: 2021-09-09 02:59 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2021-09-07 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ivan Godard 2009-12-12 07:38:45 UTC
This code:
template<typename T>
class   freeList {
public:
    T*      newNode() { return 0; }
    template<typename U>
    T*      newNode(U u) { return newNode()->init(u); }
    template<typename U, typename V>
    T*      newNode(U u, V v) { return newNode()->init(u, v); }
    };

class   bar {};
class   baz {};
class   foo : public freeList<bar>, public freeList<baz> { };

int main() {
    foo     f;
    bar*    b = f.newNode<bar>();
    }


gets you:
s3:~/ootbc/memspec$ g++ foo.cc
foo.cc: In function ‘int main()’:
foo.cc:17: error: request for member ‘newNode’ is ambiguous
foo.cc:8: error: candidates are: template<class U, class V> T* freeList::newNode(U, V) [with U = U, V = V, T = baz]
foo.cc:6: error: candidates are: template<class U> T* freeList::newNode(U) [with U = U, T = baz]
foo.cc:4: error: candidates are: T* freeList<T>::newNode() [with T = baz]
foo.cc:8: error:                 template<class U, class V> T* freeList::newNode(U, V) [with U = U, V = V, T = bar]
foo.cc:6: error:                 template<class U> T* freeList::newNode(U) [with U = U, T = bar]
foo.cc:4: error:                 T* freeList<T>::newNode() [with T = bar]
foo.cc:17: error: expected primary-expression before ‘>’ token
foo.cc:17: error: expected primary-expression before ‘)’ token
Comment 1 Paolo Carlini 2009-12-12 09:47:47 UTC
The basic error, at line 17, is definitely correct, and, for example, ICC errors out exactly in the same way. I suppose you would like to see a nicer, in some sense, list of candidates? (by the way, mailine is slightly better anyway, because doesn't make the mistake of printing the words "candidates are:" multiple times)
Comment 2 Ivan Godard 2009-12-14 04:51:34 UTC
Well, I see several issues with the diagnostics. 

1) The call is not ambiguous, because if it were (only) ambiguous then removing the base "baz" would disambiguate. Instead the case without "baz" gets you:
   s3:~/ootbc/memspec$ g++ foo.cc
   foo.cc: In function ‘int main()’:
   foo.cc:27: error: no matching function for call to ‘foo::newNode()’
That is, a call cannot be both ambiguous and have no matching function.

2) The reported list of overloads include those which have the wrong number of arguments.

3) The diagnostic contains "freeList::newNode(U, V) [with U = U, V = V, T = bar]" as the call citation, which makes it look like the local U matches a (non-existent in this case) global U and local V matches a global V, the way that local T matches global bar. Perhaps it could say something like "freeList::newNode(U, V) [with U = ?, V = ?, T = bar]"

Comment 3 Paolo Carlini 2009-12-14 08:44:55 UTC
I do not have the time to get into the language details, but I think you should investigate the conformance of the code a bit more: recent SunStudio and Comeau also agree with ICC and GCC on this.
Comment 4 Paolo Carlini 2009-12-14 09:27:17 UTC
Likewise VC++ v8
Comment 5 Ivan Godard 2009-12-14 14:58:18 UTC
There's no question that there was an error here. The report is on the diagnostic. I'm sorry if I hadn't made that clear.
Comment 6 Paolo Carlini 2011-09-28 21:21:40 UTC
For the record, ICC also thinks it's ambiguous.
Comment 7 Manuel López-Ibáñez 2011-10-22 12:27:11 UTC
Clang prints:

/tmp/webcompile/_26276_0.cc:17:19: error: no matching member function for call to 'newNode'
    bar*    b = f.newNode<bar>();
                ~~^~~~~~~~~~~~
/tmp/webcompile/_26276_0.cc:6:13: note: candidate function template not viable: requires 1 argument, but 0 were provided
    T*      newNode(U u) { return newNode()->init(u); }
            ^
/tmp/webcompile/_26276_0.cc:6:13: note: candidate function template not viable: requires 1 argument, but 0 were provided
/tmp/webcompile/_26276_0.cc:8:13: note: candidate function template not viable: requires 2 arguments, but 0 were provided
    T*      newNode(U u, V v) { return newNode()->init(u, v); }
            ^
/tmp/webcompile/_26276_0.cc:8:13: note: candidate function template not viable: requires 2 arguments, but 0 were provided

which is nicer except for the duplicated messages.

The first thing that should go away is the "expected primary-expression" stuff.
Comment 8 Jonathan Wakely 2011-10-22 13:00:32 UTC
(In reply to comment #2)
> Well, I see several issues with the diagnostics. 
> 
> 1) The call is not ambiguous, because if it were (only) ambiguous then removing
> the base "baz" would disambiguate. Instead the case without "baz" gets you:
>    s3:~/ootbc/memspec$ g++ foo.cc
>    foo.cc: In function ‘int main()’:
>    foo.cc:27: error: no matching function for call to ‘foo::newNode()’
> That is, a call cannot be both ambiguous and have no matching function.

The call is ambiguous, because the name is found in two different base classes, see the rules in [class.member.lookup].  Removing one of the bases means the names are only found from one scope, and so overload resolution can proceed, which fails because there are no viable functions.

Name lookup happens first, then overload resolution.
In your case the result of name lookup is ambiguous, like this:

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

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

even though A::f(int) is a better match, the call is ambiguous.

Clang's diagnostic refers to viable functions, which is wrong, because finding viable functions happens as part of overload resolution, which shouldn't even happen here because name lookup is ambiguous.

> 2) The reported list of overloads include those which have the wrong number of
> arguments.

That's by design. Maybe that's the function you meant to call.  If you call a function with the wrong number of args you want the compiler to tell you name lookup found something, but overload resolution failed because the number of arguments didn't match.

> 3) The diagnostic contains "freeList::newNode(U, V) [with U = U, V = V, T =
> bar]" as the call citation, which makes it look like the local U matches a
> (non-existent in this case) global U and local V matches a global V, the way
> that local T matches global bar. Perhaps it could say something like
> "freeList::newNode(U, V) [with U = ?, V = ?, T = bar]"

That might be an improvement, yes.  That's the only issue I see here.
Comment 9 Jonathan Wakely 2011-10-22 13:07:50 UTC
(In reply to comment #8)
> That might be an improvement, yes.  That's the only issue I see here.

Actually, there is another issue in the list of candidates:

template<class U> T* freeList::newNode(U) [with U = U, T = baz]
T* freeList<T>::newNode() [with T = baz]

The first one should say freeList<T> not freeList.

That would help realise that the member name has been found in two classes.

With -fno-pretty-templates it's better:

f.C:17:17: error: request for member ‘newNode’ is ambiguous
f.C:8:15: error: candidates are: template<class U, class V> baz* freeList<baz>::newNode<U, V>(U, V)
f.C:6:15: error:                 template<class U> baz* freeList<baz>::newNode<U>(U)
f.C:4:13: error:                 baz* freeList<baz>::newNode()
f.C:8:15: error:                 template<class U, class V> bar* freeList<bar>::newNode<U, V>(U, V)
f.C:6:15: error:                 template<class U> bar* freeList<bar>::newNode<U>(U)
f.C:4:13: error:                 bar* freeList<bar>::newNode()

This makes it easier to see that name lookup found these functions:
  freeList<baz>::newNode(U, V)
  freeList<baz>::newNode(U)
  freeList<bar>::newNode(U,V)
  freeList<bar>::newNode(U)
so you can see they come from different scopes (it would be even clearer if "bar" and "baz" weren't visually very similar!)
Comment 10 Jonathan Wakely 2011-10-22 13:21:11 UTC
(In reply to comment #9)
> The first one should say freeList<T> not freeList.

reported as PR 50828
Comment 11 Manuel López-Ibáñez 2011-10-22 13:30:10 UTC
(In reply to comment #8)
> > 2) The reported list of overloads include those which have the wrong number of
> > arguments.
> 
> That's by design. Maybe that's the function you meant to call.  If you call a
> function with the wrong number of args you want the compiler to tell you name
> lookup found something, but overload resolution failed because the number of
> arguments didn't match.

I wonder why the detailed overload failure that Nathan implemented does not trigger here. I would expect to give details of why overload failed.

g++ could also specify which ones are viable candidates, and which ones are not even viable, and for the ones not viable, explain why.

> That might be an improvement, yes.  That's the only issue I see here.

There is also the issue of the "expected primary-expression" triggered two times.
Comment 12 Ivan Godard 2011-10-22 15:52:54 UTC
Manual said:
g++ could also specify which ones are viable candidates, and which ones are not
even viable, and for the ones not viable, explain why.
Comment 13 Jonathan Wakely 2011-10-22 19:43:08 UTC
(In reply to comment #11)
> I wonder why the detailed overload failure that Nathan implemented does not
> trigger here. I would expect to give details of why overload failed.

Name lookup fails due to the ambiguity, so overload resolution never happens.  The detailed overload resolution diagnostics won't be printed if overload resolution isn't done.

> g++ could also specify which ones are viable candidates, and which ones are not
> even viable, and for the ones not viable, explain why.

There are no viable candidates, because overload resolution is not performed.

Why bother performing overload resolution if the result of name lookup is ambiguous?  How does it help you resolve the ambiguity?

The ambiguity can be resolved by qualifying the name or with a using declaration, but overload resolution is irrelevant for either of them: you qualify a *name* and a using declaration names a *name*, not an overload
Comment 14 Manuel López-Ibáñez 2011-10-23 00:46:15 UTC
(In reply to comment #13)
> (In reply to comment #11)
> > I wonder why the detailed overload failure that Nathan implemented does not
> > trigger here. I would expect to give details of why overload failed.
> 
> Name lookup fails due to the ambiguity, so overload resolution never happens. 
> The detailed overload resolution diagnostics won't be printed if overload
> resolution isn't done.

So you are saying that the candidates printed are simply all the matching names that name lookup is able to find?

If so, I understand that the way g++ implements overload resolution makes not trivial to give diagnostics, but still as Ivan agrees, it would be nice to be able to sort viable candidates first, then not viable with details about why they are not. This would require factoring out code to test overloads and report diagnostics.

> > g++ could also specify which ones are viable candidates, and which ones are not
> > even viable, and for the ones not viable, explain why.
> 
> There are no viable candidates, because overload resolution is not performed.
> 
> Why bother performing overload resolution if the result of name lookup is
> ambiguous?  How does it help you resolve the ambiguity?

Yourself explained why above:

"Maybe that's the function you meant to call.  If you call a function with the wrong number of args you want the compiler to tell you name lookup found something, but overload resolution failed because the number of arguments didn't match."
Comment 15 Manuel López-Ibáñez 2011-10-23 00:47:42 UTC
(In reply to comment #12)
> Manual said:
> g++ could also specify which ones are viable candidates, and which ones are not
> even viable, and for the ones not viable, explain why.

http://en.wikipedia.org/wiki/Manual

http://en.wikipedia.org/wiki/Manuel
Comment 16 Ivan Godard 2011-10-23 01:28:48 UTC
(In reply to comment #15)
> (In reply to comment #12)
> > Manual said:
> > g++ could also specify which ones are viable candidates, and which ones are not
> > even viable, and for the ones not viable, explain why.
> 
> http://en.wikipedia.org/wiki/Manual
> 
> http://en.wikipedia.org/wiki/Manuel

Whoever said it, they were right :-)
Comment 17 Jonathan Wakely 2011-10-23 01:54:17 UTC
But for this testcase I don't want to be told overload resolution passed or failed, I want to be told it's ambiguous, because that's the error in the testcase that prevents it compiling, and the diagnostic should help fix that problem.

You say clang's output is nicer, I think you should look again.  Notice there are 6 overloads:
freeList<bar>f(), freeList<bar>f(U), and freeList<bar>f(U,V),
freeList<baz>f(), freeList<baz>f(U), and freeList<baz>f(U,V),

Clang prints 4 errors (which appear to be duplicates, but actually refer to freeList<bar> and freeList<baz> but that's not shown).  Why are the other two overloads not shown?  Why doesn't it mention that the call is ambiguous?
It fails to state the problem, and misleadingly implies overload resolution failed.

G++ mentions all six and correctly says they're ambiguous.  It would be better if it said why (name lookup found "newNode" in multiple base classes) as clang does for this code (which it gets right):

template<class T>
struct A {
template<class U>
  void f(U);
};
struct C : A<void>, A<int> { };

int main() {
  C c;
  c.f<char>();
}
Comment 18 Jonathan Wakely 2011-10-23 02:03:41 UTC
(In reply to comment #12)
> Manual said:
> g++ could also specify which ones are viable candidates, and which ones are not
> even viable, and for the ones not viable, explain why.

None of the functions are viable, because 'newNode' is ambiguous.


So your suggestion seems to be:

request for member is ambiguous
Overload 1: not viable, reason: ambiguous.
Overload 2: not viable, reason: ambiguous.
Overload 3: not viable, reason: ambiguous.
Overload 4: not viable, reason: ambiguous.
Overload 5: not viable, reason: ambiguous.
Overload 6: not viable, reason: ambiguous.

rather than what we have now:

request for member is ambiguous
Overload 1
Overload 2
Overload 3
Overload 4
Overload 5
Overload 6
[unhelpful nonsense about expected primary expression]

I think the second one is better (apart from the primary expression part)

Or maybe you could have:

request for member is ambiguous
Overload 1: not viable, reason: ambiguous.
Overload 2: not viable, reason: ambiguous.
Overload 3: not viable, reason: ambiguous, and wrong number of args
Overload 4: not viable, reason: ambiguous, and wrong number of args
Overload 5: not viable, reason: ambiguous, and wrong number of args
Overload 6: not viable, reason: ambiguous, and wrong number of args

I don't think that helps - changing the number of arguments wouldn't make the testcase compile. Resolving the ambiguous lookup would.
Comment 19 Andrew Pinski 2021-09-07 18:21:08 UTC
This is what clang now prints:
<source>:18:19: error: member 'newNode' found in multiple base classes of different types
    bar*    b = f.newNode<bar>();
                  ^
<source>:5:13: note: member found by ambiguous name lookup
    T*      newNode() { return 0; }
            ^
<source>:5:13: note: member found by ambiguous name lookup

Which is better than GCC's but does not print out what the multiple base classes are.