Fixes #4253. Respond_to? returns private methods.

The basic fix is that in respond_to? we do not return true for protected methods. I made a few internal changes: 1. deprecated is RubyModule.isMethodBound(name, checkVisibility, checkRespondTo) 2. added RubyModule doesMethodRespondTo(name, checkVisibility) 3. changed what I think is appropriate consumers to call doesMethodRespondTo but in worst case it will call isMethodBound(name, checkVisibility) which is what it would have done anyways before this change. The only risk is that I converted something which wants isMethodBound but that would indicate some method further upstream calling a respond-to method and not really wanting respond to behavior. The things I changed we obvious in that they had respondto names. 4. I made a Helpers.doesMethodRespondTo so that RespondToCallsite and RubyModule can share the same logic.
jruby · Nov 23, 2016 · d081169 · d081169
1 parent 36a172f
commit d081169
Show file tree

Hide file tree

Showing 7 changed files with 29 additions and 37 deletions.
diff --git a/core/src/main/java/org/jruby/RubyBasicObject.java b/core/src/main/java/org/jruby/RubyBasicObject.java
@@ -581,13 +581,12 @@ public RubyClass getType() {
     @Override
     public final boolean respondsTo(String name) {
         final Ruby runtime = getRuntime();
-
         final DynamicMethod respondTo = getMetaClass().searchMethod("respond_to?");
 
         // fastest path; builtin respond_to? and respond_to_missing? so we just check isMethodBound
         if ( respondTo.equals(runtime.getRespondToMethod()) &&
              getMetaClass().searchMethod("respond_to_missing?").equals(runtime.getRespondToMissingMethod()) ) {
-            return getMetaClass().isMethodBound(name, false);
+            return getMetaClass().doesMethodRespondTo(name, false);
         }
 
         final ThreadContext context = runtime.getCurrentContext();
@@ -2048,8 +2047,7 @@ public void checkFrozen() {
      * benefit is important for this method.
      */
     public final RubyBoolean respond_to_p(IRubyObject mname) {
-        final String name = mname.asJavaString();
-        return getRuntime().newBoolean(getMetaClass().isMethodBound(name, true));
+        return getRuntime().newBoolean(getMetaClass().doesMethodRespondTo(mname.asJavaString(), true));
     }
 
     public final RubyBoolean respond_to_p19(IRubyObject mname) {
@@ -2068,7 +2066,7 @@ public final RubyBoolean respond_to_p19(IRubyObject mname, IRubyObject includePr
     private RubyBoolean respond_to_p19(IRubyObject mname, final boolean includePrivate) {
         final Ruby runtime = getRuntime();
         final String name = mname.asJavaString();
-        if ( getMetaClass().isMethodBound(name, !includePrivate, true) ) return runtime.getTrue();
+        if (getMetaClass().doesMethodRespondTo(name, !includePrivate)) return runtime.getTrue();
         // MRI (1.9) always passes down a symbol when calling respond_to_missing?
         if ( ! (mname instanceof RubySymbol) ) mname = runtime.newSymbol(name);
         ThreadContext context = runtime.getCurrentContext();

diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java
@@ -1818,19 +1818,17 @@ public static String undefinedMethodMessage(final String name, final String modN
      */
     public boolean isMethodBound(String name, boolean checkVisibility) {
         DynamicMethod method = searchMethod(name);
-        if (!method.isUndefined()) {
-            return !(checkVisibility && method.getVisibility() == PRIVATE);
-        }
-        return false;
+
+        return !method.isUndefined() && !(checkVisibility && method.getVisibility() == PRIVATE);
+    }
+
+    public boolean doesMethodRespondTo(String name, boolean checkVisibility) {
+        return Helpers.doesMethodRespondTo(searchMethod(name), checkVisibility);
     }
 
+    @Deprecated
     public boolean isMethodBound(String name, boolean checkVisibility, boolean checkRespondTo) {
-        if (!checkRespondTo) return isMethodBound(name, checkVisibility);
-        DynamicMethod method = searchMethod(name);
-        if (!method.isUndefined() && !method.isNotImplemented()) {
-            return !(checkVisibility && method.getVisibility() == PRIVATE);
-        }
-        return false;
+        return checkRespondTo ? doesMethodRespondTo(name, checkVisibility): isMethodBound(name, checkVisibility);
     }
 
     public IRubyObject newMethod(IRubyObject receiver, String methodName, boolean bound, Visibility visibility) {

diff --git a/core/src/main/java/org/jruby/javasupport/JavaPackage.java b/core/src/main/java/org/jruby/javasupport/JavaPackage.java
@@ -207,9 +207,8 @@ public IRubyObject respond_to_p(final ThreadContext context, IRubyObject name, I
 
     private IRubyObject respond_to(final ThreadContext context, IRubyObject mname, final boolean includePrivate) {
         String name = mname.asJavaString();
-        if ( getMetaClass().isMethodBound(name, !includePrivate, true) ) {
-            return context.runtime.getTrue();
-        }
+
+        if (getMetaClass().doesMethodRespondTo(name, !includePrivate)) return context.runtime.getTrue();
         /*
         if ( ( name = BlankSlateWrapper.handlesMethod(name) ) != null ) {
             RubyBoolean bound = checkMetaClassBoundMethod(context, name, includePrivate);

diff --git a/core/src/main/java/org/jruby/runtime/Helpers.java b/core/src/main/java/org/jruby/runtime/Helpers.java
@@ -51,6 +51,8 @@
 import org.jcodings.specific.UTF8Encoding;
 import org.jcodings.unicode.UnicodeEncoding;
 
+import static org.jruby.runtime.Visibility.PRIVATE;
+import static org.jruby.runtime.Visibility.PROTECTED;
 import static org.jruby.runtime.invokedynamic.MethodNames.EQL;
 import static org.jruby.runtime.invokedynamic.MethodNames.OP_EQUAL;
 import static org.jruby.util.CodegenUtils.sig;
@@ -2843,4 +2845,15 @@ public static IRubyObject invoke(ThreadContext context, IRubyObject self, String
         return Helpers.invoke(context, self, name, IRubyObject.NULL_ARRAY, callType, Block.NULL_BLOCK);
     }
 
+    /**
+     *
+     * We have respondTo logic in RubyModule and we have a special callsite for respond_to?.
+     * This method is just so we can share that logic.
+     */
+    public static boolean doesMethodRespondTo(DynamicMethod method, boolean checkVisibility) {
+        if (method.isUndefined() || method.isNotImplemented()) return false;
+
+        return !(checkVisibility &&
+                (method.getVisibility() == PRIVATE || method.getVisibility() == PROTECTED));
+    }
 }
diff --git a/core/src/main/java/org/jruby/runtime/callsite/RespondToCallSite.java b/core/src/main/java/org/jruby/runtime/callsite/RespondToCallSite.java
@@ -2,6 +2,7 @@
 
 import org.jruby.Ruby;
 import org.jruby.RubySymbol;
+import org.jruby.runtime.Helpers;
 import org.jruby.runtime.ThreadContext;
 import org.jruby.runtime.builtin.IRubyObject;
 import org.jruby.RubyClass;
@@ -162,22 +163,9 @@ protected IRubyObject cacheAndCall(IRubyObject caller, RubyClass selfType, Threa
     }
 
     private static RespondToTuple recacheRespondsTo(CacheEntry respondToMethod, String newString, RubyClass klass, boolean checkVisibility, ThreadContext context) {
-        Ruby runtime = context.runtime;
         CacheEntry respondToLookupResult = klass.searchWithCache(newString);
-        IRubyObject respondsTo;
-        if (!respondToLookupResult.method.isUndefined() && !respondToLookupResult.method.isNotImplemented()) {
-            respondsTo = checkVisibilityAndCache(respondToLookupResult, checkVisibility, runtime);
-        } else {
-            respondsTo = runtime.getFalse();
-        }
-        return new RespondToTuple(newString, checkVisibility, respondToMethod, respondToLookupResult, respondsTo);
-    }
+        boolean respondsTo = Helpers.doesMethodRespondTo(respondToLookupResult.method, checkVisibility);
 
-    private static IRubyObject checkVisibilityAndCache(CacheEntry respondEntry, boolean checkVisibility, Ruby runtime) {
-        if (!checkVisibility || respondEntry.method.getVisibility() != Visibility.PRIVATE) {
-            return runtime.getTrue();
-        } else {
-            return runtime.getFalse();
-        }
+        return new RespondToTuple(newString, checkVisibility, respondToMethod, respondToLookupResult, context.runtime.newBoolean(respondsTo));
     }
 }
diff --git a/spec/tags/ruby/core/kernel/respond_to_missing_tags.txt b/spec/tags/ruby/core/kernel/respond_to_missing_tags.txt
diff --git a/spec/tags/ruby/core/kernel/respond_to_tags.txt b/spec/tags/ruby/core/kernel/respond_to_tags.txt