Bug 263666 - [handles] [plan] Odd behavior in cross references view
Summary: [handles] [plan] Odd behavior in cross references view
Status: RESOLVED FIXED
Alias: None
Product: AJDT
Classification: Tools
Component: UI (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P3 major (vote)
Target Milestone: 2.0.0   Edit
Assignee: AJDT-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 263668 264441
Blocks: 269835
  Show dependency tree
 
Reported: 2009-02-04 12:05 EST by Andrew Eisenberg CLA
Modified: 2009-05-15 14:24 EDT (History)
2 users (show)

See Also:


Attachments
Project that exhibits this problem (1.00 KB, application/x-zip-compressed)
2009-02-10 11:49 EST, 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-02-04 12:05:02 EST
From the Mailing list:

---------------------
The following aspect is giving me interesting behaviour:

 0 package x;
 1 public aspect OverrideOptions
 2 {
 3    /**
 4     * Comment A
 5     */
 6    boolean around() : execution( public boolean A.a() ) && this( A )
 7    {
 8        return false;
 9    }
10
11    /**
12     * Comment B
13     */
14    int around() : execution(private int B.b(..)) && this(B){
15        return 0;
16    }
17 }

While the compiler is doing the right thing, the cross references view
gives some interesting behaviour.

When I open the aspect, the cursor is at the beginning of line 0, and
the CR view shows:
       x

As I move the cursor down (always at left edge), lines 1-3  give:
- OverrideOptions
 - around()
   - advises
     - B.b(boolean)
     - A.a()

Lines 4-6 give:
- around()
 - advises
   - B.b(boolean)
   - A.a()

Lines 7-16 give:
... Around()

And line 17 gives:
- OverrideOptions
 - around()
   - advises
     - B.b(boolean)
     - A.a()

Factoring out the first pointcut as a separate pointcut declaration
gives even more interesting behaviour.  The first advice now
successfully claims to advise both target classes, when before it was
none.

Version: 1.6.4.20090130154246
AspectJ version: 1.6.4.20090129145400
----------------------

At this point I don't know how much this has to do with the model and how much this has to do with the x-ref view itself.  Looks like the markers on the side are showing some odd behavior as well.  My guess is that this has something to do with handle identifiers.
Comment 1 Andrew Eisenberg CLA 2009-02-04 12:27:54 EST
At least part of this problem is an AspectJ problem.  The handle count does not seem to be created properly.

The first around advice has this AspectJ handle, but so does the second.
=Bug256989/src<x*OverrideOptions.aj}OverrideOptions&around

Instead the second AspectJ handle should have an occurrence count appended to it:
=Bug256989/src<x*OverrideOptions.aj}OverrideOptions&around!2

Going to raise this as an aspectj bug as well.
Comment 2 Andrew Clement CLA 2009-02-04 12:32:41 EST
Andrew - I guess the model consistency checking code doesn't pick things like this up does it? Because it thinks it did the right thing?
Comment 3 Andrew Eisenberg CLA 2009-02-04 12:39:43 EST
Unfortunately, the model checker thinks it is correct because the handle for the second around advice actually exists, even though it is incorrect.

However, I can change the model checker so that it does a full round trip converting from IProgramElement to IJavaElement back to IProgramElement and then to IJavaElement.  Then I can check to see if the two IJavaElements are the same as well as the IProgramElements.  This would find the problem.
Comment 4 Andrew Eisenberg CLA 2009-02-06 12:01:31 EST
After the fix of 263668, this is working 85% right now.  

The handles are getting created properly.  Gutter markers and navigation are
working properly.

The only thing not working is that the xref view seems not to refresh properly
all the time.  When moving cursor up and down sometimes the wrong around advice
is being created.
Comment 5 Andrew Eisenberg CLA 2009-02-09 18:15:06 EST
Ahhh...this is a hashcode problem.  Seems that the advice element info for both of these around advices have the same hashcode, which they obviously should not.  But, this means that the JavaModelManager is overwriting one element info with another.
Comment 6 Andrew Eisenberg CLA 2009-02-09 22:51:07 EST
OK.  So, I was wrong about this being a hashcode problem.  The hashcodes (and equals) methods look wrong to me because they don't include the occurrence count, but other JavaElements don't include occurrence counts either, so this may not be an issue.

I am fairly certain it is a parser problem.  Consider two versions of the above code. 


package x;
public aspect OverrideOptions
{
   /** 
    * Comment A
    */
   boolean around() : execution(public boolean A.a() ) && this( A )
   {
       return false;
   }
   /**  
    * Comment B
    */
   char around() : execution(private int B.b(..)) && this(B) {
       return 'a'; 
   }
}  


package x;
public aspect OverrideOptions
{
   /** 
    * Comment A
    */
   boolean around() : execution(public boolean A.a() ) && this( A )
   {
       return false;
   }
   /**  
    * Comment B
    */
   char around() : execution(private int B.b(..)) && this(B) {
       return 'a'; 
   }
}  



In this version, the parser assigns a source element start of the second advice at character 106, or here, where the **** are "boolean around() : execution(****public boolean A.a() )".  This means the second advice is starting in the middle of the first advice's pointcut!  

However, delete the "public" in that pointcut, and all source positions are calculated correctly.
Comment 7 Andrew Eisenberg CLA 2009-02-09 23:46:02 EST
Starting to think that this is an AspectJ problem.  Here is the smallest failing example I can find.  It also fails for call and within code pointcuts.  It also fails if the public is changed to any other modifier.

public aspect AA {
   before() : execution(public void A.a() ) { }   
   int x;
}

public class A {
	public void a() {
		a();
	}
}

So, the declaration after the advice has the incorrect declarationSourceStart.  It takes the declarationSourceStart to be the value found for the 'public' keyword.  What may be happening is that the parser thinks that the public is the start of a new "interesing" declaration and so it stores its location, even when it shouldn't.
Comment 8 Andrew Clement CLA 2009-02-10 11:09:58 EST
I'm happy to take a look, but need to know some precise details.  What exactly is in the source file - or is this two source files as they are both public types?  Tabs or spaces for whitespace?  When you say "the declaration after the advice" do you mean 'int x'?  When you say it gets the start of 'public' - is that in the first source file - or are both public types in the one source file (not legal...) and you mean the public attached to class A.

---
public aspect AA {
   before() : execution(public void A.a() ) { }   
   int x;
}
---

---
public class A {
        public void a() {
                a();
        }
}
---
Comment 9 Andrew Eisenberg CLA 2009-02-10 11:49:53 EST
Created attachment 125265 [details]
Project that exhibits this problem

Here is the project form of the above java code.  Should answer all your questions in the previous post.
Comment 10 Andrew Clement CLA 2009-02-10 11:55:51 EST
ok, I installed the project - but where is the bug?

I have my cross reference view open, if the aspect is open in the editor - i double click 'before' in the xref view and it highlights 'before' in the editor.  I double click A.aj in the xref view, it jumps to the A.aj file and highlights a in 'public void >a<() {'.  I can then navigate back to the aspect by double clicking AA.before in the xref view.

What is broken ? which view do I see the breakage in?
Comment 11 Andrew Eisenberg CLA 2009-02-10 12:02:33 EST
(In reply to comment #10)
> 
> What is broken ? which view do I see the breakage in?
> 

In this line:

before() : execution(public void A.a() ) { }   

1. set your cursor at the 'n' in execution.
2. the before() in the xref view highlights
3. move to the right 2/3 spaces to after the 'p' in public
4. nothing is highlighted in the xref view

this is because the start location in the int x; is set to be the 'p' in public.

Are you seeing this behavior?
Comment 12 Andrew Clement CLA 2009-02-10 12:07:37 EST
yes, i see that behaviour - thanks.
Comment 13 Andrew Eisenberg CLA 2009-02-10 12:15:15 EST
FYI- you may want to take a look at o

org.aspectj.org.eclipse.jdt.internal.compiler.parser.Parser.consumeBasicAdviceHeaderName

The problem seems to be happening shortly after this method is executed.  Somewhere around the time that the 'public' in the PCD is consumed.

Also, note that if you remove the offending 'public' keyword, then the file is parsed properly.
Comment 14 Andrew Clement CLA 2009-03-25 16:01:21 EDT
fixed (fix is in AJDT).  Well the start of 'int x' is now the correct start position, don't know if that will address all related situations.  The around advice case was actually a different fix because it is a different kind of advice, that is now 'fixed' also (or at least the positions are right).

please reopen if any other situations appear or I haven't fixed what you thought I'd fix.  I have *no* automated tests for this area of the code, so it may break something else...  if that proves to be the case I will look to flesh out a new harness for this kind of thing (cos manual testing it in ajdt is no fun)