You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
moduledeffooend
This is parsed as a ModuleNode where constant_path is a DefNode. After this change, constant_path will be an ErrorRecoveryNode with its child set to the DefNode. This way clients can just check PM_NODE_TYPE(PM_ERROR_RECOVERY_NODE) to understand if the expected node is either missing (formerly MissingNode, now ErrorRecoveryNode with empty child) or an unexpected node.
As per the suggestion from @kddnewton, I folded the missing nodes and unexpected nodes into a single ErrorRecoveryNode type. 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.yml somehow 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:
The first commit captures the existing error recovery scenarios in a Ruby test.
Then we rename MissingNode to ErrorRecoveryNode.
Then we setup the function validation macro.
Then there is a separate commit for each node type with an on error case in one of its fields in config.yml.
Finally, the last commit removes on error from config.yml as per this suggestion:
I would go ahead and get rid of the on error stuff in config.yml
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!
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.
FYI the Ruby code already has logic to check field kinds in the Ruby nodes constructors, search for check_field_kind in the codebase (context: #3022).
Are the added checks here redundant perhaps, or do they cover more somehow?
Are the added checks here redundant perhaps, or do they cover more somehow?
The validations I added are on the C side. They check if the node is an expected type and then wrap it in an ErrorRecoveryNode if not. I believe that's different to check_field_kind which is only on the Ruby side, so in that sense they are not redundant.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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!