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

Conflict when running Checker Framework and NullAway in the same build #462

Closed
xenoterracide opened this issue Apr 28, 2021 · 15 comments
Closed

Comments

@xenoterracide
Copy link

xenoterracide commented Apr 28, 2021

/home/xeno/IdeaProjects/brix/config-loader/spi/src/main/java/com/xenoterracide/brix/configloader/spi/ConfigValueProcessor.java:23: error: An unhandled exception was thrown by the Error Prone static analysis plugin.
public class ConfigValueProcessor {
       ^
     Please report this at https://github.com/google/error-prone/issues/new and include the following:
  
     error-prone version: 2.6.0
     BugPattern: NullAway
     Stack Trace:
     com.google.common.util.concurrent.ExecutionError: java.lang.NoSuchMethodError: 'java.lang.Object org.checkerframework.dataflow.cfg.node.NodeVisitor.visitImplicitThisLiteral(org.checkerframework.dataflow.cfg.node.ImplicitThisLiteralNode, java.lang.Object)'
  	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2049)
  	at com.google.common.cache.LocalCache.get(LocalCache.java:3951)
  	at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:3974)
  	at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:4935)
  	at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:4941)
  	at com.uber.nullaway.dataflow.DataFlow.dataflow(DataFlow.java:149)
  	at com.uber.nullaway.dataflow.DataFlow.finalResult(DataFlow.java:222)
  	at com.uber.nullaway.dataflow.AccessPathNullnessAnalysis.getNonnullFieldsOfReceiverAtExit(AccessPathNullnessAnalysis.java:143)
  	at com.uber.nullaway.NullAway.guaranteedNonNullForConstructor(NullAway.java:1588)
  	at com.uber.nullaway.NullAway.checkConstructorInitialization(NullAway.java:1565)
  	at com.uber.nullaway.NullAway.checkFieldInitialization(NullAway.java:1424)
  	at com.uber.nullaway.NullAway.matchClass(NullAway.java:1154)
  	at com.google.errorprone.scanner.ErrorProneScanner.processMatchers(ErrorProneScanner.java:450)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:548)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:151)
  	at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:808)
  	at jdk.compiler/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 jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
  	at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:113)
  	at jdk.compiler/com.sun.source.util.TreeScanner.visitCompilationUnit(TreeScanner.java:144)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:561)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:151)
  	at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCCompilationUnit.accept(JCTree.java:591)
  	at jdk.compiler/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 jdk.compiler/com.sun.tools.javac.api.MultiTaskListener.finished(MultiTaskListener.java:132)
  	at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1418)
  	at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1365)
  	at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:960)
  	at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.lambda$doCall$0(JavacTaskImpl.java:104)
  	at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.handleExceptions(JavacTaskImpl.java:147)
  	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.gradle.internal.compiler.java.IncrementalCompileTask.call(IncrementalCompileTask.java:77)
  	at org.gradle.api.internal.tasks.compile.AnnotationProcessingCompileTask.call(AnnotationProcessingCompileTask.java:94)
  	at org.gradle.api.internal.tasks.compile.ResourceCleaningCompilationTask.call(ResourceCleaningCompilationTask.java:57)
  	at org.gradle.api.internal.tasks.compile.JdkJavaCompiler.execute(JdkJavaCompiler.java:55)
  	at org.gradle.api.internal.tasks.compile.JdkJavaCompiler.execute(JdkJavaCompiler.java:40)
  	at org.gradle.api.internal.tasks.compile.daemon.AbstractDaemonCompiler$CompilerWorkAction.execute(AbstractDaemonCompiler.java:135)
  	at org.gradle.workers.internal.DefaultWorkerServer.execute(DefaultWorkerServer.java:63)
  	at org.gradle.workers.internal.AbstractClassLoaderWorker$1.create(AbstractClassLoaderWorker.java:49)
  	at org.gradle.workers.internal.AbstractClassLoaderWorker$1.create(AbstractClassLoaderWorker.java:43)
  	at org.gradle.internal.classloader.ClassLoaderUtils.executeInClassloader(ClassLoaderUtils.java:97)
  	at org.gradle.workers.internal.AbstractClassLoaderWorker.executeInClassLoader(AbstractClassLoaderWorker.java:43)
  	at org.gradle.workers.internal.FlatClassLoaderWorker.run(FlatClassLoaderWorker.java:32)
  	at org.gradle.workers.internal.FlatClassLoaderWorker.run(FlatClassLoaderWorker.java:22)
  	at org.gradle.workers.internal.WorkerDaemonServer.run(WorkerDaemonServer.java:85)
  	at org.gradle.workers.internal.WorkerDaemonServer.run(WorkerDaemonServer.java:55)
  	at org.gradle.process.internal.worker.request.WorkerAction$1.call(WorkerAction.java:138)
  	at org.gradle.process.internal.worker.child.WorkerLogEventListener.withWorkerLoggingProtocol(WorkerLogEventListener.java:41)
  	at org.gradle.process.internal.worker.request.WorkerAction.run(WorkerAction.java:135)
  	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
  	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
  	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
  	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:414)
  	at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)
  	at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:48)
  	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
  	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
  	at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:56)
  	at java.base/java.lang.Thread.run(Thread.java:829)
  Caused by: java.lang.NoSuchMethodError: 'java.lang.Object org.checkerframework.dataflow.cfg.node.NodeVisitor.visitImplicitThisLiteral(org.checkerframework.dataflow.cfg.node.ImplicitThisLiteralNode, java.lang.Object)'
  	at org.checkerframework.dataflow.cfg.node.ImplicitThisLiteralNode.accept(ImplicitThisLiteralNode.java:21)
  	at org.checkerframework.dataflow.analysis.AbstractAnalysis.callTransferFunction(AbstractAnalysis.java:336)
  	at org.checkerframework.dataflow.analysis.ForwardAnalysisImpl.callTransferFunction(ForwardAnalysisImpl.java:373)
  	at org.checkerframework.dataflow.analysis.ForwardAnalysisImpl.performAnalysisBlock(ForwardAnalysisImpl.java:128)
  	at org.checkerframework.dataflow.analysis.ForwardAnalysisImpl.performAnalysis(ForwardAnalysisImpl.java:105)
  	at com.uber.nullaway.dataflow.DataFlow$1.load(DataFlow.java:88)
  	at com.uber.nullaway.dataflow.DataFlow$1.load(DataFlow.java:80)
  	at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3529)
  	at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2278)
  	at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2155)
  	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2045)

this is the offending class

package com.xenoterracide.brix.configloader.spi;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.mitchellbosecke.pebble.PebbleEngine;
import com.xenoterracide.brix.cli.api.CliConfiguration;
import com.xenoterracide.brix.configloader.api.ImmutableProcessedConfig;
import com.xenoterracide.brix.configloader.api.ImmutableProcessedFileConfiguration;
import com.xenoterracide.brix.configloader.api.ProcessedConfig;
import com.xenoterracide.brix.configloader.api.ProcessedFileConfiguration;
import io.vavr.control.Try;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.springframework.stereotype.Component;

import java.io.StringWriter;
import java.nio.file.Path;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.stream.Collectors;

@Component
public class ConfigValueProcessor {
  private final Logger log = LogManager.getLogger( this.getClass() );

  private final PebbleEngine engine;

  private final CliConfiguration cliConfiguration;

  private final ObjectMapper mapper;

  private final Path configDir;

  ConfigValueProcessor(
    CliConfiguration cliConfiguration,
    PebbleEngine stringEngine,
    ObjectMapper mapper,
    Path foundConfig
  ) {
    this.engine = stringEngine;
    this.cliConfiguration = cliConfiguration;
    this.mapper = mapper;
    this.configDir = foundConfig.getParent();
  }

  public ProcessedConfig from( RawConfig config ) {
    var fcs
      = config.getFileConfigurations()
      .stream()
      .map( this::from )
      .collect( Collectors.toList() );

    var processed = ImmutableProcessedConfig.builder().fileConfigurations( fcs ).build();
    log.debug( "processed: {}", processed );
    return processed;
  }

  ProcessedFileConfiguration from( RawFileConfiguration config ) {
    var context = this.getContext( config.getContext() );
    var bldr = ImmutableProcessedFileConfiguration.builder();
    bldr.overwrite( config.getOverwrite() );
    bldr.context( context );

    config.getSource().ifPresent( src -> {
      bldr.source( Path.of( this.processTemplate( src, context ) ) );
    } );
    return bldr.build();
  }

  @SuppressWarnings("unchecked")
  Map<String, Object> getContext( Map<String, String> context ) {
    var map = new HashMap<String, Object>();
    map.putAll( context );
    map.putAll( mapper.convertValue( cliConfiguration, Map.class ) );
    map.put( "configDir", configDir.getParent() );
    return Collections.unmodifiableMap( map );
  }

  String processTemplate( String template, Map<String, Object> context ) {
    var writer = new StringWriter();
    Try.run( () -> engine.getTemplate( template ).evaluate( writer, context ) ).get();
    return writer.toString();
  }
}
------------------------------------------------------------
Gradle 7.0
------------------------------------------------------------

Build time:   2021-04-09 22:27:31 UTC
Revision:     d5661e3f0e07a8caff705f1badf79fb5df8022c4

Kotlin:       1.4.31
Groovy:       3.0.7
Ant:          Apache Ant(TM) version 1.10.9 compiled on September 27 2020
JVM:          11.0.10 (AdoptOpenJDK 11.0.10+9)
OS:           Linux 5.11.10-200.fc33.x86_64 amd64
\--- com.uber.nullaway:nullaway:0.+ -> 0.9.1
     +--- org.checkerframework:dataflow:3.6.0
     |    +--- org.checkerframework:checker-qual:3.6.0 -> 3.8.0
     |    \--- org.checkerframework:javacutil:3.6.0
     |         +--- org.checkerframework:checker-qual:3.6.0 -> 3.8.0
     |         \--- org.plumelib:plume-util:1.1.4
     |              \--- org.plumelib:reflection-util:0.2.2
Fedora 33
5.11.10-200.fc33.x86_64
 10:25:10 up 26 days, 11:31,  1 user,  load average: 1.36, 1.71, 1.68
              total        used        free      shared  buff/cache   available
Mem:            15G         12G        651M        1.2G        2.7G        1.9G
Swap:           12G        2.8G        9.2G

this issue does not exist if I switch my version to 0.8.+ instead of just 0.+

@msridhar
Copy link
Collaborator

Thanks for the report! This must be related to not shadowing Checker Framework jars anymore, but I am stumped as to what could be happening. Two questions:

  • Is this reproducible on an open-source project on GitHub? If so could you push a branch where we can repro?
  • Would it be possible to try with Error Prone 2.5.1 to see if you still see the same error? This is a shot in the dark but just want to check if possible.

@xenoterracide
Copy link
Author

yes, I could, brix is open source, I just have to roll back to these changes

@msridhar
Copy link
Collaborator

Figured this out. TL;DR: running Checker Framework and NullAway together is causing problems.

I see this in the output of ./gradlew :config-loader:api:dependencies:

checkerFramework - A copy of JDK classes with Checker Framework type qualifiers inserted.
+--- org.checkerframework:checker:3.+ -> 3.12.0
|    +--- org.checkerframework:checker-qual:3.12.0
|    \--- org.checkerframework:checker-util:3.12.0
|         \--- org.checkerframework:checker-qual:3.12.0
+--- org.checkerframework:checker:{strictly 3.12.0} -> 3.12.0 (c)
+--- org.checkerframework:checker-qual:{strictly 3.12.0} -> 3.12.0 (c)
\--- org.checkerframework:checker-util:{strictly 3.12.0} -> 3.12.0 (c)

The org.checkerframework:checker jar includes some of its dependencies, in particular all classes in the org.checkerframework:dataflow dependency. This is what is causing the error you are seeing: there are some inconsistent class files in the annotation processor path, between NullAway's dependence on dataflow 3.6 and the classes in the checker 3.12 jar.

We could temporarily fix this by bumping NullAway's org.checkerframework:dataflow dependency to 3.12.0, but I am afraid the problem will return in the future. Fundamentally NullAway is conservative about bumping our org.checkerframework:dataflow dependency since it is performance-critical for us, and a bump requires internal testing to ensure no significant performance regression. So generally we won't be keeping up with the latest Checker Framework releases. My best suggestion here for a stable build (unfortunately) is to not run Checker Framework and NullAway simultaneously. There is an example of how to config Gradle to keep things separate here (for an older issue involving Error Prone, but the same idea applies):

