Duplicated annotations when weaving methods with same name and same number of parameters

[surli]

Bug originally reported on https://bugs.eclipse.org/bugs/show_bug.cgi?id=582107 but reporting it there too since I'm not sure which channel should be preferred.

We discovered that apparently weaving two methods with same name and same number of parameters automatically leads to putting annotations of the first method to both the first and second method in the generated class, removing the possible existing annotations for it. We're using AspectJ 1.9.19.

So in our case we have an aspect file containing two methods:

    @Deprecated(since = "2.2M2")
    public void XWikiDocument.rename(String newDocumentName, XWikiContext context) throws XWikiException
    {
        // Do stuff
    }

    @Deprecated(since = "12.5RC1")
    public void XWikiDocument.rename(DocumentReference newReference, XWikiContext context) throws XWikiException
    {
        // Do other stuff
    }

The obtained bytecode contains:

    @Deprecated(since = "2.2M2")
    public void rename(String newDocumentName, XWikiContext context) throws XWikiException
    {
        // Do stuff
    }

    @Deprecated(since = "2.2M2")
    public void rename(DocumentReference newReference, XWikiContext context) throws XWikiException
    {
        // Do other stuff
    }

When trying to understand the behaviour, I added another annotation to the first method:

    @Deprecated(since = "2.2M2")
    @javax.validation.constraints.Size(message = "toto")
    public void XWikiDocument.rename(String newDocumentName, XWikiContext context) throws XWikiException
    {
        // Do stuff
    }

    @Deprecated(since = "12.5RC1")
    public void XWikiDocument.rename(DocumentReference newReference, XWikiContext context) throws XWikiException
    {
        // Do other stuff
    }

And I obtained:

    @Deprecated(since = "2.2M2")
    @javax.validation.constraints.Size(message = "toto")
    public void rename(String newDocumentName, XWikiContext context) throws XWikiException
    {
        // Do stuff
    }

    @Deprecated(since = "2.2M2")
    @javax.validation.constraints.Size(message = "toto")
    public void rename(DocumentReference newReference, XWikiContext context) throws XWikiException
    {
        // Do other stuff
    }

Note that we don't have a method called rename with 2 parameters in the original XWikiDocument.java.

[kriegaex]

@surli, here is the right place to report new issues. I am going to close the Bugzilla one.

weaving two methods with same name and same number of parameters automatically leads to putting annotations of the first method to both the first and second method in the generated class

Weaving how? What kind of aspect? Please provide a reproducer. Thanks.

[surli]

Weaving how? What kind of aspect? Please provide a reproducer. Thanks.

I created a toy project reproducing the bug in https://github.com/surli/sandbox/tree/bug-aspectj/aspectjbug The aspect is defined in https://github.com/surli/sandbox/blob/bug-aspectj/aspectjbug/aspectjbug-legacy/src/main/aspect/org/example/AppCompatibilityAspect.aj and aims at weaving the empty class from the other module: https://github.com/surli/sandbox/blob/bug-aspectj/aspectjbug/aspectjbug-module/src/main/java/org/example/App.java

The configuration for weaving is given in the pom: https://github.com/surli/sandbox/blob/bug-aspectj/aspectjbug/aspectjbug-legacy/pom.xml#L50

I obtain the following binary when executing it:

public class App {
    public App() {
    }

    public static void main(String[] args) {
        System.out.println("Hello World!");
    }

    @Deprecated(
        since = "2.2M2"
    )
    @Size(
        message = "toto"
    )
    public void foo(int var1) {
        AppCompatibilityAspect.ajc$interMethod$org_example_AppCompatibilityAspect$org_example_App$foo(this, var1);
    }

    @Deprecated(
        since = "2.2M2"
    )
    @Size(
        message = "toto"
    )
    public void foo(String var1) {
        AppCompatibilityAspect.ajc$interMethod$org_example_AppCompatibilityAspect$org_example_App$foo(this, var1);
    }
}

There might be an easier way to reproduce but it's the closest we have in our project and easiest for me to have something quickly.

