Groovy
  1. Groovy
  2. GROOVY-7312

Compiler generates invalid inner class constructor

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.0
    • Fix Version/s: 2.4.7
    • Component/s: class generator, Compiler
    • Labels:
      None

      Description

      replacement description:

      Intf.groovy
      interface Intf {
        def foo = { "bar" }
      }
      

      Will create an inner class that lacks the static modifier for the inner class table. Compare with

      JavaInterface.java
      public interface JavaInterface {
        class NestedInInterface {}
      }
      

      for reference

      Original description:
      The JLS specifies that an implicitly-declared constructor of a non-private inner class "implicitly declares one formal parameter representing the immediately enclosing instance of the class" (Section 8.8.9, see also 8.8.1).

      Intf.groovy
      interface Intf {
        def foo = { "bar" }
      }
      

      The above code creates an inner class (Intf$1, not exactly sure what it's for) with a default constructor that has no parameters:

      $ javap -p -classpath classes 'Intf$1'
      Compiled from "Intf.groovy"
      class Intf$1 implements groovy.lang.GroovyObject {
        ...
        public Intf$1();
        ...
      }
      

      While not a major issue, this non-conformance can break interoperability with anything that expects to work with Java classes. This particular example came up while trying to use a similarly-declared Groovy class from a Scala class, where the Scala compiler was unable to parse the generated inner class.

      For reference, here is an example Java Inner class and it's compiled representation. Note the constructor parameter on the inner class.

      JavaObj.java
      public class JavaObj {
        public class Inner {}
      }
      
      $ javap -p -classpath classes 'JavaObj$Inner'
      public class JavaObj$Inner {
        final JavaObj this$0;
        public JavaObj$Inner(JavaObj);
      }
      

        Issue Links

          Activity

          Hide
          Jochen Theodorou added a comment -

          I think there is a misunderstanding. The Java spec differentiates between nested classes and inner classes. A nested class basically is a static inner class. But it has the consequence, that for example this informal first parameter will not appear. Since an interface is no full class in the sense of the JLS, it cannot have inner classes, only nested classes. The open block you used in Intf usually generates an inner class, but that is optional. If the open block has no references to the surrounding class, then it can be a nested class instead. In case of Intf the nest class is the only possible case, using an open block in a static method would be another case. So if Scala had a problem with this class, then for other reasons. It is of course possible that some flags are set wrong or such. But of course you are not supposed to directly use that class either.

          Show
          Jochen Theodorou added a comment - I think there is a misunderstanding. The Java spec differentiates between nested classes and inner classes. A nested class basically is a static inner class. But it has the consequence, that for example this informal first parameter will not appear. Since an interface is no full class in the sense of the JLS, it cannot have inner classes, only nested classes. The open block you used in Intf usually generates an inner class, but that is optional. If the open block has no references to the surrounding class, then it can be a nested class instead. In case of Intf the nest class is the only possible case, using an open block in a static method would be another case. So if Scala had a problem with this class, then for other reasons. It is of course possible that some flags are set wrong or such. But of course you are not supposed to directly use that class either.
          Hide
          Adam Lewandowski added a comment -

          I see, thanks for the explanation, which makes sense. Backing up what you said, I constructed a nested class inside of a java interface:

          JavaInterface.java
          public interface JavaInterface {
            class NestedInInterface {}
          }
          

          And it also creates a similar no-arg constructor for the nested class:

          $ javap -p  -classpath classes 'JavaInterface$NestedInInterface'
          public class JavaInterface$NestedInInterface {
            public JavaInterface$NestedInInterface();
          }
          

          Scala, incidentally, wasn't trying to use the class, just parse it. I will raise this issue there. Thanks for your help.

          Show
          Adam Lewandowski added a comment - I see, thanks for the explanation, which makes sense. Backing up what you said, I constructed a nested class inside of a java interface: JavaInterface.java public interface JavaInterface { class NestedInInterface {} } And it also creates a similar no-arg constructor for the nested class: $ javap -p -classpath classes 'JavaInterface$NestedInInterface' public class JavaInterface$NestedInInterface { public JavaInterface$NestedInInterface(); } Scala, incidentally, wasn't trying to use the class, just parse it. I will raise this issue there. Thanks for your help.
          Hide
          Adam Lewandowski added a comment -

          Looking closer, I do think there is something wrong with the generated inner class, or at least inconsistent w/ the way javac handles this case. Javac marks this inner class as static in the InnerClasses attribute of it's class file, but groovy does not.

          Using the same example classes as above, for the JavaInterface$NestedInInterface class:

          $ javap -p -v -classpath classes 'JavaInterface$NestedInInterface'
          Classfile .../JavaInterface$NestedInInterface.class
          public class JavaInterface$NestedInInterface
          InnerClasses:
          public static #14= #3 of #12; //NestedInInterface=class JavaInterface$NestedInInterface of class JavaInterface
          minor version: 0
          major version: 51
          flags: ACC_PUBLIC, ACC_SUPER

          And for the groovy Intf$1 class:

          $ javap -p -v -classpath classes 'Intf$1'
          Classfile .../Intf$1.class
          class Intf$1 implements groovy.lang.GroovyObject
          InnerClasses:
          #107= #2 of #75; //1=class Intf$1 of class Intf
          minor version: 0
          major version: 49
          flags: ACC_SUPER, ACC_SYNTHETIC

          It is the lack of the 'static' indicator that causes scalac problems. Without that, I don't think it can tell that this is a nested class and treats it as an inner class instead.

          Show
          Adam Lewandowski added a comment - Looking closer, I do think there is something wrong with the generated inner class, or at least inconsistent w/ the way javac handles this case. Javac marks this inner class as static in the InnerClasses attribute of it's class file, but groovy does not. Using the same example classes as above, for the JavaInterface$NestedInInterface class: $ javap -p -v -classpath classes 'JavaInterface$NestedInInterface' Classfile .../JavaInterface$NestedInInterface.class public class JavaInterface$NestedInInterface InnerClasses: public static #14= #3 of #12; //NestedInInterface=class JavaInterface$NestedInInterface of class JavaInterface minor version: 0 major version: 51 flags: ACC_PUBLIC, ACC_SUPER And for the groovy Intf$1 class: $ javap -p -v -classpath classes 'Intf$1' Classfile .../Intf$1.class class Intf$1 implements groovy.lang.GroovyObject InnerClasses: #107 = #2 of #75; //1=class Intf$1 of class Intf minor version: 0 major version: 49 flags: ACC_SUPER, ACC_SYNTHETIC It is the lack of the 'static' indicator that causes scalac problems. Without that, I don't think it can tell that this is a nested class and treats it as an inner class instead.
          Hide
          Jochen Theodorou added a comment -

          adding static should not be a problem

          Show
          Jochen Theodorou added a comment - adding static should not be a problem
          Hide
          ASF GitHub Bot added a comment -

          GitHub user shils opened a pull request:

          https://github.com/apache/groovy/pull/257

          GROOVY-7312 Add ACC_STATIC flag to inner classes of interfaces

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/shils/groovy GROOVY-7312

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/groovy/pull/257.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #257


          commit 0012b17aa72e7f4ee6acaef93ea59d74c9559fc1
          Author: Shil Sinha <shil.sinha@gmail.com>
          Date: 2016-02-09T23:00:16Z

          GROOVY-7312 Add ACC_STATIC flag to inner classes of interfaces


          Show
          ASF GitHub Bot added a comment - GitHub user shils opened a pull request: https://github.com/apache/groovy/pull/257 GROOVY-7312 Add ACC_STATIC flag to inner classes of interfaces You can merge this pull request into a Git repository by running: $ git pull https://github.com/shils/groovy GROOVY-7312 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/groovy/pull/257.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #257 commit 0012b17aa72e7f4ee6acaef93ea59d74c9559fc1 Author: Shil Sinha <shil.sinha@gmail.com> Date: 2016-02-09T23:00:16Z GROOVY-7312 Add ACC_STATIC flag to inner classes of interfaces
          Hide
          ASF GitHub Bot added a comment -

          Github user shils commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/257#discussion_r52389570

          — Diff: src/main/org/codehaus/groovy/classgen/asm/ClosureWriter.java —
          @@ -80,7 +80,11 @@ public void writeClosure(ClosureExpression expression) {

          // generate closure as public class to make sure it can be properly invoked by classes of the
          // Groovy runtime without circumventing JVM access checks (see CachedMethod for example).

          • ClassNode closureClass = getOrAddClosureClass(expression, ACC_PUBLIC);
            + int mods = ACC_PUBLIC;
            + if (classNode.isInterface()) {
              • End diff –

          Classes for closures defined in static methods could (should?) also be flagged as static, but I left that case as is since it wasn't mentioned in GROOVY-7312.

          Show
          ASF GitHub Bot added a comment - Github user shils commented on a diff in the pull request: https://github.com/apache/groovy/pull/257#discussion_r52389570 — Diff: src/main/org/codehaus/groovy/classgen/asm/ClosureWriter.java — @@ -80,7 +80,11 @@ public void writeClosure(ClosureExpression expression) { // generate closure as public class to make sure it can be properly invoked by classes of the // Groovy runtime without circumventing JVM access checks (see CachedMethod for example). ClassNode closureClass = getOrAddClosureClass(expression, ACC_PUBLIC); + int mods = ACC_PUBLIC; + if (classNode.isInterface()) { End diff – Classes for closures defined in static methods could (should?) also be flagged as static, but I left that case as is since it wasn't mentioned in GROOVY-7312 .

            People

            • Assignee:
              Shil Sinha
              Reporter:
              Adam Lewandowski
            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development