Bug 86521 - [8 Regression] GCC 8 selects incorrect overload of ref-qualified conversion operator template
Summary: [8 Regression] GCC 8 selects incorrect overload of ref-qualified conversion o...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 8.0
: P2 normal
Target Milestone: 8.4
Assignee: Jason Merrill
URL:
Keywords: rejects-valid
: 89596 89793 90897 (view as bug list)
Depends on:
Blocks: 90546
  Show dependency treegraph
 
Reported: 2018-07-14 12:50 UTC by Yannick Le Pennec
Modified: 2024-01-03 22:50 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-03-11 00:00:00


Attachments
patch to prefer copy elision (1.17 KB, patch)
2019-03-12 19:42 UTC, Jason Merrill
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yannick Le Pennec 2018-07-14 12:50:34 UTC
Please consider the following code, built with `g++ -std=c++17 -Wall -Wextra -pedantic`.
It used to be accepted by GCC 7.3, but this is no longer the case since GCC 8.
Godbolt for convenience: https://godbolt.org/g/oXFQex

It was broken since r258755, which makes this code ICE instead. The ICE itself was fixed in r259123, but said fix now makes GCC reject it with the below error.
This is still the case on today's r262658. As far as I can tell, GCC seems to be incorrectly selecting the `const&` overload instead of the `&&` one.

#include <utility>

template<typename T>
struct require_cast {
    T val;

    template<typename U>
    explicit operator U () && {
        return std::move(val);
    }

    template<typename U>
    explicit operator U const& () const& {
        return val;
    }
};

struct base {
    base() = default;
    base(base&&) = default;
    base& operator=(base&&) = default;

    base(base const&) = delete;
    base& operator=(base const&) = delete;
};

struct derived : base {};

int main() {
    require_cast<derived> d;
    (void)static_cast<base>(std::move(d));
    (void)static_cast<derived>(std::move(d));
}


repro.c++: In function ‘int main()’:
repro.c++:34:41: error: use of deleted function ‘base::base(const base&)’
     (void)static_cast<base>(std::move(d));
                                         ^
repro.c++:23:5: note: declared here
     base(base const&) = delete;
     ^~~~
repro.c++:35:44: error: use of deleted function ‘derived::derived(const derived&)’
     (void)static_cast<derived>(std::move(d));
                                            ^
repro.c++:27:8: note: ‘derived::derived(const derived&)’ is implicitly deleted because the default definition would be ill-formed:
 struct derived : base {};
        ^~~~~~~
repro.c++:27:8: error: use of deleted function ‘base::base(const base&)’
repro.c++:23:5: note: declared here
     base(base const&) = delete;
     ^~~~
Comment 1 Jakub Jelinek 2018-07-26 11:20:33 UTC
GCC 8.2 has been released.
Comment 2 Jakub Jelinek 2019-02-22 15:22:16 UTC
GCC 8.3 has been released.
Comment 3 Matthijs van Duin 2019-03-11 10:35:29 UTC
First off, your example is more complicated than it needs to be. A more minimal test case would be:

#include <utility>

struct Dest {
	Dest() = default;
	Dest( Dest && ) = default;
	Dest( Dest const & ) = delete;
};

struct Source {
	Dest val;
	operator Dest () && {
		return std::move( val );
	}
	operator Dest const & () const & {
		return val;
	}
};

int main() {
	Source x;
	static_cast<Dest>( std::move( x ) );
}


Second, notice that the two conversions are not really directly comparable since one converts to directly Dest while the other converts to an expression used to invoke a constructor of Dest. While it seems desirable for the former to take preference over the latter, I'm not enough of a language lawyer to be able to figure out what the C++ standard actually requires overload resolution to do in this situation.

Replacing
	operator Dest () && {
by
	operator Dest && () && {
fixes the problem, and has the additional benefit of avoiding unnecessary temporary materialization in situations like:

void foo( Dest && );
int main() {
	Source x;
	foo( std::move( x ) );
}
Comment 4 Jason Merrill 2019-03-12 03:19:01 UTC
The cast is ambiguous

To construct a 'base', we consider the two constructors

1) base(const base&);
2) base(base&&);

