Skip to content
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

checkArity confusingly raises line of outer caller for nested calls #5103

Open
andrewvc opened this issue Mar 22, 2018 · 6 comments
Open

checkArity confusingly raises line of outer caller for nested calls #5103

andrewvc opened this issue Mar 22, 2018 · 6 comments
Milestone

Comments

@andrewvc
Copy link

Environment

Provide at least:

  • JRuby version: 9.1.13.0
  • Mac OSX

Expected Behavior

I have code something like:

#Java interface
interface MyJavaInterface {
  void interfaceMethod(argItTakes)
}

#Ruby
class MyClass
  include org.example.MyJavaInterface

  # Note that we accidentally specify an extra arg here
  def interfaceMethod(argItTakes, extraArgThatIsAccident)
  end

  def someOtherMethod
    callJavaMethodThatInvokesAboveInterfaceMethodButWithOnlyOneArg
  end
end

Actual Behavior

JRuby generates an ArgumentError: wrong number of arguments (1 for 2) that points to the line in someOtherMethod, when the real error is that we implemented the interface incorrectly. The stack trace shouldn't filter out the java in this case, but do the full stack trace to expose this.

@kares
Copy link
Member

kares commented Mar 22, 2018

basically boils down to having some-form of static Java checks in a dynamic language (Ruby).
at the time you include the interface its methods aren't implemented (and might actually never be - that is fine in a dynamic language). when the proxy is generated there might be some room for args checking and maybe issuing a warning, that might be useful to some extend - also slightly tricky to get right since in some cases dispatch reaches method_missing and you might not have all of the methods implemented and if you do there's no way knowing which will be actually used (called) so we might warn too much ...

as for the stack-trace could you please clarify what you mean.
doubt JRuby wants to switch trace type based on what calls triggered the issue.

@andrewvc
Copy link
Author

@kares yes, this does seem hard to get right. The thing is, it is a bad user experience at the moment because JRuby quite confidently tells you you have an args issue at a given line, when the problem is really somewhere else.

I don't know if there is an efficient easy way to solve this. It would be nice if possible however.

The current stack, roughly speaking, is RrubyMethod(A) -> JavaMethod(B) -> RubyMethodThatIncorrectlyImplementsJavaInterface(C).

The error message today makes it seem like A is where the bug is, when the real problem is somewhere between B and C

@kares kares added this to the Won't Fix milestone Mar 26, 2018
@headius
Copy link
Member

headius commented Mar 26, 2018

I feel like the real problem here is that we have no context for the failed call, because the dispatch logic for the Ruby-based interface impl doesn't insert any context. It's a sort of hidden method_missing dispatch happening under the covers, but since it's implemented in "native" code it doesn't show up in the Ruby trace.

The dispatch logic here could perhaps be improved to provide a better error, perhaps something like "interface method SomeIfc.someMethod was not implemented in class SomeRubyClass".

@andrewvc
Copy link
Author

@headius that error message would have been amazing here :)

@headius
Copy link
Member

headius commented Mar 26, 2018

It occurs to me there's a slight snag with my approach: we still need method_missing to work properly. So...

  1. We can't rescue every dispatch-related error here and rewrite them, nor would we want to in case they're part of someone's (weird) API.
  2. We can't do dispatch much differently since we need to attempt method_missing, which then leads back to 1.

What we might be able to do is create a new call site type specifically for interface dispatches that follows method_missing protocol:

  • Use respond_to? and respond_to_missing? to check for the existence of an appropriate method target. Dispatch if found. Name overloading will potentially mean multiple searches here, so it would be worth getting invokedynamic involved.
  • If those fail, check for a user-defined method_missing. Dispatch if found, and then it's out of our hands.
  • If there's no user-defined method_missing, we use our own version that reports a better error but still raises the same exception type.

Tricky but doable.

@headius
Copy link
Member

headius commented Jul 18, 2020

A similar issue is documented in #6335. The case here is slightly different, since we are simply not defining the expected interface method on the generated class, but the basic cause (arity check happens during call plumbing and not inside the method) is the same.

headius added a commit to headius/jruby that referenced this issue Aug 5, 2021
Jitted code handles arity-checking differently than in the
interpreter. Where the interpreter always pushed a backtrace
element before the arity check, the JVM does the arity-check in a
separately-generated varargs wrapper (if the method is specific-
arity) or before setting a line number (if the method is variable-
arity). This has the effect of either omitting the error frame
altogether (specific) or having the line number be -1 (variable).

This commit makes the following modifications to allow the arity
error line to be shown in jitted code:

* Set the current line number immediately upon creating the method
  so that it is set in the JVM before the arity check.
* Process jitted method names with the varargs marker if they were
  not preceded by the actual method body. The varargs-marked
  method can only appear on its own or immediately after the real
  jitted method body.

This fixes all known cases where the argument error line is
omitted or not set properly in the JVM trace.

In addition, this patch moves the management of the current line
number into the IRBytecodeAdapter, rather than maintaining it in
the JVMVisitor instance.

Fixes jruby#6335.

This may also improve the case described in jruby#5103, since all JVM
frames will have proper line numbers at the point of error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants