Skip to content

Handle String-like and IO-like objects for Ripper.sexp #3891

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.

Sign up for GitHub

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

Open
eregon wants to merge 1 commit into
base: main
Choose a base branch
from
+41 −1
Open

Handle String-like and IO-like objects for Ripper.sexp #3891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion lib/prism/translation/ripper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,17 @@ def self.lex_state_name(state)

# Create a new Translation::Ripper object with the given source.
def initialize(source, filename = "(ripper)", lineno = 1)
@source = source
if source.is_a?(IO)
@source = source.read
Copy link
Collaborator

@Earlopain Earlopain Jan 30, 2026

Choose a reason for hiding this comment

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

This could be backed by Prism.parse_stream but since we need the whole source anyways I think this is fine. BTW, wouldn't work for the gets case since prism also expects eof? to be present

elsif source.respond_to?(:gets)
@source = +""
while line = source.gets
@source << line
end
else
@source = source.to_str
end

This comment was marked as duplicate.

Sign in to view

Comment on lines +483 to +492
Copy link
Collaborator

@Earlopain Earlopain Jan 30, 2026

Choose a reason for hiding this comment

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

Maybe like this to skip respond_to when it's already a string? It's better for cruby but a bit more complex. I remeber something like this being done in the json gem.

Suggested change
if source.is_a?(IO)
@source = source.read
elsif source.respond_to?(:gets)
@source = +""
while line = source.gets
@source << line
end
else
@source = source.to_str
end
case source
when String then @source = source
when IO then @source = source.read
else
if source.respond_to?(:gets)
@source = +""
while line = source.gets
@source << line
end
else
@source = source.to_str
end
end

Copy link
Member Author

@eregon eregon Jan 30, 2026

Choose a reason for hiding this comment

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

I think the is_a? and respond_to? are reasonably fast on CRuby, so I would keep it simple as it is.
Not a big fan of case/when as it adds extra indentation.


@filename = filename
@lineno = lineno
@column = 0
Expand Down
30 changes: 30 additions & 0 deletions test/prism/ruby/ripper_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,36 @@ def test_tokenize
assert_equal(Ripper.tokenize(source), Translation::Ripper.tokenize(source))
end

def test_sexp_coercion
string_like = Object.new
def string_like.to_str
"a"
end
assert_equal Ripper.sexp(string_like), Translation::Ripper.sexp(string_like)

File.open(__FILE__) do |file1|
File.open(__FILE__) do |file2|
assert_equal Ripper.sexp(file1), Translation::Ripper.sexp(file2)
end
end

File.open(__FILE__) do |file1|
File.open(__FILE__) do |file2|
object1_with_gets = Object.new
object1_with_gets.define_singleton_method(:gets) do
file1.gets
end

object2_with_gets = Object.new
object2_with_gets.define_singleton_method(:gets) do
file2.gets
end

assert_equal Ripper.sexp(object1_with_gets), Translation::Ripper.sexp(object2_with_gets)
end
end
end

# Check that the hardcoded values don't change without us noticing.
def test_internals
actual = Translation::Ripper.constants.select { |name| name.start_with?("EXPR_") }.sort
Expand Down