for each of them we could convert the argument by either

3) operator U () &&
4) operator U const& () const&

For #1 we want to convert to const base&.  For direct reference binding, #4 is the only candidate, and it is viable.  For #2 we want to convert to base&&, only #3 is a candidate for direct reference binding, and it is viable.

The two user-defined conversion sequences are not comparable because they use different conversion operators, so the initialization is ambiguous.
Comment 5 Jason Merrill 2019-03-12 03:19:54 UTC
Author: jason
Date: Tue Mar 12 03:19:22 2019
New Revision: 269602

URL: https://gcc.gnu.org/viewcvs?rev=269602&root=gcc&view=rev
Log:
	PR c++/86521 - wrong overload resolution with ref-qualifiers.

Here we were wrongly treating binding a const lvalue ref to an xvalue as
direct binding, which is wrong under [dcl.init.ref] and [over.match.ref].

	* call.c (build_user_type_conversion_1): Don't use a conversion to a
	reference of the wrong rvalueness for direct binding.

Added:
    trunk/gcc/testsuite/g++.dg/cpp0x/overload-conv-3.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/call.c
    trunk/libstdc++-v3/testsuite/20_util/is_constructible/value-2.cc
Comment 6 Jason Merrill 2019-03-12 03:38:06 UTC
(In reply to Jason Merrill from comment #4)
> The cast is ambiguous
> 
> To construct a 'base', we consider the two constructors
> 
> 1) base(const base&);
> 2) base(base&&);
> 
> for each of them we could convert the argument by either
> 
> 3) operator U () &&
> 4) operator U const& () const&
> 
> For #1 we want to convert to const base&.  For direct reference binding, #4
> is the only candidate, and it is viable.  For #2 we want to convert to
> base&&, only #3 is a candidate for direct reference binding, and it is
> viable.
> 
> The two user-defined conversion sequences are not comparable because they
> use different conversion operators, so the initialization is ambiguous.

...although perhaps the C++17 mandatory copy elision should alter this calculation: initializing the object from the result of #3 doesn't actually use the copy constructor.  In that case, we'd just be using #3, making it better than #4+#2.  The standard doesn't currently say this, but it probably should.
Comment 7 Jason Merrill 2019-03-12 19:42:15 UTC
Created attachment 45952 [details]
patch to prefer copy elision

This implements that, but I'm going to hold off a bit before committing it.
Comment 8 Jason Merrill 2019-03-13 23:35:22 UTC
Author: jason
Date: Wed Mar 13 23:34:51 2019
New Revision: 269667

URL: https://gcc.gnu.org/viewcvs?rev=269667&root=gcc&view=rev
Log:
	PR c++/86521 - C++17 copy elision in initialization by constructor.

This is an overlooked case in C++17 mandatory copy elision: We want overload
resolution to reflect that initializing an object from a prvalue does not
involve a copy or move constructor even when [over.match.ctor] says that
only constructors are candidates.  Here I implement that by looking through
the copy/move constructor in joust.

	* call.c (joust_maybe_elide_copy): New.
	(joust): Call it.

Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/call.c
    trunk/gcc/testsuite/g++.dg/cpp0x/overload-conv-3.C
    trunk/gcc/testsuite/g++.dg/overload/conv-op2.C
Comment 9 Jason Merrill 2019-03-25 18:29:59 UTC
Fixed on trunk so far.
Comment 10 Jason Merrill 2019-04-03 15:46:11 UTC
*** Bug 89596 has been marked as a duplicate of this bug. ***
Comment 11 Marek Polacek 2019-06-17 18:39:26 UTC
*** Bug 90897 has been marked as a duplicate of this bug. ***
Comment 12 Hannes Hauswedell 2019-06-18 14:53:26 UTC
Is this resolved for GCC 8.4 now?
Comment 13 Marek Polacek 2019-06-18 14:55:14 UTC
(In reply to Hannes Hauswedell from comment #12)
> Is this resolved for GCC 8.4 now?

Not yet.
Comment 14 Rémi Verschelde 2020-02-24 08:15:25 UTC
I can confirm that the issue is still valid in the latest snapshot for 8.4 (20200221).

Here's another reproduction code we came up with independently, but it's quite similar to the one in comment 3: https://github.com/godotengine/godot/pull/36436#issuecomment-589913281

PR c++/86521 still slated for inclusion in 8.4.0?

We tested it and included it in the Mageia 7 gcc package, it does seem to fix the issue. http://svnweb.mageia.org/packages?view=revision&revision=1549659
Comment 15 GCC Commits 2020-02-26 05:37:24 UTC
The releases/gcc-8 branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:f93784da474823ad563a9dfd6fd535a017b4bc9f

commit r8-10078-gf93784da474823ad563a9dfd6fd535a017b4bc9f
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Feb 26 00:33:52 2020 -0500

    PR c++/86521 - wrong overload resolution with ref-qualifiers.
    
    Here we were wrongly treating binding a const lvalue ref to an xvalue as
    direct binding, which is wrong under [dcl.init.ref] and [over.match.ref].
    
    gcc/cp/ChangeLog
    2020-02-26  Jason Merrill  <jason@redhat.com>
    
    	PR c++/86521 - wrong overload resolution with ref-qualifiers.
    	* call.c (build_user_type_conversion_1): Don't use a conversion to a
    	reference of the wrong rvalueness for direct binding.
Comment 16 GCC Commits 2020-02-26 05:37:34 UTC
The releases/gcc-8 branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:32988aac5be4fa472823e21d2d0eb877faca5667

commit r8-10080-g32988aac5be4fa472823e21d2d0eb877faca5667
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Feb 26 00:33:52 2020 -0500

    PR c++/86521 - C++17 copy elision in initialization by constructor.
    
    This is an overlooked case in C++17 mandatory copy elision: We want overload
    resolution to reflect that initializing an object from a prvalue does not
    involve a copy or move constructor even when [over.match.ctor] says that
    only constructors are candidates.  Here I implement that by looking through
    the copy/move constructor in joust.
    
    gcc/cp/ChangeLog
    2020-02-26  Jason Merrill  <jason@redhat.com>
    
    	PR c++/86521 - C++17 copy elision in initialization by constructor.
    	* call.c (joust_maybe_elide_copy): New.
    	(joust): Call it.
Comment 17 Jason Merrill 2020-02-26 14:54:56 UTC
Fixed for 8.4+.
Comment 18 GCC Commits 2023-06-14 14:04:11 UTC
The trunk branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:4ec6b627cb008e31ea3d1ee93a209297f56c6a3e

commit r14-1810-g4ec6b627cb008e31ea3d1ee93a209297f56c6a3e
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Mar 3 15:04:25 2023 -0500

    c++: tweak c++17 ctor/conversion tiebreaker [DR2327]
    
    In discussion of this issue CWG decided that the change of behavior on
    well-formed code like overload-conv-4.C is undesirable.  In further
    discussion of possible resolutions, we discovered that we can avoid that
    change while still getting the desired behavior on overload-conv-3.C by
    making this a tiebreaker after comparing conversions, rather than before.
    This also simplifies the implementation.
    
    The issue resolution has not yet been finalized, but this seems like a clear
    improvement.
    
            DR 2327
            PR c++/86521
    
    gcc/cp/ChangeLog:
    
            * call.cc (joust_maybe_elide_copy): Don't change cand.
            (joust): Move the elided tiebreaker later.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/cpp0x/overload-conv-4.C: Remove warnings.
            * g++.dg/cpp1z/elide7.C: New test.
Comment 19 Jonathan Wakely 2023-07-11 12:06:43 UTC
*** Bug 89793 has been marked as a duplicate of this bug. ***