Additional changes for JRUBY-4799:

* Rewrite the logic to pick methods off a class and its superclasses to do a better job handling interfaces, non-public classes, and so on.
* Make the access check be a catch-all, so any exception raised will mean we don't have that access.

spec/java_integration/fixtures/SuperWithInterface.java

@@ -0,0 +1,138 @@
+package java_integration.fixtures;
+
+import java.util.Collection;
+import java.util.Iterator;
+
+public class SuperWithInterface implements Collection<Object> {
+    private static class SubClassWithoutInterfaces extends SuperWithInterface implements Runnable {
+        public void run() {}
+
+	    // for testing that a single unique method on child does not cause all parent methods to disappear
+	    protected boolean add(String s) {return false;}
+	    public boolean add(Object s) {return true;}
+    }
+
+    public static SuperWithInterface getSubClassInstance() {
+        return new SubClassWithoutInterfaces();
+    }
+
+    public static class SuperWithoutInterface {
+	    // no interfaces, so we need to test that this doesn't cause
+	    // the equivalent child method to get removed
+    	public boolean add(Object s) {
+			throw new UnsupportedOperationException("Not supported yet.");
+		}
+    }
+
+    public static class SubWithInterface extends SuperWithoutInterface implements Collection<Object> {
+		private boolean add(String e) {
+			return false;
+		}
+
+	    public boolean add(Object e) {
+			return true;
+	    }
+
+    	// impl methods, just to have them
+	    public int size() {
+	        throw new UnsupportedOperationException("Not supported yet.");
+	    }
+
+	    public boolean isEmpty() {
+	        throw new UnsupportedOperationException("Not supported yet.");
+	    }
+
+	    public boolean contains(Object o) {
+	        throw new UnsupportedOperationException("Not supported yet.");
+	    }
+
+	    public Iterator<Object> iterator() {
+	        throw new UnsupportedOperationException("Not supported yet.");
+	    }
+
+	    public Object[] toArray() {
+	        throw new UnsupportedOperationException("Not supported yet.");
+	    }
+
+	    public <T> T[] toArray(T[] a) {
+	        throw new UnsupportedOperationException("Not supported yet.");
+	    }
+
+	    public boolean remove(Object o) {
+	        throw new UnsupportedOperationException("Not supported yet.");
+	    }
+
+	    public boolean containsAll(Collection<?> c) {
+	        throw new UnsupportedOperationException("Not supported yet.");
+	    }
+
+	    public boolean addAll(Collection<? extends Object> c) {
+	        throw new UnsupportedOperationException("Not supported yet.");
+	    }
+
+	    public boolean removeAll(Collection<?> c) {
+	        throw new UnsupportedOperationException("Not supported yet.");
+	    }
+
+	    public boolean retainAll(Collection<?> c) {
+	        throw new UnsupportedOperationException("Not supported yet.");
+	    }
+
+	    public void clear() {
+	        throw new UnsupportedOperationException("Not supported yet.");
+	    }
+}
+
+    // impl methods, just to have them
+    public int size() {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+
+    public boolean isEmpty() {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+
+    public boolean contains(Object o) {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+
+    public Iterator<Object> iterator() {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+
+    public Object[] toArray() {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+
+    public <T> T[] toArray(T[] a) {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+
+    public boolean add(Object e) {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+
+    public boolean remove(Object o) {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+
+    public boolean containsAll(Collection<?> c) {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+
+    public boolean addAll(Collection<? extends Object> c) {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+
+    public boolean removeAll(Collection<?> c) {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+
+    public boolean retainAll(Collection<?> c) {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+
+    public void clear() {
+        throw new UnsupportedOperationException("Not supported yet.");
+    }
+}

spec/java_integration/methods/binding_spec.rb

@@ -0,0 +1,50 @@
+require File.dirname(__FILE__) + "/../spec_helper"
+
+java_import "java_integration.fixtures.SuperWithInterface"
+
+describe "A private subclass of a public superclass with interfaces" do
+  it "defines all methods from those interfaces" do
+    sub_without = SuperWithInterface.sub_class_instance
+
+    # sub_without_cls = sub_without.class
+    # 
+    # sub_without_cls.instance_methods.should include("size")
+    # sub_without_cls.instance_methods.should include("empty?")
+    # sub_without_cls.instance_methods.should include("contains?")
+    # sub_without_cls.instance_methods.should include("iterator")
+    # sub_without_cls.instance_methods.should include("to_array")
+    # sub_without_cls.instance_methods.should include("add")
+    # sub_without_cls.instance_methods.should include("remove")
+    # sub_without_cls.instance_methods.should include("contains_all?")
+    # sub_without_cls.instance_methods.should include("add_all")
+    # sub_without_cls.instance_methods.should include("remove_all")
+    # sub_without_cls.instance_methods.should include("retain_all")
+    # sub_without_cls.instance_methods.should include("clear")
+
+    # test that add can handle values other than string, since a
+    # once-upon-a-time change caused child classes to only bind
+    # unique methods, which in this case would be private boolean add(String)
+    sub_without.add(1).should == true
+  end
+end
+
+describe "A private subclass with interfaces" do
+  it "defines all methods from those interfaces" do
+    sub_without = SuperWithInterface.sub_class_instance
+
+    sub_without_cls = sub_without.class
+
+    sub_without_cls.instance_methods.should include("run")
+  end
+end
+
+describe "A public subclass with interfaces extending a superclass that duplicates some of those methods" do
+  it "should still bind all methods from the child" do
+    sub_with = SuperWithInterface::SubWithInterface.new
+
+    # test that add can handle values other than string, since a
+    # once-upon-a-time change caused child classes to only bind
+    # unique methods, which in this case would be private boolean add(String)
+    sub_with.add(1).should == true
+  end
+end

src/org/jruby/javasupport/JavaClass.java

@@ -88,6 +88,7 @@
 import org.jruby.runtime.callback.Callback;
 import org.jruby.util.ByteList;
 import org.jruby.util.IdUtil;
+import org.jruby.util.SafePropertyAccessor;
 
 
 @JRubyClass(name="Java::JavaClass", parent="Java::JavaObject")
@@ -102,7 +103,14 @@ public class JavaClass extends JavaObject {
         try {
             AccessController.checkPermission(new ReflectPermission("suppressAccessChecks"));
             canSetAccessible = true;
-        } catch (AccessControlException ace) {
+        } catch (Throwable t) {
+            // added this so if things are weird in the future we can debug without
+            // spinning a new binary
+            if (SafePropertyAccessor.getBoolean("jruby.ji.logCanSetAccessible")) {
+                t.printStackTrace();
+            }
+
+            // assume any exception means we can't suppress access checks
             canSetAccessible = false;
         }
 
@@ -1873,112 +1881,83 @@ private static boolean methodsAreEquivalent(Method child, Method parent) {
                 && Modifier.isProtected(child.getModifiers()) == Modifier.isProtected(parent.getModifiers())
                 && Modifier.isStatic(child.getModifiers()) == Modifier.isStatic(parent.getModifiers());
     }
-
-    private static Method[] getMethods(Class<?> javaClass) {
-        HashMap<String, List<Method>> nameMethods = new HashMap<String, List<Method>>();
-        ArrayList<Method> finalList = new ArrayList<Method>();
-
-        // we only bind methods declared onnon-public classes if we're able to
-        // set those methods accessible (JRUBY-4799)
-        boolean useImmediateClass = CAN_SET_ACCESSIBLE || Modifier.isPublic(javaClass.getModifiers());
-
-        // aggregate all candidate method names from child, with their method objects
-        // Instance methods only; static methods are local to the class and always bound.
-        // Don't do this class's methods if it isn't public, since only superclass
-        // methods will be available.
-        if (useImmediateClass) {
-            // class is public or we can set its methods accessible
-            for (Method m: javaClass.getDeclaredMethods()) {
-                int modifiers = m.getModifiers();
-                if (Modifier.isPublic(modifiers) || Modifier.isProtected(modifiers)) {
-                    if (Modifier.isStatic(modifiers)) {
-                        // static methods are always bound
-                        finalList.add(m);
-                    } else {
-                        List<Method> methods = nameMethods.get(m.getName());
-                        if (methods == null) {
-                            nameMethods.put(m.getName(), methods = new ArrayList<Method>());
-                        }
-                        methods.add(m);
-                    }
-                }
-            }
-        }
 
-        // add immediately-implemented interface methods in case they're not
-        // provided by superclasses or left abstract
-        for (Class c : javaClass.getInterfaces()) {
-            if (!Modifier.isPublic(c.getModifiers())) continue;
 
-            for (Method m: c.getDeclaredMethods()) {
-                List<Method> methods = nameMethods.get(m.getName());
-                if (methods == null) {
-                    nameMethods.put(m.getName(), methods = new ArrayList<Method>());
-                }
-                methods.add(m);
+    private static void addNewMethods(HashMap<String, List<Method>> nameMethods, Method[] methods, boolean includeStatic, boolean removeDuplicate) {
+        Methods: for (Method m : methods) {
+            if (Modifier.isStatic(m.getModifiers()) && !includeStatic) {
+                // Skip static methods if we're not suppose to include them.
+                // Generally for superclasses; we only bind statics from the actual
+                // class.
+                continue;
             }
-        }
-
-        // we scan all superclasses, but avoid adding superclass methods with
-        // same name+signature as subclass methods
-        // see JRUBY-3130
-        for (Class c = javaClass.getSuperclass(); c != null; c = c.getSuperclass()) {
-            try {
-                Methods: for (Method m : c.getDeclaredMethods()) {
-                    List<Method> childMethods = nameMethods.get(m.getName());
-                    if (childMethods == null) continue;
-
-                    for (Method m2 : childMethods) {
-                        if (methodsAreEquivalent(m2, m)) {
+            List<Method> childMethods = nameMethods.get(m.getName());
+            if (childMethods == null) {
+                // first method of this name, add a collection for it
+                childMethods = new ArrayList<Method>();
+                childMethods.add(m);
+                nameMethods.put(m.getName(), childMethods);
+            } else {
+                // we have seen other methods; check if we already have
+                // an equivalent one
+                for (Method m2 : childMethods) {
+                    if (methodsAreEquivalent(m2, m)) {
+                        if (removeDuplicate) {
+                            // Replace the existing method, since the super call is more general
+                            // and virtual dispatch will call the subclass impl anyway.
+                            // Used for instance methods, for which we usually want to use the highest-up
+                            // callable implementation.
                             childMethods.remove(m2);
-                            if (childMethods.isEmpty()) nameMethods.remove(m.getName());
-                            continue Methods;
+                            childMethods.add(m);
+                        } else {
+                            // just skip the new method, since we don't need it (already found one)
+                            // used for interface methods, which we want to add unconditionally
+                            // but only if we need them
                         }
+                        continue Methods;
                     }
                 }
-            } catch (SecurityException e) {
+                // no equivalent; add it
+                childMethods.add(m);
             }
         }
-
-        // now only bind the ones that remain
+    }
+
+    private static Method[] getMethods(Class<?> javaClass) {
+        HashMap<String, List<Method>> nameMethods = new HashMap<String, List<Method>>();
 
-        // first the immediate class, if we used it above
-        if (useImmediateClass) {
-            try {
-                for (Method m : javaClass.getDeclaredMethods()) {
-                    int modifiers = m.getModifiers();
-                    if (Modifier.isPublic(modifiers) || Modifier.isProtected(modifiers)) {
-                        if (!nameMethods.containsKey(m.getName())) continue;
-                        finalList.add(m);
-                    }
+        // we scan all superclasses, but avoid adding superclass methods with
+        // same name+signature as subclass methods (see JRUBY-3130)
+        for (Class c = javaClass; c != null; c = c.getSuperclass()) {
+            // only add class's methods if it's public or we can set accessible
+            // (see JRUBY-4799)
+            if (Modifier.isPublic(c.getModifiers()) || CAN_SET_ACCESSIBLE) {
+                // for each class, scan declared methods for new signatures
+                try {
+                    // add methods, including static if this is the actual class,
+                    // and replacing child methods with equivalent parent methods
+                    addNewMethods(nameMethods, c.getDeclaredMethods(), c == javaClass, true);
+                } catch (SecurityException e) {
                 }
-            } catch (SecurityException e) {
             }
-        }
 
-        // then superclasses
-        for (Class c = javaClass.getSuperclass(); c != null; c = c.getSuperclass()) {
-            try {
-                for (Method m : c.getDeclaredMethods()) {
-                    int modifiers = m.getModifiers();
-                    if (Modifier.isPublic(modifiers) || Modifier.isProtected(modifiers)) {
-                        if (!nameMethods.containsKey(m.getName())) continue;
-                        finalList.add(m);
-                    }
+            // then do the same for each interface
+            for (Class i : c.getInterfaces()) {
+                try {
+                    // add methods, not including static (should be none on
+                    // interfaces anyway) and not replacing child methods with
+                    // parent methods
+                    addNewMethods(nameMethods, i.getMethods(), false, false);
+                } catch (SecurityException e) {
                 }
-            } catch (SecurityException e) {
             }
         }
+
+        // now only bind the ones that remain
+        ArrayList<Method> finalList = new ArrayList<Method>();
 
-        // then immediately implemented interfaces
-        for (Class c : javaClass.getInterfaces()) {
-            try {
-                for (Method m : c.getDeclaredMethods()) {
-                    if (!nameMethods.containsKey(m.getName())) continue;
-                    finalList.add(m);
-                }
-            } catch (SecurityException e) {
-            }
+        for (Map.Entry<String, List<Method>> entry : nameMethods.entrySet()) {
+            finalList.addAll(entry.getValue());
         }
 
         return finalList.toArray(new Method[finalList.size()]);