Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NullPointerException in SuggestedFixes.compilesWithFix #1873

Closed
schlosna opened this issue Oct 20, 2020 · 4 comments
Closed

NullPointerException in SuggestedFixes.compilesWithFix #1873

schlosna opened this issue Oct 20, 2020 · 4 comments

Comments

@schlosna
Copy link
Contributor

Description of the problem:

While upgrading to error-prone 2.4.0 as part of palantir/atlasdb#5044 I encountered the following NullPointerException from error-prone. com.palantir.baseline.errorprone.LambdaMethodReference uses SuggestedFixes.compilesWithFix as part of determining whether a suggested fix for a lambda that can be converted to a method reference.

/Volumes/git/palantir/atlasdb/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java:1378: error: An unhandled exception was thrown by the Error Prone static analysis plugin.
        assertThatExceptionOfType(TransactionLockTimeoutException.class).isThrownBy(() -> transaction.commit());
                                                                                    ^
     Please report this at https://github.com/google/error-prone/issues/new and include the following:

     error-prone version: 2.4.0
     BugPattern: LambdaMethodReference
     Stack Trace:
     java.lang.NullPointerException
        at com.google.errorprone.fixes.SuggestedFixes.compilesWithFix(SuggestedFixes.java:1214)
        at com.google.errorprone.fixes.SuggestedFixes.compilesWithFix(SuggestedFixes.java:1100)
        at com.google.errorprone.fixes.SuggestedFixes.compilesWithFix(SuggestedFixes.java:1082)
        at com.palantir.baseline.errorprone.LambdaMethodReference.lambda$convertMethodInvocations$1(LambdaMethodReference.java:170)
        at java.util.Optional.filter(Optional.java:178)
        at com.palantir.baseline.errorprone.LambdaMethodReference.convertMethodInvocations(LambdaMethodReference.java:170)
        at com.palantir.baseline.errorprone.LambdaMethodReference.checkMethodInvocation(LambdaMethodReference.java:117)
        at com.palantir.baseline.errorprone.LambdaMethodReference.matchLambdaExpression(LambdaMethodReference.java:74)
        at com.google.errorprone.scanner.ErrorProneScanner.processMatchers(ErrorProneScanner.java:451)
        at com.google.errorprone.scanner.ErrorProneScanner.visitLambdaExpression(ErrorProneScanner.java:699)
        at com.google.errorprone.scanner.ErrorProneScanner.visitLambdaExpression(ErrorProneScanner.java:152)
        at com.sun.tools.javac.tree.JCTree$JCLambda.accept(JCTree.java:1805)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:113)
        at com.sun.source.util.TreeScanner.visitMethodInvocation(TreeScanner.java:509)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:753)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:152)
        at com.sun.tools.javac.tree.JCTree$JCMethodInvocation.accept(JCTree.java:1644)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.visitExpressionStatement(TreeScanner.java:433)
        at com.google.errorprone.scanner.ErrorProneScanner.visitExpressionStatement(ErrorProneScanner.java:635)
        at com.google.errorprone.scanner.ErrorProneScanner.visitExpressionStatement(ErrorProneScanner.java:152)
        at com.sun.tools.javac.tree.JCTree$JCExpressionStatement.accept(JCTree.java:1454)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
        at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
        at com.sun.source.util.TreeScanner.visitBlock(TreeScanner.java:248)
        at com.google.errorprone.scanner.ErrorProneScanner.visitBlock(ErrorProneScanner.java:522)
        at com.google.errorprone.scanner.ErrorProneScanner.visitBlock(ErrorProneScanner.java:152)
        at com.sun.tools.javac.tree.JCTree$JCBlock.accept(JCTree.java:1026)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
        at com.sun.source.util.TreeScanner.visitMethod(TreeScanner.java:206)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethod(ErrorProneScanner.java:742)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethod(ErrorProneScanner.java:152)
        at com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:898)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
        at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:113)
        at com.sun.source.util.TreeScanner.visitClass(TreeScanner.java:187)
        at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:550)
        at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:152)
        at com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:808)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:113)
        at com.sun.source.util.TreeScanner.visitCompilationUnit(TreeScanner.java:144)
        at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:562)
        at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:152)
        at com.sun.tools.javac.tree.JCTree$JCCompilationUnit.accept(JCTree.java:591)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:56)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:58)
        at com.google.errorprone.scanner.ErrorProneScannerTransformer.apply(ErrorProneScannerTransformer.java:43)
        at com.google.errorprone.ErrorProneAnalyzer.finished(ErrorProneAnalyzer.java:152)
        at com.sun.tools.javac.api.MultiTaskListener.finished(MultiTaskListener.java:120)
        at com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1404)
        at com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1353)
        at com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:946)
        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 org.gradle.api.internal.tasks.compile.AnnotationProcessingCompileTask.call(AnnotationProcessingCompileTask.java:93)
        at org.gradle.api.internal.tasks.compile.ResourceCleaningCompilationTask.call(ResourceCleaningCompilationTask.java:57)
        at org.gradle.api.internal.tasks.compile.JdkJavaCompiler.execute(JdkJavaCompiler.java:54)
        at org.gradle.api.internal.tasks.compile.JdkJavaCompiler.execute(JdkJavaCompiler.java:39)
        at org.gradle.api.internal.tasks.compile.daemon.AbstractDaemonCompiler$CompilerWorkAction.execute(AbstractDaemonCompiler.java:113)
        at org.gradle.workers.internal.DefaultWorkerServer.execute(DefaultWorkerServer.java:47)
        at org.gradle.workers.internal.AbstractClassLoaderWorker$1.create(AbstractClassLoaderWorker.java:46)
        at org.gradle.workers.internal.AbstractClassLoaderWorker$1.create(AbstractClassLoaderWorker.java:36)
        at org.gradle.internal.classloader.ClassLoaderUtils.executeInClassloader(ClassLoaderUtils.java:98)
        at org.gradle.workers.internal.AbstractClassLoaderWorker.executeInClassLoader(AbstractClassLoaderWorker.java:36)
        at org.gradle.workers.internal.FlatClassLoaderWorker.execute(FlatClassLoaderWorker.java:31)
        at org.gradle.workers.internal.WorkerDaemonServer.execute(WorkerDaemonServer.java:56)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at org.gradle.process.internal.worker.request.WorkerAction.run(WorkerAction.java:118)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
        at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
        at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:182)
        at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:164)
        at org.gradle.internal.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:412)
        at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)
        at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:48)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:56)
        at java.lang.Thread.run(Thread.java:748)
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
1 error
56 warnings

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

git clone https://github.com/palantir/atlasdb.git
cd atlasdb
git checkout ds/error-prone-npe
./gradlew :atlasdb-tests-shared:compileTestJava

What version of Error Prone are you using?

2.4.0

Reproduces with OpenJDK 15 and OpenJDK 8

$ ./gradlew --version

------------------------------------------------------------
Gradle 6.7
------------------------------------------------------------

Build time:   2020-10-14 16:13:12 UTC
Revision:     312ba9e0f4f8a02d01854d1ed743b79ed996dfd3

Kotlin:       1.3.72
Groovy:       2.5.12
Ant:          Apache Ant(TM) version 1.10.8 compiled on May 10 2020
JVM:          15 (Azul Systems, Inc. 15+36)
OS:           Mac OS X 10.15.7 x86_64

Have you found anything relevant by searching the web?

This seems similar to some error-prone issues:

@cushon
Copy link
Collaborator

cushon commented Jan 6, 2021

Bread crumbs for future debugging:

     java.lang.NullPointerException
        at com.google.errorprone.fixes.SuggestedFixes.compilesWithFix(SuggestedFixes.java:1214)

is

https://github.com/google/error-prone/blob/v2.4.0/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java#L1214

That's sort of surprising, I wonder which part of that ended up being null. (The easiest way to find out is probably to repro with 15 for its improved NPE messages.)

@schlosna
Copy link
Contributor Author

schlosna commented Jan 6, 2021

Hi @cushon thanks for looking at this. Per API docs, javax.compiler.Diagnostic#getSource() can return null if no source object is associated with the diagnostic, so might want to handle that case and set diagnosticInSameCompilationUnit false if diagnostic source is null and modifiedFileUri != null. I'm happy to put up a PR to make this more null tolerant if you're interested.

Thanks!

@cushon
Copy link
Collaborator

cushon commented Jan 6, 2021

Thanks, that makes sense. If you're willing to send a PR that'd be great.

I think we might want to treat the case where getSource() == null as equivalent to diagnosticInSameCompilationUnit being true, though--if the re-compilation fails with a possible new diagnostic that isn't associated with a particular file, it could still have been introduced by the fix?

re: improved NPE messages, just for fun this is what JDK 15 gives us:

     java.lang.NullPointerException: Cannot invoke "javax.tools.JavaFileObject.toUri()" because the return value of "javax.tools.Diagnostic.getSource()" is null
        at com.google.errorprone.fixes.SuggestedFixes.compilesWithFix(SuggestedFixes.java:1214)

I was curious what specific diagnostic was causing that problem, since even though it's specified that way I thought they were usually associated with files. Rebuilding @ 2.4.0 with:

diff --git a/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java b/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java
index 315b5e58e..360dade69 100644
--- a/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java
+++ b/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java
@@ -1210,6 +1210,9 @@ public class SuggestedFixes {
     boolean warningInSameCompilationUnit = false;
     for (Diagnostic<? extends JavaFileObject> diagnostic : diagnosticListener.getDiagnostics()) {
       warningIsError |= diagnostic.getCode().equals("compiler.err.warnings.and.werror");
+      if (diagnostic.getSource() == null) {
+        throw new AssertionError(diagnostic);
+      }
       boolean diagnosticInSameCompilationUnit =
           diagnostic.getSource().toUri().equals(modifiedFileUri);
       switch (diagnostic.getKind()) {

Shows that the underlying diagnostic was:

warning: unknown enum constant org.immutables.value.Value.Style.ImplementationVisibility.PACKAGE
    reason: class file for org.immutables.value.Value$Style$ImplementationVisibility not found

Somewhat interestingly the NPE doesn't reproduce with Error Prone built at head, but adding the null-check anyways still seems like a good idea.

@schlosna
Copy link
Contributor Author

schlosna commented Jan 7, 2021

Nice use of the richer NPE error messages and interesting find on the underlying warning triggering this specific diagnostic case (slightly related to immutables/immutables#291 ). I created #2064 for the quick NPE fix, though haven't had time to see if there's a good way to unit test this without some major refactoring as SuggestedFixes.compilesWithFix is private, so open to thoughts there.

stevie400 pushed a commit to HubSpot/error-prone that referenced this issue Jan 15, 2021
In some cases, compiler diagnostics may not have an associated source,
and for testing whether suggested fixes compile these should be treated
as originating from the same compilation unit of the suggested fix.

Fixes google#1873

Fixes google#2064

COPYBARA_INTEGRATE_REVIEW=google#2064 from schlosna:ds/1873-SuggestedFixes-NPE ddd042b
PiperOrigin-RevId: 350812718
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants