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

bind (abstract) methods when implementing interface #3809

Merged
merged 3 commits into from Apr 27, 2016

Conversation

Projects
None yet
3 participants
@kares
Member

kares commented Apr 19, 2016

there are two ways Ruby interface implementations are being generated :

  • explicitly using Iface.impl { |name, *args| }
  • or implicitly when a block is provided for a (commonly functional) inteface last argument (proc-to-iface)

both of these add method_missing to the Ruby class which leads to issues when there's a naming clash in the hierarchy (e.g. Kernel#test with java.util.function.Predicate#test). these are resolved by binding implemented interface methods on the implementing Ruby class - always executing the desired user provided bits (except where he would ImplClass.class_eval { def test ... } the method of course).

on the Java (codegen) side, under Java 8, this further revealed that static methods were unnecessarily being generated and that interface's default methods are always being overridden ...

  1. for Iface.impl the behaviour of implementing all methods (including defaults) was kept.
    impl(:foo, :bar) already allows for implementing method names to be specified and an additional impl(false) to generate just the minimum abstract method(s) required is introduced. new code also warns on impl(:foo) when "foo" is not actually an interface method.
  2. for proc-to-iface conversion overriding everything (including default) makes no sense esp. since its mostly used with functional interfaces. so this was changed and allows for slightly advanced concepts with Java 8 (e.g. java.util.function.Function#compose) to work correctly.

all-in-all these changes should be welcoming by JI users - it might break compatibility in very corner cases but its usually for the best to review those.

resolves #3475

kares added some commits Apr 15, 2016

[ji] fix potential interface method conflicts from Ruby hierarchy on …
…MyIface.impl

by having all iface required methods setup in the implementation Ruby class to always dispatch to impl logic. 

also improves performance due dispatching directly to method_missing
[ji] only override abstract iface methods (by default on proc-impl) u…
…nless present

we're keeping Iface.impl backwards compatible to override all instance methods by 
default (unless methods are specified or we introduced impl(false) to keep defaults)

for proc-to-iface (functionak) impls implementing default methods makes no sense
... use cases such as Function#compose with a Ruby impl did not work previously
[ji] at last - deal with Ruby - Java method conflicts with interface …
…impl using a block

we're now add an internal "impl" method for each prescribed abstract interface method

this is expected to resolve conflicting issues (e.g using Java 8 streams) such as #3475

@kares kares added this to the JRuby 9.1.0.0 milestone Apr 19, 2016

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Apr 20, 2016

Member

@kares can you think of any breakage this will cause? Your last sentence implies there might be breakage but do you actually know of any?

Member

enebo commented Apr 20, 2016

@kares can you think of any breakage this will cause? Your last sentence implies there might be breakage but do you actually know of any?

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Apr 20, 2016

Member

@enebo only thing I can think of if users realize that these use method_missing and they would re-def it. although it does make very little sense to do so. we can manage if there's complains but doubt there will be.

Member

kares commented Apr 20, 2016

@enebo only thing I can think of if users realize that these use method_missing and they would re-def it. although it does make very little sense to do so. we can manage if there's complains but doubt there will be.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Apr 20, 2016

Member

@kares if someone might realize we used method_missing as our implementation and then monkeypatch it to change behavior I am ok with that risk.

@headius what do you think? Can you think of any risks here?

Member

enebo commented Apr 20, 2016

@kares if someone might realize we used method_missing as our implementation and then monkeypatch it to change behavior I am ok with that risk.

@headius what do you think? Can you think of any risks here?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 27, 2016

Member

Ok, this one seems fine to me and I think the impl is right.

Member

headius commented Apr 27, 2016

Ok, this one seems fine to me and I think the impl is right.

@kares kares merged commit e097afb into master Apr 27, 2016

0 of 3 checks passed

continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@kares kares deleted the ji-iface branch Apr 27, 2016

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