https://github.com/kelloggm/checkerframework-gradle-plugin#incompatibility-with-error-prone

@xenoterracide if you have any other thoughts / suggestions let us know.

@xenoterracide
Copy link
Author

Ugh, stop relying on checker framework.... Lol

Maybe make like gradle and have an internal version that has transformed package names. No really, that's not a bad idea. I'm not certain how Gradle does this and I'm pretty sure guava has the option too, but it's essentially just a package rename or addition. Like com.uber.nullaway.org.checkerframework...

Other than that I've got nothing, I'll have to make a call. It's likely I won't use one or the other, not certain which yet, or I'll have to do what you're suggesting which I've thought about doing for other reasons.

@msridhar
Copy link
Collaborator

Yeah, we want to avoid re-packaging and shading third-party deps, so that's not an option right now. And we think that there will be few users that need to run NullAway and Checker Framework simultaneously. I'll put it on the TODO list to do a bump of Checker dataflow, but as noted above I don't think that's a long-term solution.

@xenoterracide
Copy link
Author

Yeah, we want to avoid re-packaging and shading third-party deps, so that's not an option right now. And we think that there will be few users that need to run NullAway and Checker Framework simultaneously. I'll put it on the TODO list to do a bump of Checker dataflow, but as noted above I don't think that's a long-term solution.

I don't have A better solution than those. I'll have to pick one of the 2 options since I'm not going to implement double compilation for every java project I do.

@xenoterracide
Copy link
Author

xenoterracide commented Apr 30, 2021

I also want to mention that checker framework explicitly states that it will break API with any given release, so I think that it may be best simply to say that using with checker framework is not supported somewhere due to this fact. Alternatively, you could provide a BOM and document supported versions.

@msridhar
Copy link
Collaborator

Thanks, @xenoterracide. I also contribute to Checker Framework, so let me think more to see if there is a reasonable solution we haven't thought of.

@xenoterracide
Copy link
Author

One thought I've had is that gradle java needs to have a 'compilerPluginClasspath' this would at least allow a version lock and constraint. However I'm not prepared to write that feature request as I don't fully understand plugins

@msridhar
Copy link
Collaborator

msridhar commented May 3, 2021

@xenoterracide the latest here:

  1. I'm working with the Checker Framework maintainers to change how their Maven artifacts are packaged to make version conflicts more evident, and also to do a bit more shading of deps.
  2. I created Update to Checker dataflow 3.13.0 #468 to update our Checker dataflow dependence. Once that lands, and we cut a new NullAway release, you should be able to run Checker Framework and NullAway together again.

Once 1 gets worked out, at least we should be able to discover future incompatibility issues more quickly. Also, in my experience the APIs for the dataflow and javacutil Checker Framework artifacts have generally been pretty stable. So, I would hope there wouldn't be too many conflicts moving forward, though of course there are no guarantees.

@msridhar
Copy link
Collaborator

msridhar commented Jun 3, 2021

Update here: once #485 lands and we cut a new release, there should be no incompatibility between Checker Framework and NullAway anymore

@msridhar msridhar changed the title internal exception no such method Conflict when running Checker Framework and NullAway in the same build Jun 3, 2021
@msridhar
Copy link
Collaborator

FYI NullAway 0.9.2 has been released, and that version no longer conflicts with the Checker Framework

@xenoterracide
Copy link
Author

@msridhar curious, did you ever succeed in the whole backwards compatibility thing with how they package their jars? even if they did, I'd guess that it doesn't 100% solve the problem if nullaway were on a significantly different version. I wonder if I should give checker a try again, this problem has kept me away from it, since their attitude was "we aren't using semver and reserve the right to break ABI on any and do every upgrade"

@msridhar
Copy link
Collaborator

msridhar commented Mar 20, 2024

Hi @xenoterracide at this point, I would expect that there is no issue with running the Checker Framework and NullAway in the same build. NullAway now depends on a shaded dataflow artifact created just for our project; see #485. So there should be no conflict or semver issues, as the two tools can depend on completely different versions of common libraries. (In fact, Error Prone itself also has its own shaded version of the relevant Checker Framework libraries, so there should be no conflict there either.) If you do run into any issues please let us know and we can take a look.

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

No branches or pull requests

2 participants