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

bind (abstract) methods when implementing interface #3809

Merged
merged 3 commits into from
Apr 27, 2016
Merged

bind (abstract) methods when implementing interface #3809

merged 3 commits into from
Apr 27, 2016

Conversation

kares
Copy link
Member

@kares 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 3 commits April 18, 2016 19:10
…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
…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
…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
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
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
@kares kares deleted the ji-iface branch April 27, 2016 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proc-to-interface dispatch method collision using Java 8 streams
3 participants