Bug 493965

Comment 1 Jay Arthanareeswaran 2016-05-19 01:09:45 EDT

Sasi please take a look.

Comment 2 Stephan Herrmann 2016-05-19 17:10:18 EDT

Good catch. From a quick glance this situation has been created by the fix for bug 395002
Please see that prior to that change the previous if:
  if (otherReferenceType.isInterface())...
would always return, so below that point it was safe to assume !otherReferenceType.isInterface().

Now, that there are ways to drop out of the then-block without returning, we should probably change the "if" in question into an "else if", or add "return false" as the last thing into the previous then-block to get closer to the previous behavior.
RunAllJava8Tests is happy with either change.

But, does anyone have a test case where any of this makes a difference??

BTW, bug 395002 was fixed in 4.3M4, so this is not a recent regression (if any).

Comment 3 Raffi Khatchadourian 2016-05-20 15:52:28 EDT

(In reply to comment #2)
> ...
> But, does anyone have a test case where any of this makes a difference??

Yes, when the implicit parameter is a binding representing an interface.

Comment 4 Stephan Herrmann 2016-05-20 16:07:04 EDT

(In reply to Raffi Khatchadourian from comment #3)
> (In reply to comment #2)
> > ...
> > But, does anyone have a test case where any of this makes a difference??
> 
> Yes, when the implicit parameter is a binding representing an interface.

I don't understand, which implicit parameter?  Please attach an example.
Note, we need more than one binding, we need the context to actually pass through all the checks in on a very specific flow path. I'm not convinced that this path is actually possible at runtime.

In other words, we need an example where compilation produces a wrong result due to the current implementation.

Comment 5 Raffi Khatchadourian 2016-05-24 10:18:23 EDT

(In reply to Stephan Herrmann from comment #4)
> (In reply to Raffi Khatchadourian from comment #3)
> > (In reply to comment #2)
> > > ...
> > > But, does anyone have a test case where any of this makes a difference??
> > 
> > Yes, when the implicit parameter is a binding representing an interface.
> 
> I don't understand, which implicit parameter?  Please attach an example.
> Note, we need more than one binding, we need the context to actually pass
> through all the checks in on a very specific flow path. I'm not convinced
> that this path is actually possible at runtime.
> 
> In other words, we need an example where compilation produces a wrong result
> due to the current implementation.

I don't have an example where compilation fails, but this method is called in the path of org.eclipse.jdt.core.dom.ITypeBinding.isAssignmentCompatible(ITypeBinding), which is a public API. This bug will appear when both the implicit and explicit arguments to the above method call are bindings that represent interfaces. For example:

boolean m(ITypeBinding typeBinding, ITypeBinding otherTypeBinding) {
    //suppose typeBinding and otherTypeBinding represent interface
    //bindings and that typeBinding is assignment compatible with otherTypeBinding.
    return typeBinding.isAssignmentCompatible(otherTypeBinding);
    //this will incorrectly return false.
}

Comment 6 Till Brychcy 2016-05-24 10:46:11 EDT

(In reply to Raffi Khatchadourian from comment #5)
> I don't have an example where compilation fails, but this method is called
> in the path of
> org.eclipse.jdt.core.dom.ITypeBinding.isAssignmentCompatible(ITypeBinding),
> which is a public API. This bug will appear when both the implicit and
> explicit arguments to the above method call are bindings that represent
> interfaces. For example:
> 
> boolean m(ITypeBinding typeBinding, ITypeBinding otherTypeBinding) {
>     //suppose typeBinding and otherTypeBinding represent interface
>     //bindings and that typeBinding is assignment compatible with
> otherTypeBinding.
>     return typeBinding.isAssignmentCompatible(otherTypeBinding);
>     //this will incorrectly return false.
> }

Shouldn't this be handled by
			if (otherReferenceType.isInterface()) { // could be annotation type
				if (implementsInterface(otherReferenceType, true))
					return true;

Comment 7 Stephan Herrmann 2016-05-24 10:49:21 EDT

(In reply to Till Brychcy from comment #6)
> (In reply to Raffi Khatchadourian from comment #5)
> > boolean m(ITypeBinding typeBinding, ITypeBinding otherTypeBinding) {
> >     //suppose typeBinding and otherTypeBinding represent interface
> >     //bindings and that typeBinding is assignment compatible with
> > otherTypeBinding.
> >     return typeBinding.isAssignmentCompatible(otherTypeBinding);
> >     //this will incorrectly return false.
> > }
> 
> Shouldn't this be handled by
> 			if (otherReferenceType.isInterface()) { // could be annotation type
> 				if (implementsInterface(otherReferenceType, true))
> 					return true;

Exactly.

(In reply to Raffi Khatchadourian from comment #5)
> I don't have an example where compilation fails

In that case this bug has low priority to me, since I'm not convinced that it is practically relevant.

I'm quite sure if your argumentation were right, we would have hundreds of failures in our test suite, at least. It needs a corner case to make a difference, if any.