Comment 1 attila lendvai 2004-04-23 09:34:48 EDT

it's 1.2RC1

Comment 2 Andrew Clement 2004-04-23 10:15:39 EDT

I have just fixed a bug vaguely related to this (bug 59440) to do with not 
importing a type used in a constructor ITD.  This one is different but I am 
slightly confused, the ITD:

private Serializable		HibernateEntityRenderingHelper.id;

is a field ITD, yet the NPE is in the InterTypeMethodDeclaration type.

I've tried to find a case as simple as the one I did in 59440 but I can't so 
far - it doesn't seem to matter if the type of the ITD can't be found or the 
type upon which we are ITDing can't be found - I still get a sensible error 
message about unresolved types.  I've also tried binary weaving (including -
g:none on the stuff going into the binary weave), but still I can't recreate 
the problem.  is there any way you could attach the whole aspect to this bug?

Comment 3 attila lendvai 2004-04-23 10:33:16 EDT

Created attachment 9895 [details]
This is the file in question, it has several Serializable references

Comment 4 Andrew Clement 2004-04-26 12:03:23 EDT

thanks for appending more of the code.  I can see its a more complicated 
scenario than the one I had been playing with.  Here are the relevant ITD bits 
of code I've been looking at from your sample:

public abstract aspect AbstractHibernateEntityRenderingHelperMixin extends 
AbstractHibernateRenderingHelperMixin
{
public interface HibernateEntityRenderingHelper extends 
HibernateRenderingHelper {};
	
declare parents:(AbstractHibernateEntityScreen ||
                 AbstractHibernateTreeViewScreen)
		implements HibernateEntityRenderingHelper;

private Serializable HibernateEntityRenderingHelper.id;

...
...
  public void HibernateEntityRenderingHelper.setEntityById(Serializable id) {
    this.id = id;
  }
...
}

However, I've tried creating a small example that mirrors what this class does 
and it just won't fail, without the Serializable import I get a sensible error 
message and not the NPE.  Here is my current experiment:

==========================
//import java.io.Serializable;

public abstract aspect Sample {

  public interface If1 extends If2 {};

  declare parents: ( A || B) implements If1;

  private Serializable If1.id;

  public void If1.setID(Serializable s) {
    this.id = s;
  }

}

interface If2 { }

class A { }

class B { }
===========================

All I can think of now is that there is some type not exposed to the weaver 
that should be and it manifests as the NPE.

Can I ask:
- Are you doing a binary weave to apply this aspect to a system?
- If you are, do you put all the components of the framework on inpath/injars 
or do you have some of them on the classpath?

Comment 5 attila lendvai 2004-04-26 12:38:24 EDT

this is how i call ajc: (no binary weaving)

		<iajc
			encoding="UTF-8"
			source="1.4"
			debug="true"
			debuglevel="lines,vars,source"
			destdir="${build}/web-compiled"
			includes="**/*.java,**/*.aj" fork="true"
		>
			<sourceroots>
				<pathelement location="src/web/java"/>
			</sourceroots>

			<aspectpath>
				<pathelement path="../web-framework/build/web-framework.jar"/>
			</aspectpath>

			<classpath refid="web.build.classpath"/>
		</iajc>

the web-framework depends on a few jars, but IIRC they are all on the classpath 
when compiling. also the web-framework is created from three injar'ed files plus 
my sources, and they all go into the web-framework.jar

i will add some new attachments with the base classes in a few secs.

a note: in the inheritance of the aspect there's one called MutableContentMixin 
that comes from the aspectpath'ed web-framework.jar

another note: ajc has some problems with inner types + inheritance (at least i 
had strange experiences). if an aspect extends another one that has an inner 
type, then sometimes i had to explicitly import that inner type to overcome an 
internal error. also i don't know the rules but if there are three inheriting 
aspects involved the lower one can not be seen... or something like this.

hope it helps!

Comment 6 attila lendvai 2004-04-26 12:39:43 EDT

Created attachment 9967 [details]
AbstractHibernateRenderingHelperMixin.java

Comment 7 attila lendvai 2004-04-26 12:40:27 EDT

Created attachment 9968 [details]
POJORenderingHelperMixin.java

Comment 8 attila lendvai 2004-04-26 12:41:00 EDT

Created attachment 9969 [details]
ComponentFactoryMixin.java

Comment 9 attila lendvai 2004-04-26 12:41:56 EDT

Created attachment 9971 [details]
MutableContentMixin.java

Comment 10 attila lendvai 2004-04-30 07:52:01 EDT

i'm truck again with this bug... having some logging in ajc would help a lot! 
and i could help you with sending the debug level log file...

Comment 11 attila lendvai 2004-04-30 07:55:11 EDT

a little additional info: now it happened because i did not have someting on the 
classpath that was imported in the aspects. hope it helps.

Comment 12 Andrew Clement 2004-05-06 07:06:44 EDT

Created attachment 10333 [details]
Debug version of InterTypeMethodDeclaration

I reallllllly want to recreate this bug and see whats going on.  I just can't
seem to manage it.  So, I have attached a jar to this bug that includes a
patched version of InterTypeMethodDeclaration that should print out what the
various fields are set to at the point of the NPE.  Currently on the line where
the NPE is reported there could be several things that are NULL, it would be
nice to confirm exactly which it is.

Please can you patch your aspectjtools.jar with the contents of this one?
Probably something like this:
1) Backup aspectjtools.jar in aspectj1.2\lib
2) Unzip this jar into aspectj1.2\lib
3) jar -uvf aspectjtools.jar org

And then recreate the problem and paste the output into here?  I want to cut
1.2 RC2 either tonight or tomorrow morning, and this is probably the last bug
to resolve before doing that...

The alternative could be that you zip up your whole system and send it to me
(privately) to reproduce locally - is that possible?   I'm certainly willing to
try this approach.

Comment 13 Andrew Clement 2004-05-06 11:15:03 EDT

Phew.  I've had a separate discussion 'off-bugzilla' about this bug.  There 
were some problems with ensuring the compiler being used was 1.2rc1, 
eventually when we got the right compiler AJC then failed in a slightly 
different way than the NPE.  After receiving all the code in the failing 
system, I have (miraculously!) created a minimal testcase.  The bug that needs 
fixing is actually a silent compiler failure.  Here are two files:

B.java contains:
public abstract aspect B {
  public void C.method(Serializable s) {
  }
}
class C {
}

D.java contains:
class D { }

Now: 

"ajc B.java D.java" gives no messages back to the user but creates no class 
files.

"ajc D.java B.java" gives:
C:\temp\area\B.java:2 error Serializable cannot be resolved 
  (or is not a valid type) for the argument s of the method method2c
public void C.method(Serializable s) {
                     ^^^^^^^^^
1 error

This is an error message missing from the first version of calling ajc that 
tells the user why he got no class files !

Here is the classic comment in the source:

// if binding is null, we failed to find a type used 
// in the method params, this error
// has already been reported. 
throw new AbortCompilation();

Can you guess what is happening in this failing case ...

The aspect must be abstract for it to fail silently.  Now I have that minimal 
case I can hopefully fix it.

Comment 14 Andrew Clement 2004-05-06 12:56:46 EDT

Created attachment 10359 [details]
Fix for silent compiler death

This jar includes one class file that you should apply to aspectjtools.jar,
following the same approach as documented earlier in the bug.  Hopefully it
will now report errors rather than silently failing.  Let me know if it
works...

Comment 15 Andrew Clement 2004-05-07 03:58:34 EDT

The bug in the code is as follows.  During processing of class B.java, we 
correctly discover the error: 

"Serializable cannot be resolved (or is not a valid type) ...."

It is added as an error to the list of errors against the B.java compilation 
result.  However, when we get to InterTypeMethodDeclaration.build() we go 
through this code:

binding = classScope.referenceContext.binding.resolveTypesFor(binding);
if (binding == null) {
  // if binding is null, we failed to find a type used in the method params, 
this error
  // has already been reported.
  throw new AbortCompilation();
}

It is true that the error about the missing parameter type has already been 
recorded.  It hasn't been *output* to the console yet though, it has just been 
recorded against the compilation result.  The AbortCompilation exception is 
thrown and we handle it in Compiler.handleInternalException (line 403).

In jumping from where the AbortCompilation() was thrown into 
handleInternalException, we have skipped the 'acceptResult' processing which 
is how the errors against a compilation result are reported to the user.  
Because we have skipped it, the handleInternalException processing attempts to 
sort it out by calling it.  Unfortunately the AbortCompilation result was 
thrown without indicating the compilationresult that was being processed at 
the time, this means we drive this piece of code in handleInternalException:

// Exception may tell which compilation result it is related, and which 
problem caused it
CompilationResult result = abortException.compilationResult;
if ((result == null) && (unit != null))
  result = unit.compilationResult; // current unit being processed ?
if ((result == null) && (unitsToProcess != null) && (totalUnits > 0))
  result = unitsToProcess[totalUnits - 1].compilationResult;

Without a compilation result set in the AbortCompilation exception (and unit 
is null also in this case) - we 'have a guess' (!!!) at the compilationresult, 
and assume it is the last in the list to be processed.

Later on down the handleInternalException code we then make this call:
/* hand back the compilation result */
if (!result.hasBeenAccepted) {
  requestor.acceptResult(result.tagAsAccepted());
}

So, what does that mean in our case?

We have two files, B and D.  An error is stored against B (the missing 
Serializable).  We throw an AbortCompilation exception without a result whilst 
processing B, so the exception processing code has a guess and picks D as the 
compilation result (the last one in the list of those to be processed).  We 
call acceptResult() for D's compilation result - which has no errors on it, 
and then we end compilation... without ever reporting the problem recorded 
against B !

If we compile the files the other way round ... then an error is stored 
against B, we throw an AbortCompilation exception without a result, it has a 
guess and by sheer fluke picks the right one as we passed them in as D, B.  We 
call acceptResult and correctly report the error before abruptly finishing 
compilation.

The fix?

When we throw the AbortCompilation exception, we include the right information 
about the compilationResult so that the exception processing code doesn't have 
to guess:

  throw new AbortCompilation(compilationResult)

Amazing how the whole bug report boiled down to a 1 line change - I like those 
kind of fixes ;)

Comment 16 Andrew Clement 2004-05-07 11:34:54 EDT

Fix and testcase checked in.

Comment 17 Adrian Colyer 2004-05-13 05:18:00 EDT

fixed by Andy.