Community
Participate
Working Groups
If you make a public ITD of a field on an interfacein AspectJ 1.2, then the compiler mangles it, making it unusable from external clients. It is highly desirable that all public ITD's be accessible, even from pure Java clients. E.g., interface Marker {} public class Base implements Marker {} aspect X { public int Marker.x; } if you then run javap on Base you get: Compiled from "Base.aj" public class Base extends java.lang.Object implements Marker{ public int ajc$interField$X$Marker$x; public Base(); public int ajc$interFieldGet$X$Marker$x(); public void ajc$interFieldSet$X$Marker$x(int); }
some design work needed on this issue.... scheduling for aj5m4
The issue raised by this is that if Base has a field x already then the current design still works. We should consider moving to the same "default implementation" semantics that itdms on interfaces have : define the field if it doesn't exist already (if there's a type clash, obviously that's an error). Let's look at the implications of this change and see if we can do it with minimal disturbance for 1.5.0 RC1.
I'm currently experimenting with this...
Some info on what I've discovered. I changed it so that an ITD field on an interface is a 'default' field that can appear on implementors of that interface that don't already have the field. 1. If the field is already there and of the same type, the ITD doesnt add a field. 2. If the field is already there and of a different type, an error message is produced. 3. If you ITD a field directly on the implementor as well as a similar field on the interface then the ITD on the implementor wins. This led to 2 tests failing in the whole test suite... 1. CompileAndRunTestCase.testInterType This fails because it attempts to refer to two different fields, one ITD'd directly on a type and one ITD'd on an interface that the type happens to implement. With the new mechanism there is only *one* field. 2. Ajc10xTests.test161 fails for reasons of visibility. A public ITD is made onto an interface. An implementor of that interface has a field of the same type and name but with 'default' visibility. It is regarded as a problem since the visibility is seemingly 'reduced' between the interface declaration of the field (??) and the implementors declaration of the field. So: In (2) is a warning for this situation reasonable? (I probably need to finesse it a little from the error text that currently comes out) (implementation pain: It wasn't a trivial implementation. The problem is that accessor methods are generated on the interface for the field. If you don't let the munger also match on the implementor then it never generates the implementation of these accessor methods. So, I had to allow the munger to also match on the implementor. I then allowed the munging of new fields to look at whether the field already exists. this means in the case of two field mungers matching a type then one will create the field and the other will use it. This works fine - the accessor methods are filled in to use the field created by the other munger (or a field thats already there). Of course, we have to ensure the ordering is correct so that the ITD that applied the field directly to the type runs before the one that just wants to build accessors - so I had to tweak some of the processing in ResolvedType.addInterTypeMunger). With this implementation, you will get accessor methods in the interface and implementors but the field will have the original name you specified.
This looks good. I think the advantages of the approach far outweigh the disadvantages caused by the current implementation. In the first failing test, an attempt to disambiguate between two fields becomes non-sensical under the semantics that a public field decl on an interface is a "default implementation" - because there is only one field - so this is a small semantic change, but a good one. In the second case we should produce a warning "cannot reduce visibility of field InterfaceName.fieldName". Note that we have to be careful with the wording of that message - it is not the normal "reduction in visibility of inherited member" deal, because under the proposed new semantics the field is *not* actually inherited from the interface. It's just a default field declaration (and initializer) that gets used if the interface implementor does not declare their own. But it still has to be an error since clients expect the field to be public.
I've never really liked adding instance fields on interfaces, and I wouldn't have minded prohibiting public ones. Does this mean results differ depending on whether the field is declared public? That seems weird. Instead of one hiding the other, it's aliased. for C implements I { int i = 0; } D implements I { } if I say in an aspect int I.i = 2; int I.get() { return i; } // get 2 always but then public int I.i = 2; // get 0 when instanceof C?, 2 when instanceof D? int I.get() { return i; } Even in this regime, I'd like notice of collision, if not to have to declare precedence to resolve it. I'm also wondering about scope. Let's say class C {} // topmost class implementing I class D extends C { int i = 0; } ... int I.i = 10; int I.getI() { return i; } declare parents: C extends I; What's the result of new D().getI(); new C().getI(); Does this make a difference? ((I)new D()).getI(); ((I)new C()).getI(); I.e., D.i normally would hide C.i if i were declared directly on C, right? But the declaring type if I.getI() is I. hmm. Does the declaring type of i differ when it is adopted from C rather than from I? (Sorry, a little fuddled here.)
Sorry - I meant to add on declaring type: which set(* *) matches? if I say in an aspect 0 int I.i = 10 Here, is declaring type is [C] or [C,I]? 1 C implements I { int i = 0; } 2 D implements I { D() { i = 1; }} Declaring type is [I]? 3 E implements I { } 4 F implements I { F() { i = 1; }} What about these? set(int C.i) // 1, 2, but not 0 set(int D.i) // 1, 2, but not 0 set(int E.i) // 3, 4? 0? set(int F.i) // 3, 4? 0? set(int I.i) // 3, 4? not 1, 2, 0?
Adrian and I have been discussing this bug. There are basically two things to address: - the mangling of the field created in the target implementor of the interface - the mangled accessor methods created in the interface and whose implementations are generated in the implementor of the interface. My prototype does the first part, not mangling the name on the implementor. But what can we do about the accessors? We cannot remove them because that makes referring to 'Interface.itdField' impossible - and if we don't mangle the names of the accessors then they will be prettier but may clash with members existing in the target implementor. And we can't assume that if they do clash then the implementor has implemented them exactly as we'd expect. (also, reminder to self, this would mean changing our mechanism from what we have at the moment to consider public/private/default visiblity ITD fields on interfaces as different kinds of ITD - we'd only avoid mangling the public ones - the others would continue to work as they do today). given all that and I've not even started addressing Wes' remarks ... this isn't going to be in 1.5.0...
Created attachment 29691 [details] Zip contains two patches that prototype the change Patch for this - not easily merged because it also contains some changes for java5 bridge methods - i just didnt want to lose the prototype work.
Can we resurrect this? I just hit the issue again. The classes in my use case are listed below. The problem is when I attempt to query (via OpenJPA in this case) some class that implements HasLabel (and thereby receives the introduced implementation), OpenJPA barfs: Caused by: <openjpa-2.1.1-r422266:1148538 nonfatal user error> org.apache.openjpa.persistence.ArgumentException: An error occurred while parsing the query filter "SELECT c from Thing c WHERE c.label = :label". Error message: No field named "label" in "Thing". ... Expected one of the available field names in "org.example.model.Thing" My workaround is to use reflection utility methods to find the mangled name and use that to formulate my JPA queries... :( -matthew ===== package org.example.model.interfaces; public interface HasLabel { String getLabel(); } ===== package org.example.model.aspects.volleyball; import javax.persistence.Column; import org.example.model.annotations.Internal; public aspect HasLabel { private interface Mixin extends org.example.model.interfaces.HasLabel {} declare parents: org.example.model..* && org.example.model.interfaces.HasLabel+ && !org.example.model.interfaces.HasLabel implements Mixin; @Column(name = "label", unique = true) private String Mixin.label; public String Mixin.getLabel() { return label; } @Internal public void Mixin.setLabel(String label) { this.label = label; } } =====
Hi guys, Different project, same issue. Is there any way to get this added to the next release? Thanks, Matthew
FIXED I resurrected the patch I created *7* years ago and applied it. ITD fields on interfaces can now be thought of as 'default fields' (like ITD methods are default methods) so an implementor of the interface will get the field if they don't have it but if they do have it, then the pre-existing one will be used. As with the other change for itd fields (onto classes) the old behaviour can be activated using -Xset:itdVersion=1. There may be some visibility nits to iron out. The accessor methods are still created but if the class already has the field the accessors will be returning the value of that field (the accessors are only used when remapping attempts to access the field on the interface type).
Thanks a lot for fixing this! :)
