groovy
  1. groovy
  2. GROOVY-4965

shim classes generated incorrectly in 1.8.1 (was fine in 1.8.0) for some static inner-class usages

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.8.1
    • Fix Version/s: 1.8.2, 1.9-beta-3
    • Component/s: Compiler
    • Labels:
      None
    • Number of attachments :
      0

      Description

      Static inner classes that are imported into Groovy scripts just fine in 1.8 are causing compilation issues in 1.8.1

      To reproduce:
      1) Java class Foo.Bar where Bar is public static within Foo.
      2) Groovy script references the Foo.Bar as a type with an arglist to a method.

      3) Groovy/Maven/Shim compiler puts a $ in the generated Java source name instead of a dot.

      As I said, all that changed between working and not working is the version of Groovy. This worked in 1.8.0 and at least some recent 1.7.x versions.

        Activity

        Hide
        Paul King added a comment -

        I suspect this was fixed as part of GROOVY-4939. Any chance you could try a snapshot, otherwise a better example would be good because the one I just created according to above appears to work in trunk. Thanks.

        Show
        Paul King added a comment - I suspect this was fixed as part of GROOVY-4939 . Any chance you could try a snapshot, otherwise a better example would be good because the one I just created according to above appears to work in trunk. Thanks.
        Hide
        Paul Hammant added a comment -

        I'm, trying to run the Groovy build and it barfs foe me: some plexus classCastException issue, and I'm timing out on this.

        I'll wait for a formal release of 1.8.2 and report then what it feels like.

        Show
        Paul Hammant added a comment - I'm, trying to run the Groovy build and it barfs foe me: some plexus classCastException issue, and I'm timing out on this. I'll wait for a formal release of 1.8.2 and report then what it feels like.
        Hide
        Paul Hammant added a comment -

        The uploaded 1.8.2-SNAPSHOT (dated 30th July) still exhibits the same defect.

        Show
        Paul Hammant added a comment - The uploaded 1.8.2-SNAPSHOT (dated 30th July) still exhibits the same defect.
        Hide
        Peter Niederwieser added a comment -

        I ran into the same problem when trying to upgrade Gradle to Groovy 1.8. I tried with with latest rev. 22728 (July 30th). Here is the generated method:

        public static  java.lang.Object assertSetContains(org.gradle.api.file.FileCollection set, java.util.Set<java.lang.String> filenames, java.lang.Iterable<org.gradle.api.file.FileCollection$AntType> types, boolean generic) { return null;}
        

        FileCollection.AntType is an inner enum written in Java. Again notice the $.

        Show
        Peter Niederwieser added a comment - I ran into the same problem when trying to upgrade Gradle to Groovy 1.8. I tried with with latest rev. 22728 (July 30th). Here is the generated method: public static java.lang. Object assertSetContains(org.gradle.api.file.FileCollection set, java.util.Set<java.lang. String > filenames, java.lang.Iterable<org.gradle.api.file.FileCollection$AntType> types, boolean generic ) { return null ;} FileCollection.AntType is an inner enum written in Java. Again notice the $ .
        Hide
        Guillaume Laforge added a comment -

        With the following test, I couldn't reproduce the problem experienced in Gradle, neither with trunk nor 1.8.x:

        /**
         * GROOVY-4965: shim classes generated incorrectly in 1.8.1 (was fine in 1.8.0) for some static inner-class usages
         * 
         * @author Guillaume Laforge
         */
        class InterfaceInnerEnumUsedAsGenericsParamTest extends StringSourcesStubTestCase {
        
            Map<String, String> provideSources() {
                [
                        'org/gradle/api/file/FileCollection.java': '''
                            package org.gradle.api.file;
        
                            public interface FileCollection {
                                enum AntType {
                                    MatchingTask, FileSet, ResourceCollection
                                }
                            }
                        ''',
        
                        'org/gradle/api/tasks/AntBuilderAwareUtil.groovy': '''
                            package org.gradle.api.tasks
        
                            import org.gradle.api.file.FileCollection
        
                            class AntBuilderAwareUtil {
                                static def assertSetContains(FileCollection set, Set<String> filenames, Iterable<FileCollection.AntType> types, boolean generic = true) {
                                    true
                                }
                            }
                        '''
                ]
            }
        
            void verifyStubs() {
                println stubJavaSourceFor('org.gradle.api.tasks.AntBuilderAwareUtil')
            }
        }
        
        Show
        Guillaume Laforge added a comment - With the following test, I couldn't reproduce the problem experienced in Gradle, neither with trunk nor 1.8.x: /** * GROOVY-4965: shim classes generated incorrectly in 1.8.1 (was fine in 1.8.0) for some static inner -class usages * * @author Guillaume Laforge */ class InterfaceInnerEnumUsedAsGenericsParamTest extends StringSourcesStubTestCase { Map< String , String > provideSources() { [ 'org/gradle/api/file/FileCollection.java': ''' package org.gradle.api.file; public interface FileCollection { enum AntType { MatchingTask, FileSet, ResourceCollection } } ''', 'org/gradle/api/tasks/AntBuilderAwareUtil.groovy': ''' package org.gradle.api.tasks import org.gradle.api.file.FileCollection class AntBuilderAwareUtil { static def assertSetContains(FileCollection set, Set< String > filenames, Iterable<FileCollection.AntType> types, boolean generic = true ) { true } } ''' ] } void verifyStubs() { println stubJavaSourceFor('org.gradle.api.tasks.AntBuilderAwareUtil') } }
        Hide
        Paul Hammant added a comment -

        I've managed to build trunk using ant, commenting out the hanging tests, ignoring the maven-deploy stage, then doing a manual install later.

        The bug is still there with a TRUNK checkout from moments ago.

        Can someone direct me to the class that writes shim classnames? I'll add a .replace("$",".") where appropriate, and report back.

        Show
        Paul Hammant added a comment - I've managed to build trunk using ant, commenting out the hanging tests, ignoring the maven-deploy stage, then doing a manual install later. The bug is still there with a TRUNK checkout from moments ago. Can someone direct me to the class that writes shim classnames? I'll add a .replace("$",".") where appropriate, and report back.
        Hide
        Guillaume Laforge added a comment -

        Hi Paul, you can look at org.codehaus.groovy.tools.javac.JavaStubGenerator.
        And you can look at the printParams() method, which is most probably where the action takes place – if it's in the types of the parameters of your methods that the problem arises.
        Ideally, it would be good to have a reproducable test case like the one I pasted in my previous comment, so that we know it won't happen again once fixed.

        Show
        Guillaume Laforge added a comment - Hi Paul, you can look at org.codehaus.groovy.tools.javac.JavaStubGenerator. And you can look at the printParams() method, which is most probably where the action takes place – if it's in the types of the parameters of your methods that the problem arises. Ideally, it would be good to have a reproducable test case like the one I pasted in my previous comment, so that we know it won't happen again once fixed.
        Hide
        Paul Hammant added a comment -

        Thanks Guillaume,

        Line 589 or so, genericsTypes[i].toString() should be genericsTypes[i].toString().replace("$",".")
        Either that or reuse/recurse into printType(..) as the same replacement is done elsewhere in the code, but not for this particular path.

        I can confirm, that my issue with live client code (hence it's not so easy to share) disappeared.

        Show
        Paul Hammant added a comment - Thanks Guillaume, Line 589 or so, genericsTypes [i] .toString() should be genericsTypes [i] .toString().replace("$",".") Either that or reuse/recurse into printType(..) as the same replacement is done elsewhere in the code, but not for this particular path. I can confirm, that my issue with live client code (hence it's not so easy to share) disappeared.
        Hide
        blackdrag blackdrag added a comment -

        Paul, you made me delete a very long comment You are obviously right that printType should be reused here. Can you make a patch that works for you Paul?

        Show
        blackdrag blackdrag added a comment - Paul, you made me delete a very long comment You are obviously right that printType should be reused here. Can you make a patch that works for you Paul?
        Hide
        Paul Hammant added a comment - - edited

        Sounds like y'all are on the cusp of a change that doesn't lower test coverage

        However, answering the questions posed in the deleted comment:

        genericsTypes[i].getType().isGenericsGenericsPlaceHolder() is false.
        genericsTypes[i].toString() looks like so my.package.MyClass$MyInnerClass
        

        Here's the one-liner diff, that aims at reuse of existing logic (that does the right thing). Tested of course:

        Index: src/main/org/codehaus/groovy/tools/javac/JavaStubGenerator.java
        ===================================================================
        --- src/main/org/codehaus/groovy/tools/javac/JavaStubGenerator.java    (revision 22747)
        +++ src/main/org/codehaus/groovy/tools/javac/JavaStubGenerator.java    (working copy)
        @@ -589,7 +589,7 @@
                 out.print('<');
                 for (int i = 0; i < genericsTypes.length; i++) {
                     if (i != 0) out.print(", ");
        -            out.print(genericsTypes[i].toString());
        +            printTypeName(out, genericsTypes[i].getType());
                 }
                 out.print('>');
             }
        
        Show
        Paul Hammant added a comment - - edited Sounds like y'all are on the cusp of a change that doesn't lower test coverage However, answering the questions posed in the deleted comment: genericsTypes[i].getType().isGenericsGenericsPlaceHolder() is false. genericsTypes[i].toString() looks like so my.package.MyClass$MyInnerClass Here's the one-liner diff, that aims at reuse of existing logic (that does the right thing). Tested of course: Index: src/main/org/codehaus/groovy/tools/javac/JavaStubGenerator.java =================================================================== --- src/main/org/codehaus/groovy/tools/javac/JavaStubGenerator.java (revision 22747) +++ src/main/org/codehaus/groovy/tools/javac/JavaStubGenerator.java (working copy) @@ -589,7 +589,7 @@ out.print('<'); for (int i = 0; i < genericsTypes.length; i++) { if (i != 0) out.print(", "); - out.print(genericsTypes[i].toString()); + printTypeName(out, genericsTypes[i].getType()); } out.print('>'); }
        Hide
        Paul King added a comment -

        I have been unable to replicate an error case yet but the latest snapshot (I believe) should have the "replace" patch in place - the "printTypeName" patch broke some other behavior so I didn't go that way. It would be interesting to see if the latest snapshot fixes your issue.

        Show
        Paul King added a comment - I have been unable to replicate an error case yet but the latest snapshot (I believe) should have the "replace" patch in place - the "printTypeName" patch broke some other behavior so I didn't go that way. It would be interesting to see if the latest snapshot fixes your issue.
        Hide
        Paul Hammant added a comment -

        close this issue. i moved my inner class to outer.

        Show
        Paul Hammant added a comment - close this issue. i moved my inner class to outer.
        Hide
        Paul King added a comment -

        Just waiting feedback from Peter to see if this is still a problem for Gradle. If not I will close the issue.

        Show
        Paul King added a comment - Just waiting feedback from Peter to see if this is still a problem for Gradle. If not I will close the issue.
        Hide
        Peter Niederwieser added a comment -

        I eventually worked around it like Paul (converted the nested class to a top-level class). If you are interested, the groovy-1.8 branch of Gradle is available on GitHub.

        Show
        Peter Niederwieser added a comment - I eventually worked around it like Paul (converted the nested class to a top-level class). If you are interested, the groovy-1.8 branch of Gradle is available on GitHub.
        Hide
        Paul King added a comment -

        Fix to replace '$' with '.' was added but no test case could reproduce the original problem. Assuming fixed.

        Show
        Paul King added a comment - Fix to replace '$' with '.' was added but no test case could reproduce the original problem. Assuming fixed.

          People

          • Assignee:
            Paul King
            Reporter:
            Paul Hammant
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: