-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix constants with heredoc values #408
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Alexandre Terrasa <[email protected]>
Signed-off-by: Alexandre Terrasa <[email protected]>
|
||
qux | ||
|
||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the extra tests!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works when run against my codebase.
Agreed that fixing this up at the Prism layer is more elegant!
@@ -88,6 +88,53 @@ def test_parse_constants | |||
assert_equal(rbi, tree.string) | |||
end | |||
|
|||
def test_parse_constants_with_heredoc | |||
rbi = <<~RBI | |||
A = <<-EOF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if these examples are exhaustive enough. For example, a common pattern in the CRuby codebase is to use something like:
Foo = "#{<<~begin}#{<<~end}"
begin
foo
foo
end
like in here: https://github.com/ruby/ruby/blob/eafcdc153560fd391dabd60705cb88e3f72d7b47/test/ruby/test_file_exhaustive.rb#L760-L773 and this is the main reason why heredocs aren't necessarily contiguous in their begin-end range, and why the tag that represents the heredoc is conceptually separate from the body of it.
I think we need to test to see how multiple heredocs on the same line will interact with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need the constant value for the RBI? Can't we just replace it with T.unsafe(nil)
and add the relevant type for it and move on?
I am saying this not just for constants that have a heredoc value, but all constants in general.
As reported in #406, constants values are truncated so parsing this input:
will incorrectly output the following RBI:
The problem stems from the location Prism returns for an heredoc.
Consider this snippet:
Prism will return the following AST:
Note how the
closing_loc
points to the correct location at the end of theEOF
marker yet the node location (@ StringNode (location: (1,6)-(1,12))
) only includes the opening marker.Digging a bit in Prism issues, I found ruby/prism#1260 explaining this behavior is actually desirable.
Given this, I think the best way is to adjust the Prism location when we encounter a constant that holds a heredoc value.
I also fixed the printing of multiline constants so we insert a blank line as we do with other multiline definitions.
Closes #406.