Bug 96645 - [12/13/14/15 Regression] [CWG2335] std::variant default constructor and unparsed DMI
Summary: [12/13/14/15 Regression] [CWG2335] std::variant default constructor and unpar...
Status: ASSIGNED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 9.0
: P2 normal
Target Milestone: 13.4
Assignee: Jason Merrill
URL:
Keywords: rejects-valid
: 88165 106613 (view as bug list)
Depends on:
Blocks: NSDMI 101227
  Show dependency treegraph
 
Reported: 2020-08-17 07:58 UTC by Sergey
Modified: 2024-07-19 12:39 UTC (History)
15 users (show)

See Also:
Host:
Target:
Build:
Known to work: 8.4.0
Known to fail: 10.2.0, 11.0, 9.3.0
Last reconfirmed: 2020-08-17 00:00:00


Attachments
patch to make dependency on unparsed DMI a hard error (1.39 KB, patch)
2022-03-22 16:15 UTC, Jason Merrill
Details | Diff
patch to make it an error for __is_constructible (1.52 KB, patch)
2022-03-23 13:36 UTC, Jason Merrill
Details | Diff
patch for tentative early DMI parsing (7.29 KB, patch)
2022-04-01 14:14 UTC, Jason Merrill
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey 2020-08-17 07:58:08 UTC
Appears in version 9 and above.
Flags: -std=c++17

Code:

#include <variant>

void testVariant()
{
    struct DataWithVariant {
        using Member = std::variant<int, float>;
        Member d;
    };
    auto d = DataWithVariant{};
}

void testVarStruct()
{
    struct DataWithStruct {
        struct A {
            int number = 5; // compiles, if remove initialization
        };

        struct B {
            bool flag = false;
        };

        using Member = std::variant<A, B>;
        Member data;
    };
    auto d = DataWithStruct{};
}

int main() {
    testVariant();
    testVarStruct();
}




Compiler output:
<source>: In function 'void testVarStruct()':

<source>:26:29: error: use of deleted function 'std::variant<_Types>::variant() [with _Types = {testVarStruct()::DataWithStruct::A, testVarStruct()::DataWithStruct::B}]'

   26 |     auto d = DataWithStruct{};

      |                             ^

In file included from <source>:1:

/opt/compiler-explorer/gcc-9.1.0/include/c++/9.1.0/variant:1287:7: note: 'std::variant<_Types>::variant() [with _Types = {testVarStruct()::DataWithStruct::A, testVarStruct()::DataWithStruct::B}]' is implicitly deleted because the default definition would be ill-formed:

 1287 |       variant() = default;

      |       ^~~~~~~

/opt/compiler-explorer/gcc-9.1.0/include/c++/9.1.0/variant:1287:7: error: use of deleted function 'constexpr std::_Enable_default_constructor<false, _Tag>::_Enable_default_constructor() [with _Tag = std::variant<testVarStruct()::DataWithStruct::A, testVarStruct()::DataWithStruct::B>]'

In file included from /opt/compiler-explorer/gcc-9.1.0/include/c++/9.1.0/variant:38,

                 from <source>:1:

/opt/compiler-explorer/gcc-9.1.0/include/c++/9.1.0/bits/enable_special_members.h:110:15: note: declared here

  110 |     constexpr _Enable_default_constructor() noexcept = delete;

      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~





It compiles if move nested structs in global namespace or remove initializer.
Comment 1 Jonathan Wakely 2020-08-17 14:21:56 UTC
This is not a bug in std::variant:


template<bool B>
struct bool_constant
{
  static constexpr bool value = B;
  using type = bool_constant;
};

using true_type = bool_constant<true>;

template<typename T>
struct is_default_constructible
: bool_constant<__is_constructible(T)>
{ };

void testVarStruct()
{
    struct DataWithStruct {
        struct A {
            int number = 5; // compiles, if remove initialization
        };

        is_default_constructible<A>::type t = true_type{};
    };
}

v.C: In function 'void testVarStruct()':
v.C:22:47: error: could not convert 'true_type{}' from 'bool_constant<true>' to 'bool_constant<false>'
   22 |         is_default_constructible<A>::type t = true_type{};
      |                                               ^~~~~~~~~~~
      |                                               |
      |                                               bool_constant<true>


This stopped working with r269032 which changed the result of the __is_constructible built-in.

Clang also rejects this, for the same reason, but EDG accepts it.
Comment 2 Sergey 2020-08-17 14:24:28 UTC
(In reply to Jonathan Wakely from comment #1)
> This is not a bug in std::variant:
> 
> 
> template<bool B>
> struct bool_constant
> {
>   static constexpr bool value = B;
>   using type = bool_constant;
> };
> 
> using true_type = bool_constant<true>;
> 
> template<typename T>
> struct is_default_constructible
> : bool_constant<__is_constructible(T)>
> { };
> 
> void testVarStruct()
> {
>     struct DataWithStruct {
>         struct A {
>             int number = 5; // compiles, if remove initialization
>         };
> 
>         is_default_constructible<A>::type t = true_type{};
>     };
> }
> 
> v.C: In function 'void testVarStruct()':
> v.C:22:47: error: could not convert 'true_type{}' from 'bool_constant<true>'
> to 'bool_constant<false>'
>    22 |         is_default_constructible<A>::type t = true_type{};
>       |                                               ^~~~~~~~~~~
>       |                                               |
>       |                                               bool_constant<true>
> 
> 
> This stopped working with r269032 which changed the result of the
> __is_constructible built-in.
> 
> Clang also rejects this, for the same reason, but EDG accepts it.

Clang compiles it if use libc++ instead of libstdc++
Comment 3 Jonathan Wakely 2020-08-17 14:30:52 UTC
It doesn't compile the code in comment 1. That code doesn't use any standard library components, so it doesn't make any difference whether you use libc++ or not.
Comment 4 Jonathan Wakely 2020-08-17 14:40:18 UTC
And libc++'s std::variant is still affected by the same issue, but instead of the default constructor being deleted it just has the wrong exception specification:

#include <variant>

void testVarStruct()
{
    struct DataWithStruct {
        struct A {
            int number = 5; // compiles, if remove initialization
        };

        using Member = std::variant<A>;
        Member data;
        static_assert(std::is_nothrow_default_constructible_v<Member>);
    };
}

The difference is that GCC defines the std::variant default constructor as defaulted, but libc++ defines it as:

constexpr variant()
noexcept(is_nothrow_default_constructible_v<variant_alternative<0, Types...>)

That means the constructor isn't deleted for libc++, it just has the wrong noexcept-specifier.

It looks like the nested class DataWithStruct::A isn't considered complete until after DataWithStruct is complete, but I'm not sure why that is.
Comment 5 Sergey 2020-08-18 07:49:52 UTC
(In reply to Jonathan Wakely from comment #4)
> And libc++'s std::variant is still affected by the same issue, but instead
> of the default constructor being deleted it just has the wrong exception
> specification:
> 

Should I report the issue in clang bugtracker?
Comment 6 Jakub Jelinek 2021-01-25 13:53:47 UTC
If I pass tf_error instead of tf_none in constructible_expr's build_special_member call, I get:
pr96645.C: In instantiation of ‘struct is_default_constructible<testVarStruct()::DataWithStruct::A>’:
pr96645.C:22:36:   required from here
pr96645.C:12:17: error: default member initializer for ‘testVarStruct()::DataWithStruct::A::number’ required before the end of its enclosing class
   12 | : bool_constant<__is_constructible(T)>
      |                 ^~~~~~~~~~~~~~~~~~~~~
so I guess the reason for the failure is that tsubst hasn't processed the nsdmis yet.
Comment 7 Patrick Palka 2021-02-15 15:28:11 UTC
(In reply to Jonathan Wakely from comment #4)
> It looks like the nested class DataWithStruct::A isn't considered complete
> until after DataWithStruct is complete, but I'm not sure why that is.

Maybe the note in [class.mem.general]/7 is relevant:

  A complete-class context of a nested class is also a complete-class context of any enclosing class, if the nested class is defined within the member-specification of the enclosing class.

We can't determine if A is constructible until we parse the initializer for DataWithStruct::A::number.  And according to the above, we can't parse this initializer until DataWithStruct is complete.

Looks like PR81359 is closely related.
Comment 8 Jason Merrill 2021-04-02 18:20:11 UTC
(In reply to Patrick Palka from comment #7)
> 
> Maybe the note in [class.mem.general]/7 is relevant:
> 
>   A complete-class context of a nested class is also a complete-class
> context of any enclosing class, if the nested class is defined within the
> member-specification of the enclosing class.
> 
> We can't determine if A is constructible until we parse the initializer for
> DataWithStruct::A::number.  And according to the above, we can't parse this
> initializer until DataWithStruct is complete.

Right.
 
> Looks like PR81359 is closely related.

Yes.  Perhaps PR81359 or PR88368, or both, were wrongly resolved.

We cannot correctly resolve is_nothrow_constructible<A> until we've parsed the DMI.  Given that, we have three options:

1) Conservatively say no.
2) Optimistically guess yes.
3) Non-SFINAE error.

PR81359 changed our behavior from 3 to 1.

#2 seems the clear worst choice, as it can lead to things unexpectedly throwing.

#3 means people have to jump through hoops to make their code compile.

#1 means silently pessimized code for anything that relies on std::is_nothrow_constructible<A> in the rest of the translation, since the value is permanently cached.

If we choose #1, we have another choice for is_constructible<A>: should it be true (giving A() a throwing exception-spec) or false?

PR88368 changed our choice from true to false.

Any opinions on what our behavior should be?  Should there be an LWG issue?

This is related to CWG1890, and the general issue that we don't currently parse on demand like we do instantiate on demand for templates.

So one workaround is to wrap DataWith* in a dummy template:

#include <type_traits>
using namespace std;

template <class Dummy = void>
struct DataWithStruct {
  struct A {
    int number = 5;
  };

  /*typename*/ is_nothrow_constructible<A>::type t = true_type{};
};

DataWithStruct<> d; // OK

or move the nested class out so we can finish parsing it before the use:

#include <variant>

void testVarStruct()
{
  struct A {
    int number = 5;
  };
  struct B {
    bool flag = false;
  };
  struct DataWithStruct {
    using Member = std::variant<A, B>;
    Member data;
  };
  auto d = DataWithStruct{};
}
Comment 9 Jason Merrill 2021-04-02 18:21:21 UTC
(In reply to Jason Merrill from comment #8)
> This is related to CWG1890, and the general issue that we don't currently
> parse on demand like we do instantiate on demand for templates.

("We" in this sentence is the C++ standard.)
Comment 10 Jonathan Wakely 2021-04-03 11:27:29 UTC
(In reply to Jason Merrill from comment #8)
> Any opinions on what our behavior should be?  Should there be an LWG issue?

Yes, we want an LWG issue. That might then result in a new core issue too, if we can't reasonably do the right thing.
Comment 11 Richard Biener 2021-06-01 08:18:20 UTC
GCC 9.4 is being released, retargeting bugs to GCC 9.5.
Comment 12 Eyal Rozenberg 2021-09-06 19:56:01 UTC
(In reply to Jason Merrill from comment #8)
> We cannot correctly resolve is_nothrow_constructible<A> until we've parsed
> the DMI.  Given that, we have three options:
> 
> 1) Conservatively say no.
> 2) Optimistically guess yes.
> 3) Non-SFINAE error.
>
> ("We" in this sentence is the C++ standard.)

But in this page, "we" is the compiler. IIUC, the standard does not allow for determing is_nothrow_constructible<A>. Am I correct? If that really is the case, shouldn't the compiler emit an error saying that?

Alternatively, when not following the standard strictly, why should it not be option (4.): Ignore the official restriction on determining (nothrow) constructibility, make a best-effort attempt to determine it anyway ( which in this example should succeed), and report failure otherwise.

?

> PR81359 changed our behavior from 3 to 1.

I searched that bug page for the rationale, and couldn't quite get it.
Comment 13 Jonathan Wakely 2021-11-23 17:08:46 UTC
Tim Song says "This is another variant of CWG2335, which is currently blocked on implementation experience and wording."
Comment 14 Jason Merrill 2022-03-22 14:12:23 UTC
(In reply to Eyal Rozenberg from comment #12)
> (In reply to Jason Merrill from comment #8)
> > We cannot correctly resolve is_nothrow_constructible<A> until we've parsed
> > the DMI.  Given that, we have three options:
> > 
> > 1) Conservatively say no.
> > 2) Optimistically guess yes.
> > 3) Non-SFINAE error.
> >
> > ("We" in this sentence is the C++ standard.)
> 
> But in this page, "we" is the compiler. IIUC, the standard does not allow
> for determing is_nothrow_constructible<A>. Am I correct? If that really is
> the case, shouldn't the compiler emit an error saying that?

Indeed, that's the question.

> Alternatively, when not following the standard strictly, why should it not
> be option (4.): Ignore the official restriction on determining (nothrow)
> constructibility, make a best-effort attempt to determine it anyway ( which
> in this example should succeed), and report failure otherwise.
> 
> ?

If we can define such a best-effort attempt, it could be a candidate for standardization.
 
> > PR81359 changed our behavior from 3 to 1.
> 
> I searched that bug page for the rationale, and couldn't quite get it.

The patch made the error about depending on an unparsed initializer subject to SFINAE, so that the implicit default ctor just isn't a viable candidate in this context.  So for the testcase in comment #1, A is not default-constructible until we reach the end of DataWithStruct.

The problem comes when we cache this answer in the is_default_constructible class; now the rest of the compilation thinks A isn't default-constructible because we happened to check it within DataWithStruct.

This seems analogous to type completeness; if A were forward-declared, and the definition moved after the is_default_constructible test, we'd have the same situation.  But that the standard library traits require that their argument be complete, which avoids the caching problem at the top level, though we might still end up caching based on the completeness of some intermediary.

The traits completeness requirement seems to suggest that making the unparsed initializer a hard error again is the right way to go.  But I'm uneasy about making this change at this point in GCC 12 stage 4; I'm definitely not going to backport it.
Comment 15 Jason Merrill 2022-03-22 16:15:19 UTC
Created attachment 52663 [details]
patch to make dependency on unparsed DMI a hard error

