Is this overload resolution ambiguous?

[Brian Bi]

Here's the test case: https://bugs.llvm.org/show_bug.cgi?id=18359#c2

struct Bar {};

struct Foo {
    operator Bar const & () const & { return data; }
    operator Bar &&      () &&      { return static_cast<Bar&&>(data); }
    Bar data;
};

int main() {
    Bar x;
    x = Foo();
}

I believe that Clang is actually correct and that the overload resolution is ambiguous (but it shouldn't be).

According to [over.ics.ref]/2, the implicit conversion sequence (to either `const Bar&` or `Bar&&`) is the same conversion sequence required to convert `Foo` to the referenced type by copy-initialization, namely `const Bar` or `Bar` (but the cv-qualification doesn't matter). Of course, the rvalue-qualified conversion operator should be chosen for that conversion, since `Foo()` is an rvalue.

So we have our implicit conversion sequences:

1. call `operator Bar&&()` and bind the result to `const Bar&` (to call the copy constructor)
   
2. call `operator Bar&&()` and bind the result to `Bar&&` (to call the move constructor)
   

But "any difference in top-level cv-qualification is subsumed by the initialization
itself and does not constitute a conversion." So, according to this, these two user-defined conversion sequences both have no second standard conversion. This implies that neither is better than the other, so the call is ambiguous.

If my interpretation of the standard is correct, then it seems that the current overload resolution rules have made this call ambiguous when it's apparently "obvious" to many people that it shouldn't be ambiguous. Perhaps [over.ics.rank]/3.3 needs to be changed, adding a sentence to disambiguate this situation.

--

Brian Bi

[bogdan]

As far as I can tell, Clang is indeed right, but for a different reason than the one in your explanation. (All quotes going forward are from N4713.)

These are not indirect bindings; they're direct bindings to the result of a conversion function, governed by the second part of [over.ics.ref]/1.

Overload resolution considers two candidates, the copy and move assignment operators of Bar. It looks for implicit conversion sequences from Foo() to the respective parameters, which are references, so those sequences, modelled as initializations, are governed by [dcl.init.ref] with the assistance of [over.match.ref]. For const Bar&, only the first conversion function is a candidate, according to [dcl.init.ref]/5.1.2 and [over.match.ref]/1.1, and it's viable. For Bar&&, the algorithm in [dcl.init.ref] falls through to 5.2.1.2, and [over.match.ref]/1.1 again has only one candidate, the second conversion function, again viable.

So, our two candidates have user defined conversion sequences for their respective parameters. These user defined conversion sequences use different conversion functions, so [over.ics.rank]/3.3 doesn't apply; the two conversion sequences are indistinguishable; overload resolution fails.

An indication that this is what happens is that changing the qualifiers of the first conversion function to just & makes the code compile. In this case, the first conversion function is no longer viable for the conversion to const Bar&, so [dcl.init.ref] falls through to 5.2.1.2 for this initialization as well. The two conversion sequences for the two assignment operators now use the same conversion function, so they can be compared, and the second standard conversion sequence of the conversion to Bar&& is better than the other one due to [over.ics.rank]/3.2.3, so the move assignment operator wins.

Also note that the corresponding initialization Bar x = Foo(); works. In this case, overload resolution uses [over.match.copy] and we have four candidates: the copy and move constructors of Bar and the two conversion functions of Foo. The second conversion function wins because it has the best conversion sequence for its implicit object parameter.

I doubt that the committee would be willing to change, say, [dcl.init.ref] to make this case work - too much potential for silent changes of behaviour in existing code. So, here are some possible quick fixes:

1. Add a volatile qualifier to the cv-qualifiers to the first conversion sequence:

operator const Bar&() volatile const & { return const_cast<const Foo*>(this)->data; }

volatile const & cannot bind to rvalues, so this makes the code compile for the same reason as the first change above (to a plain & qualifier) while preserving the ability to call the conversion function on const Foo lvalues. In the body of the conversion function, care must be taken to avoid the volatile-qualified access path that may cause performance issues. Now, if you actually have a genuine volatile Foo, the code in the body of the conversion function will trigger undefined behaviour.

2. Just return plain Bar from both conversion functions:

struct Bar { };

struct Foo
{
    operator Bar() const & { return data; }
    operator Bar() &&      { return static_cast<Bar&&>(data); }

    Bar data;
};

int main()
{
    Bar x;
    x = Foo();
}

This works because for binding to const Bar& [dcl.init.ref] again falls through to 5.2.1.2, and this time both conversion functions are viable, but the second one wins because it has a better conversion to its implicit object parameter.

For cases like this one, this achieves pretty much the same thing as the original code - a copy from the inner Bar for Foo lvalues and a move from the inner Bar for Foo rvalues. However, if we have a function that takes a const Bar& not to make a copy internally, but just to call some observer member functions on it, passing a Foo lvalue to it will make a (temporary) copy of a Bar, which is precisely what the function was trying to avoid. Not good, unless there's a guarantee (automatically enforced...) that the codebase doesn't contain such cases.

3. Since you want Foo lvalues to be implicitly convertible to Bar lvalues, and the same for rvalues, maybe an option could be to forget about conversion functions and to... gasp... derive from Bar. Blasphemy, yes, but in some cases it could be an option. For example, in the original code in the bug report to Clang, Bar is shared_ptr<int>. You're not going to have pointers to shared_ptrs and then delete them... are you? :-)