Project

General

Profile

Actions

Bug #17725

closed

Prepend breaks ability to override optimized methods

Added by joshuadreed (Josh Reed) over 3 years ago. Updated almost 3 years ago.

Status:
Closed
Assignee:
-
Target version:
-
ruby -v:
ruby 3.0.0p0 (2020-12-25 revision 95aff21468) [x86_64-darwin19]
[ruby-core:102884]

Description

Prepending any module to String and Hash (and possibly other or all classes?) blocks the ability to alias

As an example:

module Dummy; end
# String.prepend(Dummy)
class String
  alias_method(:old_plus, :+)
  def + other
    puts 'blah blah'
    old_plus(other)
  end
end

'a' + 'b'

> blah blah
module Dummy; end
String.prepend(Dummy)
class String
  alias_method(:old_plus, :+)
  def + other
    puts 'blah blah'
    old_plus(other)
  end
end

'a' + 'b'

>

Prepending after an alias does not affect the previous alias

Updated by joshuadreed (Josh Reed) over 3 years ago

module Dummy; end
class Foo
  def ten
    10
  end
end

Foo.prepend(Dummy)
class Foo
  alias_method(:old_ten, :ten)
  def ten
    puts 'blah blah'
    old_ten
  end
end

Foo.new.ten

> blah blah

Okay, doesn't seem to affect every class.

I do suspect it may be to do with refinement, so I will check into that next.

Updated by joshuadreed (Josh Reed) over 3 years ago

So, I set out to test my hypothesis regarding refinements. I've never used refinements prior to this, so it's still possible there's an interaction somewhere, but at least not in the way I was thinking.

module Dummy; end

class Foo
  def ten
    10
  end
end

module Bar
  refine Foo do
    def ten
      11
    end
  end
end

class Baz
  using Bar
  def check
    Foo.new.ten
  end
end

Baz.prepend(Dummy)

class Baz
  alias_method(:old_check, :check)
  def check
    puts 'blah blah'
    old_check
  end
end

p Baz.new.check

>blah blah
>11

Updated by joshuadreed (Josh Reed) over 3 years ago

One more detail.

module Dummy; end
class String
  alias_method(:old_plus, :+)
  def + other
    puts 'blah blah'
    old_plus(other)
  end
end


String.prepend(Dummy)

class String
  alias_method(:old_plus2, :+)
  def + other
    puts 'blah blah2'
    old_plus2(other)
  end
end

>blah blah2
>blah blah

If the method was aliased prior to prepending, the method is perfectly re-alias-able.

Updated by nobu (Nobuyoshi Nakada) over 3 years ago

  • Subject changed from Prepend Breaks Ability to Alias to Prepend breaks ability to override optimized methods

Alias is not concerned.

# bug-17725.rb
String.prepend(Module.new) unless ARGV.empty?
class String
  def + other
    'blah blah'
  end
end

p 'a' + 'b'
$ ruby bug-17725.rb 
"blah blah"

$ ruby bug-17725.rb 1
"ab"

Seems the redefinition flag is not set by prepend.

Updated by nobu (Nobuyoshi Nakada) over 3 years ago

nobu (Nobuyoshi Nakada) wrote in #note-4:

Seems the redefinition flag is not set by prepend.

This is not accurate, actual method owner class needs to be checked too.

Updated by nobu (Nobuyoshi Nakada) over 3 years ago

Hmmmm?

$ ruby -v
ruby 3.1.0dev (2021-03-16T15:19:37Z master a47697aa44) [x86_64-darwin19]

$ ruby --enable=gems bug-17725.rb 
"blah blah"

$ ruby --enable=gems bug-17725.rb 1
"ab"

$ ruby --disable=gems bug-17725.rb
"blah blah"

$ ruby --disable=gems bug-17725.rb 1
"blah blah"

$ ruby --disable=gems -rrubygems bug-17725.rb
"blah blah"

$ ruby --disable=gems -rrubygems bug-17725.rb 1
"ab"

Updated by xtkoba (Tee KOBAYASHI) over 3 years ago

FWIW,

# bug17725-test.rb
eval <<EOS
# encoding: ascii-8bit
MY_STRING = ''
EOS

