-
Notifications
You must be signed in to change notification settings - Fork 174
Validate expected nodes #3843
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
base: main
Are you sure you want to change the base?
Validate expected nodes #3843
Conversation
1033105 to
7f060ba
Compare
FYI the Ruby code already has logic to check field kinds in the Ruby nodes constructors, search for |
|
I should add: this PR LGTM from a quick look at the diff |
This PR aims to address #3828 by validating expected node types and wrapping unexpected node types in an
ErrorRecoveryNode. This should make it easier for consumer programs to handle invalid Ruby code. For example:This is parsed as a
ModuleNodewhereconstant_pathis aDefNode. After this change,constant_pathwill be anErrorRecoveryNodewith itschildset to theDefNode. This way clients can just checkPM_NODE_TYPE(PM_ERROR_RECOVERY_NODE)to understand if the expected node is either missing (formerlyMissingNode, nowErrorRecoveryNodewith emptychild) or an unexpected node.As per the suggestion from @kddnewton, I folded the missing nodes and unexpected nodes into a single
ErrorRecoveryNodetype. I also added tests for currently known error recovery scenarios and switched to validating expected node types at the callsites rather than in the constructors.From an implementation perspective, I think it could be nicer to have comprehensive validations in the constructors because it doesn't require understanding any of the parsing logic (I think my changes are correct, but likely harder to review than validations in constructors). They could also potentially be codegen'd from
config.ymlsomehow but I suppose we'd be doing unnecessary work in cases where unexpected nodes are not possible.I split the changes into a series of small commits that should make it more straightforward to understand what is changing for each node type:
MissingNodetoErrorRecoveryNode.on errorcase in one of its fields inconfig.yml.on errorfromconfig.ymlas per this suggestion:However, I'm not 100% sure this is what was meant, I might have misunderstood.
This is a somewhat substantial change, so I understand if it's a little more difficult to review or feedback on. I'm very happy to make any changes to the approach and implementation, please just let me know!