Coverage remained the same when pulling 01351d8 on alberskib:master into c17169c on mockito:master.

bump: Please review changes
In case of any questions or doubts I will provide explanation

Interesting, I'll need to check a bit more your changes when I will have some free time to code, and see if anything is broken. Also did you tried on your build farm to see if it's compiling well?

Also could you add more description in the PR body, like why you did and how it solves the problem, it'll avoid a roundtrip on the following issues 434 and 212.

However I wouldn't yet remove the bug tests, as they should now pass, right ?

And most importantly thanks for the investigation and patch.

I also removed report_why_this_exception_happen test in ParentClassNotPublicVeryWeirdBugTest. I believe that this test is added in order to inform developer when he (accidentally) fix this bug.
Please compare this test to the is_valid_mocked in the same class.

It was great pleasure for me to work on this issue as I learnt a lot of. I would like to contribute to this project even more. Could you tell me which issues are the most important? (I know that there are priorities but maybe some of them are special)

Coverage remained the same when pulling c054123 on alberskib:master into 8293511 on mockito:master.

Coverage remained the same when pulling 01351d8 on alberskib:master into 8293511 on mockito:master.

Hey @alberskib finally I got some time to take a look at this very interesting tweak.

Could you detail this part ?

Why?
Usually bridge methods delegates invocations by INVOKE_VIRTUAL operation (bytecode inside implementation of bridge method) but sometimes it uses INVOKE_STATIC(some specific situtatuion). As a result INVOKE_VIRTUAL was handled by cglib proxy but INVOKE_STATIC not. It leads to the situtatuion that method invocation by using bridge method with INVOKE_STATIC operation cannot be mocked.

From my experience I only saw invokespecial (inside the bridge method) in order to invoke the superclass declaration (or invoke non public member). And invokevirtual when overriding a generic method. Could that be a typo ;)

For example look at this bridge method for the case described in issue 212 (parent class not public)

  public boolean value();
    Signature: ()Z
    flags: ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC
    LineNumberTable:
      line 24: 0
    LocalVariableTable:
      Start  Length  Slot  Name   Signature
             0       5     0  this   Lparent_class_not_public/ExtendingNotPublicParent;
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0       
         1: invokespecial #3                  // Method parent_class_not_public/ExtendingNotPublicParent.value:()Z
         4: ireturn       
      LineNumberTable:
        line 24: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
               0       5     0  this   Lparent_class_not_public/ExtendingNotPublicParent;

The second code shows the bytecode for when the method is a specialized version of the generic parent. You can spot the invokevirtual (in the bridge method) that will look for this class overriding of value()

  public java.lang.String value();
    Signature: ()Ljava/lang/String;
    flags: ACC_PUBLIC
    LineNumberTable:
      line 7: 0
    LocalVariableTable:
      Start  Length  Slot  Name   Signature
             0       3     0  this   Lparent_class_not_public/ExtendingNotPublicParent;
    Code:
      stack=1, locals=1, args_size=1
         0: ldc           #2                  // String 
         2: areturn       
      LineNumberTable:
        line 7: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
               0       3     0  this   Lparent_class_not_public/ExtendingNotPublicParent;

  public java.lang.Object value();
    Signature: ()Ljava/lang/Object;
    flags: ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC
    LineNumberTable:
      line 3: 0
    LocalVariableTable:
      Start  Length  Slot  Name   Signature
             0       5     0  this   Lparent_class_not_public/ExtendingNotPublicParent;
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0       
         1: invokevirtual #3                  // Method value:()Ljava/lang/String;
         4: areturn       
      LineNumberTable:
        line 3: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
               0       5     0  this   Lparent_class_not_public/ExtendingNotPublicParent;

Also is it possible to add a dedicated test for the Comparable.compareTo describing the invocation with erasure ?

Also I still have a concerns about method equivalence (real vs bridge) regarding every feature of mockito.

Typically rewriting the assertions of your test pass too. But I still fear something will go wrong in some special cases where bridge method are involved. I may need to recheck how these internals were done :)

    @Test
    public void should_generic_method_be_mocked_successfully(){
        //given
        GenericClassExtension instance = mock(GenericClassExtension.class);
        when(instance.getId()).thenReturn(GET_ID_RESULT);

        //when
        String invokeVirtualResult = instance.getId();
        String invokeInterfaceResult = ((SimpleInterface)instance).getId();

        //then
        assertThat(invokeVirtualResult).isSameAs(GET_ID_RESULT);
        assertThat(invokeInterfaceResult).isSameAs(GET_ID_RESULT);
        verify(instance, times(2)).getId();
    }

Anyway this looks like very good work / research !

Unfortunately my fears came true, Mockito takes do not identify properly bridge methods, it especially appear when stubbing and verifying while the production code uses the upper class, with erasure.

package org.mockitousage.bugs;

import static org.fest.assertions.Assertions.assertThat;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;

import java.util.Observable;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.runners.MockitoJUnitRunner;

@RunWith(MockitoJUnitRunner.class)
public class BridgeMethodAndPolymorphimTest {

    @Test
    public void should_detect_erasure_bridge_method_when_verifying___when_production_code_invokes_interface() {
        // given
        SpecializedChild mock = mock(SpecializedChild.class);

        // when
        new ProductionCodeInvokingInterface().invoke(mock, "A");

        // then
        verify(mock).wontFail("A");
    }

    @Test
    public void should_detect_erasure_bridge_method_when_stubbing___when_production_code_invokes_interface() {
        // given
        SpecializedChild mock = mock(SpecializedChild.class);
        Observable value_to_be_returned = new Observable();
        given(mock.wontFail("A")).willReturn(value_to_be_returned);

        // when
        Observable actual_returned_value = new ProductionCodeInvokingInterface().invoke(mock, "A");

        // then
        assertThat(actual_returned_value).isSameAs(value_to_be_returned);
    }

    @Test
    public void should_detect_erasure_bridge_method_when_verifying___when_production_code_invokes_child() {
        // given
        SpecializedChild mock = mock(SpecializedChild.class);

        // when
        new ProductionCodeInvokingSpecializedChild().invoke(mock, "A");

        // then
        verify(mock).wontFail("A");
    }

    @Test
    public void should_detect_erasure_bridge_method_when_stubbing___when_production_code_invokes_child() {
        // given
        SpecializedChild mock = mock(SpecializedChild.class);
        Observable value_to_be_returned = new Observable();
        given(mock.wontFail("A")).willReturn(value_to_be_returned);

        // when
        Observable actual_returned_value = new ProductionCodeInvokingSpecializedChild().invoke(mock, "A");

        // then
        assertThat(actual_returned_value).isSameAs(value_to_be_returned);
    }


    static class ProductionCodeInvokingInterface {
        public <A, B extends CharSequence> A invoke(Interface<A, B> theInterface, B b) {
            return theInterface.wontFail(b);
        }
    }

    static class ProductionCodeInvokingSpecializedChild {
        public Observable invoke(SpecializedChild theChild, String b) {
            return theChild.wontFail(b);
        }
    }


    static interface Interface<A, B extends CharSequence> {
        A wontFail(B b);
    }

    static class SpecializedChild implements Interface<Observable, String> {
        @Override
        public Observable wontFail(String s) {
            throw new IllegalStateException("and yet it fails");
        }

        // will generate bridge method with signature like :
        // Object wontFail(CharSequence b);
    }
}

By the way note that erasure, do not always erase to Object

// class version 50.0 (50)
// access flags 0x20
// signature Ljava/lang/Object;Lorg/mockitousage/bugs/BridgeMethodAndPolymorphimTest$Interface<Ljava/util/Observable;Ljava/lang/String;>;
// declaration: org/mockitousage/bugs/BridgeMethodAndPolymorphimTest$SpecializedChild implements org.mockitousage.bugs.BridgeMethodAndPolymorphimTest$Interface<java.util.Observable, java.lang.String>
class org/mockitousage/bugs/BridgeMethodAndPolymorphimTest$SpecializedChild implements org/mockitousage/bugs/BridgeMethodAndPolymorphimTest$Interface  {

  // compiled from: BridgeMethodAndPolymorphimTest.java
  // access flags 0x8
  static INNERCLASS org/mockitousage/bugs/BridgeMethodAndPolymorphimTest$SpecializedChild org/mockitousage/bugs/BridgeMethodAndPolymorphimTest SpecializedChild
  // access flags 0x608
  static abstract INNERCLASS org/mockitousage/bugs/BridgeMethodAndPolymorphimTest$Interface org/mockitousage/bugs/BridgeMethodAndPolymorphimTest Interface

  // access flags 0x0
  <init>()V
   L0
    LINENUMBER 86 L0
    ALOAD 0
    INVOKESPECIAL java/lang/Object.<init> ()V
    RETURN
   L1
    LOCALVARIABLE this Lorg/mockitousage/bugs/BridgeMethodAndPolymorphimTest$SpecializedChild; L0 L1 0
    MAXSTACK = 1
    MAXLOCALS = 1

  // access flags 0x1
  public wontFail(Ljava/lang/String;)Ljava/util/Observable;
   L0
    LINENUMBER 89 L0
    NEW java/lang/IllegalStateException
    DUP
    LDC "and yet it fails"
    INVOKESPECIAL java/lang/IllegalStateException.<init> (Ljava/lang/String;)V
    ATHROW
   L1
    LOCALVARIABLE this Lorg/mockitousage/bugs/BridgeMethodAndPolymorphimTest$SpecializedChild; L0 L1 0
    LOCALVARIABLE s Ljava/lang/String; L0 L1 1
    MAXSTACK = 3
    MAXLOCALS = 2

  // access flags 0x1041
  public volatile bridge wontFail(Ljava/lang/CharSequence;)Ljava/lang/Object;
   L0
    LINENUMBER 86 L0
    ALOAD 0
    ALOAD 1
    CHECKCAST java/lang/String
    INVOKEVIRTUAL org/mockitousage/bugs/BridgeMethodAndPolymorphimTest$SpecializedChild.wontFail (Ljava/lang/String;)Ljava/util/Observable;
    ARETURN
   L1
    LOCALVARIABLE this Lorg/mockitousage/bugs/BridgeMethodAndPolymorphimTest$SpecializedChild; L0 L1 0
    LOCALVARIABLE x0 Ljava/lang/CharSequence; L0 L1 1
    MAXSTACK = 2
    MAXLOCALS = 2
}

Nice catch! I will take a look into this tomorrow. I know what is the problem but I need to find solution.

The problem is with Invocation matcher. This class actually does not take into account bridge methods when comparing method invocations(before this change it was not necessary). If you know some other places where method equality is checked it will be great if you tell me. Thanks to it I will be able to handle all potential problems that this change could introduce.

Yes good diving, this is the InvocationMatcher where the matching logic is done for stubbing and verifying, especially the hasSameMethod. Good thing that CGLIB proxies give the actual invoked method, that means we know if a bridge method was invoked.

I've made a simple adjustment in the code, the whole test suite pass in Mockito now, but I'd like to test this change in the real world. And see if we can cover more stuff regarding bridge methods (i'm thinking if there's a case where overloading is meddling with generics overriding).

Here's my change.

    public boolean hasSameMethod(Invocation candidate) {
        //not using method.equals() for 1 good reason:
        //sometimes java generates forwarding methods when generics are in play see JavaGenericsForwardingMethodsTest
        Method m1 = invocation.getMethod();
        Method m2 = candidate.getMethod();
        boolean isM2Bridge = m2.isBridge();

        if (m1.getName() != null && m1.getName().equals(m2.getName())) {
            /* Avoid unnecessary cloning */
            Class[] params1 = m1.getParameterTypes();
            Class[] params2 = m2.getParameterTypes();
            if (params1.length == params2.length) {
                for (int i = 0; i < params1.length; i++) {
                    if (hasNonMatchingArgTypes(isM2Bridge, params1[i], params2[i]))
                        return false;
                    }
                return true;
            }
        }
        return false;
    }

    private boolean hasNonMatchingArgTypes(boolean isM2Bridge, Class argType1, Class argType2) {
        return isM2Bridge ? !argType2.isAssignableFrom(argType1) : argType1 != argType2;
    }

Unfortunately I think that it does not solve problem. Imagine next situtation(please take a look into code not the result of test):

@RunWith(MockitoJUnitRunner.class)
public class BridgeMethodAndPolymorphimTest {
    @Test
    public void should_detect_erasure_bridge_method_when_verifying___when_production_code_invokes_interface() {
        // given
        SpecializedChild mock = mock(SpecializedChild.class);

        // when
        new ProductionCodeInvokingInterface().invoke(mock, new Observable());

        // then
        verify((Interface<Observable, String>)mock).wontFail(Matchers.<String>any());
    }

     static class ProductionCodeInvokingInterface {
        public <A, B> A invoke(InterfaceForInterface<A,B> theInterface, B b) {
            return theInterface.wontFail(b);
        }
    }

    static class ProductionCodeInvokingSpecializedChild {
        public Observable invoke(SpecializedChild theChild, String b) {
            return theChild.wontFail(b);
        }
    }

    static interface InterfaceForInterface<A, B>{
        A wontFail(B b);
    }


    static interface Interface<A, B extends CharSequence>{
        A wontFail(B b);
    }

    static class SpecializedChild implements Interface<Observable, String>, InterfaceForInterface<String, Observable> {

        @Override
        public Observable wontFail(String s) {
            throw new IllegalStateException("and yet it fails");
        }

        @Override public String wontFail(Observable observable) {
            return null;
        }

        // will generate bridge method with signature like :
        // Object wontFail(CharSequence b);
    }
}

I can imagine situation where InvocationMatcher must check whether methods are equals based on next parameters:

• m1 is 'bridge', m2 is not
• m1 is not 'bridge', m2 is 'bridge'
• m1 and m2 are 'bridge' but they invoke different real methods
• m1 and m2 is 'bridge' and they invoke the same methods
  I have some ideas how to provide proper implementation:
• the most powerfull is to use some kind of library that works on bytecode - thanks to it we will be able to check to which method does bridge is delegating. As a result we will be sure that m1 and m2 lead to the same method.
• use information about methods declared in class. When we receive bridge method we will try to match (by argument types) method that this bridge will invoke. It will be difficult for some special sitatuation in which class contains 2 bridges:

wontFail(Object)
wontFail(CharSequence)

with two real methods (not bridge)

wontFail(Observable)
wontFail(String)

and Invocation Matcher must decide whether:
wontFail(Object) is bridge for wontFail(String)- in this specific situation it is not but argument types does not negate this possibility.
I'm pretty sure that provided arguments will handle such situation but if someone will use AnyMatcher?
I think that it is quite difficult problem: I'm thinking about correct solution but it will take some time.
Any comments and clues are more than welcome.

I agree with you! The proposed patch was just an exploration of how to fix the issue.

Indeed this is one of the case I had in mind that could potentially cause problems for this issue.

// bridge methods
wontFail(Object)
wontFail(CharSequence)

// real methods (overload)
wontFail(Observable)
wontFail(String)

This case may be the real killer of this PR, as we cannot regress on this one.

But let's create tests for each cases, and advance one step at a time to see on far it's possible to go. Also I don't think it's a good idea to reprocess the bytecode (CGLIB already do that and it's slow), as the whole class file has to be read to just look at bridge methods (even if cached). If we had our own bytecode engine that would be a different story.

Your proposed test case could make sense if the test is rewritten this way :

    @Test
    public void should_detect_erasure_bridge_method_when_verifying___when_production_code_invokes_interface() {
        // given
        Interface<Observable, String> mock = mock(SpecializedChild.class);

        // when
        new ProductionCodeInvokingInterface().invoke(mock, new Observable());

        // then
        verify(mock).wontFail(Matchers.<String>any());
    }

I think that the best idea to solve this issue is to write some method getRealMethod that will find bridge delegation. Thanks to it code will looks like this:

 public boolean hasSameMethod(Invocation candidate) {
        //not using method.equals() for 1 good reason:
        //sometimes java generates forwarding methods when generics are in play see JavaGenericsForwardingMethodsTest
        Method m1 = getRealMehod(invocation.getMethod());
        Method m2 = getRealMehod(candidate.getMethod());

        if (m1.getName() != null && m1.getName().equals(m2.getName())) {
            /* Avoid unnecessary cloning */
            Class[] params1 = m1.getParameterTypes();
            Class[] params2 = m2.getParameterTypes();
           if (params1.length == params2.length) {
                for (int i = 0; i < params1.length; i++) {
                if (params1[i] != params2[i])
                    return false;
                }
                return true;
            }
        }
        return false;
    }

    private Method getRealMehod(Method m){
        if (!m.isBridge()){
            return m
        }
       // some code
    }

It looks like that finding method to which bridge is delegating is impossible by matching argument types. Please take a look at next example:

....
 static class AClass{ }
static class BClass extends AClass{  }
static class CClass extends BClass {  }
static class DClass extends CClass{   }
static class EClass extends DClass{   }
static interface IntA<A extends BClass>{
    int test (A a);
}
static interface IntB<B extends CClass>{
    int test (B b);
}

    static class SpecializedAB implements IntA<DClass>, IntB<EClass> {
        @Override
        public int test(DClass s) {
            return 1;
        }
        @Override public int test(EClass s) {
            return 1;
        }
    }
....

Will result in

  org.mockitousage.bugs.BridgeMethodAndPolymorphimTest$SpecializedAB();
    Code:
       0: aload_0
       1: invokespecial #1                  // Method java/lang/Object."<init>":()V
       4: return
  public int test(org.mockitousage.bugs.BridgeMethodAndPolymorphimTest$DClass);
    Code:
       0: iconst_1
       1: ireturn
  public int test(org.mockitousage.bugs.BridgeMethodAndPolymorphimTest$EClass);
    Code:
       0: iconst_1
       1: ireturn

  public int test(org.mockitousage.bugs.BridgeMethodAndPolymorphimTest$BClass);
    Code:
       0: aload_0
       1: aload_1
       2: checkcast     #2                  // class org/mockitousage/bugs/BridgeMethodAndPolymorphimTest$DClass
       5: invokevirtual #3                  // Method test:(Lorg/mockitousage/bugs/BridgeMethodAndPolymorphimTest$DClass;)I
       8: ireturn
  public int test(org.mockitousage.bugs.BridgeMethodAndPolymorphimTest$CClass);
    Code:
       0: aload_0
       1: aload_1
       2: checkcast     #4                  // class org/mockitousage/bugs/BridgeMethodAndPolymorphimTest$EClass
       5: invokevirtual #5                  // Method test:(Lorg/mockitousage/bugs/BridgeMethodAndPolymorphimTest$EClass;)I
       8: ireturn
}

Generally it is really difficult problem. Let's imagine that we find great way to determine method to which bridge is delegating invocation(not necessarily by parsing bytecode - alternative way is to analyze call stack during proxy creation). In this situation we comeback to the entry point of this problem as such analyze will lead to the same situation(INOKE_SPECIAL etc).
Maybe I am wrong but more I dig into problem more I start thinking that this PR should be abandoned and some another solution should be proposed

Yeah that's what I feared. Still I don't want to close it yet. So if progress is done there... We could still modify CGLIB.... in order to provide a bridge method callback, or something else so we know what method the bridge method is calling. But I'm quite un confortable patching CGLIB for several reasons (maintenance mostly).

Awesome research. Do you guys want to keep this PR open for now?

Guys, I'm going to close this PR to manage WIP in Mockito. I hope that's ok. Feel free to continue discussing it here or on the mailing list. I'd love to fix this problem.

By the way I had managed to get in touch with some people behind CGLIB, and while they seem busy with other stuff they put CGLIB on github. The repo might change of owner though (like a CGLIB org). Anyway that's good news.

So we already discussed that CGLIB must be extended to provide at invocation time the method that the bridge will call. One warning I will issue is that bridge method's bytecode may vary depending on the compiler (IBM J9, Eclipse, different version, etc), however bridge methods are simple enough to deal with that.

And I won't mind keeping this PR open. Maybe we could assign a Research milestone.