typetools / checker-framework

Pluggable type-checking for Java
http://checkerframework.org/
Other
1.01k stars 353 forks source link

add ability to load annotation classes from processor path #2606

Open msridhar opened 5 years ago

msridhar commented 5 years ago

We have a checker that has one sub-checker, with the checker and sub-checker residing in different jar files. When we place the jars on the classpath, the checker runs fine, but when we place both jars on the processor path, the subchecker fails to road.

Zip for repro: repro-cf-crash.zip

Unzip and run this command in the repro-cf-crash directory to see the crash:

$ javac -processorpath typesafe-builder-checker.jar:returnsrecv-checker-master-SNAPSHOT.jar:checker-2.8.1.jar:spring-expression-5.1.7.RELEASE.jar:lombok-1.18.8.jar:auto-value-annotations-1.6.5.jar Foo.java
error: InvocationTargetException when invoking constructor for class org.checkerframework.checker.returnsrcvr.ReturnsRcvrVisitor; Underlying cause: InvocationTargetException when invoking constructor for class org.checkerframework.checker.returnsrcvr.ReturnsRcvrAnnotatedTypeFactory; Underlying cause: Cannot load annotation interface org.checkerframework.checker.returnsrcvr.qual.MaybeThis
  Exception: java.lang.reflect.InvocationTargetException; Stack trace: sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
  sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
  sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
  java.lang.reflect.Constructor.newInstance(Constructor.java:423)
  org.checkerframework.common.basetype.BaseTypeChecker.invokeConstructorFor(BaseTypeChecker.java:274)
  org.checkerframework.common.basetype.BaseTypeChecker.createSourceVisitor(BaseTypeChecker.java:212)

But if the jars are placed in the classpath, the checker runs fine:

$ javac -classpath typesafe-builder-checker.jar:returnsrecv-checker-master-SNAPSHOT.jar:checker-2.8.1.jar:spring-expression-5.1.7.RELEASE.jar:lombok-1.18.8.jar:auto-value-annotations-1.6.5.jar Foo.java
warning: You do not seem to be using the distributed annotated JDK.  To fix the problem, supply javac an argument like:  -Xbootclasspath/p:.../checker/dist/ .  Currently using: jdk8.jar
1 warning

The checker is in typesafe-builder-checker.jar and the sub-checker is in returnsrecv-checker-master-SNAPSHOT.jar, and the checker is being run via processor auto-discovery (though I don't think that matters for the crash). Here is the code specifying the sub-checker:

https://github.com/kelloggm/typesafe-builder-checker/blob/7b61943a291b47bdb65689abd3b7bbd73c4c8f2f/src/main/java/org/checkerframework/checker/builder/TypesafeBuilderChecker.java#L17-L22

msridhar commented 5 years ago

/cc @ranmanli @kelloggm

kelloggm commented 5 years ago

The crash you're observing occurs because the qualifiers that your typechecker is attempting to place in class files are not on the classpath. That is, the jar that contains the annotations is the same as the jar that contains the checker, and the checker needs the annotations on the classpath.

One option would be to split up the annotations and the checker into different artifacts, and only put the annotations on the classpath. This is what the Checker Framework itself does (the difference between the checker.jar and checker-qual.jar is that the former contains the typecheckers and framework, while the latter contains only annotations).

For small-scale experiments, it's probably fine to just have the checker on the classpath.

However, it does seems odd that to run a checker, the annotations for that checker must be on the classpath, even if they aren't used in the source code.

msridhar commented 5 years ago

Thanks @kelloggm yes that is what is going on. I guess it's reasonable to require the annotations to be on the classpath in most cases, since the source code may have explicit annotations. So this is a bit of a corner case. On the other hand, there may be cases where checkers rely almost entirely on inference, so it may be good to not crash in this scenario.

wmdietl commented 5 years ago

The problem is that the Checker Framework doesn't know whether an annotation will appear in the source code or not. The line of code that raises the exception: https://github.com/typetools/checker-framework/blob/61021998147678320365ba64b0ec53704ccd1f17/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java#L693

is trying to build the qualifier hierarchy and fails to load that annotation. Simply ignoring that error would build an incorrect qualifier hierarchy and not give you a good result later on.

(In the July release we actually improved this error: https://github.com/typetools/checker-framework/blob/master/javacutil/src/main/java/org/checkerframework/javacutil/AnnotationBuilder.java#L142 should raise a more useful error message earlier-on with a hint that the classpath is the problem.)

msridhar commented 5 years ago

I think this one can actually be closed? I am convinced that CF requiring that checker annotations are in the classpath is actually fine. I'll go ahead and close, but we can re-open if I mis-understood.

msridhar commented 4 years ago

I'd like to re-open this issue and re-focus on the possibility of making checkers run even if the annotation classes are only present on the processorpath, not the classpath. We have one real-world deployment scenario where we wanted to run a checker on a large code base without any user-written annotations. It was much more painful to get the checker running because of the need to inject the annotation jars into the classpath, in addition to adding the checker jars to the processor path. I suspect that in other real-world scenarios, it will be easier to inject jars on the processor path than the classpath. Injecting jars into the processor path is more common, e.g., to run common annotation processors everywhere.

@wmdietl would it be terribly difficult to get the framework to load annotations from the processor path if they are not present on the class path?

/cc @lazaroclapp

wmdietl commented 4 years ago

I'll have another look.