Bug 271201 - [handles] [inpath] Invalid handles for ITDs on in path
Summary: [handles] [inpath] Invalid handles for ITDs on in path
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: Build (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P2 normal (vote)
Target Milestone: 1.6.5   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 271269
  Show dependency tree
 
Reported: 2009-04-03 19:46 EDT by Andrew Eisenberg CLA
Modified: 2009-04-16 23:24 EDT (History)
2 users (show)

See Also:


Attachments
Interesting test projects (24.89 KB, application/octet-stream)
2009-04-06 18:16 EDT, Andrew Eisenberg CLA
no flags Details
DeclareParents target on in path Produces invalid handle (3.43 KB, application/octet-stream)
2009-04-07 16:10 EDT, Andrew Eisenberg CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Eisenberg CLA 2009-04-03 19:46:55 EDT
Currently, when an aspect declares an ITD on a type that is coming in from the in path, a relationship is created, but the target handle for it is invalid.


In Project Foo:

package snippet;
import g.G;
public aspect AdvisesLinked {
	int g.G.x = 9;
}

In project FooInpath (on the in path of Foo)
package g;

public class G {

}

Currently, the target handle on the "declared on" relationship looks like {G.java
This is not useful to me.

Instead, I would like something like this:
=Foo/inpath<g(G.class[G

Note the project name, and using inpath as the faked up source folder.  Also, notice that this is returning G.class (the class file), not the java file.  This allows the same mechanism to work regardless of whether it is coming in as a jar on the in path, or from a project.

There is still some work that I would need to do on the AJDT side.  I can't assume that the target IProgramElement will exist.  I need to remove that logic.
Comment 1 Andrew Clement CLA 2009-04-04 17:25:20 EDT
do you care about advice relationships?  Or just ITDs as you write here? 
Comment 2 Andrew Eisenberg CLA 2009-04-04 23:12:38 EDT
Eventually, I would like to have something similar for advice, but perhaps we can first focus on ITDs since getting the handles right will be simpler.
Comment 3 Andrew Clement CLA 2009-04-05 23:31:42 EDT
ok. i have reacquainted myself with that area of the code.  Really the bug is that you have any handle at all for an inpath element (you won't see them for advice I don't believe, just ITDs at the moment).  The incremental builder was never designed to support relationship endpoints that are not in the model.  However, as we look to move to a situation where we don't have any model elements for pure class files, we need to support these 'phantom handles' and rely on JDT plus tricks to find them.

You can see that incremental doesn't support them as the relationship will vanish on the first incremental build that doesn't involve the relationship endpoints.  That is because the incremental repair logic is designed to remove any relationships where an endpoint handle does not point to a model element.   Supporting incremental builds for phantom handles is something I will look at in a different bug.

Here, my handles are now of the form:

=Foo/;<g(G.class[G

the ';' tells you it is inpath.  Seems a shame to make it longer than necessary by including the word 'inpath' and avoids some crazy situation where someone has as source folder called inpath.  Having just 1 char to check should help in the related bug of supporting incremental compilation of these handles.

Oh - and this may be why Neale sees things vanishing - as relationships will get the chop on that first incremental build.
Comment 4 Andrew Eisenberg CLA 2009-04-05 23:36:06 EDT
Thanks.  This version of the handle works for me.  There is a little bit of work I need to do in AJDT so that I can take advantage of this new kind of handle.  But, I assume that this is not in AJDT yet anyway.
Comment 5 Andrew Clement CLA 2009-04-06 12:05:00 EDT
first attempt is now in AJDT - only has minimal testing on my side so far though so i'll leave this open for a bit.
Comment 6 Andrew Eisenberg CLA 2009-04-06 12:41:36 EDT
Two things (both are suggestions that i will have to think more about to see if they really are good ideas):

1. I wonder if we should change handles coming from the aspect path as well.  Right now they look something like: =Foo/binaries<g(G.class
Perhaps it should look the same as in path and maybe use a different character, maybe something like: =Foo/,<g(G.class[G  (not sure if using a different character is really necessary).

2. I also wonder if it would be a good idea to include the workspace path to the binary folder/jar where the class is located.  Eg- (if G.class were in Foo/myjar.jar), then:
=Foo/myjar.jar<g(G.class[G  (and maybe we can still keep the ';'
This might make it easier for AJDT to find the IJavaElement.  And this path can be discovered by the compiler through a callback to the output location manager (similar to how the source location is discovered).

I will try to get comments 1-5 working first and then determine if what I outline here makes sense for AJDT.
Comment 7 Andrew Clement CLA 2009-04-06 13:20:30 EDT
i was thinking of doing your (1), yes.  I have yet to decide if the distinction is important - although currently ';' does identify a handle to something not in the model whereas aspectpath handles are in the model.  I will continue to think about this.

For (2), I can change it to whatever is easiest for you.  Currently i've done nothing to support inpath jars, so you will likely have the same 'useless' handles as you had previously - initially I only looked at this for classes from inpath directories to see if the basics will work.  And I know which jar a class file came from - do I really need to call back to you?  Will AJDT give a different answer for the originating jar than the actual jar file I open to get the class file out of?
Comment 8 Andrew Clement CLA 2009-04-06 13:29:00 EDT
i guess if you want to add more than just the jar name for some reason, we could look to add another callback.
Comment 9 Andrew Eisenberg CLA 2009-04-06 13:37:37 EDT
> And I know which jar a class
> file came from - do I really need to call back to you?  Will AJDT give a
> different answer for the originating jar than the actual jar file I open to get
> the class file out of?

Well, yes.  It is the same issue as with source folders.  The compiler knows the file-system path of the jar/binary folder.  This is probablty not the workspace-relative path (eg- linked resources will be a problem).

If AspectJ can pass in the fully qualified path to the jar/binary folder, AJDT can respond with the proper workspace relative path.
Comment 10 Andrew Clement CLA 2009-04-06 15:36:09 EDT
(I'm entirely ignoring jars for now, that is rather different situation to folders. lets tackle one thing at a time)

I've added getInpathMap() to the output location manager, this should return a map of File objects to handle elements you want.  For example:

Map getInpathMap() {
  Map m = new HashMap();
  m.put("c:/workspace/project/blah/bin","wibble");
  return m;
}

then the handle for a class g/G.class in blah/bin would be:

=Foo/;wibble<g(G.class[G

Comment 11 Neale Upstone CLA 2009-04-06 17:21:41 EDT
Just seen this in the recent build.  In the morning, I'll update my 3.4.2 install and see how it affects the builds for my 2 project structures.
Comment 12 Andrew Eisenberg CLA 2009-04-06 18:16:08 EDT
Created attachment 131065 [details]
Interesting test projects

Attached are the two projects I am working with.

Notice that the Foo project has a reference to the FooOther project as well as to a binary folder.

I have AJDT mostly working with this.  References seem to be taking me where I need to go (except for one situation).

On the aspectj side, I am not sure if this is something you just haven't touched yet, but it seems that these relationships are all disappearing on incremental build.  

Here is what I do:

1. import projects
2. full build
3. notice that relationships exist
4. do whitespace change and save
5. relationships disappear

(note that since I have not committed the ajdt work into cvs yet, you may not see the relationships in the editor's gutter, but you should be able to see them in the debugger)
Comment 13 Andrew Clement CLA 2009-04-06 18:21:10 EDT
> On the aspectj side, I am not sure if this is something you just haven't
> touched yet, but it seems that these relationships are all disappearing on
> incremental build.

yep, that is exactly what should happen right now. See bug 271265 which covers enhancing incremental compilation analysis to allow for handles to non-existent files.  As I mentioned a couple of appends ago - it might be what Neale was seeing with a bunch of stuff vanishing.

Comment 14 Neale Upstone CLA 2009-04-07 06:00:40 EDT
:) with yesterday's build, I now have no markers (neither cross refs nor weave info) visible, either after clean build or incremental.

I guess that unit tests this area of AJ to AJDT might be a bit thin...?
Comment 15 Andrew Eisenberg CLA 2009-04-07 16:10:24 EDT
Created attachment 131173 [details]
DeclareParents target on in path Produces invalid handle

This project shows a declare parents target on the in path.  The handle created here is invalid.

Seems to me that this might be a similar fix as you have for ITDs.
Comment 16 Andrew Eisenberg CLA 2009-04-07 16:12:23 EDT
Oops.  I realize now that the project I attached has a jar on the inpath.  This is not supported yet.  So, you can ignore it.
Comment 17 Andrew Clement CLA 2009-04-07 16:39:12 EDT
given that further inpath incremental for directories is a nightmare, I might knock something up for jar handles now (at least the incremental analysis will be easy for those, just one timestamp to check - no we won't look at individual elements of the jar...)
Comment 18 Andrew Clement CLA 2009-04-07 18:38:44 EDT
just added support for inpath handles into jars (committed into AJDT).  Not sure what will happen with them on an incremental build...
Comment 19 Andrew Eisenberg CLA 2009-04-12 17:10:14 EDT
Great work!

I tried it and it seems to be working for me right now.  I expanded the existing AJDT tests for ITDs on inpath to include jar files and rechecking the relationships after an incremental build (everything passes).

Is there anything more left to do here?  I guess the next step would be advice.
Comment 20 Andrew Eisenberg CLA 2009-04-16 20:03:43 EDT
Just tested in AJDT for the case when an aspect is deleted.  The in path relationships are deleted.  However, this may be because there was a drop back to a full build.

Is it not the case that when an aspect changes that affects some things on the inpath there is always a full build?
Comment 21 Andrew Clement CLA 2009-04-16 23:24:33 EDT
> Is it not the case that when an aspect changes that affects some things on
> the inpath there is always a full build?

not sure. i can't keep all the possible combinations in my brain at the same time.

I guess we are done on this bug then.  advice can follow in another bug.