Regression: std::variant is not a literal type

[Quuxplusone]

Bugzilla Link	PR39232
Status	RESOLVED FIXED
Importance	P enhancement
Reported by	Louis Dionne (ldionne@apple.com)
Reported on	2018-10-09 11:04:47 -0700
Last modified on	2018-11-26 08:15:13 -0800
Version	unspecified
Hardware	PC All
CC	llvm-bugs@lists.llvm.org, mclow.lists@gmail.com, mcypark@gmail.com, richard-llvm@metafoo.co.uk
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

The following program does not compile under Clang ToT:

================================================
#include <variant>

struct ValueBase {
protected:
    constexpr ValueBase() noexcept { }
    constexpr ValueBase(ValueBase const&) noexcept { }
    constexpr ValueBase(ValueBase&&) noexcept { }
};

struct NoCtors : ValueBase {
    NoCtors() = delete;
    NoCtors(NoCtors const&) = delete;
};

int main() {
    using V = std::variant<int, NoCtors>;
    constexpr V v;
}
================================================

Live reproduction: https://wandbox.org/permlink/b0trjzltSeq2Hirb

This reproducer was lifted from libc++'s test case
test/std/utilities/variant/variant.variant/variant.status/index.pass.cpp. It
started failing with Clang's r343131:

Author: Richard Smith <richard-llvm@metafoo.co.uk>
Date:   Wed Sep 26 19:00:16 2018 +0000

    P1008R1 Classes with user-declared constructors are never aggregates in
    C++20.

[Quuxplusone]

(In reply to Louis Dionne from comment #0)
> The following program does not compile under Clang ToT:

To be clear: it doesn't compile in C++20 mode. There's no regression that I can
see for any non-experimental language mode. (But please correct me if I'm
wrong.)

> struct NoCtors : ValueBase {
>     NoCtors() = delete;
>     NoCtors(NoCtors const&) = delete;
> };

In C++14, this was not a literal type (but there's no problem there because we
didn't have std::variant).

In C++17, this became a literal type because NoCtors became an aggregate (we
allowed aggregates to have base classes).

In C++20, it's not a literal type again, like in C++14 and before, because
there's no way to construct an instance in a constant expression.

On the compiler side, I think we're doing the right thing. Now let's look at
libc++...

> int main() {
>     using V = std::variant<int, NoCtors>;
>     constexpr V v;
> }

std::variant seems underspecified with respect to whether and when it's a
literal type. I can't find any specification of whether

  using V = std::variant<literal_type, not_literal_type>;

is a literal type.

(We do specify under what circumstances it has a constexpr constructor and a
trivial destructor, but that's not enough -- it also needs to have all bases
and members of literal types, which means that if V is to be a literal type, it
cannot be implemented in terms of a union of literal_type and non_literal_type -
- which, ironically, libc++ does precisely to support use in constant
expressions.

> Live reproduction: https://wandbox.org/permlink/b0trjzltSeq2Hirb

Here's a simplified repro that doesn't depend on the C++20 change:

https://wandbox.org/permlink/z8Ko8Y9j2aaPc62M

If we want to support such cases in libc++'s std::variant, I believe it is
possible, but it's awkward:

 * If all the variant elements are of literal type, you must use a union to hold the elements or you will be unable to initialize them in a constant expression.
 * If any element is not of a literal type, you must not use a union member to hold that element, or the variant as a whole will not be a literal type.

So I think there is only one way to do it (up to trivial equivalences): form a
union containing all the literal types and a suitably-sized-and-aligned char
array, use the latter to hold all the other types.

But it sounds like we could do with some LWG clarification of intent.

[Quuxplusone]

Right, it seems like the test case is probably now wrong. We don't try to make variant<literal_type, non_literal_type> be a literal type, and even the `variant being a literal type is an extension as far as I know.

[Quuxplusone]

I have committed revision 344546, which marks the two failing tests as being "C++17-only" for now, with a note that they should be updated when this bug is resolved.

[Quuxplusone]

Long term fix up for review here: https://reviews.llvm.org/D54767. Basically, the tests seem wrong to me.

[Quuxplusone]

Fixed in r347568.