Project

General

Profile

Bug #10943

Singleton class expression (class << obj) should make be indivisual namespaces

Added by ko1 (Koichi Sasada) about 4 years ago. Updated almost 4 years ago.

Status:
Closed
Priority:
Normal
Target version:
-
ruby -v:
1.9.0 to trunk
[ruby-core:<unknown>]

Description

Abstract

Singleton class scopes should make their own namespace for constants.
However, Ruby versions from Ruby 1.9.0 do not respect this specification.

Background and Problem

Singleton class is useful feature to define object specific methods, especially for Class objects.

obj = Object.new

# Define something obj's singleton class.
class << obj
  def foo
  end
end

# Idiom to make class methods
class C
  class << self
    def foo      # Define C.foo method
    end
  end
end

A class syntax has another job: making new namespace, especially for constants.

class C
  CONST = 1
  p CONST    #=> 1
  def foo
    p CONST  #=> 1
  end
end

Singleton class defintion should also introduce namespace for constant.

obj = Object.new
class << obj
  CONST = 1
  def foo
    CONST
  end
end

p obj.foo #=> 1

No problem.

Problem is sharing a singleton class definition with multiple objects.

objs = []
$i = 0

2.times{
  objs << obj = Object.new
  class << obj
    CONST = ($i += 1)
    def foo
      CONST
    end
  end
}

p objs[0].foo
p objs[1].foo

Please think about the answers (outputs).
The above code makes two singleton classes independently.
So that constant namespace should be made for each singleton classes.

In fact, before Ruby 1.9.0, this program outputs "1\n2\n". Maybe your answer is same.

However after Ruby 1.9.0, this program outputs "2\n2\n". This is a bug (not intentional behavior).
JRuby and Rubinius also output not correct answers (interestingly JRuby prints "1\n1").

$ ruby -v t.rb
ruby 2.3.0dev (2015-01-27 trunk 49421) [x86_64-linux]
2
2
$ jruby-1.7.19/bin/jruby -v t.rb
jruby 1.7.19 (1.9.3p551) 2015-01-29 20786bd on OpenJDK 64-Bit Server VM 1.7.0_75-b13 +jit [linux-amd64]
1
1
$ jruby-9.0.0.0.pre1/bin/jruby -v t.rb
jruby 9.0.0.0.pre1 (2.2.0p0) 2015-01-20 d537cab OpenJDK 64-Bit Server VM 24.75-b04 on 1.7.0_75-b13 +jit [linux-amd64]
1
1
$ rbx-2.5.2/bin/rbx -v t.rb
rubinius 2.5.2 (2.1.0 7a5b05b1 2015-01-30 3.4 JI) [x86_64-linux-gnu]
2
2

I asked Matz and his answer is "This is a bug behavior".

Moreover, on the independent namespace can make new classes/modules.

obj = Object.new
class << obj
  class X
    # make <singleton class::X>
  end
end

and it also has problem with multiple definitions.

objs = []
$xs = []
$i = 0

2.times{
  objs << obj = Object.new
  class << obj
    CONST = ($i += 1)
    class X
      $xs << self
      CONST = ($i += 1)
      def foo
        CONST
      end
    end

    def x
      X
    end
  end
}

p $xs       #=> [#<Class:0x25d56cc>::X, #<Class:0x25d55f0>::X]
p objs[0].x #=> #<Class:0x25d55f0>::X <- should be #<Class:0x25d56cc>::X
p objs[1].x #=> #<Class:0x25d55f0>::X
p $xs[0].new.foo #=> 4 <- should be 2
p $xs[1].new.foo #=> 4

This is a bug.

(BTW, mruby works correctly!)

Reason of this behavior

On MRI, the reason of this bug is wrong sharing a namespace with
multiple namespaces. On MRI, the term "CREF" is a name of namespace data
structure.

Simply saying, I couldn't recognize such case, sharing a namespace by
multiple namespaces. I was surprising that each singleton class
expression make their own namespace. So that I store CREF data into each
local ISeq (method bytecode). It means that each bytecode knows their
own CREF. I had believed that one bytecode only has one namespace (CREF).
But this assumption is not correct, as I described above.

Solution

I decided to renew this feature. ISeq data should not have their own
CREF, but method only should have. Push CREF onto each method frame
(value stack, same location of SVAR). There is a (not small) patch and I
will commit soon for Ruby 2.3.

Previous versions

This is bug fix, but it changes Ruby semantics largely. I'm not sure how
to treat it.

Matz said that "we can not expect how affect this change for existing applications, so that no
need for older versions".

Please discuss about it.

Acknowledgement

This bug was found during investigating [Bug #10871].

Associated revisions

Revision d84f9b16
Added by ko1 (Koichi Sasada) about 4 years ago

  • fix namespace issue on singleton class expressions. [Bug #10943]
  • vm_core.h, method.h: remove rb_iseq_t::cref_stack. CREF is stored to rb_method_definition_t::body.iseq_body.cref.
  • vm_insnhelper.c: modify SVAR usage. When calling ISEQ type method, push CREF information onto method frame, SVAR located place. Before this fix, SVAR is simply nil. After this patch, CREF (or NULL == Qfalse for not iseq methods) is stored at the method invocation. When SVAR is requierd, then put NODE_IF onto SVAR location, and NDOE_IF::nd_reserved points CREF itself.
  • vm.c (vm_cref_new, vm_cref_dump, vm_cref_new_toplevel): added.
  • vm_insnhelper.c (vm_push_frame): accept CREF.
  • method.h, vm_method.c (rb_add_method_iseq): added. This function accepts iseq and CREF.
  • class.c (clone_method): use rb_add_method_iseq().
  • gc.c (mark_method_entry): mark method_entry::body.iseq_body.cref.
  • iseq.c: remove CREF related codes.
  • insns.def (getinlinecache/setinlinecache): CREF should be cache key because a different CREF has a different namespace.
  • node.c (rb_gc_mark_node): mark NODE_IF::nd_reserved for SVAR.
  • proc.c: catch up changes.
  • struct.c: ditto.
  • insns.def: ditto.
  • vm_args.c (raise_argument_error): ditto.
  • vm_eval.c: ditto.
  • test/ruby/test_class.rb: add a test.

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@49874 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 49874
Added by ko1 (Koichi Sasada) about 4 years ago

  • fix namespace issue on singleton class expressions. [Bug #10943]
  • vm_core.h, method.h: remove rb_iseq_t::cref_stack. CREF is stored to rb_method_definition_t::body.iseq_body.cref.
  • vm_insnhelper.c: modify SVAR usage. When calling ISEQ type method, push CREF information onto method frame, SVAR located place. Before this fix, SVAR is simply nil. After this patch, CREF (or NULL == Qfalse for not iseq methods) is stored at the method invocation. When SVAR is requierd, then put NODE_IF onto SVAR location, and NDOE_IF::nd_reserved points CREF itself.
  • vm.c (vm_cref_new, vm_cref_dump, vm_cref_new_toplevel): added.
  • vm_insnhelper.c (vm_push_frame): accept CREF.
  • method.h, vm_method.c (rb_add_method_iseq): added. This function accepts iseq and CREF.
  • class.c (clone_method): use rb_add_method_iseq().
  • gc.c (mark_method_entry): mark method_entry::body.iseq_body.cref.
  • iseq.c: remove CREF related codes.
  • insns.def (getinlinecache/setinlinecache): CREF should be cache key because a different CREF has a different namespace.
  • node.c (rb_gc_mark_node): mark NODE_IF::nd_reserved for SVAR.
  • proc.c: catch up changes.
  • struct.c: ditto.
  • insns.def: ditto.
  • vm_args.c (raise_argument_error): ditto.
  • vm_eval.c: ditto.
  • test/ruby/test_class.rb: add a test.

Revision 49874
Added by ko1 (Koichi Sasada) about 4 years ago

  • fix namespace issue on singleton class expressions. [Bug #10943]
  • vm_core.h, method.h: remove rb_iseq_t::cref_stack. CREF is stored to rb_method_definition_t::body.iseq_body.cref.
  • vm_insnhelper.c: modify SVAR usage. When calling ISEQ type method, push CREF information onto method frame, SVAR located place. Before this fix, SVAR is simply nil. After this patch, CREF (or NULL == Qfalse for not iseq methods) is stored at the method invocation. When SVAR is requierd, then put NODE_IF onto SVAR location, and NDOE_IF::nd_reserved points CREF itself.
  • vm.c (vm_cref_new, vm_cref_dump, vm_cref_new_toplevel): added.
  • vm_insnhelper.c (vm_push_frame): accept CREF.
  • method.h, vm_method.c (rb_add_method_iseq): added. This function accepts iseq and CREF.
  • class.c (clone_method): use rb_add_method_iseq().
  • gc.c (mark_method_entry): mark method_entry::body.iseq_body.cref.
  • iseq.c: remove CREF related codes.
  • insns.def (getinlinecache/setinlinecache): CREF should be cache key because a different CREF has a different namespace.
  • node.c (rb_gc_mark_node): mark NODE_IF::nd_reserved for SVAR.
  • proc.c: catch up changes.
  • struct.c: ditto.
  • insns.def: ditto.
  • vm_args.c (raise_argument_error): ditto.
  • vm_eval.c: ditto.
  • test/ruby/test_class.rb: add a test.

Revision 49874
Added by ko1 (Koichi Sasada) about 4 years ago

  • fix namespace issue on singleton class expressions. [Bug #10943]
  • vm_core.h, method.h: remove rb_iseq_t::cref_stack. CREF is stored to rb_method_definition_t::body.iseq_body.cref.
  • vm_insnhelper.c: modify SVAR usage. When calling ISEQ type method, push CREF information onto method frame, SVAR located place. Before this fix, SVAR is simply nil. After this patch, CREF (or NULL == Qfalse for not iseq methods) is stored at the method invocation. When SVAR is requierd, then put NODE_IF onto SVAR location, and NDOE_IF::nd_reserved points CREF itself.
  • vm.c (vm_cref_new, vm_cref_dump, vm_cref_new_toplevel): added.
  • vm_insnhelper.c (vm_push_frame): accept CREF.
  • method.h, vm_method.c (rb_add_method_iseq): added. This function accepts iseq and CREF.
  • class.c (clone_method): use rb_add_method_iseq().
  • gc.c (mark_method_entry): mark method_entry::body.iseq_body.cref.
  • iseq.c: remove CREF related codes.
  • insns.def (getinlinecache/setinlinecache): CREF should be cache key because a different CREF has a different namespace.
  • node.c (rb_gc_mark_node): mark NODE_IF::nd_reserved for SVAR.
  • proc.c: catch up changes.
  • struct.c: ditto.
  • insns.def: ditto.
  • vm_args.c (raise_argument_error): ditto.
  • vm_eval.c: ditto.
  • test/ruby/test_class.rb: add a test.

Revision 49874
Added by ko1 (Koichi Sasada) about 4 years ago

  • fix namespace issue on singleton class expressions. [Bug #10943]
  • vm_core.h, method.h: remove rb_iseq_t::cref_stack. CREF is stored to rb_method_definition_t::body.iseq_body.cref.
  • vm_insnhelper.c: modify SVAR usage. When calling ISEQ type method, push CREF information onto method frame, SVAR located place. Before this fix, SVAR is simply nil. After this patch, CREF (or NULL == Qfalse for not iseq methods) is stored at the method invocation. When SVAR is requierd, then put NODE_IF onto SVAR location, and NDOE_IF::nd_reserved points CREF itself.
  • vm.c (vm_cref_new, vm_cref_dump, vm_cref_new_toplevel): added.
  • vm_insnhelper.c (vm_push_frame): accept CREF.
  • method.h, vm_method.c (rb_add_method_iseq): added. This function accepts iseq and CREF.
  • class.c (clone_method): use rb_add_method_iseq().
  • gc.c (mark_method_entry): mark method_entry::body.iseq_body.cref.
  • iseq.c: remove CREF related codes.
  • insns.def (getinlinecache/setinlinecache): CREF should be cache key because a different CREF has a different namespace.
  • node.c (rb_gc_mark_node): mark NODE_IF::nd_reserved for SVAR.
  • proc.c: catch up changes.
  • struct.c: ditto.
  • insns.def: ditto.
  • vm_args.c (raise_argument_error): ditto.
  • vm_eval.c: ditto.
  • test/ruby/test_class.rb: add a test.

Revision 49874
Added by ko1 (Koichi Sasada) about 4 years ago

  • fix namespace issue on singleton class expressions. [Bug #10943]
  • vm_core.h, method.h: remove rb_iseq_t::cref_stack. CREF is stored to rb_method_definition_t::body.iseq_body.cref.
  • vm_insnhelper.c: modify SVAR usage. When calling ISEQ type method, push CREF information onto method frame, SVAR located place. Before this fix, SVAR is simply nil. After this patch, CREF (or NULL == Qfalse for not iseq methods) is stored at the method invocation. When SVAR is requierd, then put NODE_IF onto SVAR location, and NDOE_IF::nd_reserved points CREF itself.
  • vm.c (vm_cref_new, vm_cref_dump, vm_cref_new_toplevel): added.
  • vm_insnhelper.c (vm_push_frame): accept CREF.
  • method.h, vm_method.c (rb_add_method_iseq): added. This function accepts iseq and CREF.
  • class.c (clone_method): use rb_add_method_iseq().
  • gc.c (mark_method_entry): mark method_entry::body.iseq_body.cref.
  • iseq.c: remove CREF related codes.
  • insns.def (getinlinecache/setinlinecache): CREF should be cache key because a different CREF has a different namespace.
  • node.c (rb_gc_mark_node): mark NODE_IF::nd_reserved for SVAR.
  • proc.c: catch up changes.
  • struct.c: ditto.
  • insns.def: ditto.
  • vm_args.c (raise_argument_error): ditto.
  • vm_eval.c: ditto.
  • test/ruby/test_class.rb: add a test.

Revision d93cfe5d
Added by nagachika (Tomoyuki Chikanaga) over 3 years ago

  • insns.def (defineclass): introduce an ad-hoc patch to avoid an issue reported on [Bug #10871].

This patch does not fix completely. For example, method definition
in a block (like 1.times{def ...; end}) still causes same issue.
To solve all, we need a huge patch and it seems difficult for
stable branch.

Use Ruby 2.3 and later to solve this issue completely.
(See [Bug #10943])

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_2@51673 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 51673
Added by nagachika (Tomoyuki Chikanaga) over 3 years ago

  • insns.def (defineclass): introduce an ad-hoc patch to avoid an issue reported on [Bug #10871].

This patch does not fix completely. For example, method definition
in a block (like 1.times{def ...; end}) still causes same issue.
To solve all, we need a huge patch and it seems difficult for
stable branch.

Use Ruby 2.3 and later to solve this issue completely.
(See [Bug #10943])

Revision f363bbdf
Added by ko1 (Koichi Sasada) over 3 years ago

  • insns.def (getinlinecache/setinlinecache): compare ic->ic_cref and current cref only when cached CREF list includes singleton class.

Singleton classes have own namespaces, so that we need to check
cref as a key (#10943).

However, if current CREF list does not include singleton class,
no need to check CREF beacuse it should be same name space.

  • vm_insnhelper.c (vm_get_const_key_cref): add a function returns
    CREF only when it includes singleton class.

  • vm_core.h: constify iseq_inline_cache_entry::ic_cref.

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@52371 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 52371
Added by ko1 (Koichi Sasada) over 3 years ago

  • insns.def (getinlinecache/setinlinecache): compare ic->ic_cref and current cref only when cached CREF list includes singleton class.

Singleton classes have own namespaces, so that we need to check
cref as a key (#10943).

However, if current CREF list does not include singleton class,
no need to check CREF beacuse it should be same name space.

  • vm_insnhelper.c (vm_get_const_key_cref): add a function returns
    CREF only when it includes singleton class.

  • vm_core.h: constify iseq_inline_cache_entry::ic_cref.

Revision 52371
Added by ko1 (Koichi Sasada) over 3 years ago

  • insns.def (getinlinecache/setinlinecache): compare ic->ic_cref and current cref only when cached CREF list includes singleton class.

Singleton classes have own namespaces, so that we need to check
cref as a key (#10943).

However, if current CREF list does not include singleton class,
no need to check CREF beacuse it should be same name space.

  • vm_insnhelper.c (vm_get_const_key_cref): add a function returns
    CREF only when it includes singleton class.

  • vm_core.h: constify iseq_inline_cache_entry::ic_cref.

Revision 52371
Added by ko1 (Koichi Sasada) over 3 years ago

  • insns.def (getinlinecache/setinlinecache): compare ic->ic_cref and current cref only when cached CREF list includes singleton class.

Singleton classes have own namespaces, so that we need to check
cref as a key (#10943).

However, if current CREF list does not include singleton class,
no need to check CREF beacuse it should be same name space.

  • vm_insnhelper.c (vm_get_const_key_cref): add a function returns
    CREF only when it includes singleton class.

  • vm_core.h: constify iseq_inline_cache_entry::ic_cref.

Revision 52371
Added by ko1 (Koichi Sasada) over 3 years ago

  • insns.def (getinlinecache/setinlinecache): compare ic->ic_cref and current cref only when cached CREF list includes singleton class.

Singleton classes have own namespaces, so that we need to check
cref as a key (#10943).

However, if current CREF list does not include singleton class,
no need to check CREF beacuse it should be same name space.

  • vm_insnhelper.c (vm_get_const_key_cref): add a function returns
    CREF only when it includes singleton class.

  • vm_core.h: constify iseq_inline_cache_entry::ic_cref.

Revision 52371
Added by ko1 (Koichi Sasada) over 3 years ago

  • insns.def (getinlinecache/setinlinecache): compare ic->ic_cref and current cref only when cached CREF list includes singleton class.

Singleton classes have own namespaces, so that we need to check
cref as a key (#10943).

However, if current CREF list does not include singleton class,
no need to check CREF beacuse it should be same name space.

  • vm_insnhelper.c (vm_get_const_key_cref): add a function returns
    CREF only when it includes singleton class.

  • vm_core.h: constify iseq_inline_cache_entry::ic_cref.

History

Updated by ko1 (Koichi Sasada) about 4 years ago

  • Description updated (diff)

Updated by Eregon (Benoit Daloze) about 4 years ago

Without this change it is possible to determine which CREF or "lexical scope for constant" is used for resolving constants at parse time.
The change implies that class << expr now possibly creates multiple such scopes (class/module Name cannot since that Name is static).
I agree it makes sense from a user point of view though.

How will you implement Module.nesting then?
Attach a CREF to each method, which is only attached in the class<<expr body and keep a stack of CREFs when opening/closing class/module?

#3

Updated by ko1 (Koichi Sasada) about 4 years ago

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

Applied in changeset r49874.


  • fix namespace issue on singleton class expressions. [Bug #10943]
  • vm_core.h, method.h: remove rb_iseq_t::cref_stack. CREF is stored to rb_method_definition_t::body.iseq_body.cref.
  • vm_insnhelper.c: modify SVAR usage. When calling ISEQ type method, push CREF information onto method frame, SVAR located place. Before this fix, SVAR is simply nil. After this patch, CREF (or NULL == Qfalse for not iseq methods) is stored at the method invocation. When SVAR is requierd, then put NODE_IF onto SVAR location, and NDOE_IF::nd_reserved points CREF itself.
  • vm.c (vm_cref_new, vm_cref_dump, vm_cref_new_toplevel): added.
  • vm_insnhelper.c (vm_push_frame): accept CREF.
  • method.h, vm_method.c (rb_add_method_iseq): added. This function accepts iseq and CREF.
  • class.c (clone_method): use rb_add_method_iseq().
  • gc.c (mark_method_entry): mark method_entry::body.iseq_body.cref.
  • iseq.c: remove CREF related codes.
  • insns.def (getinlinecache/setinlinecache): CREF should be cache key because a different CREF has a different namespace.
  • node.c (rb_gc_mark_node): mark NODE_IF::nd_reserved for SVAR.
  • proc.c: catch up changes.
  • struct.c: ditto.
  • insns.def: ditto.
  • vm_args.c (raise_argument_error): ditto.
  • vm_eval.c: ditto.
  • test/ruby/test_class.rb: add a test.

Updated by ko1 (Koichi Sasada) about 4 years ago

Benoit Daloze wrote:

How will you implement Module.nesting then?
Attach a CREF to each method, which is only attached in the class<<expr body and keep a stack of CREFs when opening/closing class/module?

Module.nesting checks (live) stack frames.

Updated by nagachika (Tomoyuki Chikanaga) almost 4 years ago

Just memorandum.

I've partially backported r49898 which contains a fix for r49874 into ruby_2_2 branch.
If you want to backport r49874 into ruby_2_2 branch, the first hunk of r49898 should be applied too.

Also available in: Atom PDF