Community
Participate
Working Groups
The class JDTLikeHandle provider doesn't quite produce handles the same as JDT. To prevent a need to build a duplicate world in AJDT where the handles are correct it would be better to make the handles produced by AspectJ correct and immediately usable as IJavaElement handles in as many cases as possible
Fixes in so far: 1. Included source folders in the model. These were never there previously which meant the use of source folders in eclipse projects immediately meant handles from AspectJ would be incorrect. Including source folders had two side effects: (a) ajdoc broke completely as some of the code to search for elements in the model was assuming packages immediately came under root whereas now there may be source folders in between. (b) a package may exist at two places in the same tree if types declared in that package are in different source folders. 2. Changed definition of !<n> suffixes for advice. Only if absolutely necessary are the numbers included (ie. two pieces of before advice may not need a number suffix if they vary by signature). 3. Included the !<n> suffix for multiple anonymous inner types declarations. 4. Fixed names of anonymous inner types to not include {..} in their handles 5. Fixed names of named inner types to avoid inclusion of a number at the start of their name. Eg. was 1NamedClass but is now just NamedClass 6. When the root is not a .lst file, it is assumed to be a project and so the handle prefixes the name with an '=', a la JDT. More to be done: handles into stuff in other projects (via classpath and perhaps directly as project references).
I looked into how JDT handles are generated for package roots that are not physically located in the project. Here is what I have. In all of these cases, I am showing the handle to the package fragment root. To handle compilation units add to the handle: <package.name{cuname.java To handle compilation units add to the handle: <package.name*cuname.aj To handle class files add to the handle: <package.name(cuname.class Jar file in the project: =a/lib.jar Jar file in another project: =a/\/b\/lib.jar in a sub folder of another project (notice that the \ is used as an escape character for the /s): =a/\/b\/jarFolder\/lib.jar external folder: =a/\/Users\/andrew\/Eclipse\/Workspaces\/runtime-workspace-AJDT1.6\/b\/jarFolder\/lib.jar binary folders linked from another project (b/java/bin): =a/\/b\/java\/bin externally linked binary folder (I'm actually not sure what this means and how to determine the .link# part): =a/\/.org.eclipse.jdt.core.external.folders\/.link0 references to source folders in other projects (when there is a project reference on the classpath) This is just like a standard reference in another project: =b/src reference to a linked source folder (src-linked is the linked name, even though the actual folder exists somewhere on the file system): =a/src-linked In order to get the actual location of this folder, it is necessary to do something like this: javaProject.getPackageFragmentRoot("src-linked").getResource().getLocation()
(In reply to comment #1) > > 4. Fixed names of anonymous inner types to not include {..} in their handles > I was slightly wrong about this one. I had told you earlier that anonymous inner types should look like "new ParentType". Actually, it seems that anonymous types are, well...anonymous. They have no name. So, the handle for the anonymous class in this code: package a; aspect A { void b(String x) { new Condition { }; } } is this: =Proj/src<a*A.aj}A~b~QString;[ and not this (as I said before): =Proj/src<a*A.aj}A~b~QString;[new Condition Also, note that in this case: package a; aspect A { void b(String x) { new Condition { }; new Condition { }; } } The two anonymous types are: =Proj/src<a*A.aj}A~b~QString;[ =Proj/src<a*A.aj}A~b~QString;[!2
Just found something else. It seems that AJDT ignores the "returning" parameter of afterreturning advice. It just ignores it. I think it has something to do with how the file gets parsed. Currently, AspectJ appends the returning parameter at the end of the parameter list for the advice. The affect is this: package p; aspect Foo { after(String x) returning(String y) : (...) { } } in aspectJ: =Proj/src<p*Foo.aj[Foo*&afterReturning&QString;&QString; in AJDT (notice one fewer parameters): =Proj/src<p*Foo.aj[Foo*&afterReturning&QString; I think the easiest solution would be to change AspectJ (rather than go change the parser for AJDT). So that aspectJ simply does not add the "returning" parameter to the end of the handle. So, we would have this situation: package p; aspect Foo { after(String x) returning(String y) : (...) { } after(String x) returning(int y) : (...) { } } The handles would be: =Proj/src<p*Foo.aj[Foo*&afterReturning&QString; =Proj/src<p*Foo.aj[Foo*&afterReturning&QString;!2
comment 3 issue addressed and new AJ committed to support it. Given the complete anonymity, what are the handles for two different anonymous types? aspect A { void b(String x) { new SomeOldThing { }; new SomeOtherOldThing { }; } } Are they [ and [!2 ? If so I may need to fix something. I need to think about the situation in comment 4 - we may not persist enough information in the model from the declaration to know which param to discard from the handle. I am also fixing the compile error in AJDT HEAD - returning null for the new interface method should make it just behave as before.
Thanks for getting this done. (In reply to comment #5) >Given the > complete anonymity, what are the handles for two different anonymous types? > Are they [ and [!2 ? If so I may need to fix something. Yes, that is what they are. > I need to think about the situation in comment 4 - we may not persist enough > information in the model from the declaration to know which param to discard > from the handle. Hmmm...if it isn't an easy thing to do on the aspectj side, I can look into doing it for AJDT, but a quick glance at the code showed me that it's not straight forward.
Regarding handles for binary and external class files and compilation units, I provided a lot of different examples, but I wonder if AJDT can get around this in most situations. I need these handles so that the opposite end of a reference can be navigated to. I also need a proper link name. Does the compiler have access to the compilation unit name and line number for the opposite end of a relationship even if the target is binary?
From comment 6, I've now fixed handles for differing anonymous class declarations. If you could cope with the after returning extra parameter on the AJDT side, that would be nice. It seems a shame to go against the eclipse model for a member that defines the handle to include the name and all params. On comment 7, for a binary we don't know the compilation unit it came from and the line number info is likely to be very hit and miss, since even if line number entries are included for methods, it doesn't mean we can be that specific for many join points. What is a 'proper link name' ? Where do you try and get it from? Is it just the funky bit of the handle that points elsewhere? (as described in comment 2). I guess my goal for the changes up to 1.6.2 is to get all 'within the same project' handles to match whatever AJDT needs them to. For 1.6.3 I would then tackle external project refs.
(In reply to comment #8) > From comment 6, I've now fixed handles for differing anonymous class > declarations. Thanks. > If you could cope with the after returning extra parameter on the AJDT side, > that would be nice. It seems a shame to go against the eclipse model for a > member that defines the handle to include the name and all params. I'll look into this. > On comment 7, for a binary we don't know the compilation unit it came from and > the line number info is likely to be very hit and miss, since even if line > number entries are included for methods, it doesn't mean we can be that > specific for many join points. > What is a 'proper link name' ? Should have been a little clearer here. The link name is the human readable text that is displayed on labels and menu items so that programmers know what the link refers to. Given enough information (eg- qualified name, element type, etc), AJDT can recreate this. Actually, it's possible that if AJDT is given enough information to create the link, the link name should be possible to create, too. But if AspectJ doesn't have this information, then the whole thing might be moot. For AJDT 1.6.1, I'd like to see navigation and labeling work for in-project references, and fail gracefully for external references (eg- with a label "In jar aspect" as before).
Been exploring the binary handles a bit more. I think we will only need them (for the time being) for relationships to aspects faulted in on the aspect path. Bottom line is that the compiler isn't given enough information to recreate the proper handles (but we already knew this). In this comment, I just want to be more clear as to exactly what is expected and why it can't work. Next comment will be about a possible work around. Currently, a typical handle of this kind looks like this: =WeaveMe<bar[MyBar.class (binary)}MyBar&before There are several issues here. Some are easy to fix and some not so easy. The proper JDT handle would look like this: =WeaveMe/\/MyAspectLibrary\/bin<bar(MyBar.class[MyBar&before (notice the "(" to signify a class file, the source location, and the lack of ".class (binary)") But even this doesn't tell the whole story. There are two difficulties for the compiler to recreate this kind of handle: 1. the path /MyAspectLibrary/bin is workspace relative. The compiler doesn't have the concept of a workspace and would return a full path, which would be wrong. It would have to look something like this: =WeaveMe/\/Users\/andrew\/Eclipse\/Workspaces\/junit-workspace\/MyAspectLibrary\/bin<bar(MyBar.class[MyBar&before 2. the class file referenced, MyBar.class, is actually a source file that is easily accessible in the workspace. The correct handle should have been: =MyAspectLibrary/src<bar*MyBar.aj&before but the compiler doesn't know that there are sources available for this class. It just knows about the directory, so it can't recreate the handle for the source file.
Here's a possible solution: 1. with the IProgramElement object, get the fully qualified name of the type 2. grab a reference to that type using the Java project 3. use the source location in the IProgramElement to determine the offset into the compilation unit 4. call ITypeRoot.getElementAt(offset) Should be able to get the proper IJavaElement that way. A problem that I just noticed, though. The source location contains the line number only, the offset field seems to be blank. I can translate from line number to offset, but it requires walking through the entire file.
(In reply to comment #11) > Here's a possible solution: I now have this working.
For method calls, here is currently what the compiler provides: =Bean Example/src<bean{Demo.java[Demo~main~\[QString;?method-call(void bean.Point.setX(int)) All that needs to happen is to append !0!0!0!0!I to the end, to get this: =Bean Example/src<bean{Demo.java[Demo~main~\[QString;?method-call(void bean.Point.setX(int))!0!0!0!0!I And standard count rules still apply. So the second method call would be this: =Bean Example/src<bean{Demo.java[Demo~main~\[QString;?method-call(void bean.Point.setX(int))!0!0!0!0!I!2 If you want to get really fancy, then you can fill in the 0s properly. (Taken from LocalVariable), the 0s in order correspond to: int declarationSourceStart int declarationSourceEnd int nameStart int nameEnd Filling any one of these values with something proper would save a lot of time computing values later. But if it is expensive to compute this in the compiler, then it can wait until later.
I have the declaration start position in the program element, nothing else right now (well, line number). i'm nervous about introducing positional information in the handles as previously when it was included they were rather fragile (slight source changes earlier in the file made them incorrect). So I need to think some more on whether it would be safe in all cases for code handles. Besides this possible change to code handles and the binary handles stuff, is there anything else that needs doing imminently? (like before my 1.6.2 release) or is there already enough to make a difference in the AJDT functionality and prevent some degree of model duplication?
(In reply to comment #14) > I have the declaration start position in the program element, nothing else > right now (well, line number). Thanks. Should make a difference > Besides this possible change to code handles and the binary handles stuff, is > there anything else that needs doing imminently? (like before my 1.6.2 release) > or is there already enough to make a difference in the AJDT functionality and > prevent some degree of model duplication? We need to think about persisting the model, or at least the relationships. Perhaps this can happen on the AJDT side. If IRelationshipMap (or its concret implementation) is serializable, then AJDT should be able to manage storing and retrieving it. Also, there are still a few core test cases that are failing. May require some changes to AspectJ to make them work again. I haven't even started looking at UI tests. Other than that, I don't think there are new things that need to be added, but rather just small fixes that need to be made.
We're getting there. Only 4 tests are failing from AJDT core. Here is what's happening: 1. The aspectj compiler can not create the hierarchy of the following aspect: package p; import java.util.ArrayList; public class ErrorAspect { ArrayList arr = new ArrayList() { // public boolean add(Object o) { // doNothing(); // super.add(o); // }; } } but change "aspect" to "class" and things seem fine. 2. the handle for declare parents is producing a spurious count (!2) here: package p; aspect GetInfo { declare warning : set(int Demo.x) : "field set"; declare parents : Demo implements Serializable; } comment out the "declare warning and the count goes away. 3. handles for binary classes in the default folder do not contain a package reference (should have an empty package reference). Example: =WeaveMe2[MyBar3.class (binary)}MyBar3&around should be: =WeaveMe2/src<[MyBar3.class (binary)}MyBar3&around actually, even better would be to remove the ".class (binary)" We're getting there...
Addressing comment 16. For case (1) the code has errors in, and we haven't defined what state the model is in for broken code yet. However, I was confused that it did something different for class/aspect, but when I try it I get the same thing, the model looks like this when it is 'aspect' ErrorAspect.java [java source file] C:\temp\ajcSandbox\aspectj16_2\ajcTest54804.tmp\BrokenHandles\src\ErrorAspect.java:1::0 hid is =BrokenHandles<p{ErrorAspect.java import declarations [import reference] hid is =BrokenHandles<p{ErrorAspect.java#import declarations and this when it is 'class': ErrorAspect.java [java source file] C:\temp\ajcSandbox\aspectj16_2\ajcTest4028.tmp\BrokenHandles\src\ErrorAspect.java:1::0 hid is =BrokenHandles<p{ErrorAspect.java import declarations [import reference] hid is =BrokenHandles<p{ErrorAspect.java#import declarations What difference do you see in your setup? For case (2) the declare members generated into a class are given increasing counts (regardless of whether they actually need it for differentiation) and the count is used to create the handle. I am not going to change the members generated in the .class file as it will damage binary compatibility. I could change it to do extra analysis when creating the handle, but it would not be cheap as i'll have to go through looking for similar declares in the same type. Given that this handle is not a JDT handle, does it matter if we have our own variations on format? (I guess the question covers other situations too, like the code handles - does it matter if we vary from standard JDT policy if the handles only point to things we understand?) For case (3) the missing 'empty' package looks like a bug - for an aspect that is in a package the model is missing the package so it looks like a general bug in the faulting in logic for binary aspects. I will fix that. On removing '.class (binary)' - I did like the differentiation that this handle points into a binary - and we don't seem to use a different qualifier character for .class files (does JDT?). If the char was different I would remove it all but for now I'm only tempted to remove ' (binary)'
oops! case (3) my mistake, bugged test program. package delimiter is ok apart from default package.
on case (3) again. Are you sure they should have an empty package delimiter? That isn't what I see for a regular source file (not an aspect from the aspectpath) defined in the project? For a class C in the default package the handle I see is: AspectPathTwo [build configuration file] hid is =AspectPathTwo src [source folder] hid is =AspectPathTwo/src C.java [java source file] C:\temp\ajcSandbox\aspectj16_2\ajcTest4670.tmp\AspectPathTwo\src\C.java:1::0 hid is =AspectPathTwo/src{C.java import declarations [import reference] hid is =AspectPathTwo/src{C.java#import declarations C [class] C:\temp\ajcSandbox\aspectj16_2\ajcTest4670.tmp\AspectPathTwo\src\C.java:1::13 hid is =AspectPathTwo/src{C.java[C no < between the /src and { so is it in fact broken for all handles and there should be < for the default package?
looks like the defined classfile delimiter is '(' so if I removed '.class (binary)' I should change the prefixing char to '(' for binary handles like this - what do you think?
(In reply to comment #20) > looks like the defined classfile delimiter is '(' so if I removed '.class > (binary)' I should change the prefixing char to '(' for binary handles like > this - what do you think? > Yes. And, yes, there needs to be a default package delimiter. When I execute this: JavaCore.create("=AspectPathTwo/src{C.java") the return value is not correct: (org.eclipse.jdt.internal.core.JarPackageFragmentRoot) /AspectPathTwo/src{C.java (not open) But when I add the default empty package in, I get the correct result: JavaCore.create("=AspectPathTwo/src<{C.java") (org.eclipse.jdt.internal.core.CompilationUnit) C.java (not open) [in <default> [in src [in AspectPathTwo]]]
And I made mistake about case #3. The correct handle for a class file in the default package would be something like this: =WeaveMe2/bin<(MyBar3.class[MyBar3 or if in a jar file =WeaveMe2/library.jar<(MyBar3.class[MyBar3
I'll have to think about this. It probably won't make much of a difference. The nice thing about keeping track of counts on the AJDT side is that JDT manages it for you when a file is parsed. However, it is possible to override this behavior (which is what was done in the past). (In reply to comment #17) > For case (2) the declare members generated into a class are given increasing > counts (regardless of whether they actually need it for differentiation) and > the count is used to create the handle. I am not going to change the members > generated in the .class file as it will damage binary compatibility. I could > change it to do extra analysis when creating the handle, but it would not be > cheap as i'll have to go through looking for similar declares in the same type. > Given that this handle is not a JDT handle, does it matter if we have our own > variations on format? (I guess the question covers other situations too, like > the code handles - does it matter if we vary from standard JDT policy if the > handles only point to things we understand?)
(In reply to comment #17) > Addressing comment 16. > > For case (1) the code has errors in, and we haven't defined what state the > model is in for broken code yet. However, I was confused that it did something > different for class/aspect, but when I try it I get the same thing, the model > looks like this when it is 'aspect' Ha! I didn't even notice that this was a syntax error. When I add the ; in the correct location, AspectJ is able to correctly parse and produce a hierarchy. I am not too concerned about hierarchies for syntactically incorrect files (even though JDT is able to do it in some cases). This is just a good lesson for me that I should ensure that incorrect syntax is handled gracefully.
Uh oh...here's a problem that I should have seen earlier, but didn't. AspectJ allows compilation units to be placed in any location relative to the package root (and its hierarchy reflects that), whereas Java only allows CUs to be placed in a directory structure that mimics its package structure (and its hierarchy reflects that). What this means is that AspectJ creates handles based on package declarations and Java creates handles based on file locations. Therefore, when the location doesn't match with the package declaration, we cannot travel back and forth between program elements and java elements. It might be possible to do a check every time we translate between the two and update the handles if necessary. But it would be ugly and time consuming. Going from J to AJ: need to check the package declaration and recreate the handle based on that. Going from AJ to J: need to check the file locaiton and recreate the handle based on that.
I had imagined we would require that when working in Eclipse that people put their source files in the right place as there is no need to do otherwise and in fact not doing it would probably leave you fighting with the eclipse tools. Whether this is something we enforce or just something we say is required - I'm easy.
ok, some changes incoming that are a bit more serious than I intended, in order to get the handles right for default package entries and binary references. There is now a blank (representing default) PACKAGE type node in the model, below the source folder and above the type declaration. This ensures the handles correctly include a '<' ahead of the type. The binary handles now have ' (binary)' removed and the delimiter is '(' (and so the type of that node is changed from CLASS to CLASSFILE - which i think was a bug). The handles for binary aspectpath entry are now: In package 'pkg':=AspectPathTwo/binaries<pkg(Asp.class}Asp&before In default package: =AspectPathTwo/binaries<(Asp2.class}Asp2&before I kept the .class part as the suffix seemed to still be included for source files (.java/.aj). what is the binaries thing? Well in order that faulted in types from the aspectpath dont get mixed up with the source from the project, there is a fixed known folder below the root where they go, it is called 'binaries', and yes it will cause an issue if someone calls their source folder 'binaries' (mad...) once the tests are passing, i'll put it into ajdt, i hope these new entries don't cause an issue
New aspectj is now in ajdt 1.6 head, what a mess. No idea if the tests pass ;)
some rogue debug in that driver, will update again shortly
follow on work will be covered in bug 249216. Closing this to detail work done for AspectJ 1.6.2
