oracle / truffleruby

A high performance implementation of the Ruby programming language, built on GraalVM.
https://www.graalvm.org/ruby/
Other
2.98k stars 180 forks source link

method_source compatibility problems introduced with TruffleRuby 24.0.0 #3551

Open nirvdrum opened 2 months ago

nirvdrum commented 2 months ago

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 ' # ./spec/method_source/code_helpers_spec.rb:21:in `upto' # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in ' 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 ' # ./spec/method_source/code_helpers_spec.rb:21:in `upto' # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in ' 3) MethodSource::CodeHelpers should not raise an error on broken lines: p = <' # ./spec/method_source/code_helpers_spec.rb:21:in `upto' # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in ' 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 ' # ./spec/method_source/code_helpers_spec.rb:21:in `upto' # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in ' 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 ' # ./spec/method_source/code_helpers_spec.rb:21:in `upto' # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in ' 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 ' # ./spec/method_source/code_helpers_spec.rb:21:in `upto' # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in ' 7) MethodSource::CodeHelpers should not raise an error on broken lines: pouts(<' # ./spec/method_source/code_helpers_spec.rb:21:in `upto' # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in ' 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 ' # ./spec/method_source/code_helpers_spec.rb:21:in `upto' # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in ' 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 ' # ./spec/method_source/code_helpers_spec.rb:21:in `upto' # ./spec/method_source/code_helpers_spec.rb:21:in `block (3 levels) in ' 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 ' # ------------------ # --- 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 = <

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 commented 2 months ago

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 https://github.com/ruby/prism/issues/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 commented 2 months ago

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

eregon commented 2 months ago

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.