Ambiguous method warning when there does not appear to be any ambiguity #2865

Closed
trejkaz opened this Issue Apr 22, 2015 · 9 comments

Comments

Projects
None yet
3 participants
@trejkaz

trejkaz commented Apr 22, 2015

I have two methods with the following signatures:

    SourceItem openFile(String file, Map<?, ?> settings) throws FileNotFoundException;
    void openFile(String file, Consumer<SourceItem> itemConsumer) throws FileNotFoundException;

When I try to call the second one:

factory.open_file('path/to/file') do |item|
  item.text
end

I get a warning:

.../rspec_helpers.rb:93 warning: ambiguous Java methods found, using openFile(java.util.String, java.util.Map)

I don't think there is any real ambiguity here. I can't implement Map using a block, so it should always call openFile(String, Consumer).

Reproduced on 1.7.18, 1.7.19, 9.0.0.0.pre1.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 29, 2015

Member

Actually, you can implement any interface with a block, so this is a valid message. It's just not particularly useful when the interface has many different and incompatible methods.

However, I have thought that perhaps cases like this should only coerce if there's exactly one abstract method on the likely interface. It's not something we've done formally, though.

As a workaround in the short term you can select the method you want using java_send, java_method, or java_alias. Note that when specifying interface types, you currently need to add .java_class:

class my::pkg::MyClass
  java_alias :open_file_callable, :openFile, [java.lang.String, Java::your::package::Consumer.java_class])
end

The syntax for java_method and java_send is similar.

@enebo How dangerous to compatiblity might it be if we modified Java method lookup to only coerce procs when the target interface has one abstract method? It would narrow down some ambiguous cases like this.

Member

headius commented Apr 29, 2015

Actually, you can implement any interface with a block, so this is a valid message. It's just not particularly useful when the interface has many different and incompatible methods.

However, I have thought that perhaps cases like this should only coerce if there's exactly one abstract method on the likely interface. It's not something we've done formally, though.

As a workaround in the short term you can select the method you want using java_send, java_method, or java_alias. Note that when specifying interface types, you currently need to add .java_class:

class my::pkg::MyClass
  java_alias :open_file_callable, :openFile, [java.lang.String, Java::your::package::Consumer.java_class])
end

The syntax for java_method and java_send is similar.

@enebo How dangerous to compatiblity might it be if we modified Java method lookup to only coerce procs when the target interface has one abstract method? It would narrow down some ambiguous cases like this.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 29, 2015

Member

Pardon my inconsistent use of . and :: in Java class names. The :: form is necessary in a class statement because . is disallowed there.

Member

headius commented Apr 29, 2015

Pardon my inconsistent use of . and :: in Java class names. The :: form is necessary in a class statement because . is disallowed there.

@trejkaz

This comment has been minimized.

Show comment
Hide comment
@trejkaz

trejkaz Apr 29, 2015

Since I control the interface in this instance I could probably bump the method name on the Java side to get around it I guess... I was just trying to keep the interface as clean as possible. I also wondered whether there were an annotation to explicitly mark the methods intended to be used with blocks...

I see what you're saying though I think... the intent is to support implementing multiple overloads all going to the same block, I gather... somehow I assumed it wasn't possible to implement multiple methods with a single block.

It would be enough for me if the existence of a single abstract-method were used as a tiebreaker in this situation even if multi-method interfaces still worked in other more general situations. But I haven't taken a close look at how this part of type coercion works, so I'll let the experts decide. :)

trejkaz commented Apr 29, 2015

Since I control the interface in this instance I could probably bump the method name on the Java side to get around it I guess... I was just trying to keep the interface as clean as possible. I also wondered whether there were an annotation to explicitly mark the methods intended to be used with blocks...

I see what you're saying though I think... the intent is to support implementing multiple overloads all going to the same block, I gather... somehow I assumed it wasn't possible to implement multiple methods with a single block.

It would be enough for me if the existence of a single abstract-method were used as a tiebreaker in this situation even if multi-method interfaces still worked in other more general situations. But I haven't taken a close look at how this part of type coercion works, so I'll let the experts decide. :)

@trejkaz

This comment has been minimized.

Show comment
Hide comment
@trejkaz

trejkaz May 8, 2015

The minimal fix for 1.7 is something like this.

The method I modified for 1.7 doesn't seem to exist anymore on master, though, which complicates things.

Edit: Okay, I see, my change depends on commit c1a174f which was part of issue #2595, which is committed to 1.7 but not master. This makes sense, since both are sort of similar, looking at the argument type and using that to disambiguate the parameter lists.

trejkaz commented May 8, 2015

The minimal fix for 1.7 is something like this.

The method I modified for 1.7 doesn't seem to exist anymore on master, though, which complicates things.

Edit: Okay, I see, my change depends on commit c1a174f which was part of issue #2595, which is committed to 1.7 but not master. This makes sense, since both are sort of similar, looking at the argument type and using that to disambiguate the parameter lists.

@trejkaz

This comment has been minimized.

Show comment
Hide comment
@trejkaz

trejkaz Jun 16, 2015

Well, tested my change on 9.0.0.0.rc1 and it doesn't work. But not only that, I looked at the code on 1.7.20 and couldn't figure out how it was working there either. So clearly it was the wrong way to do it.

But now my attention is on this bit of CallableSelector:

            // somehow we can still decide e.g. if we got a RubyFixnum
            // then (int) constructor shoudl be preffered over (float)
            if ( ambiguous ) {
                int msPref = 0, cPref = 0;
                for ( int i = 0; i < msTypes.length; i++ ) {
                    final Class<?> msType = msTypes[i], cType = cTypes[i];
                    msPref += calcTypePreference(msType, args[i]);
                    cPref += calcTypePreference(cType, args[i]);
                }
                // for backwards compatibility we do not switch to cType as
                // the better fit - we seem to lack tests on this front ...
                if ( msPref > cPref ) ambiguous = false; // continue OUTER;     // <-- ***
            }

Here it seems to have computed that the candidate preference cPref is "not worse" than the most-specific preference msPref. (I won't imply "better" because the comparison doesn't.) But then the code deliberately (according to the comment) doesn't replace mostSpecific. I haven't tested whether replacing it makes it work, but that's interesting nonetheless as it seems to mean there is no way to break the tie by the time the code gets this far.

So is the correct way to do this to define a new Matcher in the sequence between ASSIGNABLE and DUCKABLE, which matches RubyProc against a functional interface?

trejkaz commented Jun 16, 2015

Well, tested my change on 9.0.0.0.rc1 and it doesn't work. But not only that, I looked at the code on 1.7.20 and couldn't figure out how it was working there either. So clearly it was the wrong way to do it.

But now my attention is on this bit of CallableSelector:

            // somehow we can still decide e.g. if we got a RubyFixnum
            // then (int) constructor shoudl be preffered over (float)
            if ( ambiguous ) {
                int msPref = 0, cPref = 0;
                for ( int i = 0; i < msTypes.length; i++ ) {
                    final Class<?> msType = msTypes[i], cType = cTypes[i];
                    msPref += calcTypePreference(msType, args[i]);
                    cPref += calcTypePreference(cType, args[i]);
                }
                // for backwards compatibility we do not switch to cType as
                // the better fit - we seem to lack tests on this front ...
                if ( msPref > cPref ) ambiguous = false; // continue OUTER;     // <-- ***
            }

Here it seems to have computed that the candidate preference cPref is "not worse" than the most-specific preference msPref. (I won't imply "better" because the comparison doesn't.) But then the code deliberately (according to the comment) doesn't replace mostSpecific. I haven't tested whether replacing it makes it work, but that's interesting nonetheless as it seems to mean there is no way to break the tie by the time the code gets this far.

So is the correct way to do this to define a new Matcher in the sequence between ASSIGNABLE and DUCKABLE, which matches RubyProc against a functional interface?

trejkaz added a commit to trejkaz/jruby that referenced this issue Jun 24, 2015

Prefer method taking RubyProc when a parameter is a functional interf…
…ace.

Experimental change intended to fix issue #2865.
@trejkaz

This comment has been minimized.

Show comment
Hide comment

trejkaz commented Jun 24, 2015

Attempt 2, trejkaz@8ef983d

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 21, 2015

Member

this is actually more of a "true" bug than it seems, I hoped its already fixed but its not, my notes:

  • method selector (was) "non-deterministic" in cases such as #2923
  • hoped that changes due #3136 handle these cases as well
  • ... just like they kind of did fix #3263