[kriegaex]

I am able to reproduce the effect, but am unsure when I will be able to investigate and debug this. I am quite busy these days, but want you to know that your reproducer does what it is supposed to: It reproduces the problem. 👍

It is kind of an exotic edge case for ITD, but of course it should work as you expect.

[kriegaex]

Note to myself regarding my own findings:

• The number of arguments for methods with the same name is indeed important. If multiple ITD methods have the same name and number of arguments, the problem occurs.
• The method return types can be different, the problem still occurs.
• The internal (not lexical source code) order of ITD methods in the aspect seems to be important: Always the annotations from the first one found are copied over into the target class. Even if that method contains no annotations and the other ones do contain one or more annotations, the target class methods will not be annotated, i.e. the empty list/set of annotations from the first method found will be copied.
• The problem occurs during weaving. The compiled aspect itself contains the right methods with the correct annotations, i.e. aspect compilation as such is not the problem.

This feels like some kind of equals / hashCode problem, but it could also be something else entirely.

[kriegaex]

@aclement, while working on Bugzilla 112105 back in 2005, you added this piece of code to BcelTypeMunger#getRealMemberForITDFromAspect:

// If not related to a ctor ITD then the name is enough to confirm we have the
// right one.  If it is ctor related we need to check the params all match, although
// only the erasure.
if (isCtorRelated) {
  for (int j = 0; j < memberParams.length && matchOK; j++){
  ResolvedType pMember = memberParams[j].resolve(world);
  ResolvedType pLookingFor = lookingForParams[j].resolve(world); 

  if (pMember.isTypeVariableReference()) 
    pMember = ((TypeVariableReference)pMember).getTypeVariable().getFirstBound().resolve(world);
  if (pMember.isParameterizedType() || pMember.isGenericType()) 
    pMember = pMember.getRawType().resolve(aspectType.getWorld());

  if (pLookingFor.isTypeVariableReference()) 
    pLookingFor = ((TypeVariableReference)pLookingFor).getTypeVariable().getFirstBound().resolve(world);
  if (pLookingFor.isParameterizedType() || pLookingFor.isGenericType()) 
    pLookingFor = pLookingFor.getRawType().resolve(world);

  if (debug) System.err.println("Comparing parameter "+j+"   member="+pMember+"   lookingFor="+pLookingFor);
  if (!pMember.equals(pLookingFor)){
    matchOK=false;
  }
}

I think that if (isCtorRelated) and the reasoning in your comment is not quite correct, because it directly leads to the problem seen here: For each ITD method, not just for constructors, the paramater list needs to be checked. If I remove the condition, my test in e8cdb759 passes.

Andy, can you please tell me if you think that eliminating the condition would have any possibly negative side effects? I am going to commit and probably merge it like this as a bugfix, if all other tests continue to pass. I can still revert if you object. But I really would feel better, if you could take a look.

[kriegaex]

Actually, the side effect was rather positive, because two tests started failing, uncovering problems comparing array reference types for equality. A new equals method fixes that, see 9cbbb3e5.

[kriegaex]

@surli, I published a 1.9.20-SNAPSHOT with the fix. You can try it, adding this to your POM:

<repositories>
    <repository>
        <id>ossrh-snapshots</id>
        <url>https://oss.sonatype.org/content/repositories/snapshots</url>
        <releases>
            <enabled>false</enabled>
        </releases>
        <snapshots>
            <enabled>true</enabled>
            <updatePolicy>always</updatePolicy>
        </snapshots>
    </repository>
</repositories>

[surli]

@surli, I published a 1.9.20-SNAPSHOT with the fix.

Well that was fast :) I just checked on the original project where I spotted the bug and apparently it's now running properly, so for me it looks all good :+1:

[surli]

@kriegaex any idea when we can expect a release for 1.9.20?

[kriegaex]

@surli, the release is out since last week. I have no idea why I got notified about this comment again today. I see no change, and you asked on June 26th already. At the time, I had no answer for you yet.