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.

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

Jump to bottom

method_source compatibility problems introduced with TruffleRuby 24.0.0 #3551

Open
nirvdrum opened this issue Apr 25, 2024 · 3 comments
Open

method_source compatibility problems introduced with TruffleRuby 24.0.0 #3551

nirvdrum opened this issue Apr 25, 2024 · 3 comments
Labels
compatibility

Comments

@nirvdrum
Copy link
Collaborator

nirvdrum commented Apr 25, 2024 •
edited

John Hawthorn recently wrote a blog post comparing CRuby and Crystal performance. The benchmark uses the crystalruby gem to embed Crystal code in Ruby. That gem has a dependency on method_source.

When running the benchmarks with TruffleRuby 24.0.0, I encountered a syntax error coming from the method_source gem:

> ruby -v crystal_benchmark.rb
truffleruby 24.0.0, like ruby 3.2.2, Oracle GraalVM Native [aarch64-darwin]
/Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source.rb:29:in `source_helper': Could not parse source for fib_cr: (eval):3: unexpected end of file, assuming it is closing the parent top level context (MethodSource::SourceNotFoundError)
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source.rb:115:in `source'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/crystalruby-0.2.0/lib/crystalruby/adapter.rb:128:in `extract_source'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/crystalruby-0.2.0/lib/crystalruby/adapter.rb:118:in `define_crystalized_method'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/crystalruby-0.2.0/lib/crystalruby/adapter.rb:75:in `method_added'
	from crystal_benchmark.rb:9:in `<module:Fibonnaci>'
	from crystal_benchmark.rb:6:in `<main>'
/Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source/code_helpers.rb:70:in `block in complete_expression?': (eval):3: unexpected end of file, assuming it is closing the parent top level context (SyntaxError)
	from <internal:core> core/throw_catch.rb:36:in `catch'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source/code_helpers.rb:70:in `complete_expression?'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source/code_helpers.rb:97:in `block in extract_first_expression'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source/code_helpers.rb:95:in `each'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source/code_helpers.rb:95:in `extract_first_expression'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source/code_helpers.rb:30:in `expression_at'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source.rb:27:in `source_helper'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source.rb:115:in `source'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/crystalruby-0.2.0/lib/crystalruby/adapter.rb:128:in `extract_source'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/crystalruby-0.2.0/lib/crystalruby/adapter.rb:118:in `define_crystalized_method'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/crystalruby-0.2.0/lib/crystalruby/adapter.rb:75:in `method_added'
	from crystal_benchmark.rb:9:in `<module:Fibonnaci>'
	from crystal_benchmark.rb:6:in `<main>'
/Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source/code_helpers.rb:70:in `block in complete_expression?': (eval):3: unexpected end of file, assuming it is closing the parent top level context (SyntaxError)
	from <internal:core> core/throw_catch.rb:36:in `catch'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source/code_helpers.rb:70:in `complete_expression?'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source/code_helpers.rb:97:in `block in extract_first_expression'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source/code_helpers.rb:95:in `each'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source/code_helpers.rb:95:in `extract_first_expression'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source/code_helpers.rb:35:in `expression_at'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source.rb:27:in `source_helper'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/method_source-1.1.0/lib/method_source.rb:115:in `source'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/crystalruby-0.2.0/lib/crystalruby/adapter.rb:128:in `extract_source'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/crystalruby-0.2.0/lib/crystalruby/adapter.rb:118:in `define_crystalized_method'
	from /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/crystalruby-0.2.0/lib/crystalruby/adapter.rb:75:in `method_added'
	from crystal_benchmark.rb:9:in `<module:Fibonnaci>'
	from crystal_benchmark.rb:6:in `<main>'

I cloned the method_source repo and ran its test suite. It passes 100% on TruffleRuby 23.1.2, but there are syntax errors on 24.0.0, 24.0.1, and 24.1.0-dev builds. Reproduction steps are:

git clone https://github.com/banister/method_source
git checkout v1.1.0
bundle
bundle exec rake
Test Failure Log 

> bundle exec rake
NOTE: Gem::Specification#has_rdoc= is deprecated with no replacement. It will be removed in Rubygems 4
Gem::Specification#has_rdoc= called from /Users/nirvdrum/dev/workspaces/method_source/rakefile:27.
/Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/bin/truffleruby -w -I/Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/rspec-core-3.13.0/lib:/Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/rspec-support-3.13.1/lib /Users/nirvdrum/.rbenv/versions/truffleruby-24.0.0/lib/gems/gems/rspec-core-3.13.0/exe/rspec --pattern spec/\*\*\{,/\*/\*\*\}/\*_spec.rb
FFFFFFFFF............F..............

Failures:

  1) MethodSource::CodeHelpers should not raise an error on broken lines: p = '\n'
     Failure/Error:
       catch(:valid) do
         eval("BEGIN{throw :valid}\n#{str}")
       end

     SyntaxError:
       (eval):2: expected a closing delimiter for the string literal (SyntaxError)
     # ./lib/method_source/code_helpers.rb:70:in `block in complete_expression?'
     # ./lib/method_source/code_helpers.rb:70:in `complete_expression?'
     # ./spec/method_source/code_helpers_spec.rb:22:in `block (4 levels) in <top (required)>'
     # ./spec/method_source/code_helpers_spec.rb:21:in `upto'
     # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in <top (required)>'

  2) MethodSource::CodeHelpers should not raise an error on broken lines: def\na\n(); end
     Failure/Error:
       catch(:valid) do
         eval("BEGIN{throw :valid}\n#{str}")
       end

     SyntaxError:
       (eval):2: expected a method name (SyntaxError)
     # ./lib/method_source/code_helpers.rb:70:in `block in complete_expression?'
     # ./lib/method_source/code_helpers.rb:70:in `complete_expression?'
     # ./spec/method_source/code_helpers_spec.rb:22:in `block (4 levels) in <top (required)>'
     # ./spec/method_source/code_helpers_spec.rb:21:in `upto'
     # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in <top (required)>'

  3) MethodSource::CodeHelpers should not raise an error on broken lines: p = <<FOO\nlots\nand\nlots of\nfoo\nFOO
     Failure/Error:
       catch(:valid) do
         eval("BEGIN{throw :valid}\n#{str}")
       end

     SyntaxError:
       (eval):3: could not find a terminator for the heredoc (SyntaxError)
     # ./lib/method_source/code_helpers.rb:70:in `block in complete_expression?'
     # ./lib/method_source/code_helpers.rb:70:in `complete_expression?'
     # ./spec/method_source/code_helpers_spec.rb:22:in `block (4 levels) in <top (required)>'
     # ./spec/method_source/code_helpers_spec.rb:21:in `upto'
     # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in <top (required)>'

  4) MethodSource::CodeHelpers should not raise an error on broken lines: [\n:lets,\n'list',\n[/nested/\n], things ]
     Failure/Error:
       catch(:valid) do
         eval("BEGIN{throw :valid}\n#{str}")
       end

     SyntaxError:
       (eval):2: expected a `]` to close the array (SyntaxError)
     # ./lib/method_source/code_helpers.rb:70:in `block in complete_expression?'
     # ./lib/method_source/code_helpers.rb:70:in `complete_expression?'
     # ./spec/method_source/code_helpers_spec.rb:22:in `block (4 levels) in <top (required)>'
     # ./spec/method_source/code_helpers_spec.rb:21:in `upto'
     # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in <top (required)>'

  5) MethodSource::CodeHelpers should not raise an error on broken lines: abc =~ /hello\n/
     Failure/Error:
       catch(:valid) do
         eval("BEGIN{throw :valid}\n#{str}")
       end

     SyntaxError:
       (eval):2: expected a closing delimiter for the regular expression (SyntaxError)
     # ./lib/method_source/code_helpers.rb:70:in `block in complete_expression?'
     # ./lib/method_source/code_helpers.rb:70:in `complete_expression?'
     # ./spec/method_source/code_helpers_spec.rb:22:in `block (4 levels) in <top (required)>'
     # ./spec/method_source/code_helpers_spec.rb:21:in `upto'
     # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in <top (required)>'

  6) MethodSource::CodeHelpers should not raise an error on broken lines: issue = %W/\n343/
     Failure/Error:
       catch(:valid) do
         eval("BEGIN{throw :valid}\n#{str}")
       end

     SyntaxError:
       (eval):2: expected a closing delimiter for the `%W` list (SyntaxError)
     # ./lib/method_source/code_helpers.rb:70:in `block in complete_expression?'
     # ./lib/method_source/code_helpers.rb:70:in `complete_expression?'
     # ./spec/method_source/code_helpers_spec.rb:22:in `block (4 levels) in <top (required)>'
     # ./spec/method_source/code_helpers_spec.rb:21:in `upto'
     # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in <top (required)>'

  7) MethodSource::CodeHelpers should not raise an error on broken lines: pouts(<<HI, 'foo\nbar\nHI\nbaz')
     Failure/Error:
       catch(:valid) do
         eval("BEGIN{throw :valid}\n#{str}")
       end

     SyntaxError:
       (eval):3: could not find a terminator for the heredoc (SyntaxError)
     # ./lib/method_source/code_helpers.rb:70:in `block in complete_expression?'
     # ./lib/method_source/code_helpers.rb:70:in `complete_expression?'
     # ./spec/method_source/code_helpers_spec.rb:22:in `block (4 levels) in <top (required)>'
     # ./spec/method_source/code_helpers_spec.rb:21:in `upto'
     # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in <top (required)>'

  8) MethodSource::CodeHelpers should not raise an error on broken lines: =begin\nno-one uses this syntax anymore...\n=end
     Failure/Error:
       catch(:valid) do
         eval("BEGIN{throw :valid}\n#{str}")
       end

     SyntaxError:
       (eval):2: could not find a terminator for the embedded document (SyntaxError)
     # ./lib/method_source/code_helpers.rb:70:in `block in complete_expression?'
     # ./lib/method_source/code_helpers.rb:70:in `complete_expression?'
     # ./spec/method_source/code_helpers_spec.rb:22:in `block (4 levels) in <top (required)>'
     # ./spec/method_source/code_helpers_spec.rb:21:in `upto'
     # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in <top (required)>'

  9) MethodSource::CodeHelpers should not raise an error on broken lines: puts 1, 2,\n3
     Failure/Error:
       catch(:valid) do
         eval("BEGIN{throw :valid}\n#{str}")
       end

     SyntaxError:
       (eval):2: expected an argument (SyntaxError)
     # ./lib/method_source/code_helpers.rb:70:in `block in complete_expression?'
     # ./lib/method_source/code_helpers.rb:70:in `complete_expression?'
     # ./spec/method_source/code_helpers_spec.rb:22:in `block (4 levels) in <top (required)>'
     # ./spec/method_source/code_helpers_spec.rb:21:in `upto'
     # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in <top (required)>'

  10) MethodSource Methods should return source for an *_evaled method
      Failure/Error: raise SourceNotFoundError, "Could not parse source for #{name}: #{e.message}"

      MethodSource::SourceNotFoundError:
        Could not parse source for hello_name: (eval):3: unexpected end of file, assuming it is closing the parent top level context
      # ./lib/method_source.rb:29:in `source_helper'
      # ./lib/method_source.rb:115:in `source'
      # ./spec/method_source_spec.rb:75:in `block (3 levels) in <top (required)>'
      # ------------------
      # --- Caused by: ---
      # SyntaxError:
      #   (eval):3: unexpected end of file, assuming it is closing the parent top level context (SyntaxError)
      #   ./lib/method_source/code_helpers.rb:70:in `block in complete_expression?'