what I'm seeing here is still non-determinism for the proc matching case (result depends on SDK's class.getMethods return order) which is why I believe it should be fixed on jruby-1_7 as well ... was actually able to get the map version to match on a few runs. will try to come up with a "deterministic" fix.

side note: we already have a functional-interface checking mechanism:

public static Method getFunctionalInterfaceMethod(final Class<?> iface) {
assert iface.isInterface();
Method single = null;
for ( final Method method : iface.getMethods() ) {
final int mod = method.getModifiers();
if ( Modifier.isStatic(mod) ) continue;
if ( Modifier.isAbstract(mod) ) {
try { // check if it's equals, hashCode etc. :
Object.class.getMethod(method.getName(), method.getParameterTypes());
continue; // abstract but implemented by java.lang.Object
}
catch (NoSuchMethodException e) { /* fall-thorough */ }
catch (SecurityException e) {
// NOTE: we could try check for FunctionalInterface on Java 8
}
}
else continue; // not-abstract ... default method
if ( single == null ) single = method;
else return null; // not a functional iface
}
return single;
}

Member

kares commented Aug 21, 2015

this is actually more of a "true" bug than it seems, I hoped its already fixed but its not, my notes:

  • method selector (was) "non-deterministic" in cases such as #2923
  • hoped that changes due #3136 handle these cases as well
  • ... just like they kind of did fix #3263

what I'm seeing here is still non-determinism for the proc matching case (result depends on SDK's class.getMethods return order) which is why I believe it should be fixed on jruby-1_7 as well ... was actually able to get the map version to match on a few runs. will try to come up with a "deterministic" fix.

side note: we already have a functional-interface checking mechanism:

public static Method getFunctionalInterfaceMethod(final Class<?> iface) {
assert iface.isInterface();
Method single = null;
for ( final Method method : iface.getMethods() ) {
final int mod = method.getModifiers();
if ( Modifier.isStatic(mod) ) continue;
if ( Modifier.isAbstract(mod) ) {
try { // check if it's equals, hashCode etc. :
Object.class.getMethod(method.getName(), method.getParameterTypes());
continue; // abstract but implemented by java.lang.Object
}
catch (NoSuchMethodException e) { /* fall-thorough */ }
catch (SecurityException e) {
// NOTE: we could try check for FunctionalInterface on Java 8
}
}
else continue; // not-abstract ... default method
if ( single == null ) single = method;
else return null; // not a functional iface
}
return single;
}

@kares kares self-assigned this Aug 21, 2015

kares added a commit to kares/jruby that referenced this issue Aug 23, 2015

[ji] improved more-specific type matching when matching Java methods
will now account for all parameter types in a method signature and takes
last arg proc matching into account - outcomes should be more predictable

as noted (jruby#2865 (comment)) the selection has been still subject to non-deterministic behavior (depending on reflected method orded) esp. for cases where last argument is a proc to be matched as an interface impl

fixes #2865

@kares kares closed this in b45f4fe Aug 25, 2015

@kares kares added this to the JRuby 1.7.23 milestone Aug 25, 2015

@trejkaz

This comment has been minimized.

Show comment
Hide comment
@trejkaz

trejkaz Mar 14, 2016

I'm rediscovering this issue now on 9.0.5.0 but I'm not sure whether it's because this time I am passing an RSpec double, whereas maybe if I passed a block it would be 100% reliable.

trejkaz commented Mar 14, 2016

I'm rediscovering this issue now on 9.0.5.0 but I'm not sure whether it's because this time I am passing an RSpec double, whereas maybe if I passed a block it would be 100% reliable.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 14, 2016

Member

@trejkaz it's definitely possible that the RSpec double looks ambiguous to the JI subsystem. Running on a newer version of Java can sometimes cause this to come up due to overloads and new methods added to the core Java types. If you can narrow it down to a specific reportable oddity, please open a new issue and we'll help you look into it.

Member

headius commented Mar 14, 2016

@trejkaz it's definitely possible that the RSpec double looks ambiguous to the JI subsystem. Running on a newer version of Java can sometimes cause this to come up due to overloads and new methods added to the core Java types. If you can narrow it down to a specific reportable oddity, please open a new issue and we'll help you look into it.

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