Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for #32: Allow single-element actions #49

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

byteit101
Copy link

I didn't explore removing ca4041d but in theory it should be possible to revert now.

Now works with all single-element actions, with added tests

@jcoglan
Copy link
Owner

jcoglan commented Sep 26, 2021

There are some problems I have with this code that indicate why it doesn't make sense to run action functions for references, optional rules, or anything else that doesn't create new nodes itself.

You've put in -1 for the start and end offsets here because you're re-processing a node that's already being constructed elsewhere, so you no longer know where the node starts and ends because the code for parsing it has already been completed by some other function. Passing such values to actions will cause confusing behaviour because action functions are not written to expect negative numbers, or any number that doesn't actually reflect the extent of the parsed node.

You're also passing the single node obtained from the reference rule as the elements argument to the action function, when all action functions expect an array here. In typed languages this will not compile, and in dymanic languages it will also cause confusing behaviour because Canopy's own tree nodes are enumerable. Actions relying on that may end up discarding the actual node and instead build a new one out of its children.

I don't think that wrapping the node in a list here helps either, because this conceptually doesn't align with what action functions are for: constructing new nodes in place of Canopy's built-in tree node constructor. Running actions at points where new nodes are not created is more likely to cause confusing behaviour. It causes nodes to be parsed in a context-sensitive rather than context-free way, as it would be if you attached the action to the referenced rule, rather than the rule referencing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants