groovy
  1. groovy
  2. GROOVY-4457

generic type declarations leaking across all files in a build

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.7.5
    • Fix Version/s: 1.8.5, 2.0-beta-2, 1.7.11
    • Component/s: Compiler
    • Labels:
      None
    • Number of attachments :
      1

      Description

      Simple file, A.groovy:

      class A<String> {
      }
      
      class B {
        void foo(String s) {}
      }
      

      groovyc A.groovy
      javap -private B | grep foo

      produces:

      public void foo(java.lang.Object);
      

      The 'String' is treated as a type parameter name. The reference 'String' in the foo method is mapped to this type parameter (clearly it shouldn't be) and when producing the code, String is erased to its upper bound of Object, hence foo(Object) in the bytecode.

      Change it to this:

      class A<T> {
      }
      
      class B {
        void foo(String s) {}
      }
      

      and it produces what you expect:

       public void foo(java.lang.String);
      

      The problem is the genericParameterNames collection in the ResolveVisitor is never cleared, only augmented with new entries.

      My solution that seems to work in groovy eclipse is to clear the parameter names at the end of visiting a classnode in ResolveVisitor. So at the end of

      public void visitClass(ClassNode node) {
      

      add

      genericParameterNames.clear();
      

        Issue Links

          Activity

          Hide
          Roshan Dawrani added a comment -

          I can later look into it a bit more to confirm but on a quick glance, I think that the solution suggested may not work when there are (anonymous)inner classes involved where outer class' generic parameter names remain valid inside the inner classes too.

          We don't want to end visitClass() for an inner class and clear the genericParameterNames for the outer class as well.

          Show
          Roshan Dawrani added a comment - I can later look into it a bit more to confirm but on a quick glance, I think that the solution suggested may not work when there are (anonymous)inner classes involved where outer class' generic parameter names remain valid inside the inner classes too. We don't want to end visitClass() for an inner class and clear the genericParameterNames for the outer class as well.
          Graeme Rocher made changes -
          Field Original Value New Value
          Fix Version/s 1.8-beta-3 [ 16750 ]
          Fix Version/s 1.7.X [ 15538 ]
          Fix Version/s 1.8-beta-x [ 15750 ]
          Fix Version/s 1.7.6 [ 16749 ]
          Hide
          Andy Clement added a comment -

          Yep, after sleeping on it, you need to allow for inner classes to, I'd probably just use a similar mechanic in visitClassNode that you already have in the visitConstructorOrMethod code.

          Show
          Andy Clement added a comment - Yep, after sleeping on it, you need to allow for inner classes to, I'd probably just use a similar mechanic in visitClassNode that you already have in the visitConstructorOrMethod code.
          blackdrag blackdrag made changes -
          Link This issue is related to GRECLIPSE-845 [ GRECLIPSE-845 ]
          Hide
          blackdrag blackdrag added a comment -

          a proposed Testcase:

          assertScript """
          class A<String> {}
          class B {
            void foo(String s) {}
          }
          // use the name to check the class, since the error was that String was seen as 
          // a symbol resolved to Object, not as the class String, thus a ...==String would
          // not have failed
          assert B.class.methods.find{it.name=="foo"}.parameterTypes[0].name.contains("String")
          """
          
          Show
          blackdrag blackdrag added a comment - a proposed Testcase: assertScript """ class A< String > {} class B { void foo( String s) {} } // use the name to check the class, since the error was that String was seen as // a symbol resolved to Object , not as the class String , thus a ...== String would // not have failed assert B.class.methods.find{it.name== "foo" }.parameterTypes[0].name.contains( " String " ) """
          Guillaume Laforge made changes -
          Assignee Guillaume Laforge [ guillaume ]
          Guillaume Laforge made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 1.7.X [ 15538 ]
          Fix Version/s 1.8-beta-x [ 15750 ]
          Resolution Fixed [ 1 ]
          Hide
          Roshan Dawrani added a comment -

          As suspected, the fix seems to have impact on the generics handling for inner classes.

          The following code does not work now (doesn't even compile actually)

          class A<T> {
              class B {
                void foo(T s) {}
              }
          }
          assert A.B.class.methods.find{it.name=="foo"}.parameterTypes[0].name.contains("java.lang.Object")
          

          It fails with

          D:\Roshan\GroovyDevSetup\Workspace17x\TryGroovy\src\TryGroovy.groovy: 3: unable to resolve class T 
           @ line 3, column 16.
                   void foo(T s) {}
                            ^
          
          1 error
          
          Show
          Roshan Dawrani added a comment - As suspected, the fix seems to have impact on the generics handling for inner classes. The following code does not work now (doesn't even compile actually) class A<T> { class B { void foo(T s) {} } } assert A.B.class.methods.find{it.name== "foo" }.parameterTypes[0].name.contains( "java.lang. Object " ) It fails with D:\Roshan\GroovyDevSetup\Workspace17x\TryGroovy\src\TryGroovy.groovy: 3: unable to resolve class T @ line 3, column 16. void foo(T s) {} ^ 1 error
          Roshan Dawrani made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Hide
          Andy Clement added a comment -

          Indeed, I see that the visit to the inner class does not happen during the visit to the outerclass, it happens after the outer class has been finished with. So the code from visitMethodOrConstructor cannot be used as when visiting the inner node the type variables of the outer have been forgotten.

          Show
          Andy Clement added a comment - Indeed, I see that the visit to the inner class does not happen during the visit to the outerclass, it happens after the outer class has been finished with. So the code from visitMethodOrConstructor cannot be used as when visiting the inner node the type variables of the outer have been forgotten.
          Hide
          blackdrag blackdrag added a comment -

          There are possibly 3 solutions to the inner class problem.
          (1) add a map to the inner class and set it from the resolve visitor and reuse it when parsing the inner class
          (2) have some kind of memory persistent map ClassNode->SymbolMap.... I absolutely don't like this one
          (3) start the resolving of inner classes after the outer class is done, without using the normal iteration used by CompilationUnit.

          I prefer number 3 since it does not require keeping additional information in the AST.

          Show
          blackdrag blackdrag added a comment - There are possibly 3 solutions to the inner class problem. (1) add a map to the inner class and set it from the resolve visitor and reuse it when parsing the inner class (2) have some kind of memory persistent map ClassNode->SymbolMap.... I absolutely don't like this one (3) start the resolving of inner classes after the outer class is done, without using the normal iteration used by CompilationUnit. I prefer number 3 since it does not require keeping additional information in the AST.
          Hide
          Graeme Rocher added a comment -

          Any progress on this?

          Show
          Graeme Rocher added a comment - Any progress on this?
          Hide
          blackdrag blackdrag added a comment - - edited

          since Roshan seems not to have capacity to fix this (which is not his fault), there is no progress atm

          Show
          blackdrag blackdrag added a comment - - edited since Roshan seems not to have capacity to fix this (which is not his fault), there is no progress atm
          Hide
          blackdrag blackdrag added a comment - - edited

          I guess someone else will have to take this over, unless Roshan says he still wants to fix this one

          Show
          blackdrag blackdrag added a comment - - edited I guess someone else will have to take this over, unless Roshan says he still wants to fix this one
          Hide
          Andy Clement added a comment -

          If I can find time, I'll take a proper look at the right fix.

          Show
          Andy Clement added a comment - If I can find time, I'll take a proper look at the right fix.
          blackdrag blackdrag made changes -
          Fix Version/s 1.7.6 [ 16749 ]
          Fix Version/s 1.8-beta-3 [ 16750 ]
          Hide
          CÚdric Champeau added a comment -

          Master includes the fix which breaks the non static inner class case. I will try to commit a fix.

          Show
          CÚdric Champeau added a comment - Master includes the fix which breaks the non static inner class case. I will try to commit a fix.
          Hide
          CÚdric Champeau added a comment -

          Here is the patch against master. Added a unit test for the non static inner class case. If you are ok, I will commit it.

          Show
          CÚdric Champeau added a comment - Here is the patch against master. Added a unit test for the non static inner class case. If you are ok, I will commit it.
          CÚdric Champeau made changes -
          Attachment GROOVY-4457.patch [ 58038 ]
          Guillaume Laforge made changes -
          Assignee Guillaume Laforge [ guillaume ] Cedric Champeau [ melix ]
          CÚdric Champeau made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Fix Version/s 2.0-beta-2 [ 18072 ]
          Fix Version/s 1.7.11 [ 17244 ]
          Fix Version/s 1.8.5 [ 18071 ]
          Resolution Fixed [ 1 ]
          Paul King made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              CÚdric Champeau
              Reporter:
              Andy Clement
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: