Class#subclasses should work correctly - not return internal wrappers

implementation has been changed but old behavior can be achieved ... using `klass.to_java.sublclasses` (not very useful for user-code) also synchronize subclasses retrieval on RubyClass ~ rest of related impl resolves #4741
jruby · Aug 28, 2017 · 916b1e1 · 916b1e1
1 parent 7e81079
commit 916b1e1
Showing 3 changed files with 90 additions and 30 deletions.
diff --git a/core/src/main/java/org/jruby/RubyClass.java b/core/src/main/java/org/jruby/RubyClass.java
@@ -1091,20 +1091,25 @@ protected void setModuleSuperClass(RubyClass superClass) {
         setSuperClass(superClass);
     }
 
-    public Collection<RubyClass> subclasses(boolean includeDescendants) {
-        Set<RubyClass> mySubclasses = subclasses;
-        if (mySubclasses != null) {
-            Collection<RubyClass> mine = new ArrayList<RubyClass>(mySubclasses);
+    // introduced solely to provide some level of compatibility with previous
+    // Class#subclasses implementation ... `ruby_class.to_java.subclasses`
+    public final Collection<RubyClass> subclasses() {
+        return subclasses(false);
+    }
+
+    public synchronized Collection<RubyClass> subclasses(boolean includeDescendants) {
+        Set<RubyClass> subclasses = this.subclasses;
+        if (subclasses != null) {
+            Collection<RubyClass> mine = new ArrayList<>(subclasses);
             if (includeDescendants) {
-                for (RubyClass i: mySubclasses) {
-                    mine.addAll(i.subclasses(includeDescendants));
+                for (RubyClass klass: subclasses) {
+                    mine.addAll(klass.subclasses(includeDescendants));
                 }
             }
 
             return mine;
-        } else {
-            return Collections.EMPTY_LIST;
         }
+        return Collections.EMPTY_LIST;
     }
 
     /**
@@ -1118,9 +1123,9 @@ public Collection<RubyClass> subclasses(boolean includeDescendants) {
      */
     public synchronized void addSubclass(RubyClass subclass) {
         synchronized (runtime.getHierarchyLock()) {
-            Set<RubyClass> oldSubclasses = subclasses;
-            if (oldSubclasses == null) subclasses = oldSubclasses = new WeakHashSet<RubyClass>(4);
-            oldSubclasses.add(subclass);
+            Set<RubyClass> subclasses = this.subclasses;
+            if (subclasses == null) this.subclasses = subclasses = new WeakHashSet<>(4);
+            subclasses.add(subclass);
         }
     }
 
@@ -1131,10 +1136,10 @@ public synchronized void addSubclass(RubyClass subclass) {
      */
     public synchronized void removeSubclass(RubyClass subclass) {
         synchronized (runtime.getHierarchyLock()) {
-            Set<RubyClass> oldSubclasses = subclasses;
-            if (oldSubclasses == null) return;
+            Set<RubyClass> subclasses = this.subclasses;
+            if (subclasses == null) return;
 
-            oldSubclasses.remove(subclass);
+            subclasses.remove(subclass);
         }
     }
 
@@ -1146,11 +1151,11 @@ public synchronized void removeSubclass(RubyClass subclass) {
      */
     public synchronized void replaceSubclass(RubyClass subclass, RubyClass newSubclass) {
         synchronized (runtime.getHierarchyLock()) {
-            Set<RubyClass> oldSubclasses = subclasses;
-            if (oldSubclasses == null) return;
+            Set<RubyClass> subclasses = this.subclasses;
+            if (subclasses == null) return;
 
-            oldSubclasses.remove(subclass);
-            oldSubclasses.add(newSubclass);
+            subclasses.remove(subclass);
+            subclasses.add(newSubclass);
         }
     }
 
@@ -1159,9 +1164,9 @@ public void becomeSynchronized() {
         // make this class and all subclasses sync
         synchronized (runtime.getHierarchyLock()) {
             super.becomeSynchronized();
-            Set<RubyClass> mySubclasses = subclasses;
-            if (mySubclasses != null) for (RubyClass subclass : mySubclasses) {
-                subclass.becomeSynchronized();
+            Set<RubyClass> subclasses = this.subclasses;
+            if (subclasses != null) {
+                for (RubyClass subclass : subclasses) subclass.becomeSynchronized();
             }
         }
     }
@@ -1183,9 +1188,9 @@ public void invalidateCacheDescendants() {
         super.invalidateCacheDescendants();
 
         synchronized (runtime.getHierarchyLock()) {
-            Set<RubyClass> mySubclasses = subclasses;
-            if (mySubclasses != null) for (RubyClass subclass : mySubclasses) {
-                subclass.invalidateCacheDescendants();
+            Set<RubyClass> subclasses = this.subclasses;
+            if (subclasses != null) {
+                for (RubyClass subclass : subclasses) subclass.invalidateCacheDescendants();
             }
         }
     }
@@ -1197,14 +1202,15 @@ public void addInvalidatorsAndFlush(List<Invalidator> invalidators) {
         // if we're not at boot time, don't bother fully clearing caches
         if (!runtime.isBootingCore()) cachedMethods.clear();
 
+        Set<RubyClass> subclasses = this.subclasses;
         // no subclasses, don't bother with lock and iteration
         if (subclasses == null || subclasses.isEmpty()) return;
 
         // cascade into subclasses
         synchronized (runtime.getHierarchyLock()) {
-            Set<RubyClass> mySubclasses = subclasses;
-            if (mySubclasses != null) for (RubyClass subclass : mySubclasses) {
-                subclass.addInvalidatorsAndFlush(invalidators);
+            subclasses = this.subclasses;
+            if (subclasses != null) {
+                for (RubyClass subclass : subclasses) subclass.addInvalidatorsAndFlush(invalidators);
             }
         }
     }

diff --git a/lib/ruby/stdlib/jruby/core_ext/class.rb b/lib/ruby/stdlib/jruby/core_ext/class.rb
@@ -29,7 +29,16 @@ class Class
   # Get an array of all known subclasses of this class. If recursive == true,
   # include all descendants.
   def subclasses(recursive = false)
-    JRuby.reference0(self).subclasses(recursive).to_a.freeze
+    subclasses = []
+    ObjectSpace.each_object(singleton_class) do |klass|
+      next if klass.equal? self
+      if recursive
+        subclasses << klass
+      else
+        subclasses << klass if klass.superclass.equal? self
+      end
+    end
+    subclasses
   end
 
   ##

diff --git a/test/jruby/test_jruby_core_ext.rb b/test/jruby/test_jruby_core_ext.rb
@@ -2,12 +2,48 @@
 require 'jruby'
 require 'jruby/core_ext'
 
-class TestJrubyCoreExt < Test::Unit::TestCase
+class TestJRubyCoreExt < Test::Unit::TestCase
   def test_subclasses
     superclass = Class.new
     sub1 = Class.new(superclass)
     sub2 = Class.new(superclass)
-    assert_equal([sub1.to_s, sub2.to_s].sort, superclass.subclasses.map{|c| c.to_s}.sort)
+    assert_same_contents [sub1, sub2], superclass.subclasses
+  end
+
+  module Some; end
+  module Core; end
+
+  class Base < Object
+    extend  Some
+    include Core
+  end
+
+  class User < Base
+    include Enumerable
+    include Core
+  end
+  class Role < Base; end
+  class SuperUser < User; end
+
+  def test_subclasses_with_modules
+    subclasses = Base.subclasses
+    assert_equal ['TestJRubyCoreExt::Role', 'TestJRubyCoreExt::User'], subclasses.map(&:name).sort
+
+    assert_same_contents [ Role, User, SuperUser ], Base.subclasses(true)
+
+    assert_same_contents [ SuperUser ], User.subclasses
+
+    SuperUser.extend Module.new
+    SuperUser.send :include, Module.new
+    assert_equal [ SuperUser ], User.subclasses(true)
+    assert_equal [ ], SuperUser.subclasses
+    assert_equal [ ], SuperUser.subclasses(true)
+    klass = Class.new(SuperUser)
+    assert_equal [ klass ], SuperUser.subclasses
+    assert_equal [ SuperUser, klass ], User.subclasses(true)
+
+    assert User.to_java.subclasses(true).include? SuperUser
+    assert Base.to_java.subclasses # basically that () works
   end
 
   def test_with_current_runtime_as_global
@@ -19,4 +55,13 @@ def test_with_current_runtime_as_global
     end
     assert_equal other_runtime, org.jruby.Ruby.global_runtime
   end
+
+  private
+
+  def assert_same_contents(expect, actual)
+    exp = expect.inject({}) { |h, e| h[e] = nil; h }
+    act = actual.inject({}) { |h, e| h[e] = nil; h }
+    assert_equal exp, act
+  end
+
 end