@Delegate produces invalid class file

Here is the class file that contains duplicated methods. I should also note that I used IDEA to compile it (#10666).

I tried on trunk (on Eclipse) but did not get the ClassFormatError. I will try again with 1.6.4 and see if I am able to reproduce the error.

2 small questions:

1) In the attached Barb.class, I did not notice any duplicate methods (I checked the synthetic methods too). Is the right class file attached? Which method do you see duplicated?

2) Regarding the NPE - isn't that correct because you have not set the delegate instance "foob" anywhere in your code?

If I change

  static void main(String[] args) {
    new Barb().barb()
  }

to

  static void main(String[] args) {
    new Barb(foob: new Foob()).barb()
  }

then, of-course the NPE goes away as now there a non-null delegate to delegate to.

ad 1. The class file is correct. Examples of duplicated methods are "super$1$wait" (all overloads) and "super$1$equals". I used jclasslib to check this.
ad 2. You are right, I just forgot to instantiate the delegate.

Yes, I can also see those super#xxx methods duplicated. I overlooked them earlier looking more at doIt(), barb() and getters/setters, etc.

So, just to make a note here that NPE is not the issue.

I will try with 1.6.4 and see if I can reproduce the ClassFormatError issue with it.

Tested with 1.6.4, 1.6.x and trunk but couldn't reproduce the duplicate methods (ClassFormatError) issue anywhere. With the delegate instantiated correctly,
both the calls doIt() and this.doIt() work just fine.

Environment I have: java version "1.5.0_04" / windows XP / Eclipse 3.4

I've found this issue as well, when creating a subclass of IGriffonApplication using a BaseGriffonApplication as @Delegate.
I get duplicated entries for super$xyz methods where xyz belong to the Object class. Inspecting the bytecode with javap -c yields that both the @Delegate's methods and the superclass methods are inserted in the class' bytecode.

Happens with 1.6.4, jvm 1.6.0_u16 on cmd line.

Bumping to Critical (I leave to the project despots to consider this a blocker).

This issue causes quite the problem with Griffon. We use @Delegate to generate base implementations of IGriffonApplication. There are two basic implementations that work just fine (SwingApplication and GriffonApplet) when compiled within the Griffon build (uses joint compilation btw). But it fails catastrophically when invoked from outside.

abstract class App {
    @Delegate private final griffon.util.BaseGriffonApplication _base
}

groovyc -cp griffon-rt-0.3-SNAPSHOT.jar App.groovy
javap App | sort | less

Compiled from "App.groovy"
public abstract class App extends java.lang.Object implements griffon.util.IGriffonApplication,groovy.lang.GroovyObject{
    protected groovy.lang.MetaClass $getStaticMetaClass();
    public App();
    public boolean super$1$equals(java.lang.Object);
    public boolean super$1$equals(java.lang.Object);
    public final griffon.util.IGriffonApplication getAppDelegate();
    public groovy.lang.Binding getBindings();
    public groovy.lang.MetaClass getMetaClass();
    public groovy.util.ConfigObject getBuilderConfig();
    public groovy.util.ConfigObject getConfig();
    public int super$1$hashCode();
    public int super$1$hashCode();
    public java.lang.Class getBuilderClass();
    public java.lang.Class getConfigClass();
    public java.lang.Class getEventsClass();
    public java.lang.Class super$1$getClass();
    public java.lang.Class super$1$getClass();
    public java.lang.Object createApplicationContainer();
    public java.lang.Object getEventsConfig();
    public java.lang.Object getProperty(java.lang.String);
    public java.lang.Object invokeMethod(java.lang.String, java.lang.Object);
    public java.lang.Object super$1$clone();
    public java.lang.Object super$1$clone();
    public java.lang.Object this$dist$get$2(java.lang.String);
    public java.lang.Object this$dist$invoke$2(java.lang.String, java.lang.Object);
    public java.lang.String super$1$toString();
    public java.lang.String super$1$toString();
    public java.util.Map getAddonPrefixes();
    public java.util.Map getAddons();
    public java.util.Map getBuilders();
    public java.util.Map getControllers();
    public java.util.Map getGroups();
    public java.util.Map getModels();
    public java.util.Map getMvcGroups();
    public java.util.Map getViews();
    public java.util.Properties getApplicationProperties();
    public static java.lang.Long __timeStamp;
    public static java.lang.Long __timeStamp__239_neverHappen1260928491861;
    public void addApplicationEventListener(java.lang.Object);
    public void addApplicationEventListener(java.lang.String, groovy.lang.Closure);
    public void addMvcGroup(java.lang.String, java.util.Map);
    public void event(java.lang.String);
    public void event(java.lang.String, java.util.List);
    public void initialize();
    public void ready();
    public void removeApplicationEventListener(java.lang.Object);
    public void removeApplicationEventListener(java.lang.String, groovy.lang.Closure);
    public void setAddonPrefixes(java.util.Map);
    public void setAddons(java.util.Map);
    public void setApplicationProperties(java.util.Properties);
    public void setBindings(groovy.lang.Binding);
    public void setBuilderConfig(groovy.util.ConfigObject);
    public void setBuilders(java.util.Map);
    public void setConfig(groovy.util.ConfigObject);
    public void setControllers(java.util.Map);
    public void setEventsConfig(java.lang.Object);
    public void setGroups(java.util.Map);
    public void setMetaClass(groovy.lang.MetaClass);
    public void setModels(java.util.Map);
    public void setMvcGroups(java.util.Map);
    public void setProperty(java.lang.String, java.lang.Object);
    public void setViews(java.util.Map);
    public void shutdown();
    public void startup();
    public void super$1$finalize();
    public void super$1$finalize();
    public void super$1$notify();
    public void super$1$notify();
    public void super$1$notifyAll();
    public void super$1$notifyAll();
    public void super$1$wait();
    public void super$1$wait();
    public void super$1$wait(long);
    public void super$1$wait(long);
    public void super$1$wait(long, int);
    public void super$1$wait(long, int);
    public void this$dist$set$2(java.lang.String, java.lang.Object);
    static {};
    static java.lang.Class class$(java.lang.String);
}

