Groovy
  1. Groovy
  2. GROOVY-4457

generic type declarations leaking across all files in a build

    Details

    • Type: Bug Bug
    • Status: 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

      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();
      
      1. GROOVY-4457.patch
        67 kB
        Cédric Champeau

        Activity

        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.
        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
        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.
        Hide
        Jochen Theodorou 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
        Jochen Theodorou 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
        Jochen Theodorou 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
        Jochen Theodorou 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
        Graeme Rocher added a comment -

        Any progress on this?

        Show
        Graeme Rocher added a comment - Any progress on this?
        Hide
        Jochen Theodorou 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
        Jochen Theodorou 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
        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
        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
        Hide
        Jochen Theodorou 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
        Jochen Theodorou 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 " ) """
        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.
        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.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development