groovy
  1. groovy
  2. GROOVY-4934

incorrect signature attributes in class files for inner class generics

    Details

    • Number of attachments :
      1

      Description

      When an inner type shares state with its outer type, and that shared state is generic, the signature attributes generated inside the inner class are not correct. This is not normally a problem because once created they are rarely processed again, however, Eclipse checks these things and discovers the inconsistencies, reporting an 'inconsistent class file' error when it sees a type variable referenced that is not in scope.

      Here is a simple groovy program:

      import groovy.lang.Closure
      
      import java.io.Serializable
      import java.util.Map
      
      class GormInstanceApi<D> {
      
      	void delete(D instance) {
      		new VoidSessionCallback() {
      			void doInSession() {
      				print instance
      			}
      		}
      	}
      
      }
      
      interface VoidSessionCallback {
      	void doInSession();
      }
      

      When this gets compiled, a field of type Reference is created in the inner type (in the InnerClassVisitor.visitConstructorCallExpression()) code, and this is added to the constructor. The signature attribute for the new field and the constructor of the inner type encodes a generic reference that isn't quite right - it refers to a type variable that isn't in scope. Here is the generic signature attribute for the constructor:

      (LGormInstanceApi<TD;>;Lgroovy/lang/Reference<TT;>;)V;

      The first bit is OK, the 'D' referred to is defined by the declaring type and so visible from the inner. However the 'T' is not visible - I would have expected a reference to the raw type there:

      (LGormInstanceApi<TD;>;Lgroovy/lang/Reference;)V;

      Or, if it was being used in parameterized form:

      (LGormInstanceApi<TD;>;Lgroovy/lang/Reference<Ljava/lang/Object;>;)V;

      Similarly it is wrong for the field:

      Lgroovy/lang/Reference<TT;>;;

      T is not in scope (not declared by this type or a surrounding type).

      To see this problem in Eclipse, define a project containing that code above and then create a pure java project that depends upon it. Add this file to the java project:

      class Simple<D> extends GormInstanceApi<D> {
      	 
      	void foo() {	 
      		new VoidSessionCallback() {
      			public void doInSession() {
      			}  
      		} ;
      	}    
      }  
      

      This code is enough to force JDT to load in the GormInstanceApi inner class and discover the inconsistency in signatures:

      Inconsistent classfile encountered: The undefined type parameter T is referenced from within GormInstanceApi<D>.1

      I'd propose the type of the newly created field is set to the raw type for groovy.lang.Reference, but I don't know how to create a raw ClassNode.

        Activity

        Hide
        Andy Clement added a comment -

        Ah, I see, you call them plain references. Please find a patch attached that uses the raw (plain) type reference for the new fields created inside these inner types. I'm afraid I haven't run it against your test suite, but it fixes the inconsistent class issue that eclipse finds.

        Show
        Andy Clement added a comment - Ah, I see, you call them plain references. Please find a patch attached that uses the raw (plain) type reference for the new fields created inside these inner types. I'm afraid I haven't run it against your test suite, but it fixes the inconsistent class issue that eclipse finds.
        Hide
        blackdrag blackdrag added a comment -

        the patch works for 1.8 and 1.9, but in 1.7 not.I committed the patch to 1.8/1.9 then.

        Show
        blackdrag blackdrag added a comment - the patch works for 1.8 and 1.9, but in 1.7 not.I committed the patch to 1.8/1.9 then.
        Hide
        Andy Clement added a comment -

        My users were hitting this with groovy 1.7 so I applied the same change there, this does not work by itself. It looks like it fails because getPlainNodeReference doesn't set the interfaces, only the superclass (and so the users were seeing an NPE when attempting to work with the interfaces for the plain node). So I had to make the additional change in getPlainNodeReference to pass getInterfaces() alongside getSuperclass() when building the new ClassNode.

        Show
        Andy Clement added a comment - My users were hitting this with groovy 1.7 so I applied the same change there, this does not work by itself. It looks like it fails because getPlainNodeReference doesn't set the interfaces, only the superclass (and so the users were seeing an NPE when attempting to work with the interfaces for the plain node). So I had to make the additional change in getPlainNodeReference to pass getInterfaces() alongside getSuperclass() when building the new ClassNode.
        Hide
        Andy Clement added a comment -

        This is the variant of the patch for 1.7.10 - which in addition to using a plain node reference ensures the interfaces are set for the node ref.

        Show
        Andy Clement added a comment - This is the variant of the patch for 1.7.10 - which in addition to using a plain node reference ensures the interfaces are set for the node ref.
        Hide
        blackdrag blackdrag added a comment -

        Andy, with this patch I still get a failing test:
        [groovyc] BUG! exception in phase 'class generation' in source unit '/home/blackdrag/coding/GROOVY_1_7_X/src/test/groovy/lang/ReferenceSerializationTest.groovy' ClassNode#getTypeClass for groovy.lang.Reference is called before the type class is set

        Show
        blackdrag blackdrag added a comment - Andy, with this patch I still get a failing test: [groovyc] BUG! exception in phase 'class generation' in source unit '/home/blackdrag/coding/GROOVY_1_7_X/src/test/groovy/lang/ReferenceSerializationTest.groovy' ClassNode#getTypeClass for groovy.lang.Reference is called before the type class is set
        Hide
        Andy Clement added a comment -

        ok Jochen, I'll take a look when I get some time.

        Show
        Andy Clement added a comment - ok Jochen, I'll take a look when I get some time.
        Hide
        Andy Clement added a comment -

        New version of the patch which copies the clazz across when creating a plain node reference. Seems to pass all the groovy tests.

        Show
        Andy Clement added a comment - New version of the patch which copies the clazz across when creating a plain node reference. Seems to pass all the groovy tests.
        Hide
        blackdrag blackdrag added a comment -

        Andy sorry it took so long... there are some things in the patch that make me wonder if the usage is really correct. A plain node reference is supposed to be a ClassNode that redirects to another one and contains itself no generics information.

        As such setting clazz makes no sense. Normally a redirecting ClassNode should never have clazz set. I can only assume that eclipse is using that API differently here. But this has future conflict potential.

        Next tihng is the interfaces. A redirecting node will ask the node it redirects to for the the interfaces. Therefore it makes no sense to store them directly on the redirecting node as well.

        The part of InnerClassVisitor is already in the repo

        Show
        blackdrag blackdrag added a comment - Andy sorry it took so long... there are some things in the patch that make me wonder if the usage is really correct. A plain node reference is supposed to be a ClassNode that redirects to another one and contains itself no generics information. As such setting clazz makes no sense. Normally a redirecting ClassNode should never have clazz set. I can only assume that eclipse is using that API differently here. But this has future conflict potential. Next tihng is the interfaces. A redirecting node will ask the node it redirects to for the the interfaces. Therefore it makes no sense to store them directly on the redirecting node as well. The part of InnerClassVisitor is already in the repo
        Hide
        Andy Clement added a comment -

        Hey. Been a while since I look at this, but I think I only did the 'extra stuff' to keep some of the groovy infrastructure happy, not eclipse, I was making minor changes and that running the groovy tests. So I found if the clazz or interfaces werent set, groovy tests were failing - if I recall correctly.

        Show
        Andy Clement added a comment - Hey. Been a while since I look at this, but I think I only did the 'extra stuff' to keep some of the groovy infrastructure happy, not eclipse, I was making minor changes and that running the groovy tests. So I found if the clazz or interfaces werent set, groovy tests were failing - if I recall correctly.
        Hide
        blackdrag blackdrag added a comment -

        the fix versions for 1.8 and 2.0 are not really right, but we missed to add those, so I use the current one. I finally managed to apply your patch. I actually tried to not use some of the parts you use, like not setting the interfaces, but didn't work. While not perfect, I think it is ok for 1.7, so I simply applied it

        Show
        blackdrag blackdrag added a comment - the fix versions for 1.8 and 2.0 are not really right, but we missed to add those, so I use the current one. I finally managed to apply your patch. I actually tried to not use some of the parts you use, like not setting the interfaces, but didn't work. While not perfect, I think it is ok for 1.7, so I simply applied it

          People

          • Assignee:
            blackdrag blackdrag
            Reporter:
            Andy Clement
          • Votes:
            7 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: