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

Is there a way to fail-fast on ambiguous arguments? #4894

Closed
thbar opened this Issue Dec 13, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@thbar

thbar commented Dec 13, 2017

Environment

  • JRuby: jruby 9.1.14.0
  • Operating systems: All

Context

Currently one will get the following warning in the logs when ambiguous calls are met by the lexer:

warning: ambiguous Java methods found, using com.bmc.arsys.api.Value(java.lang.String)

This is provided thanks to parser.warn.ambigous_argument, which has a great true default value, and to RubyLexer.

This automatic method choice is not stable over time, which can make it very hard to track some bugs, like one I met recently. For instance, I had some code calling:

Java::ComBmcArsysApi::Value.new(nil)

yet this API would need an argument-less constructor to handle nil values (as seen in the API doc).

This resulted in a hard-to-track bug, causing random NullPointerException during RPC calls much later in the processing.

Each restart of sidekiq was generating a different set of stacktraces, due to #4759.

Suggestions

I would love to have a way to fail-fast on ambiguous arguments.

This line of bug could have been very problematic and I would have loved to catch it much more easily.

I wonder if maybe we could have a "strict" mode during dev/staging time (or why not, production) to raise an error when such a warning is met.

Another idea would have to expose the warnings to the end user and let them decide what to do on such cases - or maybe some form of callbacks, I'm not sure yet.

This is mostly a suggestion at this stage - happy to hear thoughts from the team!

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Dec 13, 2017

Member

It certainly wouldn't be hard to add a flag to make these errors, or perhaps especially noisy warnings. But I feel like we need better syntax to specify the method you do want, so that the "especially noisy" warning might be able to offer a solution as well.

Right now, I think the best options for dispatching ambiguous arguments to an overloaded method are:

  • Narrow their types as much as possible using to_java. This still requires runtime "guessing" based on the resulting coerced types, but it can avoid ambiguity with some values (numeric overloads come to mind).
  • Use one of the reflective methods, like java_send to select a specific method. This will accurately select a specific method, but it's a rather cumbersome syntax. We also provide java_alias to bind that specific method to another name, which works well but requires more ceremony.

In the short term, it seems reasonable to add a way to make the warnings into errors, but I feel like we should at least have a good wiki page about java_send and java_alias to link in the error.

Member

headius commented Dec 13, 2017

It certainly wouldn't be hard to add a flag to make these errors, or perhaps especially noisy warnings. But I feel like we need better syntax to specify the method you do want, so that the "especially noisy" warning might be able to offer a solution as well.

Right now, I think the best options for dispatching ambiguous arguments to an overloaded method are:

  • Narrow their types as much as possible using to_java. This still requires runtime "guessing" based on the resulting coerced types, but it can avoid ambiguity with some values (numeric overloads come to mind).
  • Use one of the reflective methods, like java_send to select a specific method. This will accurately select a specific method, but it's a rather cumbersome syntax. We also provide java_alias to bind that specific method to another name, which works well but requires more ceremony.

In the short term, it seems reasonable to add a way to make the warnings into errors, but I feel like we should at least have a good wiki page about java_send and java_alias to link in the error.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Dec 13, 2017

Member

A note on the warning: I always figured that if there's multiple overloads for a method that can all apply to a given Ruby type, they're probably all mostly applicable. For example, multiple numeric overloads: we pick the widest one. String/CharSequence/Object, we always try to pick the narrowest type.

But even though this usually works well, there's a fair chance it won't pick the intended overload.

Member

headius commented Dec 13, 2017

A note on the warning: I always figured that if there's multiple overloads for a method that can all apply to a given Ruby type, they're probably all mostly applicable. For example, multiple numeric overloads: we pick the widest one. String/CharSequence/Object, we always try to pick the narrowest type.

But even though this usually works well, there's a fair chance it won't pick the intended overload.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Dec 17, 2017

Member

the warning you're seeing comes from Java Integration, this does not apply (just for potential readers) :

This is provided thanks to parser.warn.ambigous_argument, which has a great true default value, and to RubyLexer.

#4759 is just "non-fixable" but your case is slightly different, since its not proc-to-iface propagation ...
you simply need to specify which overload you want, anyway a hard fail instead of warning is doable.

Member

kares commented Dec 17, 2017

the warning you're seeing comes from Java Integration, this does not apply (just for potential readers) :

This is provided thanks to parser.warn.ambigous_argument, which has a great true default value, and to RubyLexer.

#4759 is just "non-fixable" but your case is slightly different, since its not proc-to-iface propagation ...
you simply need to specify which overload you want, anyway a hard fail instead of warning is doable.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 1, 2018

