Comment 1 Matthew Webster 2004-03-19 04:34:09 EST

Created attachment 8683 [details]
Testcase

Comment 2 Andrew Clement 2004-03-31 03:47:40 EST

The message comes out of ReturnStatement.resolve().  The return statement 
is 'return message' and the problem occurs comparing the type of the return 
value with the return type declared for the method.

When it goes wrong, the method return type is a valid resolved 
java.lang.String whilst the expressionType field in the ReturnStatement object 
is 'Unresolved type java.lang.String'.  These two are classed as incompatible 
so it fails with the error "cannot convert from java.lang.String to 
java.lang.String"

Comment 3 Andrew Clement 2004-04-01 11:59:33 EST

Here is a simpler failing testcase:

Here is HW.java (which could be a bit simpler actually..)
==========================

import java.util.ArrayList;
import java.util.Iterator;
import java.util.Properties;

public class HW extends ArrayList {

  String message = "Hello World!";

	private void check (String args) {
	}

	public void println () {
		System.out.println(message);
	}

	public static void main(String[] args) {
		HW hw = new HW();
		hw.println();		
		for (int i = 0; i < args.length; i++) {
			String jp = args[i];
			if (!hw.contains(jp)) {
				throw new RuntimeException(jp + " missing"); 
			}
		}
	}

}
============================

And here are a pair of aspects in a single file X.java:
============================
aspect F {
        private int HW.intField = 999;
}

aspect M {
        public String HW.getMessage () {
                return message;
        }
}
==========================
With these files, you can reproduce the failure as follows

ajc -outjar helloworld.jar HW.java

ajc -classpath "helloworld.jar;c:\aspectj1.1\lib\aspectjrt.jar" X.java

The failure with the 2nd intertype declaration only occurs if the first aspect 
containing an intertype on the same type is also being passed in...  It is as 
if the first intertype declaration pulls in the class (HW) upon which it is 
intertyping but doesn't bother resolving all of its contents - and when the 
second intertype comes along, we then don't bother to ensure the bits of the 
required class are fully resolved that the second intertype declaration needs.
======
I have fixed it by putting an extra bit of code in ReturnStatement.getBinding
():

if (fieldBinding.type instanceof UnresolvedReferenceBinding) 
  fieldBinding.type = ((UnresolvedReferenceBinding) fieldBinding.type).resolve
(environment());

which ensures that if we do come across something unresolved, we resolve it 
there and then.

BUT - I still don't understand why we only end up with the unresolved version 
of 'String message' if I use an intertype declaration before the failing 
one ???

FYI: When we are in getBinding() and trying to obtain the binding 
for 'message', the enclosingtype field looks like this (notice that message is 
unresolved):

public class HW
	extends java.util.ArrayList
/*   fields   */
Unresolved type java.lang.String message
/*   methods   */
void <init>() 
void check(Unresolved type java.lang.String) 
void println() 
void main(java.lang.String[]) 

Comment 4 Andrew Clement 2004-04-02 04:23:25 EST

I've done some more investigation.  The whole problem here hinges on the order 
in which things are resolved (as expected).  Whether the error message occurs 
entirely depends on whether 'String' has been resolved before 'HW' is loaded 
into the system.

If String has been resolved then HW is loaded with a 'java.lang.String 
message' field and the code all works.
If String has not been resolved then HW is loaded with an 'unresolved 
java.lang.String' field and the code fails.

We can verify this as follows.  Add a new method to our pair of failing 
aspects:

aspect F {
  public String a() {return "abc";}  <---- New method refers to 'String'
  private int HW.intField = 999;
}

aspect M {
  public String HW.getMessage () {
    return message;
  }
}

The inclusion of our new method ensures String is fully resolved before HW is 
brought into the system. and so the intertype declaration in aspect M works 
perfectly.  What we have here is someone not checking that something is 
unresolved before using it.  Resolution around the system is done lazily, see 
the javadoc for: LookupEnvironment.getTypeFromConstantPoolName():

/* Answer the type corresponding to the name from the binary file.
* Does not ask the oracle for the type if its not found in the cache...
* instead an unresolved type is returned which must be resolved before used.
*/

So the fix is to correctly resolve String when the intertype declaration needs 
to refer to it.

Comment 5 Andrew Clement 2004-04-02 06:56:40 EST

eureka.

The fix is actually to address something marked 'XXX' in the code !!!
This line:

// XXX may need to get the correct value for second parameter in the future
FieldBinding retField = sourceTypeBinding.getFieldBase(fieldName, false);  

in InterTypeMemberFinder.getField()

In the failing case we should be passing true rather than false - as it 
indicates whether to ensure the result of the get is resolved.  I just need to 
work out how to pass the flag into the method from the caller.

Comment 6 Andrew Clement 2004-04-02 08:12:13 EST

FIX CHECKED IN.

Having looked at the paths into this method, I can't see why we would ever not 
want to ensure the field is fully resolved.  I have changed the line:

FieldBinding retField = sourceTypeBinding.getFieldBase(fieldName, false);  

to

FieldBinding retField = sourceTypeBinding.getFieldBase(fieldName, true);

This fixes the bug.  The tests all still pass.  The only impact would be on 
performance but as this only affects a path when using intertype declarations 
of a particular sort.  The alternative would be to expand the signature of the 
method InterTypeMemberFinder.getField() which would require extending the 
signature of the JDT internal interface that it implements - that would be too 
messy (with an imminent 1.2 RC to be shipped..)

I have left the XXX in at the moment, since we are still hard wiring the value 
(but now to true) - I've annotated it with this bug number so in future 
someone doesn't come and try changing it back to false !

I have added a new testcase to the harness and uncommented the tests of 
Matthews that were impacted by it.

Comment 7 Adrian Colyer 2004-04-02 08:40:26 EST

fixed by andy clement