P0608R3 breaks flang build with Clang

[awson]

After merging #4713, Clang (18.1.8 and HEAD tested) fails to compile anything including flang/include/flang/Parser/parse-tree.h with errors like this:

In file included from D:/sources2/TOOLSETS/llvm/flang/lib/Parser/debug-parser.cpp:9:
In file included from D:/sources2/TOOLSETS/llvm/flang/lib/Parser\debug-parser.h:16:
In file included from D:/sources2/TOOLSETS/llvm/flang/lib/Parser\basic-parsers.h:30:
In file included from D:/sources2/TOOLSETS/llvm/flang/include\flang/Parser/parse-state.h:18:
In file included from D:/sources2/TOOLSETS/llvm/flang/include\flang/Parser\user-state.h:20:
D:/sources2/TOOLSETS/llvm/flang/include\flang/Parser/parse-tree.h(604,3): error: no matching constructor for initialization of 'std::variant<DefinedOpName, IntrinsicOperator>'
  604 |   UNION_CLASS_BOILERPLATE(DefinedOperator);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
D:/sources2/TOOLSETS/llvm/flang/include\flang/Parser/parse-tree.h(103,22): note: expanded from macro 'UNION_CLASS_BOILERPLATE'
  103 |   classname(A &&x) : u(std::move(x)) {} \
      |                      ^ ~~~~~~~~~~~~
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.42.34321\include\variant(351,11): note: in instantiation of function template specialization 'Fortran::parser::DefinedOperator::DefinedOperator<std::variant<std::optional<Fortran::parser::GenericSpec>, Fortran::parser::Abstract>, void>' requested here
  351 |         : _Head(static_cast<_Types&&>(_Args)...) {} // initialize _Head with _Args...
      |           ^
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.42.34321\include\optional(82,11): note: in instantiation of function template specialization 'Fortran::parser::GenericSpec::GenericSpec<std::variant<std::optional<Fortran::parser::GenericSpec>, Fortran::parser::Abstract>, void>' requested here
   82 |         : _Value(_STD forward<_Types>(_Args)...), _Has_value{true} {} // initialize contained value with _Args...
      |           ^
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.42.34321\include\variant(919,7): note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'remove_reference_t<variant<optional<GenericSpec>, Abstract> &>' (aka 'std::variant<std::optional<Fortran::parser::GenericSpec>, Fortran::parser::Abstract>') to 'const variant<DefinedOpName, IntrinsicOperator>' for 1st argument
  919 | class variant : private _SMF_control<_Variant_destroy_layer<_Types...>, _Types...> { // discriminated union
      |       ^~~~~~~
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.42.34321\include\variant(919,7): note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'remove_reference_t<variant<optional<GenericSpec>, Abstract> &>' (aka 'std::variant<std::optional<Fortran::parser::GenericSpec>, Fortran::parser::Abstract>') to 'variant<DefinedOpName, IntrinsicOperator>' for 1st argument
  919 | class variant : private _SMF_control<_Variant_destroy_layer<_Types...>, _Types...> { // discriminated union
      |       ^~~~~~~
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.42.34321\include\variant(939,15): note: candidate template ignored: substitution failure [with _Ty = remove_reference_t<variant<optional<GenericSpec>, Abstract> &>]: no type named 'type' in 'std::_Variant_init_helper<void, std::variant<std::optional<Fortran::parser::GenericSpec>, Fortran::parser::Abstract>, Fortran::parser::DefinedOpName, Fortran::parser::DefinedOperator::IntrinsicOperator>'
  903 |     constexpr variant(_Ty&& _Obj) noexcept(is_nothrow_constructible_v<_Variant_init_type<_Ty, _Types...>, _Ty>)
      |               ^
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.42.34321\include\variant(946,24): note: candidate template ignored: could not match 'in_place_type_t' against 'variant'
  946 |     constexpr explicit variant(in_place_type_t<_Ty>, _UTypes&&... _Args) noexcept(
      |                        ^
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.42.34321\include\variant(962,24): note: candidate template ignored: could not match 'in_place_index_t' against 'variant'
  962 |     constexpr explicit variant(in_place_index_t<_Idx>, _UTypes&&... _Args) noexcept(
      |                        ^
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.42.34321\include\variant(929,15): note: candidate constructor template not viable: requires 0 arguments, but 1 was provided
  929 |     constexpr variant() noexcept(is_nothrow_default_constructible_v<_First>)
      |               ^
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.42.34321\include\variant(954,24): note: candidate constructor template not viable: requires at least 2 arguments, but 1 was provided
  954 |     constexpr explicit variant(in_place_type_t<_Ty>, initializer_list<_Elem> _Ilist, _UTypes&&... _Args) noexcept(
      |                        ^       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.42.34321\include\variant(969,24): note: candidate constructor template not viable: requires at least 2 arguments, but 1 was provided
  969 |     constexpr explicit variant(in_place_index_t<_Idx>, initializer_list<_Elem> _Ilist, _UTypes&&... _Args) noexcept(
      |                        ^       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

Repro is complicated because generated "*.inc" files are required.

I tried to reduce failing code size, including only the code necessary for DefinedOperator structure definition to compile, but the problem disappears under these conditions.

Not sure if this a bug in STL or in Clang (or both), the latest Visual C++ 19.42.34321.1 (from 17.12.0 preview 2.0) compiles flang just fine.

[cpplearner]

Can you please attach a preprocessed source file that reproduces this error?

[awson]

Can you please attach a preprocessed source file that reproduces this error?

Ah, I completely forgot about this option.
repro.zip

I've added -fkeep-system-includes for your convenience.

[cpplearner]

Reduced:

#include <variant>
#include <optional>

struct Name {};
struct DefinedOperator {};
struct Abstract {};

struct GenericSpec {
  template <typename A>
  GenericSpec(A &&x) : u(x) {}
  GenericSpec(GenericSpec &&) = default;
  std::variant<Name, DefinedOperator> u;
};

struct InterfaceStmt {
  template <typename A>
  InterfaceStmt(A &&x) : u(x) {}
  InterfaceStmt(InterfaceStmt &&) = default;
  std::variant<std::optional<GenericSpec>, Abstract> u;
};

[CaseyCarter]

We have some constraints in variant that need short-circuiting behavior. The converting constructor and converting assignment operator templates both need to avoid checking later constraints when is_same_v<_Remove_cvref_t<_Ty>, variant> is true. A quick audit of other constraints may be a good idea. Note that the wording of the Standard doesn't indicate when short-circuiting behavior is or is not necessary in a series of constraints, we're just expected to figure it out on our own.

FWIW the only obvious workaround I see is to constrain the converting constructor templates on all 39243 of these parse node types to require that the variant type is constructible from the argument types. This may require some heavy rework of the macro DSL that defines the types - it doesn't look pleasant.

I'm still not 100% of the proximal cause. "Something" is trying to determine if std::variant<std::optional<GenericSpec>, Abstract> is move-assignable, which tries the variant(U&&) overload. The is_same_v requirement doesn't short-circuit, so we hit later constraints that effectively check if optional<GenericSpec> can be constructed from std::variant<std::optional<GenericSpec>, Abstract>, which checks if GenericSpec can be constructed from std::variant<std::optional<GenericSpec>, Abstract>, which requirement is satisfied by the converting constructor that accepts any non-lvalue. (The non-lvalue constraint isn't present in the reduced example, but it is in the original.) I'm not certain what starts this process or why. I originally thought it was the compiler checking to determine if InterfaceStmt's defaulted move constructor should be implicitly deleted, but deleting all of InterfaceStmt's special member functions has no effect. I'm also unsure why MSVC doesn't exhibit the same behavior here as Clang.

[cpplearner]

FWIW the only obvious workaround I see is to constrain the converting constructor templates on all 39243 of these parse node types to require that the variant type is constructible from the argument types. This may require some heavy rework of the macro DSL that defines the types - it doesn't look pleasant.

It seems that only the type that contains std::variant of std::optional needs to be reworked. Types that don't involve std::optional work fine despite the converting constructor.

[DavidTruby]

@cpplearner did you make any progress in fixing this in flang? I'm seeing it with clang 19.1.4 and VS 17.12.2 after a VS update yesterday. I can look into fixing it if you haven't already?

[DavidTruby]

Sorry, I just read the linked issues as well. It looks like this was broken in a PR (#4713) and then fixed in a later one (#4966). I guess #4713 made it into 17.12.2 but #4966 won't make it until 17.13p1, if I am reading things correctly?
Does this mean that building flang with clang-cl will stop being possible until 17.13 is out? Is there anything we can do to mitigate that? I suppose we can ask people to rollback their VS update.

[CaseyCarter]

It looks like this was broken in a PR (#4713) and then fixed in a later one (#4966).

Yes, that is correct.

I guess #4713 made it into 17.12.2 but #4966 won't make it until 17.13p1, if I am reading things correctly?

#4713 was in 17.12 preview 1 - long before 17.12.2 - but essentially yes.

Does this mean that building flang with clang-cl will stop being possible until 17.13 is out? Is there anything we can do to mitigate that? I suppose we can ask people to rollback their VS update.

I think the options are:

1. build with the 17.13 Preview (which is shipping),
2. rollback to 17.10,
3. apply the fix to <variant> (from https://github.com/microsoft/STL/pull/4966/files), or
4. apply a workaround to the flang sources (constrain InterfaceStmt's converting constructor template).

I'd prefer (1), but then I work almost exclusively with the current preview compiler so it's normal for me.

EDIT: I apologize for not reporting this in the LLVM repo to raise visibility, I suppose at the time I thought the issue reporter would do so.

[DavidTruby]

Ok, I think I'll add an error in the CMake for flang when using clang with the MSVC toolset at version 17.12 just telling people to use 17.11 or 17.13 with a link to this bug, does that sound reasonable to you?

[CaseyCarter]

Ok, I think I'll add an error in the CMake for flang when using clang with the MSVC toolset at version 17.12 just telling people to use 17.11 or 17.13 with a link to this bug, does that sound reasonable to you?

Yes, except I'd suggest the LTSC 17.10 rather than 17.11 for which support ended when 17.12 released.