Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix constants with heredoc values #408

wants to merge 2 commits into from

Conversation

Morriar
Copy link
Collaborator

@Morriar Morriar commented Feb 17, 2025

As reported in #406, constants values are truncated so parsing this input:

FOO = <<EOF
  bar
EOF

will incorrectly output the following RBI:

FOO = <<EOF

The problem stems from the location Prism returns for an heredoc.

Consider this snippet:

require "prism"

ruby_string = <<~RUBY
  FOO = <<~EOF
    foo
  EOF
RUBY

puts Prism.parse(ruby_string).inspect

Prism will return the following AST:

#<Prism::ParseResult:0x000000011f9556f8 @value=@ ProgramNode (location: (1,0)-(1,12))
├── flags: ∅
├── locals: []
└── statements:
    @ StatementsNode (location: (1,0)-(1,12))
    ├── flags: ∅
    └── body: (length: 1)
        └── @ ConstantWriteNode (location: (1,0)-(1,12))
            ├── flags: newline
            ├── name: :FOO
            ├── name_loc: (1,0)-(1,3) = "FOO"
            ├── value:
            │   @ StringNode (location: (1,6)-(1,12))
            │   ├── flags: ∅
            │   ├── opening_loc: (1,6)-(1,12) = "<<~EOF"
            │   ├── content_loc: (2,0)-(3,0) = "  foo\n"
            │   ├── closing_loc: (3,0)-(4,0) = "EOF\n"
            │   └── unescaped: "foo\n"
            └── operator_loc: (1,4)-(1,5) = "="

Note how the closing_loc points to the correct location at the end of the EOF 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.

Signed-off-by: Alexandre Terrasa <[email protected]>
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar Morriar added the bugfix Fix a bug label Feb 17, 2025
@Morriar Morriar self-assigned this Feb 17, 2025
@Morriar Morriar requested a review from a team as a code owner February 17, 2025 22:19

qux

)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the extra tests!

Copy link

@apiology apiology left a 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!

:shipit:

@@ -88,6 +88,53 @@ def test_parse_constants
assert_equal(rbi, tree.string)
end

def test_parse_constants_with_heredoc
rbi = <<~RBI
A = <<-EOF
Copy link
Member

@paracycle paracycle Feb 18, 2025

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.

Copy link
Member

@paracycle paracycle left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fix a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants