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

Ruby 3 support #85

Merged
merged 4 commits into from
Feb 10, 2021
Merged

Conversation

swiknaba
Copy link
Contributor

@swiknaba swiknaba commented Dec 30, 2020

closes #84

  • relax Ruby requirements from ~> 2.2 to => 2.2 (which will allow upgrading to Ruby 3 --obviously^^)
  • adds Ruby 3 to Travis config
  • adjust syntax if necessary

The CI is failing since this commit: aebf173 from Mar 2, 2020.

If you are OK, I suggest dropping support for 2.2 entirely, and set the required Ruby version to => 2.3, since 2.3 is the oldest version that you can download from the official website. Also, Ruby 2.3 was released in 2015, 5 years ago.


I've run RSpec locally against all Ruby versions that I have compiled on my machine:
✅  Ruby 2.3.8: 92 examples, 0 failures, 1 pending
✅  Ruby 2.5.8: 92 examples, 0 failures, 1 pending
✅  Ruby 2.6.6: 92 examples, 0 failures, 1 pending
✅  Ruby 2.7.2: 92 examples, 0 failures, 1 pending
✅  Ruby 3.0.0: 92 examples, 0 failures, 1 pending
❌  Ruby 3.0.0:

Failures:

  1) Fasterer::Analyzer should detect gsub 4 times
     Failure/Error: expect(analyzer.errors[:gsub_vs_tr].count).to eq(2)
     
       expected: 2
            got: 1
     
       (compared using ==)
     # ./spec/lib/fasterer/analyzer/24_gsub_vs_tr_spec.rb:9:in `block (2 levels) in <top (required)>'

Finished in 2.04 seconds (files took 0.78213 seconds to load)
92 examples, 1 failure, 1 pending

Failed examples:

rspec ./spec/lib/fasterer/analyzer/24_gsub_vs_tr_spec.rb:6 # Fasterer::Analyzer should detect gsub 4 times

=> should we:
* keep the current restriction ( 🙁 )
* wait for a fix
* fix Ruby 3 in this PR
* merge this, then fix Ruby 3 separately

update: ✅   fixed. see comments below.

=> I've created a follow-up issue so that the failing specs can be addressed separately:
closes #86

@swiknaba swiknaba changed the title Swiknaba/ruby 3 support Ruby 3 support Dec 30, 2020
@swiknaba swiknaba mentioned this pull request Dec 30, 2020
@DamirSvrtan
Copy link
Owner

Hi! Thanks for reaching out, I am currently on holidays, so I'll be able to look into this in about 10 days. Happy holidays!

@DamirSvrtan
Copy link
Owner

@swiknaba do you know the reason why that spec is failing for Ruby 3.0? As much as I understand it fails after relaxing the required ruby version.

@DamirSvrtan
Copy link
Owner

DamirSvrtan commented Jan 15, 2021

@swiknaba whenever you get a chance to answer here, we can proceed with this work. Thanks!

@swiknaba
Copy link
Contributor Author

swiknaba commented Jan 15, 2021

I thought, possibly gsub got faster in Ruby 3, after all, it mainly was about performance improvements.

# see spec/lib/fasterer/analyzer/24_gsub_vs_tr_spec.rb

require 'benchmark'

petar = "petar"
n = 5_000_000

Benchmark.bm do |benchmark|
  benchmark.report("gsub") do
    n.times do
      petar.gsub('r', 'k')
    end
  end

  benchmark.report("tr") do
    n.times do
      petar.tr('r', 'k')
    end
  end
end

however, running this under Ruby 3 yields

       user     system      total        real
gsub  2.976329   0.004855   2.981184 (  2.982168)
tr  0.878640   0.000629   0.879269 (  0.879565)

so tr is still faster when replacing a single value. Benchmarking the other examples from the failing spec

  • 'alfabet'.gsub('r', 'b'.gsub('a', 'z')) vs. 'alfabet'.tr('r', 'b'.tr('a', 'z'))
  • 'abcd abcd'.gsub(' ', '') vs. 'abcd abcd'.tr(' ', '')
  • petar.gsub('pet', 'fat') vs. petar.tr('pet', 'fat')

all result in tr being faster. Thus, the spec is indeed expected to succeed under Ruby 3.


The following example is not recognized under Ruby 3:

class Hello
  def hihi
    'alfabet'.gsub('r', 'b'.gsub('a', 'z'))
  end
end

(the one it recognizes is petar.gsub('r', 'k'))

checking the parsed result to see if the parser has some issue:
ruby 2.7.2 sexp_tree

s(:class, :Hello, nil, s(:defn, :hihi, s(:args), s(:call, s(:str, "alfabet"), :gsub, s(:str, "r"), s(:call, s(:str, "b"), :gsub, s(:str, "a"), s(:str, "z")))))

ruby 3.0.0 sexp_tree

s(:class, :Hello, nil, s(:defn, :hihi, s(:args), s(:call, s(:str, "alfabet"), :gsub, s(:str, "r"), s(:call, s(:str, "b"), :gsub, s(:str, "a"), s(:str, "z")))))

=> same result.

@swiknaba
Copy link
Contributor Author

swiknaba commented Jan 15, 2021

trimmed down this file to the problematic example:

# spec/support/analyzer/24_gsub_vs_tr.rb
class Hello
  def hihi
    'alfabet'.gsub('r', 'b'.gsub('a', 'z'))
  end
end

running spec/lib/fasterer/analyzer/24_gsub_vs_tr_spec.rb:

Ruby 2.7.2:
the lib/fasterer/analyzer.rb calls scan_by_token(token, element) twice for Ruby 2.7.2, with the following elements:

first call
s(:call, s(:str, "alfabet"), :gsub, s(:str, "r"), s(:call, s(:str, "b"), :gsub, s(:str, "a"), s(:str, "z")))

second call
s(:call, s(:str, "b"), :gsub, s(:str, "a"), s(:str, "z"))
===> offense

However, in Ruby 3.0.0 this scan_by_token is only invoked once for:

s(:call, s(:str, "alfabet"), :gsub, s(:str, "r"), s(:call, s(:str, "b"), :gsub, s(:str, "a"), s(:str, "z")))

which is no offense.

The problem is here

# lib/fasterer/analyzer.rb
      case token
      when :call, :iter
        method_call = MethodCall.new(sexp_tree)
        binding.pry
        traverse_sexp_tree(method_call.arguments_element)
        traverse_sexp_tree(method_call.receiver_element) if method_call.receiver_element
        traverse_sexp_tree(method_call.block_body) if method_call.has_block?
      else
        sexp_tree.each { |element| traverse_sexp_tree(element) }
      end
    end

method_call.arguments_element returns the following under Ruby 2.7.2:

s(:call, s(:str, "alfabet"), :gsub, s(:str, "r"), s(:call, s(:str, "b"), :gsub, s(:str, "a"), s(:str, "z")))

while with Ruby 3.0.0 this is an array:

[s(:str, "r"), s(:call, s(:str, "b"), :gsub, s(:str, "a"), s(:str, "z"))]

Digging deeper, it seems that in lib/fasterer/method_call.rb when extracting from a sexp tree using this syntax call_element[3..-1] with Ruby 3 an array is returned, however using Ruby 2.7.2 a Sexp is returned.

@swiknaba
Copy link
Contributor Author

swiknaba commented Jan 15, 2021

@DamirSvrtan I've opened an issue here: seattlerb/ruby_parser#313 since this problem is related to Ruby Parser (I think :D).

Unsure how to fix this, or how to work around this. Any idea?

@swiknaba
Copy link
Contributor Author

@DamirSvrtan solution here: seattlerb/ruby_parser#313 (comment) let's use Sexp#sexp_body rather than Sexp#[]. This still behaves the same way in Ruby 3, while #[] changed its behavior (https://bugs.ruby-lang.org/issues/6087#note-18).

@swiknaba
Copy link
Contributor Author

swiknaba commented Feb 9, 2021

ping 🙂

@DamirSvrtan
Copy link
Owner

Thanks for pinging, it slipped my radar!

@DamirSvrtan DamirSvrtan merged commit d1551e1 into DamirSvrtan:master Feb 10, 2021
@hlascelles
Copy link
Contributor

Excellent 👍

Can we get a new gem cut please?

@DamirSvrtan
Copy link
Owner

I've released Ruby 3.0 support in fasterer version 0.9.0. Thanks @swiknaba for the contribution!

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

Successfully merging this pull request may close these issues.

Failing specs for Ruby 3 Required ruby version
3 participants