Re: Bison C++ mid-rule value lost with variants

[Akim Demaille]

Hi Frank!

Thanks for the feedback about 3.1!

> Le 27 août 2018 à 00:39, Frank Heckenbach <address@hidden> a écrit :
> 
> Akim Demaille wrote:
> 
>>> Le 19 juin 2018 à 00:27, Frank Heckenbach <address@hidden> a écrit :
>> 
>> Yes, indeed.  $<> is really too low level a feature when
>> it is comes to 'active types' such as we have in C++ compared
>> to C.  So we should not strive to have it work for dubious
>> cases (a type mismatch between the declared type and the one
>> passed to $<>), and allow to not use it (typed mid-rule actions).
> 
> I agree with dropping $<>, though not quite for the same reasons.

I’m not sure our reasons are _so_ different.  My view is that
the problem we had with variants and midrule action show that
midrule actions are improperly baked.  In the model of the parser
there is no need for std style variant, because the type is always
known.  The implementation of parsers in type rich languages
(those with dependent types) do not need such an approach.

So rather than adding something I saw nowhere else in the implementation
of parsers, I’d rather fix the input.


> IMHO, "active types" aren't really the problem (as std::variant or
> an equivalent implementation can handle them), but indeed it's too
> low-level and error-prone, though that would apply to all skeletons
> (but of course, dropping it from C is impossible because of
> backward-compatibility).

I agree.  There’s just one place I don’t know too well how to
address, that for $-1, $-2, etc.  But again that’s because as
of today Bison is too limited to recover the type.


>> I'm willing to discourage the use of $<> in all the outputs,
>> but forbid them when there's really no way to support them.
> 
> Depends on your definition of "no way" (see above). ;)

You seem to have a bottom-up approach: if I use this as a
semantic value, what can I do with it.  I look at this the
other way round.  Everything I know about LR shows that this
is not needed, so I’ll avoid it, _unless_, I’m given a counter
example.

I’m teaching (well, I used to teach now) LR parsing, and I don’t
want my students (well, former) to have the impression I’m fooling
them by using something different.



> I saw your discussion with Hans about the calc++ examples. Cleaning
> that up as suggested certainly helps here.

Victor gave good ideas too.


> Most of my porting work, apart from writing the new skeletons, was
> general grammar cleanup and conversion of semantic types from raw
> pointers and containers to smart pointers and other RAII classes
> (which was my main goal of the port, of course), and changes in the
> lexer (dropping flex, but that’s another story).

I fought a lot with Flex, but it works ok in C++ too with lalr1.cc.
I have one parser here, 
https://gitlab.lrde.epita.fr/vcsn/vcsn/tree/master/lib/vcsn/dot,
and another there 
https://gitlab.lrde.epita.fr/vcsn/vcsn/tree/master/lib/vcsn/rat
for instance, using Flex.


> Otherwise, AFAIR, the biggest changes were the different "%token"
> and "%type" declarations with types instead of union members (which
> made things easier),

Yes!  I made this for variants, but even for unions it’s a clear
improvement (IMHO).


> rewriting the interface to the parser, largely
> changing (and in some cases, implementing :) the "%define"s in the
> grammar file, and some changes to my Makefiles due to the additional
> generated files. -- Actually, all of these changes were large in a
> relative sense, but small in an absolute sense,

:) :) :)

> as all those parts
> of the code (parser interface, defines, Makefile, etc.) are rather
> short (at least in my projects). So they may be more of a
> psychological hurdle, since at first glance it all looks completely
> different, and since one needs that before one can actually get
> started with the new skeleton.
> 
> I fear existing projects using the C skeleton differ too widely to
> offer a detailed porting guide for all (or most) cases, but some
> easy to use examples may help here (see above), if they show the
> important features (not too many to avoid confusing users, but not
> too few so they're actually useful to many; it's probably a delicate
> balance).

I’ll try to see if I can come up with at least a short guideline.


>>> When I did the coding, std::variant actually simplified things for
>>> me (e.g., I could avoid adding move support in Bison's variant
>>> implementation), so if I were you, I'd probably use it even if I
>>> dopped $<>, but if you want to avoid its small runtime overhead,
>>> that seems possible.
>> 
>> I don't think we can afford to simply drop all C++s pre 17.
> 
> As I wrote before, there are std::variant implementations for C++14
> (which I'm actually using mostly so far) and I think also C++11
> (I haven’t tested this).

Yes, I know.  But then, I expect license nightmares to ship
them, and also by pre 17, I also mean 98/03.



>> You did wonders with your C++17 skeleton, and it would be
>> great to port your effort in the current framework.  Would
>> you contribute to that?
> 
> For the rest of this year I'll be quite busy (as you can see from
> this late reply):, but next year I might have some time to work on
> Bison.

I have a lalr1.cc which works with std::unique_ptr, but I am
still trying to get the right balance for the code to be compatible
with ‘legacy C++’.



> However, I've ported my bigger parsers to my new skeletons and use
> them actively. So I have a solution that works for me, and the
> slight overhead of storing both the static (by the skeleton) and
> dynamic (by std::variant) types is no issue to me.

Ok.


> But I've come to rely on some of the features I implemented there.
> AFAICS, you haven't commented on them yet, so I don't know how you
> think about them. If you really object to them (or equivalent
> features), I might prefer to keep using my skeletons.

I didn’t answer because I have already too many threads open,
and I prefer to stay kinda focus on the issues I plan to address
in the short term, keeping the rest for later.  I wish I could
do more :/  And I’m sorry to keep you waiting.

> These are especially the following features (described in more
> detail in my original announcement):
> 
> - "%define extra_header_prefix", to avoid file name conflicts

I agree there is, ahem…, room for improvement in this area.


> - std::move internally, to support move-only types

Of course.  That’s my goal for 3.2.


> - automatic std::move in actions -- this can be dangerous, so cannot
>  be on by default; I do it with « %define api.rhs.access {move}"

I’ll have a look at this, I have not studied the implications
yet, but it makes sense.

> - default action (when there's no user action):
>  "$$ = std::move ($1);" (easy with std::variant; I think with
>  Bison’s variant, it requires a generated switch)

I don’t remember how it’s implemented, but with a bit of luck,
it should be straightforward, as if the action were explicit,
in which case, we fall back to the regular case.

> - pre-action for empty rules with a user action: $$ is initialized
>  to the default value of the correct type. With completely static
>  variants, this might even be natural (or may need to call the
>  default constructor in a switch), and should be officially
>  documented.
> 
>  (In contrast, for non-empty rules with a user action, I
>  pre-initialize $$ to an invalid variant, so if the user action
>  forgets to set $$, a bad_variant_access will happen on access
>  which may catch some errors in user actions. This won't be
>  possible with static variants, but I can live without that.)

Never thought about this before.

Do you happen to have test cases?


> - Not sure if there will be issues WRT "%printer" and "%destructor".
>  Since they don't work with dynamic variants, I've forbidden them
>  in my skeletons.

I don’t see why they wouldn’t work.  You can trust Bison
to provide you with the expected type, so you could
std::get from your variant.

Yet I guess a visitor is nicer anyway :)

>  I don't (want to have to) ever use "%destructor" (C++ destructors
>  should do the work in all cases), and for printing I use a single
>  overloaded function (yy_print_value). This works with dynamic
>  variants using std::visit, but I think it should also work with
>  "%printer <*>", right?

I’m not sure I understand what you mean here.


> So I hope no problems here (apart from
>  adding this one directive in my grammars which I can do then :).


Thanks a lot for taking the time to explaining all this in details!