Fix the definition of is_substitute and variant-related substitutability

[yaito3014]

• Remove variant-related check from is_substitute reverted
• Fix the wrong order of template arguments specified to variant_has_substitute
• Fix the definition of is_substitute and variant-related substitutability

Todo

• Add tests
• Fix typo pass_attibute_as_is -> pass_attribute_as_is
• ......

[saki7]

At first glance, I don't think this can work. Removing the variant_has_substitute check entirely from is_substitute means variant is never going to be considered on substitution check. If this PR is seemingly working, I suspect there are many unimplemented tests that should fail after this change. And if it is truly working, I suspect there's conflicting scenario on #42 because the only remaining parts which use variant_has_substitute is in move_to, which is heavily modified in that PR.

If it's truly not needed, please remove the forward declaration also.

Anyhow, this is a big change, and I need to see a clear description (written in English) that

• explains why variant_has_substitute was required in the old codebase and
• why removing it does not break the semantics on the current codebase.

[saki7]

Simple test case:

static_assert(is_substitute_v<int, iris::rvariant<int, std::string>>);

[saki7]

Please add this test case too:

struct X {};
struct Y {};
struct Z {};
iris::rvariant<Y, Z> from;
iris::rvariant<X, Y, Z> to;
x4::move_to(std::move(from), to);

[yaito3014]

Simple test case:

static_assert(is_substitute_v<int, iris::rvariant<int, std::string>>);

Is the motivation for is_substitute<T, Attr> "T can hold arbitrary value of Attr"? If not, I suppose it is "T can hold actual value of Attr, even if Attr can hold values that T cannot hold". In actual code, is_substitute<T, Attr> is used to check whether T can be temporary storage for Attr, not strictly requiring the "substitutability". If the former is the motivation, the suggested test case does not make sense.

[saki7]

Simple test case:

static_assert(is_substitute_v<int, iris::rvariant<int, std::string>>);

Is the motivation for is_substitute<T, Attr> "T can hold arbitrary value of Attr"? If not, I suppose it is "T can hold actual value of Attr, even if Attr can hold values that T cannot hold". In actual code, is_substitute<T, Attr> is used to check whether T can be temporary storage for Attr, not strictly requiring the "substitutability". If the former is the motivation, the suggested test case does not make sense.

To be honest I'm not sure, but either one of the below should pass:

static_assert(is_substitute_v<int, iris::rvariant<int, std::string>>); // 1
static_assert(is_substitute_v<iris::rvariant<int, std::string>, int>); // 2

If both fails, I think is_substitute is totally broken, but I can't believe it is broken. Can you elaborate on the semantics?

[yaito3014]

Currently (on 7324a7b), following code passes:

STATIC_CHECK(!x4::traits::is_substitute_v<iris::rvariant<int, double>, int>);
STATIC_CHECK( x4::traits::is_substitute_v<int, iris::rvariant<int, double>>);

And now (on f2b5afb), the inverse passes:

STATIC_CHECK( x4::traits::is_substitute_v<iris::rvariant<int, double>, int>);
STATIC_CHECK(!x4::traits::is_substitute_v<int, iris::rvariant<int, double>>);

Both definitions of is_substitute for variant pass existing tests, so we need test case involving actual parsing.

[yaito3014]

I'm planning to rename is_substitute to is_suitable_for_temporary_creation-ish name.
Now, consider following:

STATIC_CHECK(x4::traits::is_suitable_for_temporary_creation_for_v<int, int>);  // OK, identical type

STATIC_CHECK( x4::traits::is_suitable_for_temporary_creation_for_v<int, iris::rvariant<int>>);  // OK, parsed value can be assigned to variant directly
STATIC_CHECK(!x4::traits::is_suitable_for_temporary_creation_for_v<iris::rvariant<int>, int>);  // ??

STATIC_CHECK( x4::traits::is_suitable_for_temporary_creation_for_v<int, iris::rvariant<int, double>>);  // OK, parsed value can be assigned to variant directly
STATIC_CHECK(!x4::traits::is_suitable_for_temporary_creation_for_v<iris::rvariant<int, double>, int>);  // ??, parsed value can be incompatible

In the current code, the latter cases are not handled and we should decide what types can be "temporary storage" for specific types.

---

P.S.
Additionally, consider following:

STATIC_CHECK(x4::traits::is_suitable_for_temporary_creation_for_v<int, iris::rvariant<iris::rvariant<int>, double>);  // ??

This assertion fails currently because:

• is_suitable_for_temporary_creation_for<int, rvariant<rvariant<int>, double> depends on:
  • variant_has_suitable_type_for_temporary_creation_for<rvariant<rvariant<int>, double>, int> depends on:
    • is_suitable_for_temporary_creation_for<rvariant<int>, int> which is currently false

However, parsed int can be assigned to iris::rvariant<iris::rvariant<int>, double> directly, so it should be "suitable".

[yaito3014]

I think the "substitutability" can be defined by X4Movable or similar, since temporary storages are ultimately passed to move_to or traits::push_back anyway.

[saki7]

I think the "substitutability" can be defined by X4Movable or similar, since temporary storages are ultimately passed to move_to or traits::push_back anyway.

This can't be implemented because move_to uses is_substitute for overload resolution. Making is_substitute depend on move_to results in a cyclic dependency.

[saki7]

@yaito3014 Can you show me the actual test case that illustrates the substitutability on complex types? I think we're having a hard time determining the goal of this problem because we don't have a clear vision on the actual parser behavior.

The test case should include both the type-level test and parser-level tests.

[yaito3014]

If we relax constraints of can_hold (formerly is_substitute) from std::is_same to std::is_assignable, parsing like parse("x", int_ | char_) will regress, because variant_find_holdable_type (formerly variant_find_substitute) finds int from iris::rvariant<int, char> first.

P.S.
I'll try to modify variant_find_holdable_type to match exact type first, then fall-back to can_hold's loose match

[cppwarningnotifier]

Environment				C++23	C++26
x4	Clang	21	Debug	✅success	✅success
			Release	✅success	✅success
	GCC	14	Debug	✅success	✅success
			Release	✅success	✅success
	MSVC	2022	Debug	✅success	✅success
			Release	✅success	✅success
		2026	Debug	✅success	✅success
			Release	✅success	✅success