Member

I am reluctant to introduce a hard error. What about a flag you can pass that reports a stack trace for each ambiguous site?

diff --git a/core/src/main/java/org/jruby/java/dispatch/CallableSelector.java b/core/src/main/java/org/jruby/java/dispatch/CallableSelector.java
index ff52185349..dce2f79921 100644
--- a/core/src/main/java/org/jruby/java/dispatch/CallableSelector.java
+++ b/core/src/main/java/org/jruby/java/dispatch/CallableSelector.java
@@ -22,6 +22,7 @@ import org.jruby.javasupport.JavaClass;
 import org.jruby.javasupport.JavaUtil;
 import org.jruby.javasupport.ParameterTypes;
 import org.jruby.runtime.builtin.IRubyObject;
+import org.jruby.util.cli.Options;
 import org.jruby.util.collections.IntHashMap;
 import static org.jruby.util.CodegenUtils.getBoxType;
 import static org.jruby.util.CodegenUtils.prettyParams;
@@ -279,7 +280,11 @@ public class CallableSelector {
                 method = mostSpecific;
 
                 if ( ambiguous ) {
-                    runtime.getWarnings().warn("ambiguous Java methods found, using " + ((Member) ((JavaCallable) method).accessibleObject()).getName() + prettyParams(msTypes));
+                    if (Options.DEBUG_AMBIGUOUS_JAVA_CALLS.load()) {
+                        runtime.newRuntimeError("multiple Java methods found, dumping backtrace and choosing " + ((Member) ((JavaCallable) method).accessibleObject()).getName() + prettyParams(msTypes)).printStackTrace(runtime.getErr());
+                    } else {
+                        runtime.getWarnings().warn("multiple Java methods found, use -Xdebug.ambiguous.java.calls for backtrace. Choosing " + ((Member) ((JavaCallable) method).accessibleObject()).getName() + prettyParams(msTypes));
+                    }
                 }
             }
         }
diff --git a/core/src/main/java/org/jruby/util/cli/Options.java b/core/src/main/java/org/jruby/util/cli/Options.java
index 873775270a..65bee12644 100644
--- a/core/src/main/java/org/jruby/util/cli/Options.java
+++ b/core/src/main/java/org/jruby/util/cli/Options.java
@@ -188,6 +188,7 @@ public class Options {
     public static final Option<String> LOGGER_CLASS = string(DEBUG, "logger.class", new String[]{"class name"}, "org.jruby.util.log.StandardErrorLogger", "Use specified class for logging.");
     public static final Option<Boolean> DUMP_INSTANCE_VARS = bool(DEBUG, "dump.variables", false, "Dump class + instance var names on first new of Object subclasses.");
     public static final Option<Boolean> REWRITE_JAVA_TRACE = bool(DEBUG, "rewrite.java.trace", true, "Rewrite stack traces from exceptions raised in Java calls.");
+    public static final Option<Boolean> DEBUG_AMBIGUOUS_JAVA_CALLS = bool(DEBUG, "debug.ambiguous.java.calls", false, "Toggle verbose reporting of all ambiguous calls to Java objects");
 
     // TODO: Replace flag that's false on 9 with proper module checks
     public static final Option<Boolean> JI_SETACCESSIBLE = bool(JAVA_INTEGRATION, "ji.setAccessible", calculateSetAccessibleDefault(), "Try to set inaccessible Java methods to be accessible.");
Member

headius commented Mar 1, 2018

I am reluctant to introduce a hard error. What about a flag you can pass that reports a stack trace for each ambiguous site?

diff --git a/core/src/main/java/org/jruby/java/dispatch/CallableSelector.java b/core/src/main/java/org/jruby/java/dispatch/CallableSelector.java
index ff52185349..dce2f79921 100644
--- a/core/src/main/java/org/jruby/java/dispatch/CallableSelector.java
+++ b/core/src/main/java/org/jruby/java/dispatch/CallableSelector.java
@@ -22,6 +22,7 @@ import org.jruby.javasupport.JavaClass;
 import org.jruby.javasupport.JavaUtil;
 import org.jruby.javasupport.ParameterTypes;
 import org.jruby.runtime.builtin.IRubyObject;
+import org.jruby.util.cli.Options;
 import org.jruby.util.collections.IntHashMap;
 import static org.jruby.util.CodegenUtils.getBoxType;
 import static org.jruby.util.CodegenUtils.prettyParams;
@@ -279,7 +280,11 @@ public class CallableSelector {
                 method = mostSpecific;
 
                 if ( ambiguous ) {
-                    runtime.getWarnings().warn("ambiguous Java methods found, using " + ((Member) ((JavaCallable) method).accessibleObject()).getName() + prettyParams(msTypes));
+                    if (Options.DEBUG_AMBIGUOUS_JAVA_CALLS.load()) {
+                        runtime.newRuntimeError("multiple Java methods found, dumping backtrace and choosing " + ((Member) ((JavaCallable) method).accessibleObject()).getName() + prettyParams(msTypes)).printStackTrace(runtime.getErr());
+                    } else {
+                        runtime.getWarnings().warn("multiple Java methods found, use -Xdebug.ambiguous.java.calls for backtrace. Choosing " + ((Member) ((JavaCallable) method).accessibleObject()).getName() + prettyParams(msTypes));
+                    }
                 }
             }
         }
