X3: Sequence will parse into any attribute

[Kojoley]

#include <boost/spirit/home/x3.hpp>

int main()
{
    namespace x3 = boost::spirit::x3;
    char const* s = "";

    int i;
    parse(s, s, x3::int_ >> x3::int_, i);

    boost::optional<int> o;
    parse(s, s, x3::int_ >> x3::int_, o);

    boost::variant<int, long> v;
    parse(s, s, x3::int_ >> x3::int_, v);
}

I was expecting this to not compile, but it does.

The code that responsible for this: https://github.com/boostorg/spirit/blob/a36077efbd3b91bc7dbfad211bcf4533aa137159/include/boost/spirit/home/x3/operator/detail/sequence.hpp#L328-L390

Basically it is wrong and/or redundant. I do not know what was original intentions, but what actually it does - pass the attribute to all underlying parsers of a sequence. I tried to remove it and did not find a test case that will fail.

[djowel]

Looks wrong indeed. I don't recall the original intentions anymore. As long as the tests pass, I'm OK with the change.

[cppljevans]

On 1/14/19 6:16 PM, Nikita Kniazev wrote:

#include <boost/spirit/home/x3.hpp>

int main()
{
     namespace x3 = boost::spirit::x3;
     char const* s = "";

     int i;
     parse(s, s, x3::int_ >> x3::int_, i);

     boost::optional<int> o;
     parse(s, s, x3::int_ >> x3::int_, o);

     boost::variant<int, long> v;
     parse(s, s, x3::int_ >> x3::int_, v);
}

I was expecting this to not compile, but it does.

[snip]

Strange! The example code posted above is pretty simple; hence, I am pretty surprised this problem hasn't been discovered before in end-user's use of the library. But maybe that's because the result for all the parses are false, and the end-user inferred the reason it was false was the wrong attribute type and corrected the attribute type and didn't bother to complain to spirit.

I do not know what was original intentions,

Joel's reply:

https://github.com/boostorg/spirit/issues/438#issuecomment-454274116

said "I don't recall the original intentions anymore".

Since not knowing the "original intentions" of code is a problem for, you, Nikita in this case, and I've experienced similar problems in other cases, I would suggest searching for an answer on how to document such original intentions somewhere so they are not lost. Maybe this could be done with more extensive in-source comments or maybe with descriptions in a new doc-programmers_guide directory.

but what actually it does - pass the attribute to all underlying parsers of a sequence. I tried to remove it and did not find a test case that will fail.

BTW, with your removal, does the compiler show an error with the above example code?

-regards, Larry

[cppljevans]

On 1/15/19 7:22 AM, cppljevans wrote: [snip]

discovered before in end-user's use of the library. But maybe that's because the result for all the parses are false I should have clarified that I modified the original code so that the input string was "5" and the first and last iterators to parse actually pointed to the begin and end of that string. [snip]

[Kojoley]

@cppljevans The examples above have to fail at compilation. They do not meant to run. Here is an example that has to fail at compilation but runs and reports successful parsing:

#include <boost/spirit/home/x3.hpp>
#include <iostream>

int main()
{
    namespace x3 = boost::spirit::x3;
    char const* const s = "123,456", * const end = s + std::strlen(s);

    int i;
    if (parse(s, end, x3::int_ >> ',' >> x3::int_, i))
        std::cout << "result=" << i << '\n';
    else
        std::cout << "fail\n";
}

[Kojoley]

I am also going to remove support of this if there are no objections (it simplifies fix for the cases above):

    boost::fusion::vector<> v;
    parse(s, end, x3::eps >> x3::eps, v);

[Kojoley]

Well, removing https://github.com/boostorg/spirit/blob/a36077efbd3b91bc7dbfad211bcf4533aa137159/include/boost/spirit/home/x3/operator/detail/sequence.hpp#L116-L121 still allows

    boost::fusion::vector<> v;
    parse(s, end, x3::eps >> x3::eps, v);

because it is handled by https://github.com/boostorg/spirit/blob/a36077efbd3b91bc7dbfad211bcf4533aa137159/include/boost/spirit/home/x3/operator/detail/sequence.hpp#L222-L241 so I will just remove the redundant specialization.

[cppljevans]

Well, removing

spirit/include/boost/spirit/home/x3/operator/detail/sequence.hpp

Lines 116 to 121 in a36077e template <typename Parser, typename Attribute, typename Enable = void> struct pass_sequenceattribute : mpl::if< fusion::result_of::empty , pass_sequence_attribute_unused , pass_sequence_attribute_used<Parser, Attribute>>::type {}; still allows

    boost::fusion::vector<> v;
    parse(s, end, x3::eps >> x3::eps, v);

because it is handled by

spirit/include/boost/spirit/home/x3/operator/detail/sequence.hpp

Lines 222 to 241 in a36077e template <typename L, typename R, typename Attribute, typename Context> struct partition_attribute<L, R, Attribute, Context, typename enable_if_c<(!traits::has_attribute<L, Context>::value && !traits::has_attribute<R, Context>::value)>::type> { typedef unused_type l_part; typedef unused_type r_part; typedef pass_sequence_attribute_unused l_pass; typedef pass_sequence_attribute_unused r_pass;

 static unused_type left(Attribute&) 
 { 
     return unused; 
 } 

 static unused_type right(Attribute&) 
 { 
     return unused; 
 } 

};

so I will just remove the redundant specialization.

Please clarify. Which specialization is to be removed and why is it redundant?

[Kojoley]

The answer for your question is in the quoted by you message.

[cppljevans]

The answer for your question is in the quoted by you message.

I should better explain my question:

Which specialization is to be removed and why is it
redundant?

Which I'll break down in 2 parts:

• (q1) Which specialization is to be removed?
• (q2) Why is that specialization redundant?

which are explained in the following 2 sections:

(q1) Which specialization is to be removed?

The relevant comment mentions 2 template classes:

• (c1) pass_sequence_attribute:
• (c2) partition_attribute:

and indicates that (c1) is redundant:

because it is handled by

(c2), and therefore:

I will just remove the redundant specialization.

The problem I have with that statement is (c1) is a primary template:

    template <typename Parser, typename Attribute, typename Enable = void>
    struct pass_sequence_attribute :
        mpl::if_<
            fusion::result_of::empty<Attribute>
          , pass_sequence_attribute_unused
, pass_sequence_attribute_used<Parser, Attribute>>::type {};

and not a specialization; hence, cannot be removed without causing compilation failures for the actual (c1) specializations further down in detail/sequence.hpp.

The problem might be that the relevant comment has the wrong link to (c1) which should be to one showing an actual pass_sequence_attribute specialization, c1':

    template <typename L, typename R, typename Attribute>
    struct pass_sequence_attribute<sequence<L, R>, Attribute>
: pass_through_sequence_attribute<Attribute> {};

Could you please confirm that conclusion if right?

Hopefully that conclusion is right; otherwise, I'm still confused for reasons outlined above.

(q2) Why is that specialization redundant?

This section assumes the conclusion from the previous section is correct. IOW, that (c1') is the pass_sequence_attribute specialization that's the one deemed redundant.

The relevant comment justifies the removal of (c1') with:

still allows

        boost::fusion::vector<> v;
        parse(s, end, x3::eps >> x3::eps, v);

However, just because (c1') removal still allows compilation of that "eps sequence code" does not mean it's redundant, because, if that were true, then both (c1') and (c2) are redundant because the removal of both (c1') and (c2) still allows compilation of that "eps sequence code", according to my tests.

Hence, there must be some other rationale for concluding that (c1') is redundant, and I'm asking you to please provide that rationale to help me (and other potential reviewers) provide a more informed review.

[Kojoley]

(c1) is a primary template and not a specialization

Look at the pass_sequence_attribute base, the specialization is there, in mpl::if_. I'll try to help you to see it, the same behavior without using mpl::if_:

    template <typename Parser, typename Attribute
      , bool IsEmptySeq = fusion::result_of::empty<Attribute> >
    struct pass_sequence_attribute_base :
        pass_sequence_attribute_used<Parser, Attribute> {};

    template <typename Parser, typename Attribute>
    struct pass_sequence_attribute_base<Parser, Attribute, true> :
        pass_sequence_attribute_unused {};

    template <typename Parser, typename Attribute, typename Enable = void>
    struct pass_sequence_attribute :
        pass_sequence_attribute_base<Parser, Attribute> {};

Why is that specialization redundant?

It is redundant because it is not used. The partition_attribute is disallowed to split the sequence into zero size parts, instead in such case it passes unused variable to that branch via pass_sequence_attribute_unused directly.