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

Not every class has a superclass? #4528

Closed
ioquatix opened this Issue Mar 9, 2017 · 19 comments

Comments

Projects
None yet
4 participants
@ioquatix
Copy link
Contributor

ioquatix commented Mar 9, 2017

It seems like sometimes ObjectSpace.each_object(Class).collect(&:superclass) may fail in JRuby but works fine in MRI.

Example of failure here: https://travis-ci.org/rubyworks/facets/jobs/209292853

    undefined method `superclass' for #<Class:Java::OrgJrubyExtPsych>
    lib/core/facets/class/subclasses.rb:20
    18       list = []
    19       ObjectSpace.each_object(Class) do |c|
 => 20         list.unshift c if c.superclass == self
    21       end
    22       list.uniq
@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 9, 2017

I believe this is a small bug in each_object: it is returning modules as well as classes for each_object(Class).

@ioquatix

This comment has been minimized.

Copy link
Contributor Author

ioquatix commented Mar 9, 2017

Ah, awesome, so it would be a good fix. Perhaps even a candidate for a spec?

@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 9, 2017

Actually it looks even narrower. I'm only seeing a small handful of these unusual modules. They shouldn't be coming up at all.

$ jruby -e 'ObjectSpace.each_object(Class) {|c| p c.class.ancestors unless c.respond_to? :superclass}'
[Module, Object, Kernel, BasicObject]
[Module, Object, Kernel, BasicObject]
[Module, Object, Kernel, BasicObject]
[Module, Object, Kernel, BasicObject]
[Module, Object, Kernel, BasicObject]
[Module, Object, Kernel, BasicObject]
[Module, Object, Kernel, BasicObject]
[Module, Object, Kernel, BasicObject]
[Module, Object, Kernel, BasicObject]
[Module, Object, Kernel, BasicObject]
[Module, Object, Kernel, BasicObject]

Only one of these modules has a name: Java::JavaPackage. There's definitely a spec in here somewhere, but we're doing something pretty extraordinary.

@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 9, 2017

Ahh, they are indeed the pseudo-modules we use to represent the Java package structure:

$ jruby -e 'ObjectSpace.each_object(Class) {|c| p JRuby.reference(c) unless c.respond_to? :superclass}'
#<Class:Java::OrgJrubyPlatform>
#<Class:Java::Javax>
#<Class:Java::JavaxManagement>
#<Class:Java::OrgJrubyUtil>
#<Class:Java::OrgJrubyRuntime>
#<Class:Java::JavaLang>
#<Class:Java::OrgJruby>
#<Class:Java::JavaUtil>
#<Class:Java::Org>
#<Class:Java::Java>
Java::JavaPackage

I'll try to figure out why they're being returned.

@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 9, 2017

Hmm, I thought this patch would help but it doesn't seem to. Any thoughts on this one @kares?

diff --git a/core/src/main/java/org/jruby/javasupport/JavaPackage.java b/core/src/main/java/org/jruby/javasupport/JavaPackage.java
index cea546e..a5f06f9 100644
--- a/core/src/main/java/org/jruby/javasupport/JavaPackage.java
+++ b/core/src/main/java/org/jruby/javasupport/JavaPackage.java
@@ -61,6 +61,7 @@ public class JavaPackage extends RubyModule {
         RubyClass superClass = new BlankSlateWrapper(runtime, runtime.getModule(), runtime.getKernel());
         RubyClass JavaPackage = RubyClass.newClass(runtime, superClass);
         JavaPackage.setMetaClass(runtime.getModule());
+        JavaPackage.kindOf = runtime.getModule().kindOf;
         JavaPackage.setAllocator(ObjectAllocator.NOT_ALLOCATABLE_ALLOCATOR);
         ((MetaClass) JavaPackage.makeMetaClass(superClass)).setAttached(JavaPackage);
 
@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 9, 2017

Oh wait, I know why. It's the reverse problem. When ObjectSpace does each_object(Class) it uses the kindOf for Class, which is set up to just check the Java class hierarchy for RubyClass. Since the BlankSlateWrapper extends IncludedModuleWrapper, it does indeed include RubyClass in its Java hierarchy, even though we are pretending it is a module.

This will take some thought. Seems like we need to make the blank slate package modules to actually descend from RubyModule instead.

@ioquatix

This comment has been minimized.

Copy link
Contributor Author

ioquatix commented Mar 9, 2017

We worked around the problem by using a different approach, but I think it's important to fix this issue too, especially if it's not the same as what MRI does?

@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 9, 2017

@ioquatix I agree.

@kares

This comment has been minimized.

Copy link
Member

kares commented Mar 10, 2017

najs, edgy (in the sense that its not obvious) case with those wrappers - haven't thought about it.
extending RubyModule might work and it sounds reasonable -> this is good candidate for a fix in 9.2

@kares kares added this to the JRuby 9.2.0.0 milestone Mar 10, 2017

@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 10, 2017

I agree...this is fairly low impact to leave it as is for 9.1.9.0 but we can switch to a RubyModule subclass soon (nowish) for 9.2 on the ruby-2.4 branch and let it bake.

@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 13, 2017

This is an alternative way to fix this particular issue: just make "superclass" call "singleton_class":

diff --git a/core/src/main/java/org/jruby/javasupport/JavaPackage.java b/core/src/main/java/org/jruby/javasupport/JavaPackage.java
index cea546e..4c55eb5 100644
--- a/core/src/main/java/org/jruby/javasupport/JavaPackage.java
+++ b/core/src/main/java/org/jruby/javasupport/JavaPackage.java
@@ -379,6 +379,7 @@ public class JavaPackage extends RubyModule {
                 // NOTE: these should maybe get re-thought and deprecated (for now due compatibility)
                 case "__constants__" : return "constants";
                 case "__methods__" : return "methods";
+                case "superclass": return "singleton_class";
             }
 
             final int last = name.length() - 1;

It occurred to me that we can't really make the blank slate actually just extend RubyModule because there's plenty of other odd behavior that would come from having an object whose .class is a module rather than a class. We really just want this thing to be a blank slate.

I suspect the logic here predates the existence of BasicObject, which would be a better superclass for this case. Moving that direction might clean up a lot of weird issues we've had over the years with the package modules.

@kares

This comment has been minimized.

Copy link
Member

kares commented Mar 15, 2017

right, I recall know -> as JavaPackage was re-factored it needed to act-like a module but be a class (due reasons stated above).

not sure I get the benefit of superclass -> singleton_class. would leave it as is and attempt to introduce BasicObject as a superclass for all "blank-state" wrappers, that sounds as a really good fit on improving JI.

@kares kares self-assigned this Aug 13, 2017

@kares

This comment has been minimized.

Copy link
Member

kares commented Aug 14, 2017

looking into this one, seems I am able to get it right but than to test this I hit another (hard-to-track) one :

since we tend to self reflect (in tests but not just there), e.g. org.jruby.RubyModule.java_class, such code produces a surprise around the public UNDEF (and NEVER) constant on RubyBasicObject which obviously end up being "invalid" (with a null metaClass) -> so when one tries to ObjectSpace.each_object(Class) that eventually still gets an ugly surprise.

cleaning up test, for now, hopefully we can avoid most of it so that we guard against self-reflecting too much ...

kares added a commit to kares/jruby that referenced this issue Aug 14, 2017

[test] move JI specs that cause proxy loading for JRuby's Java classes
... into native Java tests - otherwise we can not test jruby#4528

kares added a commit to kares/jruby that referenced this issue Aug 15, 2017

[test] move JI specs that cause proxy loading for JRuby's Java classes
... into native Java tests - otherwise we can not test jruby#4528

kares added a commit to kares/jruby that referenced this issue Aug 18, 2017

@headius headius modified the milestones: JRuby 9.2.0.0, JRuby 9.2.1.0 May 16, 2018

@kares

This comment has been minimized.

Copy link
Member

kares commented Jun 11, 2018

there's some less self-reflection in 9.2.0 and hopefully much less in 9.2.1 ... due the standardized way of extension loading. a default boot (with JRuby's shipped gems) will than not do much JI or at least none causing org.jruby.... class proxies.
unfortunately not all gems with a JRuby ext can be "fixed" right away.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Jun 14, 2018

What is left for this issue to be resolved?

@kares

This comment has been minimized.

Copy link
Member

kares commented Jun 15, 2018

@enebo mostly #5205 esp. setting up gems loaded by default not to self reflect on JRuby's classes

@kares

This comment has been minimized.

Copy link
Member

kares commented Oct 23, 2018

JRuby's common (and internal) ext are now updated and by default JRuby does not cause self-reflection, some external native extensions still might. regarding the issue only the JavaPackage case is left wout superclass:

kares@clevo:~/workspace/oss/jruby$ bin/jruby -S irb
irb(main):001:0> ObjectSpace.each_object(Class) { |c| list.unshift c if c.superclass == self rescue puts(c) }
Java::JavaPackage
=> 1094

kares added a commit to kares/jruby that referenced this issue Oct 24, 2018

[fix] JavaPackage's meta-class to respond_to?(:superclass)
to address common behavior of Class even if its a "fake" one (jrubyGH-4528)

kares added a commit that referenced this issue Oct 25, 2018

[fix] JavaPackage's meta-class to respond_to?(:superclass)
to address common behavior of Class even if its a "fake" one (GH-4528)
@kares

This comment has been minimized.

Copy link
Member

kares commented Oct 25, 2018

put in a work-around so that the custom internals of JavaPackage super type act more Ruby-sh
esp. with regards to superclass since JavaPackage.is_a?(Class), its kind of ugly-sh but it will act more compliant to the outside - which matters. this issue is resolved in terms of the reported case.

@kares kares closed this Oct 25, 2018

@ioquatix

This comment has been minimized.

Copy link
Contributor Author

ioquatix commented Oct 25, 2018

Thanks for your huge effort to improve compatibility in this area!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.