Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] singleton meta class #5394

Merged
merged 7 commits into from
Nov 1, 2018
Merged

Conversation

kares
Copy link
Member

@kares kares commented Oct 31, 2018

JRuby seem to never have Foo.singleton_class.singleton_class working 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 :)

return attached == target ? this : super.toSingletonClass(target);
}

public RubyBasicObject getAttached() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its pretty much assumed to be of a RubyBasicObject

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

As 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?).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

@headius headius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed at least one spec from spec/ruby/languages/singleton_class_spec.rb passes that seems related to this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a couple other tests that pass from MRI, but unsure how many have any relation to this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@headius untag/unexclude if you know what they are since it appears you figured that out

@enebo enebo merged commit 70f2671 into jruby:master Nov 1, 2018
@headius headius added this to the JRuby 9.2.1.0 milestone Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class does not inherit singleton_class's class methods
3 participants