Is there a way to fail-fast on ambiguous arguments? #4894
Comments
|
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:
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 |
|
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. |
|
the warning you're seeing comes from Java Integration, this does not apply (just for potential readers) :
#4759 is just "non-fixable" but your case is slightly different, since its not proc-to-iface propagation ... |
|
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."); |
|
@headius like it since its very minimal. |
|
I will move the property under the |
|
Sorry, I remembered why I didn't want a hard error...it makes debugging all such sites very tedious. So it will be |
|
@thbar Can you put together a simple test under |
|
@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! |
Environment
jruby 9.1.14.0Context
Currently one will get the following warning in the logs when ambiguous calls are met by the lexer:
This is provided thanks to
parser.warn.ambigous_argument, which has a greattruedefault 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:
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
NullPointerExceptionduring 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!
The text was updated successfully, but these errors were encountered: