Uploaded image for project: 'Groovy'
  1. Groovy
  2. GROOVY-7655

Wrong method is chosen when using super with generics

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.4
    • Fix Version/s: 2.5.0-alpha-1
    • Component/s: None
    • Labels:
      None

      Description

      Given code:

      generics_extension_test.groovy
      interface ParamInterface{}
      
      class ParamImplementation implements ParamInterface{}
      
      class ParamExtension extends ParamImplementation {}
      
      class A<T extends ParamInterface> {
      
          def getResult(T a) {
              return "A"
          }
      }
      
      class B<T extends ParamImplementation> extends A<T> {
          def getResult(T b) {
              return "B"
          }
      }
      
      class C extends B<ParamExtension> {
          @Override
          def getResult(ParamExtension b) {
              return super.getResult(b)
          }
      }
      
      
      String result = new C().getResult(new ParamExtension())
      assert result == "B"
      

      I get output:

      Assertion failed: 
      
      assert result == "B"
             |      |
             A      false
      
      	at generics_extension_test.run(generics_extension_test.groovy:31)
      

      When calling super method, instead of looking for method in a class one hierarchy bellow, groovy goes through complex calculations for finding a better method. I debug'ed up to a point in
      groovy.lang.MetaClassImpl#chooseMostSpecificParams
      where it turned out that distance for interface was 2 and distance for class that directly extends the method was much bigger, therefore the method from class A is chosen instead of method from class B. However, looking objectively, it would seem that B.getResult is a much better fit:
      C extends B and B extends A
      parameter hierarchy is shorter (ParamExtension -> ParamImplementation -> ParamInterface)

        Activity

        Hide
        doktorj John Woodward added a comment - - edited

        This appears to be much more general than this, as I can replicate the issue starting with 2.4.0, without the use of generics. In my case, I used the following:

        class A {
            boolean aCalled = false
        
            protected Map myMethod( def item ) {
                println "A myMethod"
                aCalled = true
                return [:]
            }
        }
        
        class B extends A {
            boolean bCalled = false
        
            protected Map myMethod( def item ) {
                println "B myMethod"
                super.myMethod( item )
                bCalled = true
                return [:]
            }
        }
        
        class C extends B {
            boolean cCalled = false
        
            protected Map cMethod( def item ) {
                println "C cMethod"
                super.myMethod( item )
                //myMethod( item )
                cCalled = true
                return [:]
            }
        }
        

        along with a simple JUnit test:

        import org.junit.Test
        
        class CTest {
            @Test
            public void test() {
                def c = new C()
                def ret = c.cMethod( "stuff" )
                
                assert c.aCalled
                assert c.bCalled // Duh ... 2.4.x fails here.
                assert c.cCalled
            }
        }
        

        It's seen that all the 2.4 versions I ran (2.4.0, 2.4.3, 2.4.7) exhibit the same behavior — B.myMethod is not invoked.

        We encountered this while looking at doing a transition from the 2.3.x versions of groovy to 2.4.x, and one of our apps starting failing in an odd manner. It turns out that we did have a class coded invoking super on a method, while the class didn't override the method — resulting in the invocation of the wrong method.

        In our minds at the company I'm working for, this is a very serious bug that may prevent our moving to the 2.4.x groovy runtimes.

        Show
        doktorj John Woodward added a comment - - edited This appears to be much more general than this, as I can replicate the issue starting with 2.4.0, without the use of generics. In my case, I used the following: class A { boolean aCalled = false protected Map myMethod( def item ) { println "A myMethod" aCalled = true return [:] } } class B extends A { boolean bCalled = false protected Map myMethod( def item ) { println "B myMethod" super .myMethod( item ) bCalled = true return [:] } } class C extends B { boolean cCalled = false protected Map cMethod( def item ) { println "C cMethod" super .myMethod( item ) //myMethod( item ) cCalled = true return [:] } } along with a simple JUnit test: import org.junit.Test class CTest { @Test public void test() { def c = new C() def ret = c.cMethod( "stuff" ) assert c.aCalled assert c.bCalled // Duh ... 2.4.x fails here. assert c.cCalled } } It's seen that all the 2.4 versions I ran (2.4.0, 2.4.3, 2.4.7) exhibit the same behavior — B.myMethod is not invoked. We encountered this while looking at doing a transition from the 2.3.x versions of groovy to 2.4.x, and one of our apps starting failing in an odd manner. It turns out that we did have a class coded invoking super on a method, while the class didn't override the method — resulting in the invocation of the wrong method. In our minds at the company I'm working for, this is a very serious bug that may prevent our moving to the 2.4.x groovy runtimes.
        Hide
        blackdrag Jochen Theodorou added a comment -

        to be able to realize a super call in Groovy we have to go to some lengths in the form of helper methods. Only the helper method is able to make the call, because to realize a proper super invocation you will need an invokespecial, and that is not available by reflection or even by generating a helper class. A invokespecial instruction must come from the same class to realize a call to super. Till 2.4 we did blindly create helper methods for all possible cases. We then tried to slim down the class files a bit but reducing them to their minimum. But it seems this case here has been forgotten. There are solutions to this... overwrite the method in C or use @CompileStatic for example. Those are no options for you?

        Show
        blackdrag Jochen Theodorou added a comment - to be able to realize a super call in Groovy we have to go to some lengths in the form of helper methods. Only the helper method is able to make the call, because to realize a proper super invocation you will need an invokespecial, and that is not available by reflection or even by generating a helper class. A invokespecial instruction must come from the same class to realize a call to super. Till 2.4 we did blindly create helper methods for all possible cases. We then tried to slim down the class files a bit but reducing them to their minimum. But it seems this case here has been forgotten. There are solutions to this... overwrite the method in C or use @CompileStatic for example. Those are no options for you?
        Hide
        doktorj John Woodward added a comment -

        Not really; our concern is about precompiled/3rd party libraries more than our own code. While we could keep in mind this situation in our own code, there are limits to that, and someone, someday, is going to miss a case.

        This is one of those cases where the current invocation is different than the baseline Java. I read the 2.4 release notes pretty carefully the other night, and was looking at decompiled code and saw the reduction in generated helper methods. If this particular case is one that was overlooked, perhaps a solution in the next release would be forthcoming? It surprises me that a simple case like this would have been overlooked in the unit tests (then again, it surprises me, too, that it seems so few have noticed a case like this).

        Show
        doktorj John Woodward added a comment - Not really; our concern is about precompiled/3rd party libraries more than our own code. While we could keep in mind this situation in our own code, there are limits to that, and someone, someday, is going to miss a case. This is one of those cases where the current invocation is different than the baseline Java. I read the 2.4 release notes pretty carefully the other night, and was looking at decompiled code and saw the reduction in generated helper methods. If this particular case is one that was overlooked, perhaps a solution in the next release would be forthcoming? It surprises me that a simple case like this would have been overlooked in the unit tests (then again, it surprises me, too, that it seems so few have noticed a case like this).
        Hide
        blackdrag Jochen Theodorou added a comment -

        If the library is precompiled with pre 2.4, then the code there already contains the helper method. And if it is there, it will be used. So unless you recompile, there is no problem. Of course there is then still a problem once they recompile. But that is the case already if you use 2.3 and they use 2.4 to compile.

        I do agree, that this has to be fixed for the next release. The question is the "how" beyond a simple "restore the old behaviour". I would like to keep the amount of methods at least lowered. Most probably I will make the compiler recognize the super-call and note that this will require a mop helper method later on. Then we would still get a big reduction in generated helper methods compared to 2.4 and it would solve your problem here.

        Show
        blackdrag Jochen Theodorou added a comment - If the library is precompiled with pre 2.4, then the code there already contains the helper method. And if it is there, it will be used. So unless you recompile, there is no problem. Of course there is then still a problem once they recompile. But that is the case already if you use 2.3 and they use 2.4 to compile. I do agree, that this has to be fixed for the next release. The question is the "how" beyond a simple "restore the old behaviour". I would like to keep the amount of methods at least lowered. Most probably I will make the compiler recognize the super-call and note that this will require a mop helper method later on. Then we would still get a big reduction in generated helper methods compared to 2.4 and it would solve your problem here.
        Hide
        doktorj John Woodward added a comment -

        Agreed. In our case, looking at the move to using Mule 3.7.3, 2.4 is required (which, overall, is a good thing). Thank you for being so prompt with this; I certainly understand the "how" is the difficult part. You've made big strides with groovy and I'd love to keep using it.

        Show
        doktorj John Woodward added a comment - Agreed. In our case, looking at the move to using Mule 3.7.3, 2.4 is required (which, overall, is a good thing). Thank you for being so prompt with this; I certainly understand the "how" is the difficult part. You've made big strides with groovy and I'd love to keep using it.
        Hide
        blackdrag Jochen Theodorou added a comment -

        after thinking about how to solve the issue for a bit it was actually quite easy to find a solution. The compiler will now ensure, that methods used with super will find their mop helper method. This happens regardless of the signature, so there are still useless methods generated, but still many less than in 2.3

        Show
        blackdrag Jochen Theodorou added a comment - after thinking about how to solve the issue for a bit it was actually quite easy to find a solution. The compiler will now ensure, that methods used with super will find their mop helper method. This happens regardless of the signature, so there are still useless methods generated, but still many less than in 2.3

          People

          • Assignee:
            blackdrag Jochen Theodorou
            Reporter:
            MariusV Marius Vaitkus
          • Votes:
            5 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development