Bug 236352 - [jdt-weaving] [editor] Imports are not organized properly when editing a *.aj source.
Summary: [jdt-weaving] [editor] Imports are not organized properly when editing a *.aj...
Status: RESOLVED FIXED
Alias: None
Product: AJDT
Classification: Tools
Component: UI (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P3 critical (vote)
Target Milestone: 1.6.2   Edit
Assignee: AJDT-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 242475 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-06-09 23:41 EDT by Rahul Thakur CLA
Modified: 2008-12-11 16:13 EST (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Rahul Thakur CLA 2008-06-09 23:41:54 EDT
Build ID: I20080502-0100

Steps To Reproduce:
1. Create as Aspect source in a package few levels deep (a.b.c.d)
2. Try using a class from another library (and package). I used org.apache.log4j.Logger
3. Eclipse updates the source file with an 'import org.apache.log4j.Logger', BUT its place before the 'package' declaration. 


More information: 
AJDT Version : 1.6.0.200805141703
AspectJ version: 1.6.1.20080514120000
Comment 1 Rahul Thakur CLA 2008-06-15 19:33:55 EDT
Hi, I would like to take a shot at solving this. I did some investigation over the weekend; it seems the organize imports operation uses the standard JDT APIs. I am speculating the same is used by the Java editor as well. But I am still missing a piece in puzzle - why is the AST being updated differently in case of an *.aj editor? 
Comment 2 Andrew Eisenberg CLA 2008-06-15 20:12:38 EDT
Feel free to take a stab at it and submit a patch.

See bug 10659 for a similar issue.  In this case, a Java parser is being used to parse the file, but since it is an aspect file, the file cannot be parsed.  The import is placed at the beginning of the file because the operation doesn't know where else to place it.

I'd look into this possibility first.  Submit another comment if you'd like me to clarify.
Comment 3 Rahul Thakur CLA 2008-06-18 20:10:25 EDT
(In reply to comment #2)
> Feel free to take a stab at it and submit a patch.
> See bug 10659 for a similar issue.  In this case, a Java parser is being used
> to parse the file, but since it is an aspect file, the file cannot be parsed. 
> The import is placed at the beginning of the file because the operation doesn't
> know where else to place it.
> I'd look into this possibility first.  Submit another comment if you'd like me
> to clarify.

Right. So, the ASTParser needs to be extended to parse AspectJ sources, correct?
Comment 4 Andrew Eisenberg CLA 2008-06-19 11:23:47 EDT
I haven't actually looked into this problem yet, but that's my guess based on the description.

If this is the reason for the bug, then it is something we will be looking into for AJDT 1.6.x.
Comment 5 Andrew Clement CLA 2008-06-19 14:20:36 EDT
we have an Aj enabled AST parser, I thought the problem here was that we could not get to the point in the JDT code where it chooses a parser, so we could not tell it to use ours rather than the java one?
Comment 6 Andrew Eisenberg CLA 2008-06-19 18:03:01 EDT
That's right, Andy.  My point was that if we do decide to go ahead with weaving, we will need to plug the AJ parser into the Java parser hierarchy in the right place. So that in effect we are extending the Java parser.
Comment 7 Rahul Thakur CLA 2008-06-19 18:41:16 EDT
A search for ASTParser (for .java) in my workspace reveals quite a few places where the ASTParser is being instantiated and used (Editor, refactoring operations etc).
It seems to me that it might be an idea to abstract away the ASTParser behind an interface so the implementations can be switched transparently. But that might imply touching JDT sources as well.

Thoughts?
Comment 8 Andrew Clement CLA 2008-06-20 11:15:57 EDT
Modifying the JDT to introduce a suitable point for extension and making the AST pluggable is a good way to try and proceed.  JDT don't usually take our extensions so worst case we may move to binary weaving the JDT to introduce the extension point.

It could get a little messy as the AspectJ AST parser is not guaranteed to return the same entities as the regular JDT parser (I haven't looked) - but in some places we have our org.aspectj.* prefixed forms of JDT types.  In which case we may need to translate them into what the invoker of the AST parser wants.
Comment 9 Andrew Eisenberg CLA 2008-07-07 15:51:47 EDT
> See bug 10659 for a similar issue.
Meant to say bug 106589
Comment 10 Andrew Clement CLA 2008-08-08 17:30:40 EDT
*** Bug 242475 has been marked as a duplicate of this bug. ***
Comment 11 Andrew Eisenberg CLA 2008-12-11 15:57:55 EST
Probably addressed by JDT-weaving.  Will look into this.
Comment 12 Andrew Eisenberg CLA 2008-12-11 16:07:26 EST
Yep.  Will create a test for this.
Comment 13 Andrew Eisenberg CLA 2008-12-11 16:13:43 EST
Test has been committed.