MY_STRING + ''

String.prepend(Module.new)
class String
  def + other
    'blah blah'
  end
end

p 'a' + 'b'

gives

$ ruby --disable-gems bug17725-test.rb
"ab"
Actions #8

Updated by nobu (Nobuyoshi Nakada) over 3 years ago

  • Backport changed from 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN to 2.5: DONTNEED, 2.6: DONTNEED, 2.7: DONTNEED, 3.0: REQUIRED

Updated by xtkoba (Tee KOBAYASHI) over 3 years ago

Another example:

'' != nil

module Blah
  def == other
    'blah blah'
  end
end
String.prepend(Blah)

p 'a' == 'b' # => false

It seems that rb_vm_check_redefinition_opt_method is not working correctly in these cases. If the issue were only with String#== and String#+, a (dirty) workaround would be as follows:

--- a/vm.c
+++ b/vm.c
@@ -1854,6 +1854,12 @@ rb_vm_check_redefinition_opt_method(cons
 
 	    ruby_vm_redefined_flag[bop] |= flag;
 	}
+	else if (me->def->body.iseq.iseqptr == (rb_iseq_t *)rb_str_equal) {
+	    ruby_vm_redefined_flag[BOP_EQ] |= STRING_REDEFINED_OP_FLAG;
+	}
+	else if (me->def->body.iseq.iseqptr == (rb_iseq_t *)rb_str_plus) {
+	    ruby_vm_redefined_flag[BOP_PLUS] |= STRING_REDEFINED_OP_FLAG;
+	}
     }
 }
 

Updated by jeremyevans0 (Jeremy Evans) over 3 years ago

I've submitted a pull request to fix this: https://github.com/ruby/ruby/pull/4376

Actions #11

Updated by ko1 (Koichi Sasada) over 3 years ago

  • Status changed from Open to Closed

Applied in changeset git|fb4cf204a662a8cd9dafef6f31f2bd0db9129abe.


use me->def instead of me for opt_table

vm_opt_method_table is me=>bop table to manage the optimized
methods (by specialized instruction). However, me can be invalidated
to invalidate the method cache entry.
[Bug #17725]

To solve the issue, use me-def instead of me which simply copied
at invalidation timing.

A test by @jeremyevans https://github.com/ruby/ruby/pull/4376

Updated by nagachika (Tomoyuki Chikanaga) over 3 years ago

I think fa0279d947c3962c3f8c32852278d3ebb964cb19 should be backported too.

But it could introduce a tiny incompatibility. I'd like to investigate the another workarounds of the issue.

Updated by mk (Matthias Käppler) almost 3 years ago

@nagachika (Tomoyuki Chikanaga) @ko1 (Koichi Sasada) I noticed neither commit appears to be included in 3.0.3. Just wondering why? We currently need to build a patched 3.0.2 image that includes these two commits since we were running into this problem, and it has been working fine for us.

Updated by nagachika (Tomoyuki Chikanaga) almost 3 years ago

  • Backport changed from 2.5: DONTNEED, 2.6: DONTNEED, 2.7: DONTNEED, 3.0: REQUIRED to 2.6: DONTNEED, 2.7: DONTNEED, 3.0: REQUIRED

I gave up to find a workaround. I will backport fb4cf204a662a8cd9dafef6f31f2bd0db9129abe and fa0279d947c3962c3f8c32852278d3ebb964cb19 even though they introduce an incompatibility. I believe the incompatibility (the Array#size is going to have an independent method entry instead of being defined as the alias of Array#length) couldn't cause any problem in the real world applications...

Updated by nagachika (Tomoyuki Chikanaga) almost 3 years ago

  • Backport changed from 2.6: DONTNEED, 2.7: DONTNEED, 3.0: REQUIRED to 2.6: DONTNEED, 2.7: DONTNEED, 3.0: DONE

ruby_3_0 545d6820715a48a17d6182128c0db4198dfa76c1 merged revision(s) fb4cf204a662a8cd9dafef6f31f2bd0db9129abe,fa0279d947c3962c3f8c32852278d3ebb964cb19.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0