Bug 122370 - [@AspectJ] @DeclareParents/declare parents not equivalent
Summary: [@AspectJ] @DeclareParents/declare parents not equivalent
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: Library (show other bugs)
Version: 1.5.0   Edit
Hardware: PC Linux
: P3 critical (vote)
Target Milestone: 1.5.1   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-12-30 00:54 EST by Brian Ericson CLA
Modified: 2012-04-03 16:15 EDT (History)
0 users

See Also:


Attachments
patch containing failing testcase (7.34 KB, patch)
2006-02-16 09:41 EST, Helen Beeken CLA
no flags Details | Diff
failing testcase (4.65 KB, patch)
2006-02-16 10:40 EST, Helen Beeken CLA
aclement: iplog+
Details | Diff
patch containing fix (1.87 KB, patch)
2006-02-16 10:43 EST, Helen Beeken CLA
aclement: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Ericson CLA 2005-12-30 00:54:07 EST
Adding a setter to the Moody example makes it clear that the annotation style (@DeclareParents) results in different behavior than the classic (declare parents) style.

Calling the setter in a "declare parents"-advised object behaves as one would expect.  However, calling the setter with a "@DeclareParents"-advised object has no effect -- that is, you can call the setter (and see that it's been called) and immediately call the getter, only to find that the value didn't change.  The following examples (all in "package moody;") illustrate:

public enum Mood { HAPPY, JOLLY }

Classic style:
-------------
public aspect ClassicMoodIndicator {
   public interface Moody {
      Mood getMood();
      void setMood(Mood mood);
   }

   private Mood Moody.mood = Mood.HAPPY;

   public Mood Moody.getMood() { return mood; }
   public void Moody.setMood(Mood mood) { this.mood = mood; }

   declare parents : moody.ClassicMoodImplementor implements Moody;
}

public class ClassicMoodImplementor { }

Annotation style:
----------------
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.DeclareParents;

@Aspect
public class AnnotationMoodIndicator {
   public interface Moody {
      Mood getMood();
      void setMood(Mood mood);
   }

   public static class MoodyImpl implements Moody {
      private Mood mood = Mood.HAPPY;

      public Mood getMood() { return mood; }
      public void setMood(Mood mood) { this.mood = mood; }
   }

   @DeclareParents(value="moody.AnnotationMoodImplementor",defaultImpl=MoodyImpl.class)
   private Moody implementedInterface;
}

public class AnnotationMoodImplementor { }

JUnit TestCase:
--------------
import junit.framework.TestCase;

public class MoodTester extends TestCase {
   public MoodTester(String name) { super(name); }

   public void testClassic() {
      ClassicMoodImplementor cmi0 = new ClassicMoodImplementor();
      ClassicMoodImplementor cmi1 = new ClassicMoodImplementor();
      
      assertEquals("cmi0 should be HAPPY", Mood.HAPPY, cmi0.getMood());
      
      cmi1.setMood(Mood.JOLLY);
      assertEquals("cmi1 should be JOLLY", Mood.JOLLY, cmi1.getMood());
      assertEquals("cmi0 should be *still* be HAPPY", Mood.HAPPY, cmi0.getMood());
   }
   
   public void testAnnotation() {
      AnnotationMoodImplementor ami0 = new AnnotationMoodImplementor();
      AnnotationMoodImplementor ami1 = new AnnotationMoodImplementor();
      
      assertEquals("ami0 should be HAPPY", Mood.HAPPY, ((AnnotationMoodIndicator.Moody) ami0).getMood());
      
      ((AnnotationMoodIndicator.Moody) ami1).setMood(Mood.JOLLY);
      assertEquals("ami1 should be JOLLY", Mood.JOLLY, ((AnnotationMoodIndicator.Moody) ami1).getMood());
      assertEquals("ami0 should be *still* be HAPPY", Mood.HAPPY, ((AnnotationMoodIndicator.Moody) ami0).getMood());
   }
}

Result:
------
The test run is as follows:
..F
Time: 0.021
There was 1 failure:
1) testAnnotation(moody.MoodTester)junit.framework.AssertionFailedError: ami1 should be JOLLY expected:<JOLLY> but was:<HAPPY>
       at moody.MoodTester.testAnnotation(MoodTester.java:27)
       at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
       at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
       at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
       at moody.MoodTester.main(MoodTester.java:7)

FAILURES!!!
Tests run: 2,  Failures: 1,  Errors: 0

For classic, you can see that cmi1.setMood was successful and cmi0/cmi1 are decoupled (that is, calling setMood on cmi1 has no impact on cmi0).  The annotation style, however, fails because ami1.setMood appears to have done nothing.  The result should have been that ami1 is JOLLY and ami0 HAPPY (I include the second assert because both ami0 & ami1 ended up JOLLY in RC1).

The current behavior makes @DeclareParents unusable for retaining field state (near as I can tell, it retains state throughout the method invocation, but not beyond it, like it is creating and discarding a backing object with each method invocation).
Comment 1 Helen Beeken CLA 2006-02-16 09:41:46 EST
Created attachment 34841 [details]
patch containing failing testcase

Apply this patch to the test project.

This patch contains two tests converted from the supplied testcase. The first is the failing case with @AspectJ style and the second is the code style. The code style test is only there for investigation and shouldn't be checked into HEAD.
Comment 2 Helen Beeken CLA 2006-02-16 10:38:47 EST
Decompiling AnnotationMoodIndicator we see the following:

   0:   aload_0
   1:   getfield        #19; //Field ajc$moody_AnnotationMoodIndicator$moody_Ann
otationMoodIndicator$Moody:Lmoody/AnnotationMoodIndicator$Moody;
   4:   ifnull  7                                       <---- cause of failure
   7:   aload_0
   8:   new     #21; //class moody/AnnotationMoodIndicator$MoodyImpl
   11:  dup
   12:  invokespecial   #22; //Method moody/AnnotationMoodIndicator$MoodyImpl."<
init>":()V
   15:  putfield        #19; //Field ajc$moody_AnnotationMoodIndicator$moody_Ann
otationMoodIndicator$Moody:Lmoody/AnnotationMoodIndicator$Moody;
   18:  aload_0

for both the getMood() and setMood(moody.Mood) methods. In other words, no matter what, we always create a new instance of MoodyImpl rather than using the one we've got. Instead what we want to do is to only create a new one if we don't already have one. Changing BcelTypeMunger.mungeMethodDelegate(..) to do this results in:

   0:   aload_0
   1:   getfield        #19; //Field ajc$moody_AnnotationMoodIndicator$moody_Ann
otationMoodIndicator$Moody:Lmoody/AnnotationMoodIndicator$Moody;
   4:   ifnonnull       18
   7:   aload_0
   8:   new     #21; //class moody/AnnotationMoodIndicator$MoodyImpl
   11:  dup
   12:  invokespecial   #22; //Method moody/AnnotationMoodIndicator$MoodyImpl."<
init>":()V
   15:  putfield        #19; //Field ajc$moody_AnnotationMoodIndicator$moody_Ann
otationMoodIndicator$Moody:Lmoody/AnnotationMoodIndicator$Moody;
   18:  aload_0

and everything works as expected.
Comment 3 Helen Beeken CLA 2006-02-16 10:40:13 EST
Created attachment 34845 [details]
failing testcase

Apply this patch to the tests project.

Replaces the previous testcase patch as removes the code style case.
Comment 4 Helen Beeken CLA 2006-02-16 10:43:27 EST
Created attachment 34846 [details]
patch containing fix

Apply this patch to the weaver project.

This patch contains the fix described in comment #2.
Comment 5 Andrew Clement CLA 2006-02-16 11:52:10 EST
patch is committed - I dont think the code gen can have been tested, this is a very serious bug.  Luckily our 1.5.1 release is imminent.

It now correctly creates and *keeps* an instance for the multiple calls to get/set.

The fix will be available in a development build soon.
Comment 6 Andrew Clement CLA 2006-02-17 07:42:34 EST
fix available.