This doesn't seem to break anything in the library, only the two front-end tests that were specifically for the SFINAE behavior.
Comment 16 Jason Merrill 2022-03-22 16:22:12 UTC
A more targeted possiblity would be for __is_constructible to error if the class has unparsed DMI.
Comment 17 Patrick Palka 2022-03-22 17:31:23 UTC
(In reply to Jason Merrill from comment #16)
> A more targeted possiblity would be for __is_constructible to error if the
> class has unparsed DMI.

Sounds related to PR92067, which requests that __is_constructible(Incomplete) be a hard error.
Comment 18 Eyal Rozenberg 2022-03-22 20:56:01 UTC
(In reply to Jason Merrill from comment #14)
> > Alternatively, when not following the standard strictly, why should it not
> > be option (4.): Ignore the official restriction on determining (nothrow)
> > constructibility, make a best-effort attempt to determine it anyway ( which
> > in this example should succeed), and report failure otherwise.
> > 
> > ?
> 
> If we can define such a best-effort attempt, it could be a candidate for
> standardization.

Try to resolve is_nothrow_constructible<A> as long as this resolution does not involve DataWithMember or any of its constituents (e.g. as though we had seen the initializer expression before the definition of DataWithVariant even began). If that  succeeds - we're good; if it fails - that's an error all on its own and we (sort of) don't care about the DataWithVariant error; and if it trips the wire and tries to refer to DataWithMember or a constituent thereof - give up on the parse attempt.

Now, this is not _best_ effort, it's actually _minimal_ effort, since we're not willing to even accept use of A and its constituents, but it's still something. Not a candidate for standardization though.
Comment 19 Jonathan Wakely 2022-03-22 21:29:01 UTC
(In reply to Patrick Palka from comment #17)
> Sounds related to PR92067, which requests that
> __is_constructible(Incomplete) be a hard error.

That request is far too simplistic and needs to be considered carefully. Consider the example in PR 104242, where overload resolution for the constructor has to consider the is_constructible constraints on std::any's constructor while the type is incomplete.
Comment 20 Jason Merrill 2022-03-23 13:36:46 UTC
Created attachment 52672 [details]
patch to make it an error for __is_constructible

Thus.
Comment 21 GCC Commits 2022-03-24 18:31:46 UTC
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:346ab5a54a831ad9c78afcbd8dfe98e0e07e3070

commit r12-7804-g346ab5a54a831ad9c78afcbd8dfe98e0e07e3070
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Mar 22 01:10:44 2022 -0400

    c++: delayed parse DMI [PR96645]
    
    With the changes for PR81359 and PR88368 to make get_nsdmi errors be treated
    as substitution failure, we have the problem that if we check
    std::is_default_constructible for a complete class that still has unparsed
    default member initializers, we get an answer (false) that will be wrong
    once the DMIs have been parsed.  The traits avoid this problem for regular
    incomplete classes by giving an error if the operand is incomplete; we
    should do the same if get_nsdmi is going to fail due to unparsed DMI.
    
            PR c++/96645
    
    gcc/cp/ChangeLog:
    
            * cp-tree.h (type_has_default_ctor_to_be_synthesized): Declare.
            * class.cc (type_has_default_ctor_to_be_synthesized): New.
            (type_has_non_user_provided_default_constructor_1): Support it.
            (type_has_non_user_provided_default_constructor): Now a wrapper.
            * method.cc (complain_about_unparsed_dmi): New.
            (constructible_expr): Call it.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/ext/is_constructible3.C: Expect error.
            * g++.dg/ext/is_constructible7.C: New test.
Comment 22 Jason Merrill 2022-03-26 23:22:00 UTC
Fixed for 12, not backporting.
Comment 23 GCC Commits 2022-03-31 01:43:47 UTC
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:0ce8154f1c72e6d701bff969a007938e2f986369

commit r12-7929-g0ce8154f1c72e6d701bff969a007938e2f986369
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Mar 30 13:57:22 2022 -0400

    c++: parse trivial DMI immediately [PR96645]
    
    The recent change to reject __is_constructible for nested classes with DMI
    is, unsurprisingly, breaking some code.  Let's allow simple cases by
    immediately parsing DMI that do no name lookup; then being in complete class
    scope makes no difference.
    
            PR c++/96645
    
    gcc/cp/ChangeLog:
    
            * parser.cc (cp_parser_early_parsing_nsdmi): New.
            (cp_parser_member_declaration): Call it.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/cpp0x/nsdmi10.C: Now OK.
            * g++.dg/ext/is_constructible3.C: Likewise.
            * g++.dg/ext/is_constructible7.C: Likewise.
Comment 24 GCC Commits 2022-04-01 14:00:52 UTC
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:86d8e0c0652ef5236a460b75c25e4f7093cc0651

commit r12-7960-g86d8e0c0652ef5236a460b75c25e4f7093cc0651
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Apr 1 09:01:30 2022 -0400

    Revert "c++: delayed parse DMI [PR96645]"
    
    The breakage from r12-7804 (in WebKit, particularly) is more of a can of
    worms than I think we can address in GCC 12, so let's return to the GCC 11
    status quo for now and try again in stage 1.
    
    I think the change was correct for the current standard, but the standard
    needs a fix in this area; this is CWG issue 2335.
    
            PR c++/96645
    
    This reverts commits r12-7804 and r12-7929.
Comment 25 Jason Merrill 2022-04-01 14:07:18 UTC
Reopening.
Comment 26 Jason Merrill 2022-04-01 14:14:34 UTC
Created attachment 52734 [details]
patch for tentative early DMI parsing

This patch series (for GCC 13) adds a mode that tries to parse nested class DMI at the end of the nested class rather than at the end of all enclosing classes.  If that fails, we try again at the later point.

The first patch in the series extends our existing name-clash checking to cover nested classes as well, to catch the case where this can change name lookup results.
Comment 27 Jonathan Wakely 2022-04-22 08:26:21 UTC
*** Bug 88165 has been marked as a duplicate of this bug. ***
Comment 28 Andrew Pinski 2022-05-15 06:35:09 UTC
*** Bug 105606 has been marked as a duplicate of this bug. ***
Comment 29 Andrew Pinski 2022-08-15 04:53:54 UTC
*** Bug 106613 has been marked as a duplicate of this bug. ***
Comment 30 Richard Biener 2023-04-26 06:55:18 UTC
GCC 13.1 is being released, retargeting bugs to GCC 13.2.
Comment 31 Richard Biener 2023-07-27 09:22:10 UTC
GCC 13.2 is being released, retargeting bugs to GCC 13.3.
Comment 32 Jakub Jelinek 2024-05-21 09:10:28 UTC
GCC 13.3 is being released, retargeting bugs to GCC 13.4.