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

Skip specs failing on TruffleRuby #121

Merged
merged 3 commits into from
Jun 11, 2024
Merged

Skip specs failing on TruffleRuby #121

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fda7ea
Skip failing on TruffleRuby specs
andrykonchin May 9, 2024
70e66fd
Fix a pattern matching spec to check #deconstruct_keys arguments when…
andrykonchin May 9, 2024
cf7d992
Add links to failing on TruffleRuby ruby/spec and MRI tests
andrykonchin May 10, 2024
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
5 changes: 5 additions & 0 deletions spec/language/pattern_matching_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,8 @@
end

it "does not support variable binding" do
next skip if RUBY_ENGINE == "truffleruby"

-> {
eval <<~RUBY
case [0, 1]
Expand Down Expand Up @@ -642,6 +644,7 @@ def obj.deconstruct; [0, 1] end
it "calls #deconstruct once for multiple patterns, caching the result" do
# 2.7 doesn't cache
next skip if RUBY_VERSION =~ /^2\.7\./
next skip if RUBY_ENGINE == "truffleruby"

obj = Object.new

Expand Down Expand Up @@ -722,6 +725,8 @@ def obj.deconstruct; "" end
end

it "accepts a subclass of Array from #deconstruct" do
next skip if RUBY_ENGINE == "truffleruby"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW this test case differs a bit from the one in the ruby/spec repository. Not sure if it's done intentionally. Here the test relies on a way how CRuby accesses array elements - with #[] method. The ruby/spec's current version doesn't redefine the #[] method:

    it "accepts a subclass of Array from #deconstruct" do
      obj = Object.new
      def obj.deconstruct
        Class.new(Array).new([0, 1])
      end

      eval(<<~RUBY).should == true
        case obj
        in [1, 2]
          false
        in [0, 1]
          true
        end
      RUBY
    end

This version passes on TruffleRuby but the version with overridden #[] - doesn't.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the change to ruby/spec was made for TruffleRuby: ruby/spec@0e04268

Unfortunately, there are no additional details on that change.

MRI uses #[] (or opt_aref) to access elements returned by #deconstruct. So, you can do smth like this:

class A < Array
  def [](i) = at(-i-1)
end

a = A.new([1, 2])

case a
in [2, 1]
  true
end #=> true

I don't know if it's an intended feature or not; @k-tsj could you please take a look?

Copy link

Choose a reason for hiding this comment

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

@palkan
Yes, it's intentional. If #[] is redefined, it should be called,.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@k-tsj Thank you!

@eregon ^ That's what I mentioned yesterday.

Copy link

Choose a reason for hiding this comment

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

I see. I'm not sure it's a good idea to have that behavior in Ruby, maybe something to discuss on ruby-core.
Subclassing Array and redefining [] on its own is already clearly a bad idea, so it seems a niche edge case no one should use.

obj = Object.new
def obj.deconstruct
subarray = Class.new(Array).new(2)
Expand Down
8 changes: 5 additions & 3 deletions spec/language/ruby_pattern_matching_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,13 @@ def test_alternative_pattern
end
end

unless RUBY_ENGINE == "truffleruby"
assert_syntax_error(%q{
case 0
in a | 0
end
}, /illegal variable in alternative pattern/)
end
end

def test_var_pattern
Expand Down Expand Up @@ -1281,9 +1283,9 @@ def test_deconstruct_keys
end

# https://github.com/jruby/jruby/issues/7854
unless defined?(JRUBY_VERSION)
unless defined?(JRUBY_VERSION) || RUBY_ENGINE == "truffleruby"
assert_block do
case {}
case C.new({})
in {}
C.keys == nil
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose it's a bug in https://github.com/ruby/ruby/blob/master/test/ruby/test_pattern_matching.rb that was hidden - C.keys returned keys captured in some previous test case, that by chance matches C.new({}) and captures the nil value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch! Yeah, having case {} in and checking for C.keys doesn't make any sense here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fix in the Ruby repo - ruby/ruby#10757

Expand Down Expand Up @@ -1555,7 +1557,7 @@ def deconstruct
end

# Ruby 2.7 doesn't have cache
unless RUBY_VERSION =~ /^2\.7\./
unless RUBY_VERSION =~ /^2\.7\./ || RUBY_ENGINE == "truffleruby"

def test_deconstruct_cache
assert_block do
Expand Down
Loading