Comment 1 Matthew Webster 2006-07-05 06:02:06 EDT

Hmmm, not quite what I had in mind for this one. When I said in Bug 147678 (-outxml shouldn't list abstract aspects) “you are taking a risk” if you ignore “Can’t find type …” I didn’t mean that it would be OK if we ignored it (by turning it to a warning)! When the weaver says it can’t find something it means it won’t be able to weave the code which at the very least will result in an incorrect program and at worst code that may not verify. The trouble with raising an enhancement is that we end up discussing _one_ possible solution rather than getting to the bottom of the _actual_ problem. So let’s discuss the problem.

Firstly I’d like to respond to this comment: “I think the current default Xlint setting for cantFindType = error is just the wrong one for load-time weaving.” There is and should never be any difference between compile-, post compile- and load-time weaving. 

I can see that there are situations with LTW when we _shouldn’t_ get “Can’t find type …” because we will run the code and the weaver is using the same classpath as the JVM. However the weaver _may_ be misconfigured (non-delegating class loaders spring to mind) and an error message would be an indication of this.

There are other situations when we are weaving code that we won’t actually run. However the weaver doesn’t know this and ignoring the error is wrong. If you don’t want to weave code change the pointcut.

Finally there are the situations where we may be too aggressive in resolving types. Situations where we _can_ weave correctly without the type. We need to develop a simple testcase that demonstrates this problem that we can add to the suite and use to improve the weaver.

As far as I’m concerned “Can’t find type …” shouldn’t be an Xlint error; it should be a _hard_ error. Actually that is the purpose of Xlint: a staging post for deciding whether something is an error or not. I hope the outcome of this investigation is to eliminate those cases which we can prove are not errors and ensure those that remain are.

Comment 2 Ron Bodkin 2006-07-06 03:07:48 EDT

I think it's a good discussion to have about cantFindType to balance the needs of users with ensuring that the weaver produces valid output. I agree it's an area that can use more analysis of the root causes.

At load-time, the ideal in my mind would be the equivalent of how Java code behaves when run: missing type references only generate an error when the code is run. Not failing at load-time when unknown types are encountered in code that would result in an error at runtime is a common and important use case for cantFindType today.

>I can see that there are situations with LTW when we _shouldn’t_ get “Can’t
>find type …” because we will run the code and the weaver is using the same
>classpath as the JVM. However the weaver _may_ be misconfigured (non-delegating
>class loaders spring to mind) and an error message would be an indication of
>this.

In particular, for this case, I think it's very important that valid LTW configurations continue to work properly and the error would only be flagged if it could somehow detect a misconfiguration. I can only think of one case that could produce a misconfiguration of this type:
if a ClassLoader's getResource resolution for .class files isn't compatible with the way it resolves classes or with classes that are already defined to the loader (i.e., if there's a mismatch between the types found by the weaver and those loaded by the loader), where the BCEL mechanism wouldn't be able to find the type. I would argue that load-time weaving just can't work reliably for such a loader, regardless of how missing types are handled. Barring that, shouldn't the weaver see the exact same set of types that are available at runtime? I don't see how non-delegation would affect this, since getResource should follow the same delegation rules as loadClass. I think having the ability to both ignore and to see warnings about such missing types is valuable here: in a production system if the missing types are expected, it is bad to show even warnings at runtime.

Re:
>There are other situations when we are weaving code that we won’t actually run.
>However the weaver doesn’t know this and ignoring the error is wrong. If you
>don’t want to weave code change the pointcut.
In the case of inpath weaving, there is a real tension. I know that both I and other users have found the use of cantFindType helpful in this case. The question then turns on whether it's always reasonable
to change the pointcut.

A classic example is referring to some interface in your system, e.g., target(SomeInterface). Any unknown concrete types *might* implement the interface. In some cases, you only expect types in your system to implement it. Then might be a bit tedious but reasonable to write target(SomeInterface) && target(com.myorg..*) - except that you can't use target with anything but a single type, but maybe target(SomeInterface) && !call(* com.myorg..*(..)) would work in this case.

However, if SomeInterface is something like javax.sql.DataSource, then it gets trickier. You can't really anticipate what packages an implementer of DataSource would run in. So if you want to write an aspect to trace DataSource creation in an inpath library, if you didn't have cantFindType support, you could imagine writing a pointcut like (*)

target(DataSource) && !target(fully.qualified.MissingType1) && ... && !target(fully.qualified.MissingType75)

You could just try to enumerate all possible implementers of the type, although that would be brittle. It might be possible to exclude certain package structures where the missing library jars exist. You might try something like this:

target(DataSource) && !call(* com.ibm..*(..)) && !call(* com.oracle.toplink..*(..)) && ...

I know I've had problems like this trying to weave into Spring, Struts, and commercial O/R mapping tools, e.g.,
public aspect TrackDS {
    before(): call(* javax.sql.DataSource.*(..)) {
        System.out.println("using data source");
    }
}

All in all I think it would be good to get input from aspectj-users about the cases where cantFindType is helpful and whether they could have simply refined pointcuts to avoid the problem or whether it was actually a case where the weaver was being too greedy about wanting to know types.

>Finally there are the situations where we may be too aggressive in resolving
>types. Situations where we _can_ weave correctly without the type. We need to
>develop a simple testcase that demonstrates this problem that we can add to the
>suite and use to improve the weaver.

I agree here and will follow up with some examples where the weaver flags cantFindType warnings even though the pointcut resolution isn't actually ambiguous, e.g., call(* javax.sql.DataSource.*(..)) && target(CantMatch)

(*) Today the weaver needs cantFindType to match the enumerated pointcut ending in MissingType75

Comment 3 Matthew Webster 2006-07-06 10:00:41 EDT

Correctness is the most important attribute of the weaver because trying to track down bugs resulting in unexpected application behaviour is very hard. A pointcut is an explicit instruction that we cannot choose to ignore because we don’t have all the information available to weave just because the code may never run. The “crystal ball” enhancement has not yet been implemented.

Now I agree that there may be situations where writing a pointcut to avoid “Can’t find type …” is difficult or even impossible and you have attempted to describe some with code snippets. What we need is isolated testcased that can be included in the suite. This will allow us to explore possible changes to the weaver which allows a broader set of applications to be woven without resorting to ignoring errors.

I’m not sure if this has been made explicit elsewhere but the weaver assumes a complete type system and acts accordingly. By ignoring “Can’t find type …” we undermine this assumption and the weaver may no longer behave correctly. The result exercise should be that ignoring the error is no longer necessary and its Xlint status can be removed.

Comment 4 Matthew Webster 2006-07-07 08:36:00 EDT

Ron,

I have added a testcase to the harness that characterizes many of the situations where “Can’t find type …” may not be a problem. There are probably others involving call() && target() as well as those with args() and @annotation(). The test passes because I use –Xlint:warning but we will explore changes to the weaver that make this unnecessary.

public class TestFail {

	public void invoke () {
		Interface i = new Missing();
		i.method();
		Missing cf = new Missing();
		cf.method();
	}
	
	public static void main(String[] args) {
		new TestFail().invoke();
	}

}

public interface Interface {

	public void method ();
}

public class Missing implements Interface {

	public void method () {
		System.out.println("Missing.method()");
	}
	
}

public aspect Aspect {

	before () : call(public * method(..)) && target(Interface) {
		System.out.println("Aspect.before()");
	}
}

The first call to method() is not a problem but the second is because we try to optimize the weave and eliminate a runtime instanceof() test:

can't determine superclass of missing type Missing

The solution is to add the test if the type cannot be found. This however has an impact on the woven code when types cannot be found during binary weaving that might later be available at runtime. We should therefore issue a new Xlint warning (name & text to be determined), which will be set to ignore by default, that tells the user that sub-optimal code has been generated and the reason why i.e. missing type X. Should a user be concerned about runtime performance this warning, along with several others we already have, could be enabled during weaving.

Please confirm that this characterizes at least one of the situations you are seeing.

Comment 5 Ron Bodkin 2006-07-07 10:53:48 EDT

Hi Matthew,

That does indeed characterize one of the situations I'm talking about. I really like your idea of just adding the runtime test for types that are unknown at build-time. It would produce correct code and if users want efficient code they should supply the types. Since users weaving the code typical expect that the types won't matter, they are unlikely to care about the reduced efficiency.

As you say it would be good to add related tests for call, target, args, @annotation, @call, @target, @args, and indeed get and set to the harness.

Comment 6 Ron Bodkin 2006-07-07 11:03:53 EDT

A related test would this version of invoke:

        public void invoke () {
                UnrelatedMissing m = new UnrelatedMissing();
                m.foo();
                m.method();
        }

where you have 

public class UnrelatedMissing {
    public void foo() {}
    public void method() {}
}

For the first call, there shouldn't be any warning at all (there's no need to test for UnrelatedMissing being an Interface, since the call doesn't match). The the second one should not be advised because the runtime test fails. This is the expected situation, of course, the missing types typically do not implement the known ones. 

Comment 7 Matthew Webster 2006-07-10 11:35:45 EDT

Expanded test to include call to no-interface method that should not match:

public class TestWithMissing {

	public void invoke () {
		Interface i = new Missing();
		i.interfaceMethod();
		Missing m = new Missing();
		m.interfaceMethod();
		m.missingMethod();
	}
	
	public static void main(String[] args) {
		new TestWithMissing().invoke();
	}

}

Expanded aspects to 3 cases:
    call(public * interfaceMethod(..)) && target(Interface)
    call(public * Interface.interfaceMethod(..))
    call(public * Interface.*(..))

for the new cases we get the warning:

    can't determine whether missing type Missing is an instance of Interface

This could be solved by generating residue for call(). However we cannot do this for declare warning/error whose pointcuts must be statically determinable.

The test is run to ensure that the code has been woven and subsequently advised should the Missing type be available at runtime.

We need testcases for call() && args() as well as call() && @annotation().

Comment 8 Andrew Clement 2007-10-24 11:27:43 EDT

decide if enough users need this...

Comment 9 Andrew Clement 2008-12-03 13:57:27 EST

dont forget to consider bug 145693

Comment 10 Andrew Clement 2013-06-24 11:05:55 EDT

unsetting the target field which is currently set for something already released