Finished in 0.06801 seconds (files took 0.17483 seconds to load)
36 examples, 10 failures

Failed examples:

rspec ./spec/method_source/code_helpers_spec.rb[1:1] # MethodSource::CodeHelpers should not raise an error on broken lines: p = '\n'
rspec ./spec/method_source/code_helpers_spec.rb[1:2] # MethodSource::CodeHelpers should not raise an error on broken lines: def\na\n(); end
rspec ./spec/method_source/code_helpers_spec.rb[1:3] # MethodSource::CodeHelpers should not raise an error on broken lines: p = <<FOO\nlots\nand\nlots of\nfoo\nFOO
rspec ./spec/method_source/code_helpers_spec.rb[1:4] # MethodSource::CodeHelpers should not raise an error on broken lines: [\n:lets,\n'list',\n[/nested/\n], things ]
rspec ./spec/method_source/code_helpers_spec.rb[1:5] # MethodSource::CodeHelpers should not raise an error on broken lines: abc =~ /hello\n/
rspec ./spec/method_source/code_helpers_spec.rb[1:6] # MethodSource::CodeHelpers should not raise an error on broken lines: issue = %W/\n343/
rspec ./spec/method_source/code_helpers_spec.rb[1:7] # MethodSource::CodeHelpers should not raise an error on broken lines: pouts(<<HI, 'foo\nbar\nHI\nbaz')
rspec ./spec/method_source/code_helpers_spec.rb[1:8] # MethodSource::CodeHelpers should not raise an error on broken lines: =begin\nno-one uses this syntax anymore...\n=end
rspec ./spec/method_source/code_helpers_spec.rb[1:9] # MethodSource::CodeHelpers should not raise an error on broken lines: puts 1, 2,\n3
rspec ./spec/method_source_spec.rb:74 # MethodSource Methods should return source for an *_evaled method

