Skip to content

Commit

Permalink
[WIP] is_substitute for variant types
Browse files Browse the repository at this point in the history 
Fix boostorg#721, boostorg#722.
Experimental: use optional<variant>
  • Loading branch information
@eido79
eido79 committed Nov 3, 2022
1 parent 73a865b commit 7e62f90
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 16 deletions.
9 changes: 6 additions & 3 deletions include/boost/spirit/home/x3/operator/alternative.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,13 @@ namespace boost { namespace spirit { namespace x3 { namespace detail
>;
};

template <template <typename...> typename A, template <typename...> typename Seq, typename T>
struct type_sequence_to_alternative_attribute<A, Seq<unused_type, T>>
template <template <typename...> typename A, template <typename...> typename Seq, typename T, typename... Ts>
struct type_sequence_to_alternative_attribute<A, Seq<unused_type, T, Ts...>>
{
using type = boost::optional<T>;
using type = conditional_t<sizeof...(Ts) == 0,
boost::optional<T>,
boost::optional<A<T, Ts...>>
>;
};

template <template <typename...> class A, typename P, typename C>
Expand Down
16 changes: 10 additions & 6 deletions include/boost/spirit/home/x3/support/traits/is_substitute.hpp
100644 → 100755
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,17 @@ namespace boost { namespace spirit { namespace x3 { namespace traits
>::type>
: mpl::true_ {};

// Can T substitute a Variant<...>?
template <typename T, typename Attribute>
struct is_substitute_impl<T, Attribute,
typename enable_if<
is_variant<Attribute>
>::type>
: variant_has_substitute<Attribute, T>
{};
struct is_substitute_impl<T, Attribute, typename enable_if<is_variant<Attribute>>::type>
{
// That is:
// boost::variant<X, Y, Z> t;
// boost::variant<Y, X> attribute;
// t = attribute;
// or better: move_to(attribute, t) should work
using type = mpl::bool_<std::is_convertible_v<Attribute, T>>;
};
}

template <typename T, typename Attribute, typename Enable /*= void*/>
Expand Down
2 changes: 1 addition & 1 deletion include/boost/spirit/home/x3/support/traits/variant_find_substitute.hpp
100644 → 100755
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ namespace boost { namespace spirit { namespace x3 { namespace traits
typedef typename
mpl::eval_if<
is_same<iter_1, end>,
mpl::find_if<types, traits::is_substitute<T, mpl::_1> >,
mpl::find_if<types, traits::is_substitute<mpl::_1, T>>,
mpl::identity<iter_1>
>::type
iter;
Expand Down
2 changes: 1 addition & 1 deletion include/boost/spirit/home/x3/support/traits/variant_has_substitute.hpp
100644 → 100755
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ namespace boost { namespace spirit { namespace x3 { namespace traits
typedef typename
mpl::eval_if<
is_same<iter_1, end>,
mpl::find_if<types, traits::is_substitute<T, mpl::_1>>,
mpl::find_if<types, traits::is_substitute<mpl::_1, T>>,
mpl::identity<iter_1>
>::type
iter;
Expand Down
36 changes: 31 additions & 5 deletions test/x3/alternative.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <boost/variant.hpp>
#include <boost/fusion/include/vector.hpp>
#include <boost/fusion/include/at.hpp>
#include <boost/fusion/include/std_pair.hpp>

#include <string>
#include <iostream>
Expand Down Expand Up @@ -304,14 +305,14 @@ main()
BOOST_TEST(test_attr("abaabb", +('a' >> attr(Foo{}) | 'b' >> attr(int{})), x));
}

{ // compile checks
{ // Compile time attribute checks
using namespace boost::spirit::x3;

test_attribute_of_alternative<boost::variant<char, int>>(char_ | int_);
test_attribute_of_alternative<boost::variant<char, int, double>>(char_ | int_ | double_);
test_attribute_of_alternative<boost::variant<unused_type, char, int, double>>(char_ | int_ | double_ | eps);
test_attribute_of_alternative<boost::variant<unused_type, char, int, double>>(char_ | int_ | double_ | eps | int_);
test_attribute_of_alternative<boost::variant<unused_type, char, int, double>>(char_ | int_ | double_ | eps | int_ | eps);
test_attribute_of_alternative<boost::optional<boost::variant<char, int, double>>>(char_ | int_ | double_ | eps);
test_attribute_of_alternative<boost::optional<boost::variant<char, int, double>>>(char_ | int_ | double_ | eps | int_);
test_attribute_of_alternative<boost::optional<boost::variant<char, int, double>>>(char_ | int_ | double_ | eps | int_ | eps);
test_attribute_of_alternative<unused_type>(eps);
test_attribute_of_alternative<boost::optional<int>>(eps | int_);
test_attribute_of_alternative<boost::optional<int>>(eps | int_);
Expand All @@ -320,9 +321,34 @@ main()
test_attribute_of_alternative<int>(int_);

test_attribute_of_alternative<boost::variant<boost::fusion::deque<int, int>, char>>((int_ >> int_) | char_);
test_attribute_of_alternative<boost::variant<unused_type, char, boost::fusion::deque<int, int>>>(char_ | (int_ >> int_) | eps);
test_attribute_of_alternative<boost::optional<boost::variant<char, boost::fusion::deque<int, int>>>>(char_ | (int_ >> int_) | eps);
test_attribute_of_alternative<boost::optional<boost::fusion::deque<int, int>>>(eps | (int_ >> int_));
}

{ // boost variant order of parameters relevant in 1.78.0 ? #721
struct A {};
struct B {};
struct C {};

using namespace boost::spirit::x3;
char const* s = "";
boost::variant<std::pair<int, boost::variant<B, A>>, C> target;
auto expr = attr(int{}) >> (attr(A{}) | attr(B{})) | attr(C{});
BOOST_TEST(test_attr(s, expr, target));
}

{ // A | A not being simplified to A in 1.78.0 #722
struct A {};
struct B {};
struct C {};

using namespace boost::spirit::x3;
char const* s = "";
using X = boost::variant<B, A>;
boost::variant<std::pair<A, X>, C> target;
auto expr = ((attr(A{}) | attr(A{})) >> lit("=") >> (attr(A{}) | attr(B{}))) | attr(C{});
BOOST_TEST(test_attr(s, expr, target));
}

return boost::report_errors();
}

14 comments on commit 7e62f90

@cppljevans
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variant<A,B> and variant<B,A> being treated as "equivalent" would require changing:

spirit/doc/x3/quick_reference.qbk

Line 299 in 73a865b

[``a: A, b: B --> (a | b): variant<A, B>

somehow to reflect that. How would that be expressed? Maybe:

[``a: A, b: B --> (a | b): variant<permutation<A, B>>

where permutation<A,B> is defined as either variant<A,B> or variant<B,A>?

@eido79
Copy link
Owner Author

@eido79 eido79 commented on 7e62f90 Nov 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cppljevans,

Thanks for the review.

I'm not sure about this. I'd expect that any "sane" variant type would allow permutation of types, or converting between two variant where the "lhs" one has a superset of the types of the rhs one. In other words, I would expect the following:

variant<A, B, C> x;
variant<permutation_of<A, B, C>> y;
variant<permutation_of<B, C> z;
x = y; // OK. x can hold an instance of A or B or C
x = z; // OK. x can hold an instance of B or C
z = y; // Not OK. z can't hold an instance of A

boost::variant supports this. I didn't test yet boost::variant2 or std::variant.

In addition to that, now that I think about it, tests (and maybe infrastructure) are needed to assert the compatibility between variants over compatible fusion sequences. That is,

// include appropriate headers
variant<fusion::deque<A, A>, fusion::deque<B, B>> x;
variant<std::pair<A, A>, std::pair<B, B>> y;
x = y; // this should work, even with subsets and permutations as in the above example

@cppljevans
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This question of conversion between variant types I've just explored and found they aren't. For example, as mentioned elsewhere, this issue707 test code fails with this variant_substitute code; However, when issue707 test code is changed to contain:

    using uda_var=
    #define USE_X3_VARIANT
    #ifdef USE_X3_VARIANT
      x3::variant
    #else
      boost::variant
    #endif
      < uda_s1
      , uda_s2
      >
      ;

then it only fails when defined(USE_X3_VARIANT). So, should x3::variant be outlawed to allow issue707 to be resolved?

@eido79
Copy link
Owner Author

@eido79 eido79 commented on 7e62f90 Nov 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right: x3::variant doesn't support that direct assignment from another x3::variant with permuted types.
Outlawing variant is out of question, but, as I've already mentioned in my comments in the issue in the main spirit repo, the fix in this branch is crude.

I can see no reasons for which variant<A, B> should not be "convertible" to variant<B, A>: if a sum type can hold A and B, their order in the declaration of the type shouldn't matter.
This also applies to different "species" of variant. One should be able to conjure code to assign boost::variant<A, B> to x3::variant<A, B>or std::variant<A, B>, regardless of the order of the types.

What it's needed IMHO is a proper definition of is_substitute for variants, and corresponding support in x3::move_to (and maybe elsewhere).
Supposing that lhs and rhs are recognized by x3 as variant types, I'm thinking along the lines of

  • If rhs is directly assignable to lhs, then it's all easy
  • If not, if all the types of lhs are substitutes of at least one type of rhs, then rhs is substitute for lhs. x3::move_to should visit rhs and assign the active type to lhs (additional type traits might be needed for "custom" variants)
  • The degenerate case where rhs is an empty variant should be explicitly supported, and treated as if rhs is unused_type, i.e. lhs = rhs is a no-op

I'll try to give this a shot when I have some time to spare.

@eido79
Copy link
Owner Author

@eido79 eido79 commented on 7e62f90 Nov 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting fact: std::variant doesn't support assignment from permuted types as well. boost::variant2 however does.

@eido79
Copy link
Owner Author

@eido79 eido79 commented on 7e62f90 Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cppljevans,

JFYI, I stumbled on some surprising behavior of boost::variant, which apparently doesn't play well with is_constuctible/is_assignable traits in presence of an ambiguous conversion or explicit constructor.
My plan to use those traits to improve conversion of variants in spirit can't work unless mentioned behavior gets fixed.

I've raised an issue and sent a pull request to boost::variant... let's see what comes out of it.

@cppljevans
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eido79,

Does this change to boost::variant allow the issue707 test to pass?

Hi @cppljevans,

JFYI, I stumbled on some surprising behavior of boost::variant, which apparently doesn't play well with is_constuctible/is_assignable traits in presence of an ambiguous conversion or explicit constructor. My plan to use those traits to improve conversion of variants in spirit can't work unless mentioned behavior gets fixed.

I've raised an issue and sent a pull request to boost::variant... let's see what comes out of it.

@eido79
Copy link
Owner Author

@eido79 eido79 commented on 7e62f90 Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently it doesn't. I tried using my patch to variant in combination with "vanilla" spirit and my patched version, but the issue remains.

From a cursory look, I doubt this is due to the issue I'm trying to address in variant, but I see in the comments to boostorg#707 that you have posted a possible solution.
Given that x3 is moving to C++17, I think it might be a good idea to try to move it forward.

I'll try to investigate further when I have some time to spare.

@cppljevans
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see in the comments to boostorg#707 that you have posted a possible solution. Given that x3 is moving to C++17, I think it might be a good idea to try to move it forward.

I'll try to investigate further when I have some time to spare.

@eido79, the possible solution I mentioned won't work because, sadly, it doesn't account for permuting of the variant args. I just hadn't considered that.
During my search for that solution, I found adding debug _enabler classes to is_substitute.hpp code very helpful.
Please see is_substitute_impl_enabler. That may help you in seeing where things might go wrong. HOWEVER, it does require other code to be cloned; hence, that may be too much trouble for you. I don't know, but thought I'd offer.

@eido79
Copy link
Owner Author

@eido79 eido79 commented on 7e62f90 Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

As a quick test, I replaced x3::variant with boost::variant, which supports permutations, but the result didn't change.

is_substitute_impl_enabler looks quite useful, thanks! I'll definitely keep it in mind in case I need something like that.

@eido79
Copy link
Owner Author

@eido79 eido79 commented on 7e62f90 Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

It's been a while, but I've finally found some time to come back to this.
All in all: it's a mess. The more I look into this, the more I believe that the way spirit handles variants, especially when parsing into containers, needs to be deeply revisited.
I hope to have some quality time to dedicate to this, as this is going to require some effort even to start.

@cppljevans
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eido79 , I just uploaded an update to my unfold_left.
The updates should enable compilation with any tests you have. The update simplifies code, as explained in the README.md file.
HTH.
-Larry

@eido79
Copy link
Owner Author

@eido79 eido79 commented on 7e62f90 Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cppljevans,

Thanks for the info. Between family and ${dayjob} I'm unfortunately having virtually no spare time to dedicate to side projects, but I'll definitely check your branch out when I can.

@cppljevans
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, the unfold_left code fails with the expect.cpp test:

https://github.com/boostorg/spirit/blob/32fb6f689957c121973594e2505fd959a917291e/test/x3/expect.cpp#L108

It has something to do with unfold_left detail/sequence.hpp having no replacement for partition_attribute:

https://github.com/boostorg/spirit/blob/32fb6f689957c121973594e2505fd959a917291e/include/boost/spirit/home/x3/operator/detail/sequence.hpp#L124

Trying to figure a partition_attribute replacement.

Please sign in to comment.