Bug #17725: Prepend breaks ability to override optimized methods

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: ruby 3.0.0p0 (2020-12-25 revision 95aff21468) [x86_64-darwin19]
• Backport: 2.6: DONTNEED, 2.7: DONTNEED, 3.0: DONE

[joshuadreed (Josh Reed)]

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

[joshuadreed (Josh Reed)]

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.

[joshuadreed (Josh Reed)]

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

[joshuadreed (Josh Reed)]

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.

[nobu (Nobuyoshi Nakada)]

• 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.

[nobu (Nobuyoshi Nakada)]

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.

[nobu (Nobuyoshi Nakada)]

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"

[xtkoba (Tee KOBAYASHI)]

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"

[nobu (Nobuyoshi Nakada)]

• 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

[xtkoba (Tee KOBAYASHI)]

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;
+	}
     }
 }
 

[jeremyevans0 (Jeremy Evans)]

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

[ko1 (Koichi Sasada)]

• 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

[nagachika (Tomoyuki Chikanaga)]

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.

[mk (Matthias Käppler)]

@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.

[nagachika (Tomoyuki Chikanaga)]

• 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...

[nagachika (Tomoyuki Chikanaga)]

• 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.