fix: Prevent the commutative merging of all extra elements #562

That's one expensive boolean you've got there 😅 (64 bits of space for 1 bit of info). Not like I can come up with something better though.. one could of course make AstNode into an enum of Regular/Extra, but it would be such a massive headache to modify all the code touching AST nodes throughout the codebase to handle that change

Anyway, maybe we want to include it in the Display output while we're at it? Not sure quite how useful it would be though

I was naively hoping that Rust would be able to magically make this compact by bundling it up with another field, but that was wishful thinking… If we want to shave some bits, I guess we could probably bundle this up in the id field (for instance as least significant bit), but I am not sure it's worth it.

The ideal situation would be to be able to look up directly from theLangProfile whether a certain node type is extra or not, but that's not information that's readily available sadly.

Hm, it's a bit unfortunate that this error only happens inside commutatively_merge_lists, because its description says that it should only be called if we know that the lists are safe to merge. So for example in build_subtree_from_changeset, we:

1. check if the parent is commutative
2. if so, call this method and ? the error
3. otherwise, create a conflict tree

Wouldn't it be nicer if we could tell the caller that the children weren't commutative after all, so that they could still fall back onto some other strategy?

I think this is a case which could benefit from a more descriptive error type – something roughly like this:

enum CommutativelyMergeListsError {
    ChildrenNotCommutative(String),
    Other(String)
}

That way, the caller could match on the error kind and do the best thing

It's a change I have thought about doing in #563 but I was reluctant because it's a change to the actual merging logic which could have unintended consequences. But I agree with you that it is worth doing.

Yeah it's probably best to leave this to a future PR -- could you please add a TODO comment?

I've opened a PR for it instead: #568

I realize that this method is currently only called in this one place, but more generally, if we simplify our commutative children groups after this change, wouldn't it need to also check that the provided list of children doesn't contain any extra nodes? In fact, maybe the check should just be moved there..

I would be very keen to do that if we had a simple way to determine if a given node type is extra. At the moment we can only do it for a concrete node, sadly.

In the future, we might be able to parse the node-types.json file that is available from the crate of the parser and extract the node types marked as extra from there. Ideally we'd do this at compilation time so that we don't embed big JSON blobs in our binary, just for them to be parsed at runtime. For this to work, we'll also need our tree-sitter parsers to be generated by a recent enough version of tree-sitter, because there was a bug in the generation of this information.

If we go ahead with my current solution I'd open an issue about making this move, as it would indeed be more principled and save some memory.

if we had a simple way to determine if a given node type is extra

Oh, right, I completely overlooked this.

Let's go with this approach then, but maybe also add a FIXME comment to CommutativeParent::child_separator?