Add support for side-effect actions

[jwcraftsman]

It seems that custom actions are being used for two separate purposes: 1) constructing a parse result (e.g. AST tree nodes), and 2) manipulating external state for use by recognizers or dynamic filters. This is illustrated by the new option added in #45 and the discussion in #5 about handling significant indentation. I think it would be more straightforward to split up the custom actions as currently implemented into two separate, but very similar, groups.

The purpose of the existing actions would be to generate a parse result, and the purpose of the side-effects actions would be to manipulate some extra parsing state for use by recognizers or dynamic filters. The side-effect actions would always be called during the parse, as they are really part of the parsing process, but the result generation actions would be optional and could be called after-the-fact. The return value of the side-effect actions would be ignored.

I assume there would also be side-effect decorators and the option to use a _side_effects.py file as with actions and recognizers.

Question: Should the side-effects list be passed in via the grammar constuctor or the parser constructor? My first thought is that side-effects are more a part of the grammar and should go there, but passing them in through the parser would be fine as well.

Question: If this were implemented, would it eliminate the need for the extra option introduced in #45? Would the actions used by @alensuljkanovic fall cleanly into the side-effect category?

I have starting working on this, and think the implementation is very straight-forward, but I haven't published my work to a branch yet.

Any comments on this idea would be appreciated. Answers to these questions will also help give me direction, as they all will affect the implementation. Thanks!

[igordejanovic]

In general I like the idea and yes, it would eliminate the need for extra option introduced in #45.
Combined with state-keeping context object it would make sense.

With LR parsing it is straight forward to implement but GLR needs more thought.

[jwcraftsman]

Yes, actions with side effects currently don't work with GLR, so things are no worse off than they are now, right? However, wouldn't the context cloning changes proposed in #50 allow the side effects to work with GLR?

[igordejanovic]

Yes, actions with side effects currently don't work with GLR, so things are no worse off than they are now, right?

I would like for LR and GLR to stay as compatible as possible so user could start with LR because of the parser speed and deterministic behavior. If she sees that she needs more look-ahead for a parser to decide what to do or that language is truly ambiguous then she could just switch to GLR without changes.

However, wouldn't the context cloning changes proposed in #50 allow the side effects to work with GLR?

Yes, it looks like that could work. However, we should be cautious not to introduce performance degradation. parglare is used in production and there are projects that could benefit of improvements in performance, so at least we should strive to stay where we are now with each new feature. My gut tells me that this shouldn't degrade performance significantly but we can't be sure before it's done and tested. There are some performance tests in tests/perf folder. They are by no means proper but I find them useful to see how good or bad some change affects performance.

[jwcraftsman]

Question: Should the side-effects list be passed in via the grammar constuctor or the parser constructor? My first thought is that side-effects are more a part of the grammar and should go there, but passing them in through the parser would be fine as well.

Any thoughts on this question from my original comment?

[jwcraftsman]

I created a branch for this enhancement here:

https://github.com/codecraftingtools/parglare/tree/side-effects

This seems to work as expected so far. I reverted #45 and made side_effects a keyword argument to the Grammar constructor. No support for a separate _side_effects.py file yet. I'll look into that next. Please let me know if I'm on the wrong path.

[igordejanovic]

Question: Should the side-effects list be passed in via the grammar constuctor or the parser constructor? My first thought is that side-effects are more a part of the grammar and should go there, but passing them in through the parser would be fine as well.

Similar to actions, I think it is better to have that parameter on Parser. The rationale is that the parameter is there for override only. *_side_actions.py would be a place to define side actions as a part of the grammar and those actions are resolved at grammar loading time. Parser provided dict is there so that user can still override what grammar provides and add some that grammar doesn't provide.

BTW, I think it would be easier to discuss code in the context of a pull request. I suggest you to create a PR for each feature you are working on. It doesn't matter if it's not complete yet. It is easier to comment and review and track progress.

[jwcraftsman]

I'll move the parameter to the parser and change the parameter name to side_actions as you suggested.

I am happy to create the pull requests. I wasn't sure if the branch needed to be complete and fully tested before making a pull request, so I held off. Thanks for clarifying.

[igordejanovic]

Thanks for your work. I've just made a label wip for all work-in-progress PRs.