jira.codehaus.org

  • Log In Access more options
    • Online Help
    • Keyboard Shortcuts
    • About JIRA
    • JIRA Credits
    • What?s New
  • Dashboards Access more options (Alt+d)
  • Projects Access more options (Alt+p)
  • Issues Access more options (Alt+i)
  • groovy
  • GROOVY-4934

incorrect signature attributes in class files for inner class generics

  • Log In
  • Views
    • XML
    • Word
    • Printable

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.8.0
  • Fix Version/s: 1.8.6, 2.0-beta-3, 1.7.11
  • Component/s: bytecode, class generator
  • Labels:
    None

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.

  • Options
    • Sort By Name
    • Sort By Date
    • Ascending
    • Descending
    • Download All

Attachments

  1. Text File
    version3.patch
    18/Aug/11 12:31 PM
    2 kB
    Andy Clement

Activity

Ascending order - Click to sort in descending order
  • All
  • Comments
  • Work Log
  • History
  • Activity
Hide
Permalink
Andy Clement added a comment - 19/Jul/11 10:14 PM

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 - 19/Jul/11 10:14 PM 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
Permalink
blackdrag blackdrag added a comment - 20/Jul/11 4:45 AM

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 - 20/Jul/11 4:45 AM 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
Permalink
Andy Clement added a comment - 26/Jul/11 10:11 AM

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 - 26/Jul/11 10:11 AM 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
Permalink
Andy Clement added a comment - 05/Aug/11 12:35 PM

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 - 05/Aug/11 12:35 PM 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
Permalink
blackdrag blackdrag added a comment - 08/Aug/11 9:30 AM

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 - 08/Aug/11 9:30 AM 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
Permalink
Andy Clement added a comment - 08/Aug/11 10:12 AM

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

Show
Andy Clement added a comment - 08/Aug/11 10:12 AM ok Jochen, I'll take a look when I get some time.
Hide
Permalink
Andy Clement added a comment - 18/Aug/11 12:31 PM

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 - 18/Aug/11 12:31 PM New version of the patch which copies the clazz across when creating a plain node reference. Seems to pass all the groovy tests.
Hide
Permalink
blackdrag blackdrag added a comment - 22/Sep/11 6:18 AM

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 - 22/Sep/11 6:18 AM 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
Permalink
Andy Clement added a comment - 22/Sep/11 10:09 AM

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 - 22/Sep/11 10:09 AM 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
Permalink
blackdrag blackdrag added a comment - 01/Feb/12 12:47 PM

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 - 01/Feb/12 12:47 PM 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
Vote (7)
Watch (6)

Dates

  • Created:
    19/Jul/11 7:53 PM
    Updated:
    01/Feb/12 12:47 PM
    Resolved:
    01/Feb/12 12:47 PM
  • Atlassian JIRA (v5.0.4#731-sha1:3aa7374)
  • Report a problem
  • Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.