llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
26.71k stars 10.94k forks source link

Clang incorrectly rejects default construction of union with nontrivial member, part 2 #81774

Open StephanTLavavej opened 4 months ago

StephanTLavavej commented 4 months ago

Accepted by VS 2022 17.10 Preview 1, rejected by Clang 17.0.3.

On Compiler Explorer, accepted by GCC 13.2, rejected by Clang 17.0.1 and trunk: https://godbolt.org/z/8fGhWa1ss

Presumably related to #48416 which was previously reported and fixed, but this larger repro is still failing.

C:\Temp>type meow.cpp
enum class BasicFormatArgType { NoType, CustomType };

struct Monostate {};

struct Handle {
    Handle(int, int) {}
};

template <typename Context>
struct BasicFormatArg {
    BasicFormatArg() = default;

    BasicFormatArg(int x, int y) : ActiveState(BasicFormatArgType::CustomType), CustomState(x, y) {}

    BasicFormatArgType ActiveState = BasicFormatArgType::NoType;

    union {
        Monostate NoState = Monostate{};
        Handle CustomState;
    };
};

int main() {
    BasicFormatArg<double> arg{};
    (void) arg;
}
C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /c meow.cpp
meow.cpp

C:\Temp>clang-cl -v
clang version 17.0.3
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\Llvm\x64\bin

C:\Temp>clang-cl /EHsc /nologo /W4 /std:c++latest /c meow.cpp
meow.cpp(24,28): error: call to implicitly-deleted default constructor of 'BasicFormatArg<double>'
   24 |     BasicFormatArg<double> arg{};
      |                            ^  ~~
meow.cpp(11,5): note: explicitly defaulted function was implicitly deleted here
   11 |     BasicFormatArg() = default;
      |     ^
meow.cpp(19,16): note: default constructor of 'BasicFormatArg<double>' is implicitly deleted because field 'CustomState'
      has no default constructor
   19 |         Handle CustomState;
      |                ^
1 error generated.
llvmbot commented 4 months ago

@llvm/issue-subscribers-clang-frontend

Author: Stephan T. Lavavej (StephanTLavavej)

Accepted by VS 2022 17.10 Preview 1, rejected by Clang 17.0.3. On Compiler Explorer, accepted by GCC 13.2, rejected by Clang 17.0.1 and trunk: https://godbolt.org/z/8fGhWa1ss Presumably related to #48416 which was previously reported and fixed, but this larger repro is still failing. ``` C:\Temp>type meow.cpp ``` ```cpp enum class BasicFormatArgType { NoType, CustomType }; struct Monostate {}; struct Handle { Handle(int, int) {} }; template <typename Context> struct BasicFormatArg { BasicFormatArg() = default; BasicFormatArg(int x, int y) : ActiveState(BasicFormatArgType::CustomType), CustomState(x, y) {} BasicFormatArgType ActiveState = BasicFormatArgType::NoType; union { Monostate NoState = Monostate{}; Handle CustomState; }; }; int main() { BasicFormatArg<double> arg{}; (void) arg; } ``` ``` C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /c meow.cpp meow.cpp C:\Temp>clang-cl -v clang version 17.0.3 Target: x86_64-pc-windows-msvc Thread model: posix InstalledDir: C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\Llvm\x64\bin C:\Temp>clang-cl /EHsc /nologo /W4 /std:c++latest /c meow.cpp meow.cpp(24,28): error: call to implicitly-deleted default constructor of 'BasicFormatArg<double>' 24 | BasicFormatArg<double> arg{}; | ^ ~~ meow.cpp(11,5): note: explicitly defaulted function was implicitly deleted here 11 | BasicFormatArg() = default; | ^ meow.cpp(19,16): note: default constructor of 'BasicFormatArg<double>' is implicitly deleted because field 'CustomState' has no default constructor 19 | Handle CustomState; | ^ 1 error generated. ```
pinskia commented 4 months ago

Simplified slightly:

struct Handle {
    Handle(int) {}
};
union a {
        int NoState = 0;
        Handle CustomState;
} b;

This is rejected with the following error message:

<source>:7:3: error: call to implicitly-deleted default constructor of 'union a'
    7 | } b;
      |   ^
<source>:6:16: note: default constructor of 'a' is implicitly deleted because field 'CustomState' has no default constructor
    6 |         Handle CustomState;
      |                ^

As far as I know an union can only have one active field at a time and since using NSDMI on the field should produce a valid default constructor.

shafik commented 4 months ago

We are hitting this case here:

https://github.com/llvm/llvm-project/blob/ff409d39ce4673c70f474c3fdb7120bab8f94eef/clang/lib/Sema/SemaDeclCXX.cpp#L9445

I believe we need to make a similar change in this case along the lines of the fix in:

https://github.com/llvm/llvm-project/commit/765d8a192180f8f33618087b15c022fe758044af

something like this:

 if (SMOR.getKind() == Sema::SpecialMemberOverloadResult::NoMemberOrDeleted) {
    if (CSM == Sema::CXXDefaultConstructor && Field && Field->getParent()->isUnion()) {
      // [class.default.ctor]p2:
      //   A defaulted default constructor for class X is defined as deleted if
      //   - X is a union that has a variant member with a non-trivial default
      //     constructor and no variant member of X has a default member
      //     initializer
      const auto *RD = cast<CXXRecordDecl>(Field->getParent());
      if (!RD->hasInClassInitializer())
        DiagKind = !Decl ? 0 : 1;
    }else {
      DiagKind = !Decl ? 0 : 1;
    }
  }

This passes check-clang with a minor diagnostic fix in a test.

CC @royjacobson @AaronBallman @cor3ntin

AaronBallman commented 4 months ago

That seems roughly correct to me, good catch @shafik!