Identify unexpected nodes in error recovery scenarios

[thomasmarshall]

In error recovery scenarios (for example for invalid Ruby code) Prism can return unexpected node types in fields they wouldn't be for valid Ruby. For example:

module def foo end

Prism parses this as a ModuleNode with a DefNode in the constant_path field. The allowed/expected nodes are listed in config.yml here and we use on error: to signal that in error recovery scenarios there might be something other than ConstantReadNode or ConstantPathNode. In this case, we only have on error: MissingNode but it can actually be DefNode or something else (perhaps this is an oversight and all possible error nodes should be listed here?).

Clients using Prism may want to deal with invalid Ruby syntax in all kinds of different ways, but I suspect the most common approach would be to simply ignore unexpected nodes and maybe stop traversing that branch of the AST. In any case, clients cannot expect only valid node types in fields, as shown above.

One way for clients to deal with unexpected node types would be to guard based on each of the expected node types, for example:

if (!PM_NODE_TYPE(node, PM_CONSTANT_READ_NODE) && !PM_NODE_TYPE(node, PM_CONSTANT_PATH_NODE)) {
  // handle error case
}

In some cases this might be quite verbose, with many valid node types.

Another approach (assuming the on error indicators in config.yml are exhaustive) would be to check for known error cases:

if (PM_NODE_TYPE(node, PM_MISSING_NODE) || PM_NODE_TYPE(node, PM_DEF_NODE)) {
  // handle error case
}

Again, this could potentially be quite verbose and I'm not sure if config.yml is currently expected to exhaustively list all possible on error cases.

Proposal

Prism could introduce an UnexpectedNode type (similar to MissingNode) that would be returned in error recovery scenarios, wrapping the original node type.

if (PM_NODE_TYPE(node, PM_MISSING_NODE) || PM_NODE_TYPE(node, PM_UNEXPECTED_NODE)) {
  // handle error case
}

Or introduce a helper covering both cases:

if (PM_ERROR_NODE(node)) {
  // handle error case
}

The client could more easily just ignore error recovery cases, or they could do something appropriate with node->child in the case of UnexpectedNode.

I'm sure there are other approaches too, but this one seemed quite straightforward. However, introducing a new node type would be a breaking change, so perhaps there is a non-breaking alternative, such as adding a flag to existing node types?

Prototype

I prototyped it here but I thought I should open an issue first, to see if this is something you would consider, rather than spending more time tidying up the code for a PR. That prototype also ensures MissingNode is always added in cases where a required field is null, but that's potentially a separate issue.

[kddnewton]

Oh nice, that's elegant. It has always bothered me that we've been lying about the structure of the tree in error-recovery scenarios. I would go ahead and get rid of the on error stuff in config.yml and instead document that all nodes can be missing/unexpected. That seems like a good blanket statement, since we've just been adding stuff to config.yml piecemeal, not in a way that is actually exhaustive.

As for the prototype, I'd much prefer if we move the logic up one level. Right now it's in the constructors, which I'm hoping to template out for simplicity. (Lots of changes in here: #3772). I'd prefer if we detect the incorrect node types where we call the constructors (because usually it's a case statement or some other way of telling that we know we're in an error case).

[eregon]

I like this idea, it makes the field types (as in config.yml) much simpler as well.
I suspect for most consumers they don't need to handle error cases and then they can just rely on the correct types (as already the case).

document that all nodes can be missing/unexpected. That seems like a good blanket statement, since we've just been adding stuff to config.yml piecemeal, not in a way that is actually exhaustive.

Documenting it is fine, but if that changes the type (in e.g. RBS/RBI/Java) of some fields it will cause issues for projects relying on the type of some field to always be SomeNode.
For example https://github.com/eregon/prism-generated/blob/8302f5cca140f5cd4938a84455d1e08e7eae6b11/sig/prism/node.rbs#L325: BeginNode#statements is always StatementsNode?, changing that to StatementsNode? | UnexpectedNode | MissingNode would be incompatible typing-wise.

[kddnewton]

Ahh I forgot. No problem, I can audit them to make sure we know which ones are missing. Probably a good addition to the test suite anyway.

Also, upon further thought, I think we should merge UnexpectedNode and MissingNode into one. I think maybe ErrorRecoveryNode? And it could have an optional child tree, which if it's empty it's effectively the same as MissingNode.

[thomasmarshall]

I opened a draft PR with some of the suggestions from this thread. Is that approach roughly what you had in mind @kddnewton?