Community
Participate
Working Groups
Given bug 49295, the compiler message for code like this: interface I {} .. before() : execution(I.new(..)) { .. } // error could be: no constructor-execution on interface type I can't think of a reason why it shouldn't be an error, since it is incorrect. The current behavior (silence) leads developers to believe that their 1.1-era code still works as usual.
Ordinarily, I'd classify this as a P5 enhancement request. However, the fact that we made this "fix" to the weaver to remove constructor execution join points on interfaces (see Bug #49295) for 1.2 makes this a much harder call. In retrospect, we should have added an error or warning for this case when we made that previous change. It's harder to decide what to do at this point after 1.2rc2 has been built. I think that we should add a warning for any use of <InterfaceType>.new in a signature pattern. This is fairly easy to implement so I think we can have confidence that it won't add surprising new bugs. The implementation goes in SignaturePattern.resolveBindings after the declaringType has been resolved. It needs to check that there is a single getExactType() for declaringType and that type is an interface. It also needs to check that there is no '+' to include subtypes (declaringType.isIncludeSubtypes()). If that's all true, and the kind is CONSTRUCTOR, then a warning should be issued. I'd probably make this an Xlint warning, but can't think of the right type just now. Here's my brief risk analysis: Not making the change: The risk is that programs that were using execution (I.new(..)) will now silently behave differently because there is no join point for them to match. This can be very hard to track down. We know that this shows up in at least one actual AspectJ program in the wild based on the message to users "Previously running program broke with 3.0M8/AJDT 1.1.8" from "Miguel J. T. Pessoa Monteiro" Making the change: I think there's minimal risk that we'll break anything unrelated to this issue. The only risk I see is that we could generate additional incorrect Xlint warnings for valid AspectJ programs. So long as these are warnings, they won't prevent compilation and they will be very visible and hopefully therefore they will be promptly reported and could be fixed in a 1.2.1 release.
I've done the implementation. I'm just having trouble getting the words in the xlint message right. I currently have: no constructor join points for interface type XXX [Xlint:noInterfaceCtorJoinpoint] comments? I don't want the user to think there is a way they could mangle their pointcut or their interface definition to 'get at' a ctor jp - so it could be 'interfaces have no exposed constructor join points. Interface type XXX'? This change also causes 7 tests in the harness to fail that were using interface.new() - I'll look at those.
Fix checked in. I've used the error message Wes proposed on the lists. Depending on whether we think interface initialization joinpoints exist, I had to implement it slightly differently than described by Jim - implementing the proposed fix meant we produced a lint warning whenever we encountered a I.new (..) pattern - that means initialization(I.new(..)) was no longer allowed. Seven testcases rely on this being a valid join point. So I had to also check that the shadow kind was ConstructorExecution. Here is the complete piece of logic I added to KindedPointcut.resolveBindings: if (kind == Shadow.ConstructorExecution) { // Bug fix 60936 if (signature.getDeclaringType() != null) { World world = scope.getWorld(); TypeX exactType = signature.getDeclaringType().getExactType(); if (signature.getKind() == Member.CONSTRUCTOR && !exactType.equals(ResolvedTypeX.MISSING) && exactType.isInterface(world) && !signature.getDeclaringType().isIncludeSubtypes()) { world.getLint().noInterfaceCtorJoinpoint.signal(exactType.toString(), getSourceLocation()); } } } This works - preventing execution(I.new(..)) but allowing initialization(I.new (..)). I'm not a fan of this inconsistency though so I will append to the list to check if initialization(I.new(..)) should still be valid?
Interesting. Java never states that there is a constructor with the declared type of I. So if we do, we're adding a definition where there was none. And as Andy points out, it means the same "constructor" signature matches differently depending on the constructor-associated join point. It seems the signature is invalid also for {pre}-initialiation join points, that the seven tests are thereby invalid, and that the correct form is I+.new(..) for all those pcd's. On the other hand, if people use it and it works, what then? Extending the lint warning to the {pre}initialization join points should provoke feedback if disabling it is a problem.
The test suite served its purpose well here. Interface initialization join points are an important case to consider. We need to continue to support join points for the initialization of an interface. AspectJ adds instance variables to interfaces through intertype declarations and these variables will often need initialization code. In fact, the only way to write complicated initialization logic for intertype field declarations on an interface is by place after returning advice on its initialization join point. The syntax for these initialization join points is unfortunate. The first proposals for this pcd was to provide initialization(<typepattern>) instead of initialization(<constructor-signature>). Unfortunately, we realized that there was some value in being able to pick out a specific constructor in rare case, and that you could always get the non-specific version with initialization(<typepattern>.new(..)). This was ugly, but we didn't see any fundamental problems at the time. This discussion has shown that interface initializers are an example of a useful joinpoint that can't really be sensibly be captured by initialization (<constructor-signature>). For 1.2 we should stick with Andy's current change to just warn on execution. We could by more agressive and emit the warning for everything except initialization; however, I don't think that would be worthwhile for 1.2. The main problem that we're trying to solve here is the potentially silent implementation change for these interface constructor executions. For 1.3, we should consider adding the form initialization(<typepattern>) to the language and then warning on ALL uses of <interface>.new. Here's a short program that shows how the initialization of an interface is different from the initialization of its topmost implementing class. The code below will print: before: initialization(C()) before: initialization(I()) initializing I.ax after: initialization(I()) initializing C.x after: initialization(C()) ------------------------------------------------ public class InterfaceInit { public static void main(String[] args) { C c = new C(); } static int GetIntAndPrint(String msg) { System.out.println(msg); return 42; } } class C implements I { int x = InterfaceInit.GetIntAndPrint("initializing C.x"); } interface I {} aspect A { private int I.ax = InterfaceInit.GetIntAndPrint("initializing I.ax"); pointcut init(): initialization(new(..)) && !within(A); before(): init() { System.out.println("before: " + thisJoinPointStaticPart); } after() returning: init() { System.out.println("after: " + thisJoinPointStaticPart); } }
See Jim's proposal for initialization(typepattern). One to look at in AJ 5 M4 if we're going to do this at all...
This bug was resolved bar the remaining potential enhancement to support an initialization (<typepattern>) form. We have no plans to implement that in 1.5.0 so moving this bug to LATER status for consideration in future release planning.
oops, should have removed target milestone too...
LATER/REMIND bugs are being automatically reopened as P5 because the LATER and REMIND resolutions are deprecated.
