Project

General

Profile

Actions

Bug #10765

closed

Module#remove_method remove refined method entry.

Added by hanachin (Seiei Miyagi) about 9 years ago. Updated about 9 years ago.

Status:
Closed
Target version:
-
ruby -v:
ruby 2.3.0dev (2015-01-21 trunk 49326) [x86_64-darwin14]
[ruby-core:67722]

Description

Module#remove_method should raise a NameError
if method is not defined in refined class, such as undef.
But if method is defined in refined class, Module#remove_method should keep refined method and remove original method.

I confirmed by following examples in ruby-trunk, 2.2.0, 2.1.5, 2.0.0-p598

class C
  def foo
    "C#foo"
  end
end

module RefinementBug
  refine C do
    def foo
      "RefinementBug#foo"
    end
  end
end

using RefinementBug

class C
  remove_method :foo
end

puts C.new.foo

# expected:
#   RefinementBug#foo
#
# actual:
#   bug.rb:21:in `<main>': undefined method `foo' for #<C:0x007f9e5c087b48> (NoMethodError)
class C
end

module RefinementBug
  refine C do
    def foo
    end
  end
end

using RefinementBug

class C
  remove_method :foo
end

# expected:
#   bug2.rb:14:in `remove_method': method `foo' not defined in C (NameError)
#   from bug2.rb:14:in `<class:C>'
#   from bug2.rb:13:in `<main>'
#
# actual:
#   # => nothing raised.

Files

bug2.rb (327 Bytes) bug2.rb hanachin (Seiei Miyagi), 01/21/2015 01:46 PM
bug.rb (342 Bytes) bug.rb hanachin (Seiei Miyagi), 01/21/2015 01:46 PM
0001-vm_method.c-fix-remove-refined-method.patch (2.68 KB) 0001-vm_method.c-fix-remove-refined-method.patch hanachin (Seiei Miyagi), 01/21/2015 01:50 PM

Related issues 1 (0 open1 closed)

Related to Ruby master - Bug #10826: Refinements make instance_methods(false) return methods of superclassesClosedshugo (Shugo Maeda)02/04/2015Actions

Updated by shugo (Shugo Maeda) about 9 years ago

  • Status changed from Open to Assigned
  • Assignee set to shugo (Shugo Maeda)
Actions #3

Updated by shugo (Shugo Maeda) about 9 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

Applied in changeset r49480.


  • vm_method.c (remove_method): When remove refined
    method, raise a NameError if the method is not
    defined in refined class.

    But if the method is defined in refined class,
    it should keep refined method and remove original
    method.

    Patch by Seiei Higa. [ruby-core:67722] [Bug #10765]

Updated by shugo (Shugo Maeda) about 9 years ago

  • Backport changed from 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN to 2.0.0: REQUIRED, 2.1: REQUIRED, 2.2: REQUIRED

Updated by vo.x (Vit Ondruch) about 9 years ago

  • Status changed from Closed to Assigned

This breaks CentOS7, OpenSuse and Fedora at minimum:

http://rubyci.blob.core.windows.net/opensuse13/ruby-trunk/log/20150203T123003Z.log.html.gz
http://rubyci.blob.core.windows.net/fedora20/ruby-trunk/log/20150203T110002Z.log.html.gz
http://rubyci.blob.core.windows.net/centos7/ruby-trunk/log/20150203T110002Z.log.html.gz

/home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json/common.rb:60:in `remove_method': method `to_json' not defined in Hash (NameError)
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json/common.rb:60:in `block (3 levels) in generator='
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json/common.rb:59:in `each'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json/common.rb:59:in `block (2 levels) in generator='
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json/common.rb:58:in `class_eval'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json/common.rb:58:in `block in generator='
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json/common.rb:55:in `each'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json/common.rb:55:in `generator='
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json/ext.rb:17:in `<module:Ext>'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json/ext.rb:12:in `<module:JSON>'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json/ext.rb:9:in `<top (required)>'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/lib/rubygems/core_ext/kernel_require.rb:54:in `require'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/lib/rubygems/core_ext/kernel_require.rb:54:in `require'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json.rb:58:in `<module:JSON>'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/.ext/common/json.rb:54:in `<top (required)>'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/lib/rubygems/core_ext/kernel_require.rb:54:in `require'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/lib/rubygems/core_ext/kernel_require.rb:54:in `require'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/test/json/setup_variant.rb:10:in `<top (required)>'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/lib/rubygems/core_ext/kernel_require.rb:54:in `require'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/lib/rubygems/core_ext/kernel_require.rb:54:in `require'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/test/json/test_json.rb:5:in `<top (required)>'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/lib/rubygems/core_ext/kernel_require.rb:54:in `require'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/lib/rubygems/core_ext/kernel_require.rb:54:in `require'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/test/lib/test/unit.rb:826:in `block in non_options'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/test/lib/test/unit.rb:820:in `each'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/test/lib/test/unit.rb:820:in `non_options'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/test/lib/test/unit.rb:63:in `process_args'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/test/lib/test/unit.rb:101:in `process_args'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/test/lib/test/unit.rb:962:in `process_args'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/test/lib/test/unit.rb:967:in `run'
	from /home/hsbt/chkbuild/tmp/build/20150203T110002Z/ruby/test/lib/test/unit.rb:974:in `run'
	from ./test/runner.rb:40:in `<main>'
gmake: *** [yes-test-all] Error 1
exit 2

Updated by hanachin (Seiei Miyagi) about 9 years ago

If touch the me after rb_unlink_method_entry, it could cause error?

diff --git vm_method.c vm_method.c
index 8ad2b72..41a311c 100644
--- vm_method.c
+++ vm_method.c
@@ -775,11 +775,13 @@ remove_method(VALUE klass, ID mid)
     key = (st_data_t)mid;
     st_delete(RCLASS_M_TBL(klass), &key, &data);

+    int keep_refined_method_entry = me->def->type == VM_METHOD_TYPE_REFINED;
+
     rb_vm_check_redefinition_opt_method(me, klass);
     rb_clear_method_cache_by_class(klass);
     rb_unlink_method_entry(me);

-    if (me->def->type == VM_METHOD_TYPE_REFINED) {
+    if (keep_refined_method_entry) {
 	rb_add_refined_method_entry(klass, mid);
     }

Updated by shugo (Shugo Maeda) about 9 years ago

Seiei Higa wrote:

If touch the me after rb_unlink_method_entry, it could cause error?

It's not the problem.

r49480 exposed the following potential problem:

class X
  def foo
  end
end

class Y < X
end

module Bar
  refine Y do
    def foo
    end
  end
end

p Y.instance_methods(false).include?(:foo) # false expected, but true is returned
Actions #8

Updated by shugo (Shugo Maeda) about 9 years ago

  • Related to Bug #10826: Refinements make instance_methods(false) return methods of superclasses added

Updated by shugo (Shugo Maeda) about 9 years ago

The problem reported by Vit can be reproduced by the following command:

$ make test-all TESTS="test/ruby/test_refinement.rb test/json"

Updated by shugo (Shugo Maeda) about 9 years ago

  • Status changed from Assigned to Closed

Fixed in r49493.

Updated by vo.x (Vit Ondruch) about 9 years ago

Testing with r49495 and it seems to be fixed. Thanks.

Updated by naruse (Yui NARUSE) about 9 years ago

  • Backport changed from 2.0.0: REQUIRED, 2.1: REQUIRED, 2.2: REQUIRED to 2.0.0: REQUIRED, 2.1: REQUIRED, 2.2: DONE

ruby_2_2 r49647 merged revision(s) 49480,49493.

Updated by usa (Usaku NAKAMURA) about 9 years ago

  • Backport changed from 2.0.0: REQUIRED, 2.1: REQUIRED, 2.2: DONE to 2.0.0: DONE, 2.1: REQUIRED, 2.2: DONE

ruby_2_0_0 r49738 merged revision(s) 49222,49480,49493.

Actions #14

Updated by nagachika (Tomoyuki Chikanaga) about 9 years ago

  • Backport changed from 2.0.0: DONE, 2.1: REQUIRED, 2.2: DONE to 2.0.0: DONE, 2.1: DONE, 2.2: DONE

Backported into ruby_2_1 branch at r49992.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0