diff --git a/core/src/main/java/org/jruby/util/cli/Options.java b/core/src/main/java/org/jruby/util/cli/Options.java
index 873775270a..65bee12644 100644
--- a/core/src/main/java/org/jruby/util/cli/Options.java
+++ b/core/src/main/java/org/jruby/util/cli/Options.java
@@ -188,6 +188,7 @@ public class Options {
     public static final Option<String> LOGGER_CLASS = string(DEBUG, "logger.class", new String[]{"class name"}, "org.jruby.util.log.StandardErrorLogger", "Use specified class for logging.");
     public static final Option<Boolean> DUMP_INSTANCE_VARS = bool(DEBUG, "dump.variables", false, "Dump class + instance var names on first new of Object subclasses.");
     public static final Option<Boolean> REWRITE_JAVA_TRACE = bool(DEBUG, "rewrite.java.trace", true, "Rewrite stack traces from exceptions raised in Java calls.");
+    public static final Option<Boolean> DEBUG_AMBIGUOUS_JAVA_CALLS = bool(DEBUG, "debug.ambiguous.java.calls", false, "Toggle verbose reporting of all ambiguous calls to Java objects");
 
     // TODO: Replace flag that's false on 9 with proper module checks
     public static final Option<Boolean> JI_SETACCESSIBLE = bool(JAVA_INTEGRATION, "ji.setAccessible", calculateSetAccessibleDefault(), "Try to set inaccessible Java methods to be accessible.");
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Mar 1, 2018

Member

@headius like it since its very minimal.
one minor improvement is that maybe we shall have this Java Integration related switch under the jruby.ji prefix e.g. -Xji.ambiguous.debug -Xji.ambiguous.backtrace or something similar

Member

kares commented Mar 1, 2018

@headius like it since its very minimal.
one minor improvement is that maybe we shall have this Java Integration related switch under the jruby.ji prefix e.g. -Xji.ambiguous.debug -Xji.ambiguous.backtrace or something similar

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 19, 2018

Member

I will move the property under the ji namespace as ji.ambiguous.call.error and actually raise the exception.

Member

headius commented Mar 19, 2018

I will move the property under the ji namespace as ji.ambiguous.call.error and actually raise the exception.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 19, 2018

Member

Sorry, I remembered why I didn't want a hard error...it makes debugging all such sites very tedious. So it will be ji.ambiguous.calls.debug.

Member

headius commented Mar 19, 2018

Sorry, I remembered why I didn't want a hard error...it makes debugging all such sites very tedious. So it will be ji.ambiguous.calls.debug.

headius added a commit that referenced this issue Mar 19, 2018

@headius headius added this to the JRuby 9.1.17.0 milestone Mar 19, 2018

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 19, 2018

Member

@thbar Can you put together a simple test under test/jruby that tests this property's output?

Member

headius commented Mar 19, 2018

@thbar Can you put together a simple test under test/jruby that tests this property's output?

@headius headius closed this Mar 19, 2018

headius added a commit that referenced this issue Mar 19, 2018

@boris-petrov

This comment has been minimized.

Show comment
Hide comment
@boris-petrov

boris-petrov Mar 20, 2018

@headius - thanks for implementing this! Actually, I'm also in favor of throwing a hard error as I want things like that to fail my build (we're using Capybara with JRuby). Perhaps you could add a second option that actually throws the exception? It will be configurable so that's the best way I think. Thanks again!

boris-petrov commented Mar 20, 2018

@headius - thanks for implementing this! Actually, I'm also in favor of throwing a hard error as I want things like that to fail my build (we're using Capybara with JRuby). Perhaps you could add a second option that actually throws the exception? It will be configurable so that's the best way I think. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment