NullPointerException thrown for BugPattern ScheduledTransactionTrace when using error-prone-support on jbanking

marcwrobel commented 1 year ago

Describe the bug

While experimenting Error Prone Support on jbanking I got a java.lang.NullPointerException in tech.picnic.errorprone.bugpatterns.ScheduledTransactionTrace.

Considering the ScheduledTransactionTrace bug pattern is located in error-prone-contrib I opened an issue here instead of https://github.com/google/error-prone.

[x] I have verified that the issue is reproducible against the latest version of the project.
[x] I have searched through existing issues to verify that this issue is not already known.

Minimal Reproducible Example

The code that triggered the issue is located at https://github.com/marcwrobel/jbanking/blob/error-prone/src/main/java/fr/marcwrobel/jbanking/calendar/DayOfWeekInMonthHoliday.java. The bug can be reproduced using https://github.com/marcwrobel/jbanking/tree/error-prone.

Logs

```sh git clone git@github.com:marcwrobel/jbanking.git cd jbanking git checkout error-prone (error-prone u=)$ mvn clean install Apache Maven 3.9.2 (c9616018c7a021c1c39be70fb2843d6f5f9b8a1c) Maven home: /home/mwrobel/.asdf/installs/maven/3.9.2 Java version: 17.0.7, vendor: Eclipse Adoptium, runtime: /home/mwrobel/.asdf/installs/java/temurin-17.0.7+7 Default locale: en_US, platform encoding: UTF-8 OS name: "linux", version: "6.1.0-0.deb11.6-amd64", arch: "amd64", family: "unix" 911 [INFO] Error stacktraces are turned on. 1001 [INFO] Scanning for projects... 1215 [INFO] 1215 [INFO] -----------------------< fr.marcwrobel:jbanking >----------------------- 1215 [INFO] Building jbanking 4.2.0-SNAPSHOT 1215 [INFO] from pom.xml 1215 [INFO] --------------------------------[ jar ]--------------------------------- 1396 [INFO] 1397 [INFO] --- clean:3.2.0:clean (default-clean) @ jbanking --- 1464 [INFO] Deleting /home/mwrobel/projects/jbanking/target 1465 [INFO] 1466 [INFO] --- flatten:1.3.0:clean (flatten.clean) @ jbanking --- 1744 [INFO] Deleting /home/mwrobel/projects/jbanking/.flattened-pom.xml 1745 [INFO] 1745 [INFO] --- enforcer:3.1.0:enforce (enforce-maven) @ jbanking --- 1855 [INFO] 1855 [INFO] --- formatter:2.16.0:format (default) @ jbanking --- 2010 [INFO] Using 'UTF-8' encoding to format source files. 2039 [INFO] Number of files to be formatted: 98 3400 [INFO] Successfully formatted: 0 file(s) 3400 [INFO] Fail to format: 0 file(s) 3400 [INFO] Skipped: 98 file(s) 3400 [INFO] Read only skipped: 0 file(s) 3400 [INFO] Approximate time taken: 1s 3400 [INFO] 3400 [INFO] --- resources:3.3.0:resources (default-resources) @ jbanking --- 3466 [INFO] Copying 0 resource 3467 [INFO] Copying 1 resource 3472 [INFO] 3473 [INFO] --- flatten:1.3.0:flatten (flatten) @ jbanking --- 3485 [INFO] Generating flattened POM of project fr.marcwrobel:jbanking:jar:4.2.0-SNAPSHOT... 3548 [INFO] 3548 [INFO] --- compiler:3.10.1:compile (default-compile) @ jbanking --- 3814 [INFO] Changes detected - recompiling the module! 3816 [INFO] Compiling 49 source files to /home/mwrobel/projects/jbanking/target/classes 6490 [INFO] ------------------------------------------------------------- 6490 [WARNING] COMPILATION WARNING : 6490 [INFO] ------------------------------------------------------------- 6490 [WARNING] bootstrap class path not set in conjunction with -source 8 6490 [INFO] 1 warning 6490 [INFO] ------------------------------------------------------------- 6491 [INFO] ------------------------------------------------------------- 6491 [ERROR] COMPILATION ERROR : 6491 [INFO] ------------------------------------------------------------- 6491 [ERROR] /home/mwrobel/projects/jbanking/src/main/java/fr/marcwrobel/jbanking/calendar/DayOfWeekInMonthHoliday.java:[39,10] An unhandled exception was thrown by the Error Prone static analysis plugin. Please report this at https://github.com/google/error-prone/issues/new and include the following: error-prone version: 2.19.1 BugPattern: ScheduledTransactionTrace Stack Trace: java.lang.NullPointerException: Cannot invoke "java.util.Map.get(Object)" because "msym.visiblePackages" is null at jdk.compiler/com.sun.tools.javac.code.Symtab.lookupPackage(Symtab.java:695) at jdk.compiler/com.sun.tools.javac.code.Symtab.lookupPackage(Symtab.java:676) at jdk.compiler/com.sun.tools.javac.code.ClassFinder.loadClass(ClassFinder.java:425) at tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary.canLoadClass(ThirdPartyLibrary.java:91) at tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary.isKnownClass(ThirdPartyLibrary.java:84) at tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary.canIntroduceUsage(ThirdPartyLibrary.java:74) at tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary.lambda$new$60d6954b$1(ThirdPartyLibrary.java:59) at com.google.errorprone.VisitorState$Cache.get(VisitorState.java:702) at tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary.isIntroductionAllowed(ThirdPartyLibrary.java:70) at tech.picnic.errorprone.bugpatterns.ScheduledTransactionTrace.matchMethod(ScheduledTransactionTrace.java:55) at com.google.errorprone.scanner.ErrorProneScanner.processMatchers(ErrorProneScanner.java:449) at com.google.errorprone.scanner.ErrorProneScanner.visitMethod(ErrorProneScanner.java:739) at com.google.errorprone.scanner.ErrorProneScanner.visitMethod(ErrorProneScanner.java:150) at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:953) at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:86) at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74) at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48) at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:96) at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:111) at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:119) at jdk.compiler/com.sun.source.util.TreeScanner.visitClass(TreeScanner.java:203) at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:548) at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:150) at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:860) at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:86) at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74) at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48) at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:111) at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:119) at jdk.compiler/com.sun.source.util.TreeScanner.visitCompilationUnit(TreeScanner.java:152) at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:560) at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:150) at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCCompilationUnit.accept(JCTree.java:614) at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:60) 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:156) at jdk.compiler/com.sun.tools.javac.api.MultiTaskListener.finished(MultiTaskListener.java:132) at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1394) at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1341) at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:933) at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.lambda$doCall$0(JavacTaskImpl.java:104) at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.invocationHelper(JavacTaskImpl.java:152) at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:100) at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:94) at org.codehaus.plexus.compiler.javac.JavaxToolsCompiler.compileInProcess(JavaxToolsCompiler.java:136) at org.codehaus.plexus.compiler.javac.JavacCompiler.performCompile(JavacCompiler.java:182) at org.apache.maven.plugin.compiler.AbstractCompilerMojo.execute(AbstractCompilerMojo.java:1209) at org.apache.maven.plugin.compiler.CompilerMojo.execute(CompilerMojo.java:198) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:126) at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute2(MojoExecutor.java:342) at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute(MojoExecutor.java:330) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:213) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:175) at org.apache.maven.lifecycle.internal.MojoExecutor.access$000(MojoExecutor.java:76) at org.apache.maven.lifecycle.internal.MojoExecutor$1.run(MojoExecutor.java:163) at org.apache.maven.plugin.DefaultMojosExecutionStrategy.execute(DefaultMojosExecutionStrategy.java:39) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:160) at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:105) at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:73) at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build(SingleThreadedBuilder.java:53) at org.apache.maven.lifecycle.internal.LifecycleStarter.execute(LifecycleStarter.java:118) at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:261) at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:173) at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:101) at org.apache.maven.cli.MavenCli.execute(MavenCli.java:910) at org.apache.maven.cli.MavenCli.doMain(MavenCli.java:283) at org.apache.maven.cli.MavenCli.main(MavenCli.java:206) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced(Launcher.java:283) at org.codehaus.plexus.classworlds.launcher.Launcher.launch(Launcher.java:226) at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode(Launcher.java:407) at org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:348) 6492 [INFO] 1 error 6492 [INFO] ------------------------------------------------------------- 6492 [INFO] ------------------------------------------------------------------------ 6493 [INFO] BUILD FAILURE 6493 [INFO] ------------------------------------------------------------------------ 6494 [INFO] Total time: 5.559 s 6494 [INFO] Finished at: 2023-05-14T18:32:32+02:00 6494 [INFO] ------------------------------------------------------------------------ ```

Expected behavior

A successful build, without NullPointerException.

Setup

Operating system : Debian GNU/Linux 11 (bullseye)
Java version : 17.0.7 (Eclipse Adoptium)
Error Prone version : 2.19.1
Error Prone Support version : 0.11.0

Additional context

None

Stephan202 commented 1 year ago

Tnx for the report @marcwrobel! I just checked out the repository and branch, and can indeed reproduce the issue. Will have a look!

Stephan202 commented 1 year ago

@marcwrobel to get you unblocked, the following patch avoids the issue (it disables the only three checks that rely on the ThirdPartyLibrary class):

diff --git a/pom.xml b/pom.xml
index bbd0c3e..e9677c7 100644
--- a/pom.xml
+++ b/pom.xml
@@ -96,6 +96,10 @@
               <arg>
                 -Xplugin:ErrorProne
                 <!-- More Error Prone flags on https://errorprone.info/docs/flags. -->
+                <!-- Workaround for https://github.com/PicnicSupermarket/error-prone-support/issues/626. -->
+                -Xep:CollectorMutability:OFF
+                -Xep:FluxImplicitBlock:OFF
+                -Xep:ScheduledTransactionTrace:OFF
               </arg>
               <arg>-XDcompilePolicy=simple</arg>
             </compilerArgs>

As for the proper fix: I see where the NPE happens, and we could avoid it using an extra if-statement, but the fix I have so far isn't strictly correct. (Because if I drop <scope>test</scope> from the project's Guava dependency declaration, then relevant field remains null, so testing for that would then cause ThirdPartyLibrary.GUAVA.isIntroductionAllowed(state) to return false, which would be incorrect.) Requires more thinking.

Stephan202 commented 1 year ago

I filed #627, which should properly resolve the issue. Thanks again for reporting @marcwrobel.

NB: Good to keep in mind that this library is rather opinionated about use of Guava and AssertJ. I imagine that especially the former may not be a good fit for jbanking, as it is a zero-dependency library. It may help to omit the refaster-runner dependency for now, because for Refaster rules we currently do not have classpath-conditional logic. (Changing that is on our TODO list, but there's no ETA.)

marcwrobel commented 1 year ago

Thanks for your prompt response @Stephan202.

NB: Good to keep in mind that this library is rather opinionated about use of Guava and AssertJ. I imagine that especially the former may not be a good fit for jbanking, as it is a zero-dependency library.

Yep, I just saw that. I am thinking about moving to AssertJ, so at least it may help for that migration.

Stephan202 commented 1 year ago

Hey @marcwrobel, version 0.11.1 is now available on Maven Central :)

marcwrobel commented 1 year ago

Hi @Stephan202, just tested the new version and everything works fine now. Thanks !

PicnicSupermarket / error-prone-support