Fix Prism crash with missing class/module name#757
Draft
thomasmarshall wants to merge 1 commit into
Draft
Conversation
755791f to
17f7160
Compare
83b0a6e to
5e38b7c
Compare
b84b286 to
17ef6d6
Compare
Sorbet was crashing in Prism parser mode was when encountering malformed class or module definitions where the constant path is missing or invalid, such as: ```ruby class end ``` or ```ruby class def foo; end end ``` or ```ruby class def foo; end def bar; end end ``` For the first example, Prism generates a ClassNode with constant_path set to a MissingNode and a null body. For the second example, Prism generates a ClassNode with the constant_path set as the first DefNode and a null body. For the third example, Prism generates a ClassNode with the constant_path set as the first DefNode and the body is a StatementsNode containing the second DefNode. The original parser throws away the class/module node and only includes the body node (if present) in the tree. So in the third example it would just be the `bar` DefNode. With the Prism parser it's fairly straightforward (easier?) for us to always return the class/module node, filling in a missing constant for the name, and the same body (e.g. the `bar` DefNode), so only the invalid constant path node is discarded. We considered changing how Prism parses these examples to always return a class/module node with a MissingNode for the constant_path and all children in the body, but it's not always obvious that that's the right way to parse it. For example: ```rb class (return)::A; end class 0.X end ``` In these examples it wouldn't make sense to parse them as class nodes with a body containing `(return)::A` or `0.X`—it's more likely the intention was that these are constant paths, not body nodes. As such, I think it makes sense to just work with the existing Prism behavior and do the best we can with it. Another option was to copy the mimic parser behavior exactly (throwing away the outer class/module) but since it's easier for us to keep it, I went with the approach in this commit.
17ef6d6 to
ca77227
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Sorbet was crashing in Prism parser mode was when encountering malformed class or module definitions where the constant path is missing or invalid, such as:
or
or
For the first example, Prism generates a ClassNode with constant_path set to a MissingNode and a null body.
For the second example, Prism generates a ClassNode with the constant_path set as the first DefNode and a null body.
For the third example, Prism generates a ClassNode with the constant_path set as the first DefNode and the body is a StatementsNode containing the second DefNode.
The original parser throws away the class/module node and only includes the body node (if present) in the tree. So in the third example it would just be the
barDefNode. With the Prism parser it's fairly straightforward (easier?) for us to always return the class/module node, filling in a missing constant for the name, and the same body (e.g. thebarDefNode), so only the invalid constant path node is discarded.We considered changing how Prism parses these examples to always return a class/module node with a MissingNode for the constant_path and all children in the body, but it's not always obvious that that's the right way to parse it. For example:
In these examples it wouldn't make sense to parse them as class nodes with a body containing
(return)::Aor0.X—it's more likely the intention was that these are constant paths, not body nodes. As such, I think it makes sense to just work with the existing Prism behavior and do the best we can with it.Another option was to copy the mimic parser behavior exactly (throwing away the outer class/module) but since it's easier for us to keep it, I went with the approach in this commit.