I haven't attempted to debug the problem yet, so I don't know if this a problem in Prism or our Prism integration.
@eregon
Copy link
Member

eregon commented Apr 25, 2024 •
edited

It's because of https://github.com/banister/method_source/blob/06f21c66380c64ff05c8031c0208eef240da0e83/lib/method_source/code_helpers.rb#L125-L131
And we get different SyntaxError messages with Prism.
I checked and CRuby with RUBYOPT=--parser=prism has similar errors, so I filed ruby/prism#2734

https://github.com/banister/method_source/blob/06f21c66380c64ff05c8031c0208eef240da0e83/lib/method_source/source_location.rb#L44 makes me want to scream but on a closer look at least that code for source_location isn't used because the existing Method#source_location have precedence.

What is silly is we actually record the method source region ourselves in TruffleRuby, so maybe we should implement Method#source and that would just take precedence over the methods in the gems (since those are defined in included modules).
That would avoid the inefficient logic of trying to eval more and more lines until it parses that the gem uses.
OTOH maybe CRuby defines Method#source but returns something else than just a String, and then we'd be incompatible with that. It doesn't seem super likely though, and we could adapt then.

We don't keep the values for Method#comment and Method#class_comment though, so that seems too difficult to implement directly in TruffleRuby.

Naturally if you want to run that benchmark on truffleruby you can just remove require 'crystalruby' and fib_cr.

@eregon
Copy link
Member

eregon commented Apr 25, 2024

Re benchmarking, see https://twitter.com/eregontp/status/1783444486124638251 / https://gist.github.com/eregon/8e104d291ca64d0834b580d18b837059#file-results-txt

@eregon
Copy link
Member

eregon commented Apr 25, 2024

method_source is depended on by a bunch of repos and gems: https://github.com/banister/method_source/network/dependents
So it seems worth implementing Method#source directly in TruffleRuby.
OTOH I don't see immediately a popular gem using it, so not sure how much it matters (let us know if an app/gem you use depends on it).
But easy enough to add.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compatibility
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@nirvdrum @eregon