Skip to content

Conversation

@Morriar
Copy link
Contributor

@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.

@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:


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
Contributor Author

Choose a reason for hiding this comment

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

Wow I didn't even know you could use the heredocs like this 🤯

I added a few more tests and handled them 👍

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.

Morriar added 2 commits March 25, 2025 17:39
Signed-off-by: Alexandre Terrasa <[email protected]>
Signed-off-by: Alexandre Terrasa <[email protected]>
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Copy link
Contributor Author

Morriar commented Mar 26, 2025

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?

You got me curious and I tried: https://github.com/Shopify/rbi/compare/at-const-type?expand=1. We could parse the T.let values in the parser and replace the Const#value by a Const#type.

It becomes a bit uglier when trying to handle T::Enum values that we treat as const and use the new value to discriminate. Consider this example:

class Foo < T::Enum
  enums do
    A = new
    B = 42
    C = T.let(nil, T.nilable(Foo))
  end
end

How should we generate the proper RBI for this without knowing the value?

I think one solution would be to parse A = new as a new EnumValue node rather than a Const. It's doable but a more involved change.

@paracycle
Copy link
Member

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?

You got me curious and I tried: https://github.com/Shopify/rbi/compare/at-const-type?expand=1. We could parse the T.let values in the parser and replace the Const#value by a Const#type.

It becomes a bit uglier when trying to handle T::Enum values that we treat as const and use the new value to discriminate. Consider this example:

class Foo < T::Enum
  enums do
    A = new
    B = 42
    C = T.let(nil, T.nilable(Foo))
  end
end

How should we generate the proper RBI for this without knowing the value?

I think one solution would be to parse A = new as a new EnumValue node rather than a Const. It's doable but a more involved change.

Yes, there are edge cases indeed. Tapioca, for example, treats T::Enum values differently, albeit with runtime information, from all other kinds of constants.

@Morriar Morriar merged commit 5ddb08a into main Mar 26, 2025
8 checks passed
@Morriar Morriar deleted the at-fix-heredoc branch March 26, 2025 19:01
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