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 -

        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: