NullAway depends on Guava

[dfabulich]

It was a bit tricky to figure out how to configure NullAway to work with Bazel, though I did ultimately get it to work. https://github.com/dfabulich/bazel-checker-framework-bug/

Unfortunately, I was forced to add a dependency on Guava 22 in my Bazel WORKSPACE, and add it as a dependency of my NullAway java_plugin.

WORKSPACE:

maven_jar(
	name='jsr305',
	artifact='com.google.code.findbugs:jsr305:3.0.2',
)

maven_jar(
	name="nullaway",
	artifact="com.uber.nullaway:nullaway:0.3.2"
)

maven_jar(
	name="guava",
	artifact="com.google.guava:guava:22.0",
)

BUILD:

java_library(
	name='x',
	srcs=['X.java'],
	deps=['@jsr305//jar'],
	plugins=['nullaway'],
	javacopts=[
		'-Xep:NullAway:ERROR',
		'-XepOpt:NullAway:AnnotatedPackages=com.example',
	],
)

java_plugin(
	name='nullaway',
	deps=[
		'@nullaway//jar',
		'@guava//jar',
	],
)

If I comment out the @guava//jar line, I get a ClassNotFoundException, included below.

I think that this problem is happening because NullAway.java directly depends on a bunch of com.google.common classes.

I'm a big fan of Guava in general, but this seems like a problem; it forces my app to take on a compile-time dependency on Guava 22. If my app needs to use a different version of Guava, they'll conflict. I think it would be better if NullAway eliminated its dependency on Guava classes, and used the standard library's utilities instead.

error message with stacktrace

An exception has occurred in the compiler ((version info not available)). Please file a bug against the Java compiler via the Java bug reporting page (http://bugreport.java.com) after checking the Bug Database (http://bugs.java.com) for duplicates. Include your program and the following diagnostic in your report. Thank you.
java.util.ServiceConfigurationError: com.google.errorprone.bugpatterns.BugChecker: Provider com.uber.nullaway.NullAway could not be instantiated
	at java.util.ServiceLoader.fail(ServiceLoader.java:232)
	at java.util.ServiceLoader.access$100(ServiceLoader.java:185)
	at java.util.ServiceLoader$LazyIterator.nextService(ServiceLoader.java:384)
	at java.util.ServiceLoader$LazyIterator.next(ServiceLoader.java:404)
	at java.util.ServiceLoader$1.next(ServiceLoader.java:480)
	at com.google.common.collect.TransformedIterator.next(TransformedIterator.java:48)
	at com.google.errorprone.scanner.ScannerSupplier.fromBugCheckerClasses(ScannerSupplier.java:69)
	at com.google.errorprone.ErrorPronePlugins.loadPlugins(ErrorPronePlugins.java:53)
	at com.google.errorprone.ErrorProneAnalyzer.lambda$scansPlugins$0(ErrorProneAnalyzer.java:73)
	at com.google.common.base.Suppliers$NonSerializableMemoizingSupplier.get(Suppliers.java:164)
	at com.google.errorprone.ErrorProneAnalyzer.finished(ErrorProneAnalyzer.java:145)
	at com.google.devtools.build.buildjar.javac.plugins.errorprone.ErrorPronePlugin.postFlow(ErrorPronePlugin.java:116)
	at com.google.devtools.build.buildjar.javac.BlazeJavaCompiler.flow(BlazeJavaCompiler.java:112)
	at com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1384)
	at com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:980)
	at com.sun.tools.javac.api.JavacTaskImpl.lambda$doCall$0(JavacTaskImpl.java:100)
	at com.sun.tools.javac.api.JavacTaskImpl.handleExceptions(JavacTaskImpl.java:142)
	at com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:96)
	at com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:90)
	at com.google.devtools.build.buildjar.javac.BlazeJavacMain.compile(BlazeJavacMain.java:107)
	at com.google.devtools.build.buildjar.SimpleJavaLibraryBuilder$1.invokeJavac(SimpleJavaLibraryBuilder.java:105)
	at com.google.devtools.build.buildjar.ReducedClasspathJavaLibraryBuilder.compileSources(ReducedClasspathJavaLibraryBuilder.java:54)
	at com.google.devtools.build.buildjar.SimpleJavaLibraryBuilder.compileJavaLibrary(SimpleJavaLibraryBuilder.java:108)
	at com.google.devtools.build.buildjar.SimpleJavaLibraryBuilder.run(SimpleJavaLibraryBuilder.java:116)
	at com.google.devtools.build.buildjar.BazelJavaBuilder.processRequest(BazelJavaBuilder.java:105)
	at com.google.devtools.build.buildjar.BazelJavaBuilder.runPersistentWorker(BazelJavaBuilder.java:67)
	at com.google.devtools.build.buildjar.BazelJavaBuilder.main(BazelJavaBuilder.java:45)
Caused by: java.lang.NoClassDefFoundError: com/google/common/collect/SetMultimap
	at java.lang.Class.getDeclaredConstructors0(Native Method)
	at java.lang.Class.privateGetDeclaredConstructors(Class.java:2671)
	at java.lang.Class.getConstructor0(Class.java:3075)
	at java.lang.Class.newInstance(Class.java:412)
	at java.util.ServiceLoader$LazyIterator.nextService(ServiceLoader.java:380)
	... 24 more
Caused by: java.lang.ClassNotFoundException: com.google.common.collect.SetMultimap
	at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	... 29 more
Target //:x failed to build

[msridhar]

Before looking in detail, at a high level NullAway's dependence on Guava should only affect the annotation processor path, not the build path for your application. So something is up with the config here. Lemme dig a bit more

[msridhar]

To double-check: have you actually confirmed with a sample project that the above configuration makes Guava available on the classpath for application code?

[msridhar]

There is a bug here. I had assumed that running with Error Prone meant that Guava would just be available to annotation processors. Looks like on Bazel, EP manages to hide its version of Guava from annotation processors. So we should make our dependence explicit, I guess.

[dfabulich]

You're right; this doesn't affect the app classpath. When I add a com.google.common.collect.SetMultimap mm = null; line to my main method in X.java, it fails to compile, as desired.

I agree that an explicit dependency would be better than an implicit one, but I am a little suspicious about the interaction between EP's Guava and NullAway's Guava. (What if those version don't match up?)

I feel that it would be safer to avoid Guava altogether in a low-level utility like this, but I suppose that's just a feeling rather than a high-priority bug.

I'll rename this bug from "NullAway depends on Guava, which can conflict with my app's version of Guava" to "NullAway depends on Guava"

[msridhar]

Fair point. I hadn't worried about the Guava dependence since I assumed Error Prone exposed it anyway. (With the EP Gradle plugin it is exposed to annotation processors.) I think a short-term solution is to at least make the dependence explicit to make life easier for Bazel users. Long term, we can think about either removing the dependence or, if that's too painful, distributing a shaded version of Guava with NullAway.

BTW, thanks for kicking the tires with Bazel integration! Once we add the explicit Guava dependence and cut a new release, I'll add directions to our wiki.

[davido]

Thanks, @dfabulich!

I was able to set nulaway on Bazel, on this test repo, with diff:

diff --git a/src/main/java/org/gerritcon/mv2016/Printy.java b/src/main/java/org/gerritcon/mv2016/Printy.java
index 771ef50..bf74800 100644
--- a/src/main/java/org/gerritcon/mv2016/Printy.java
+++ b/src/main/java/org/gerritcon/mv2016/Printy.java
@@ -16,13 +16,13 @@ public class Printy {
 
   void mainImpl(String[] argv) throws IOException {
     if (argv.length > 0 && argv[0].equals("--version")) {
-      printVersion();
+      printVersion(null);
       return;
     }
     System.out.println(Joiner.on(' ').join(argv));
   }
 
-  void printVersion() throws IOException {
+  void printVersion(String param) throws IOException {
     URLClassLoader cl = (URLClassLoader) getClass().getClassLoader();
     URL url = cl.findResource("META-INF/MANIFEST.MF");
     Manifest manifest = new Manifest(url.openStream());

I'm, getting:

$ bazel build :printy_lib
..........
INFO: Analysed target //:printy_lib (11 packages loaded).
INFO: Found 1 target...
INFO: From Executing genrule @bazel_tools//tools/jdk:gen_platformclasspath [for host]:
Note: external/bazel_tools/tools/jdk/DumpPlatformClassPath.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
ERROR: /home/davido/projects/bazel_printy/BUILD:9:1: Building libprinty_lib.jar (1 source file) failed (Exit 1)
src/main/java/org/gerritcon/mv2016/Printy.java:19: error: [NullAway] passing @Nullable parameter 'null' where @NonNull is required
      printVersion(null);
                   ^
    (see http://t.uber.com/nullaway )
Target //:printy_lib failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 4.579s, Critical Path: 2.13s
FAILED: Build did NOT complete successfully

@msridhar +1 to add the integration to the WIKI.

While this works, the disadvantage of this approach is that the plugin must be provided per java_library level. It should be possible to register global EP plugins so that it's activated for all rules, like it is the case with nullaway Maven integration.

[msridhar]

@davido isn't there a way to register other annotation processors globally in Bazel, like Dagger? I'd be surprised if there isn't some scripting support so you don't have to do this all manually. In Buck we script nullaway configuration across modules via the DEFS scripting functionality.

[davido]

@msridhar Not that I'm aware of. java_library may have explicit annotation processors via plugins attributed or they can be inherited from java_library's dependencies, via: exported_plugins attribute:

https://docs.bazel.build/versions/master/be/java.html#java_library.exported_plugins

Can you share how you do it with Buck? Last time we used Buck we had to do monkey patching, and overwrite java_libary to merge auto-value annotation processor:

https://github.com/GerritCodeReview/gerrit/blob/stable-2.13/tools/default.defs#L27-L45

I migrated the build tool in gerrit code review from Buck to Bazel one year ago:

https://gerrit-review.googlesource.com/#/c/gerrit/+/91587

[msridhar]

Can you share how you do it with Buck?

We use a combination of Gradle build configuration and OkBuck to inject the NullAway dependence everywhere. And we also do some monkey patching in DEFS to tweak Error Prone configuration (via JVM args) on a per-module basis.

[msridhar]

@dfabulich I've updated the docs with your Bazel example; thanks for that! We dug through our code, and our dependence on Guava is fairly deep (e.g., for the caching libraries). So, unfortunately, we are going to have to keep the dependence for the foreseeable future. Going to close this issue, though if we see a way to remove the Guava dependence in the future we'll look into it.