Workaround JRuby incompatibilities #345

grddev · 2018-03-01T15:51:51Z

JRuby apparently only supports #arity (and not #parameters) for Java proxy methods (see jruby/jruby#2817), but verifying doubles in RSpec use #parameters to infer required method parameters, and thus doesn't work for Java classes.

This extends the previously existing JRuby workaround that returns an empty list from #parameters so that it fallbacks to the legacy method of using #arity.

In addition, it turns out that the #arity method is broken for Java proxy methods in JRuby 1.7.x. While JRuby 1.7 has been sunset, the test suite still uses it, and thus this introduces another workaround to account for that.

This looks up the corresponding JavaMethod overloads, and if there is only one, retrieves the arity from that one instead. In the presence of multiple overloads, -1 is returned.

While this obviously doesn't cover all cases, it does cover the cases the previous workaround covered, and also allows using verifying doubles for many Java classes in JRuby.

myronmarston

Thanks @grddev! I left a few comments.

myronmarston · 2018-03-03T05:20:02Z

lib/rspec/support/method_signature_verifier.rb

+            else
+              @min_non_kw_args = arity
+              @max_non_kw_args = arity
+            end


These 6 lines (from if arity < 0 to the end) show up 3 times in this file. Can you extract a helper method (something like classify_arity(arity)) for this code so it's duplicated?

myronmarston · 2018-03-03T05:21:00Z

lib/rspec/support/method_signature_verifier.rb


+    if RSpec::Support::Ruby.jruby? &&


Instead of starting a 2nd conditional that starts with if RSpec::Support::Ruby.jruby?, would you mind doing one big if RSpec::Support::Ruby.jruby? conditional (with 2 nested conditionals as needed).

Actually, is it possible to combine the two Jruby cases into one class, with some extra conditional checked when classify_parameters is called? It's confusing (and will be harder to maintain) to have two different alternate JRuby implementations.

The motivation for having them separate is that the latter one cannot use #parameters, but it could obviously be implemented by checking support for #parameters at runtime, if you prefer such a solution.

myronmarston · 2018-03-03T05:22:57Z

lib/rspec/support/method_signature_verifier.rb

    if RSpec::Support::Ruby.jruby? &&
       RubyFeatures.optional_and_splat_args_supported? &&
-       Class.new { attr_writer :foo }.instance_method(:foo=).parameters == []
+       Java::JavaLang::String.instance_method(:char_at).parameters == []


I'm trying to understand why you changed this conditional, and how this changes things. Does Class.new { attr_writer :foo }.instance_method(:foo=).parameters == [] return the same value on all versions of JRuby that Java::JavaLang::String.instance_method(:char_at).parameters == [] does?

As should hopefully be clear by the table in my overall comment, there are no versions where the attr_writer bug exists, but the Java proxy method one does not.

myronmarston · 2018-03-03T05:28:19Z

lib/rspec/support/method_signature_verifier.rb

+        end
+
+        def java_method_arity
+          if @method.owner.respond_to?(:java_class)


I'd prefer to use an early-exit guard, like so:

return unless @method.owner.respond_to?(:java_class)

myronmarston · 2018-03-03T05:28:40Z

lib/rspec/support/method_signature_verifier.rb

+          end
+        end
+
+        def java_method_arity


I don't totally get what you're doing in this method and why. Mind leaving a comment explaining it?

I explained it the following way in the new commit message, but would you prefer a comment in the code?

The workaround essentially makes use of Java's introspection to figure out matching methods (which could be more than one partly because Java supports multiple overloads, and partly because JRuby introduces aliases to make method names look more Rubyesque). If there is only a single match, we can use that methods arity directly instead of the default -1 arity.

JRuby apparently only supports #arity (and not #parameters) for Java proxy methods (see jruby/jruby#2817), but verifying doubles in RSpec use #parameters to infer required method parameters, and thus doesn't work for Java classes. Given that this issue affects every release of JRuby so far with support for #parameters, it seemed reasonable to replace the previous workaround that only affected JRuby 1.7.0 through 1.7.19.

JRuby apparently didn't even properly support #arity for Java proxy methods before 9.0.3.0 (see jruby/jruby@013e288), so this implements a workaround for these versions. The workaround essentially makes use of Java's introspection to figure out matching methods (which could be more than one partly because Java supports multiple overloads, and partly because JRuby introduces aliases to make method names look more Rubyesque). If there is only a single match, we can use that methods arity directly instead of the default -1 arity. One could in theory do better than -1 for various overload sets with multiple methods in them as well, but we have to draw a line somewhere. Given that this issue also impacts JRuby 1.7.x in 1.8-mode, it would require guards around all the #parameters calls to integrate it into the existing workaround, and thus it is implemented as a separate workaround.

grddev · 2018-03-04T14:24:13Z

Thanks for the review @myronmarston. I think I could have done a better job explaining the problem in the PR description, so let me make another attempt. I wanted to use an instance double for a Java proxy in JRuby. For those not familiar with JRuby, you can freely interface with any standard Java class through an automatically generated proxy. Briefly, you can write Java::JavaLang::String in JRuby to access a Ruby-like proxy for the Java class java.lang.String, and you can use #instance_method(:char_at) to look up the Java method java.lang.String#charAt(int).

However, the implementation of method signature information for such proxies are not stellar. Until 9.0.3.0 (and jruby/jruby@013e288), Method#arity would always return -1 for these methods, and Method#parameters still returns an empty list independent of the underlying method(s), for all JRuby versions released so far. The following table summarizes the quality of the method signature helpers for different JRuby versions and command-line flags.

JRuby version	Flags	Method#arity	Method#parameters
1.7.0 - 1.7.27	--1.8	Java proxy bug	Not supported
1.7.0 - 1.7.19	--1.9,--2.0	Java proxy bug	Java proxy bug, attr_writer bug
1.7.20 - 1.7.27	--1.9,--2.0	Java proxy bug	Java proxy bug
9.0.0.0 - 9.0.1.0		Java proxy bug	Java proxy bug
9.0.3.0 - 9.1.15.0			Java proxy bug

Given that I was on JRuby 9.1.13.0, which has a working Method#arity implementation, I first implemented a workaround for that, but when running the tests I found out that Method#arity wouldn't help in a bunch of cases, but given that the #arity workaround would have to work in 1.8 mode (without parameters), I decided to implement that workaround completely separately. In order to clarify that, I split the two workarounds into different commits with better explanations.

myronmarston · 2018-03-08T06:28:13Z

Thanks @grddev!

JonRowe · 2018-03-11T22:35:52Z

lib/rspec/support/method_signature_verifier.rb

+
+          def classify_parameters
+            super
+            return unless @method.arity == -1 && @method.owner.respond_to?(:java_class)


I believe this is breaking the build for rspec/rspec-expectations, see https://travis-ci.org/rspec/rspec-expectations/jobs/351862257

Ah, I see now that the method is supposed to work for Proc as well as Method, and #owner isn't available for Proc. The easiest way to handle this would simply to add @method.respond_to?(:owner) to the referenced early return. This will effectively disable the workaround for Proc, while retaining it for Method.

While the same problem exists for Proc, and you could observe it using the following construction,

Java::JavaLang::String.new.method(:char_at).to_proc.arity # => -1

I can't think of a workaround that would address this case, as I'm not aware of any JRuby API that allows you to figure out if a Proc is a direct wrapper around a Java method.

Could we add the proc fix for now, and add a pending spec demonstrating the problem for the scenario described, this will document our knowledge on this

Opened a PR with the proposed fix in #347

myronmarston suggested changes Mar 3, 2018

View reviewed changes

grddev added 2 commits March 4, 2018 14:29

grddev force-pushed the jruby-java-method-signature-verifier branch from a232b2b to ccdce45 Compare March 4, 2018 14:12

Extract classify_arity method to avoid duplication

02a2193

grddev force-pushed the jruby-java-method-signature-verifier branch from ccdce45 to 02a2193 Compare March 4, 2018 14:30

myronmarston approved these changes Mar 8, 2018

View reviewed changes

myronmarston merged commit 9ab1620 into rspec:master Mar 8, 2018

JonRowe reviewed Mar 11, 2018

View reviewed changes

Uh oh!

Workaround JRuby incompatibilities #345

Workaround JRuby incompatibilities #345

Uh oh!

Conversation

grddev commented Mar 1, 2018

Uh oh!

myronmarston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grddev commented Mar 4, 2018

Uh oh!

myronmarston commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!