Project

General

Profile

Actions
Copy link

Bug #10765

closed

Module#remove_method remove refined method entry.

Added by hanachin (Seiei Miyagi) over 10 years ago. Updated over 10 years ago.

Status:
Closed
Assignee:
shugo (Shugo Maeda)
Target version:
-
ruby -v:
ruby 2.3.0dev (2015-01-21 trunk 49326) [x86_64-darwin14]
Backport:
2.0.0: DONE, 2.1: DONE, 2.2: DONE
[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

Download all 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 open — 1 closed)

Related to Ruby - Bug #10826: Refinements make instance_methods(false) return methods of superclassesClosedshugo (Shugo Maeda)02/04/2015Actions
Actions
Copy link
 #2 [ruby-core:67967]

Updated by shugo (Shugo Maeda) over 10 years ago

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

Updated by shugo (Shugo Maeda) over 10 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]

Actions
Copy link
 #4 [ruby-core:67969]

Updated by shugo (Shugo Maeda) over 10 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
Actions
Copy link
 #5 [ruby-core:67978]

Updated by vo.x (Vit Ondruch) over 10 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
Actions
Copy link
 #6 [ruby-core:67982]

Updated by hanachin (Seiei Miyagi) over 10 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);
     }
Actions
Copy link
 #7 [ruby-core:67996]

Updated by shugo (Shugo Maeda) over 10 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
Copy link
 #8

Updated by shugo (Shugo Maeda) over 10 years ago

  • Related to Bug #10826: Refinements make instance_methods(false) return methods of superclasses added
Actions
Copy link
 #9 [ruby-core:67998]

Updated by shugo (Shugo Maeda) over 10 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"
Actions
Copy link
 #10 [ruby-core:68002]

Updated by shugo (Shugo Maeda) over 10 years ago

  • Status changed from Assigned to Closed

Fixed in r49493.

Actions
Copy link
 #11 [ruby-core:68007]

Updated by vo.x (Vit Ondruch) over 10 years ago

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

Actions
Copy link
 #12 [ruby-core:68182]

Updated by naruse (Yui NARUSE) over 10 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.

Actions
Copy link
 #13 [ruby-core:68302]

Updated by usa (Usaku NAKAMURA) over 10 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
Copy link
 #14

Updated by nagachika (Tomoyuki Chikanaga) over 10 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
Copy link

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0