canopy icon indicating copy to clipboard operation
canopy copied to clipboard

Fix for #32: Allow single-element actions

Open byteit101 opened this issue 4 years ago • 1 comments
trafficstars

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

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

byteit101 avatar May 06 '21 00:05 byteit101

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.

jcoglan avatar Sep 26 '21 15:09 jcoglan