Bug 255856 - @Aspect allows + with @target(), whereas code style doesn't
Summary: @Aspect allows + with @target(), whereas code style doesn't
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: DEVELOPMENT   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 1.6.3   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-19 14:22 EST by Ramnivas Laddad CLA
Modified: 2008-12-08 15:43 EST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ramnivas Laddad CLA 2008-11-19 14:22:18 EST
A few bugs, but I suspect there is only one underlying bug with various symptoms, so adding only one bug. In the code below:

1. @target(example.TargetExecutionCombinationBug.TestAnnotation)+ (note the '+') is allowed in @AspectJ syntax, but not in the code style syntax. Adding '+' also matches the corresponding execution join point.

2. Swapping the components of @target(example.TargetExecutionCombinationBug.TestAnnotation)+ && execution(* *(..)) to execution(* *(..)) && @target(example.TargetExecutionCombinationBug.TestAnnotation)+ changes the selected join points.

2. AJDT shows that it matches even other join points -- set() for the count, for example. This is with @target(example.TargetExecutionCombinationBug.TestAnnotation)+ && execution(* *(..)) pointcut.

==
package example;

import java.lang.annotation.ElementType;
import java.lang.annotation.Inherited;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import junit.framework.TestCase;

import org.aspectj.lang.Aspects;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Before;

public class TargetExecutionCombinationBug extends TestCase {

	public void testAdviceMatch() {
		TestImpl impl = new TestImpl();
		impl.method();
		
		assertEquals(0, Aspects.aspectOf(TestAtAspect.class).count);
		assertEquals(0, TestAspect.aspectOf().count);
	}
	

	@Retention(RetentionPolicy.RUNTIME)
	@Target(ElementType.TYPE)
	@Inherited
	static @interface TestAnnotation {
	}

	@TestAnnotation
	static interface TestInterface {
		void method();
	}

	static class TestImpl implements TestInterface {
		@Override
		public void method() {
		}
	}

	static aspect TestAspect {
		int count = 0;
		
		before() : @target(example.TargetExecutionCombinationBug.TestAnnotation) && execution(* *(..)) {
			count++;
		}
	}

	@Aspect
	static class TestAtAspect {
		int count = 0;
		
		@Before("@target(example.TargetExecutionCombinationBug.TestAnnotation) && execution(* *(..))") 
		public void increment() {
			count++;
		}
	}
}
Comment 1 Andrew Clement CLA 2008-12-03 14:51:39 EST
so... is the bug that we support @target(...)+ and should not allow it then?

The test program doesn't actually use @target(...)+ anywhere...
Comment 2 Ramnivas Laddad CLA 2008-12-03 18:28:44 EST
It seems that this is related to #128664. Essentially, we need to decide how we treat + with annotation-based pointcuts. Then we can either support @target()+ in both syntax or disallow in both.

Here is updated test case to use @target()+ (the second assertion fails and AJDT shows advice application to too many elements).

package example;

import java.lang.annotation.ElementType;
import java.lang.annotation.Inherited;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import junit.framework.TestCase;

import org.aspectj.lang.Aspects;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Before;

public class TargetExecutionCombinationBug extends TestCase {

	public void testAdviceMatch() {
		TestImpl impl = new TestImpl();
		impl.method();
		
		assertEquals(0, Aspects.aspectOf(TestAtAspect.class).countExecutionBeforeAtTarget);		
		assertEquals(0, Aspects.aspectOf(TestAtAspect.class).countAtTargetExecutionBefore);
		assertEquals(0, TestAspect.aspectOf().count);
	}
	

	@Retention(RetentionPolicy.RUNTIME)
	@Target(ElementType.TYPE)
	@Inherited
	static @interface TestAnnotation {
	}

	@TestAnnotation
	static interface TestInterface {
		void method();
	}

	static class TestImpl implements TestInterface {
		@Override
		public void method() {
		}
	}

	static aspect TestAspect {
		int count = 0;
		
		before() : @target(example.TargetExecutionCombinationBug.TestAnnotation) && execution(* *(..)) {
			count++;
		}
	}

	@Aspect
	static class TestAtAspect {
		int countExecutionBeforeAtTarget = 0;
		int countAtTargetExecutionBefore = 0;		
		
		@Before("execution(* *(..)) && @target(example.TargetExecutionCombinationBug.TestAnnotation)+") 
		public void incrementCountExecutionBeforeAtTarget() {
			countExecutionBeforeAtTarget++;
		}

		@Before("@target(example.TargetExecutionCombinationBug.TestAnnotation)+ && execution(* *(..))") 
		public void incrementCountAtTargetExecutionBefore() {
			countAtTargetExecutionBefore++;
		}
	
	}
}


Comment 3 Andrew Clement CLA 2008-12-03 19:55:30 EST
i agree it may  be related to that other problem.  But personally in this bug I dont like the look of the syntax: @target(...)+
Comment 4 Ramnivas Laddad CLA 2008-12-03 22:41:44 EST
(In reply to comment #3)
> i agree it may  be related to that other problem.  But personally in this bug I
> dont like the look of the syntax: @target(...)+
> 

I think, for now, we should disallow @target()+ in @AspectJ syntax to match it with the code style. Later, we can look at the whole issue in a holistic way. 

BTW, here is the test case with a typo fixed:

package example;

import java.lang.annotation.ElementType;
import java.lang.annotation.Inherited;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import junit.framework.TestCase;

import org.aspectj.lang.Aspects;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Before;

public class TargetExecutionCombinationBug extends TestCase {

	public void testAdviceMatch() {
		TestImpl impl = new TestImpl();
		impl.method();
		
		assertEquals(0, Aspects.aspectOf(TestAtAspect.class).countExecutionBeforeAtTarget);		
		assertEquals(0, Aspects.aspectOf(TestAtAspect.class).countAtTargetBeforeExecution);
		assertEquals(0, TestAspect.aspectOf().count);
	}
	

	@Retention(RetentionPolicy.RUNTIME)
	@Target(ElementType.TYPE)
	@Inherited
	static @interface TestAnnotation {
	}

	@TestAnnotation
	static interface TestInterface {
		void method();
	}

	static class TestImpl implements TestInterface {
		@Override
		public void method() {
		}
	}

	static aspect TestAspect {
		int count = 0;
		
		before() : @target(example.TargetExecutionCombinationBug.TestAnnotation) && execution(* *(..)) {
			count++;
		}
	}

	@Aspect
	static class TestAtAspect {
		int countExecutionBeforeAtTarget = 0;
		int countAtTargetBeforeExecution = 0;		
		
		@Before("execution(* *(..)) && @target(example.TargetExecutionCombinationBug.TestAnnotation)+") 
		public void incrementCountExecutionBeforeAtTarget() {
			countExecutionBeforeAtTarget++;
		}

		@Before("@target(example.TargetExecutionCombinationBug.TestAnnotation)+ && execution(* *(..))") 
		public void incrementCountAtTargetBeforeExecution() {
			countAtTargetBeforeExecution++;
		}
	
	}
}

Comment 5 Andrew Clement CLA 2008-12-08 15:43:22 EST
this is actually quite serious... the '+' causes the pointcut parser to terminate early without processing the rest of the pointcut.  So

@target(X)+ && execution(blah)

will only be read as @target(X) and so match too many joinpoints. (symptom 3)

Reversing them means the parser does actually see execution(blah) - so you get the right result (symptom 2)

and so I've fixed it by banning the use of + in that position.  Code style always did the right thing - as usual ;)