Reproducible with groovy 1.7-rc-2

Attaching a patch that does not generate duplicate MOP calls.

I have also attached the "javap App" output after the patch to confirm that no duplicate MOP methods are being generated.

Attached patch solves the problem. DelegateASTTransformation failed to recognize synthetic methods and skip them. Alex proposed the solution and I was able to verify it on my environment.

I also wanted to skip the synthetic methods there but I couldn't be sure whether skipping all the synthetic methods was safe enough or whether it would interfere with any groovy MC infrastructure. So I put the duplicate skipping logic at the place where it was affecting the least - just the synthetic MOP methods that were getting duplicated.

Skipping duplicates in DelegateASTTransformation is certainly better, if it is safe. The smaller check there will be faster.

Just to get the problem fully... @Delegate adds all non public and non static methods to the class, including the MOP methods. Since they are special to the other class this makes obviously no sense. Ok... but why are they duplicated? That means that most probably Verifier adds those methods ignoring that they may already exist. I think we should add a check in Verifier that ensures the MOP methods are added only if the methods are not already there. If that is the case, then it should cause a compilation error. This way other transforms won't make the same mistake. Checking for duplicated methods in ACG is IMHO not such a good idea, because ACG should not do any checks, it should just output the bytecode. If there are dulpicated methods at this point, then it needs to be solved on an more early point.

But I think the problem here is, that the MOP methods are added in ACG... only... if they are added by ACG, then why is it that @Delegate even sees them? That is maybe only if the other class is precompiled... is that possible?

I assume, that then a check in @Delegate alone would be good enough... but we should maybe think about adding an additional check for MOP methods in Verifier.

Duplicate methods are not added by verifier - in fact those super$1$equals kind of methods are not added as MethodNode(s) to ClassNode at all. ACG directly writes them in the bytecode when it visits the class. That's why doing the check in Verifier is not possible - as duplicates don't even exist then. We can do the check only in ACG or if filtering all synthetic methods in DelegateASTTransformation is enough, then that probably is better.

No, we cannot do that. The duplicate methods get introduced only when ACG is reached. When we are in Verifier, there are no duplictae MOP methods anywhere.

you don't get me et I think... If we check in Verifier fi there are MOP methods already (not about duplicates!) and cause a compile error here, then ACG cannot add duplicated methods, because it won't reach ACG. If it goes through, then there are no MOP methods till ACG adds them, which means there will be no duplicaes either.

Oh, I see - just check for their presence when we are in Verifier. Yes, that will also do. It is just a choice between a new compilation error (solution # 3) or avoiding duplicate MOP methods silently (solution # 1, 2).

A bit late in the game to have it fixed in 1.7.0 final.
It'll have to wait for 1.7.1

Attaching a patch that has both Alex and Jochen's solutions (in my handwriting ).

DelegateASTTransformation now skips adding synthetic methods.

Verifier has a compile check as an additional safety measure and now rejects the class if it finds MOP methods to be already present. This should avoid other, future AST transformation to make the same mistake and suck synthetic MOP methods from other classes too.

Roshan, that patch looks very good.

The issue has many watchers... I wonder why that is.

I can confirm that the @Delegate usage in our codebase works fine with trunk.

Fixed.

Thanks Roshan. This won't go into 1.7.0?! I have yet to see a real-world usage of @Delegate that works without this fix.

Guillaume said no for it to go in 1.7.0 in one of the comments here.

So I did it on all the branches except that.

it should work fine if the class you delegate to was written in Java or if it is not precompiled Groovy... for example compiling both from source at the same time should not impose a problem