Conversation
... class of class' singleton class an old issue that existed all the way back in JRuby 1.7 (jrubyGH-287)
| return attached == target ? this : super.toSingletonClass(target); | ||
| } | ||
|
|
||
| public RubyBasicObject getAttached() { |
There was a problem hiding this comment.
ah, so I initially put in a RubyClass type there since the attached wasn't clear to be (almost) always a class.
not a must, but this is pretty much internal API, right? ext should pretty much see RubyClass as all there is ...
There was a problem hiding this comment.
its pretty much assumed to be of a RubyBasicObject
There was a problem hiding this comment.
@kares I was wondering about Module#initialize_copy. It seems that can be RubyModule and not RubyClass. Does that work here? like:
module Foo; end; p Foo.send(:initialize_copy, Foo).singleton_classAs a broader question tracing through all this logic I half wonder whether rubymodule or rubyclass if we can narrow this more than basicobject (I see undef is basicobject but we could make that module/class maybe?).
There was a problem hiding this comment.
yeah, that "sinleton-class-clone-and-attache" code I wasn't sure ...
didn't touch it - was hoping to get back to it, seems to be working :
kares@clevo:~/workspace/oss/jruby$ bin/jruby -e "module Foo; end; p Foo.send(:initialize_copy, Foo).singleton_class"
#<Class:Foo>
kares@clevo:~/workspace/oss/jruby$ bin/jruby -e "module Foo; end; p Foo.send(:initialize_copy, Foo).singleton_class.singleton_class"
#<Class:#<Class:Foo>>
... super-class of such also seems fine (working as MRI) :
kares@clevo:~/workspace/oss/jruby$ bin/jruby -e "module Foo; end; p Foo.send(:initialize_copy, Foo).singleton_class.singleton_class.superclass"
#<Class:Module>
at this point I am guessing only class singleton_class-es were really broken - which is what has been handled, thus hopefully this is the right direction.
There was a problem hiding this comment.
half wonder whether rubymodule or rubyclass if we can narrow this more than basicobject (I see undef is basicobject but we could make that module/class maybe?)
yeah - except UNDEF that it should always be a RubyModule (or RubyClass/MetaClass)
did not try not using UNDEF ... it might work with passing null around 🙈
and if its to be a "public" API - maybe smt to go along in RubyClass' isSingleton() to retrieve the "attached" class (not sure about naming) ... but that is mostly just throwing in random ideas :)
There was a problem hiding this comment.
@kares yeah UNDEF I don't know how that is used in this context either. Maybe null works. At some point we probably want to more formally restrict what attached means and what it can be. RubyBasicObject pretty only eliminates a few things I don't think it could ever be already.
There was a problem hiding this comment.
actually, it might be fine as is ... was concentrating too much on class's singleton_class
in case of a object ... attaches the bare (BasicObject) object for object.singleton_class
that null case (instead of UNDEF) still seems legit, as there's some (old) TODO notes down the getAttached() path of dealing with NPEs ... which UNDEF will throw as it gets used.
There was a problem hiding this comment.
@kares I thought it might just attach for object singletons but I did not see which code path did that so I was confused. I think perhaps even thinking that the name 'attached' probably does not best reflect what the field is for. So another thing to ponder...a better name.
There was a problem hiding this comment.
open to suggestions here - already biased as got used to the attached name :)
anyway, should be good to go as is, we could iterate some more over later ...
okay for 9.2.1 or should we up hold till release?
There was a problem hiding this comment.
Yeah my comment was not directed to this PR really. I just realized attached is probably the most minimal description possible. 'singletonFor'? Anyways I think we can land this. @headius does this worry you at all?
... down the paths we're checking for != null but not `UNDEF`
NPE try-catch can go since MetaClass#attached won't be UNDEF (which is likely to been the cause - see previous commit)
headius
left a comment
headius
left a comment
There was a problem hiding this comment.
I'd like to see a pass over MRI and ruby/spec to unblock things that are now fixed, but this looks ok to me. I also generated, scaffolded, migrated, and ran a Rails 5.2.1 app using this branch and everything worked just fine.
| #assert_include methods, :foo | ||
| #assert_include methods, :bar | ||
| end | ||
|
|
There was a problem hiding this comment.
I noticed at least one spec from spec/ruby/languages/singleton_class_spec.rb passes that seems related to this.
There was a problem hiding this comment.
There's a couple other tests that pass from MRI, but unsure how many have any relation to this.
There was a problem hiding this comment.
@headius untag/unexclude if you know what they are since it appears you figured that out
3 participants
JRuby seem to never have
Foo.singleton_class.singleton_classworking as expected... mostly following super-class hierarchy with singleton classes
this gets the class hierarchy working, not sure what is left to be done for module's singleton
left the "super meta class" logic as it was for now
fixes GH-287 ... an old timer :)