Upgrade to Error Prone 2.3.3 #295

ZacSweers · 2019-04-01T10:31:10Z

Most of the noise in this diff falls under a few categories:

UnusedVariable check
Strong enforcement of immutables (no mixing, using them in autovalue)
Use Types.isSameType() for type comparisons (instead of .equals())
Use state.getSourceForNode() for representing trees rather than Tree#toString()
Use fluent APIs for assertions in tests

Resolves #286

Most of the noise in this diff falls under a few categories: - UnusedVariable check - Strong enforcement of immutables (no mixing, using them in autovalue) - Use Types.isSameType() for type comparisons (instead of .equals()) - Use state.getSourceForNode() for representing trees rather than Tree#toString() - Use fluent APIs for assertions in tests Resolves uber#286

lazaroclapp

Isn't this still blocked on faster buck support for XPlugin? We don't want to make upstream NullAway incompatible with our internal infra.

jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParams.java

nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java

ZacSweers · 2019-04-01T18:15:24Z

Isn't this still blocked on faster buck support for XPlugin? We don't want to make upstream NullAway incompatible with our internal infra.

That issue started with 2.3.2 though, which NullAway is already on. That's why I thought this was fair game :). We can also roll it back to 2.3.2 and keep most of the fixes for things it raised for now as a preparation step.

msridhar · 2019-04-01T18:20:01Z

I think it’s fine to update to EP 2.3.3 for building NullAway. But NullAway probably shouldn’t have a dependency on EP >= 2.3.2. Can we tease these apart?

ZacSweers · 2019-04-02T06:25:46Z

Let me take a look at separating them. Want to reiterate though that we're already on 2.3.2, which has the plugin change. 2.3.3 wouldn't be a new change in that regard

msridhar · 2019-04-02T14:47:22Z

Yeah we’ve been sloppy. The important thing is to not use any APIs introduced in 2.3.2 or later, for now.

ZacSweers · 2019-04-05T02:08:45Z

Alright I've separated the error prone API version from the one used to build against, and lowered the API version to 2.3.1 in d01f759

Let me know what you want to do about the other comments!

lazaroclapp

Some nits/questions/musings, but, overall, happy to merge this! Thanks, Zac! @msridhar ?

lazaroclapp · 2019-04-05T14:50:50Z

gradle/dependencies.gradle

@@ -16,7 +16,8 @@

 def versions = [
    checkerFramework       : "2.5.5",
-    errorProne             : "2.3.2",
+    errorProne             : "2.3.3", // The version of error prone running on this project
+    errorProneApi          : "2.3.1", // The version of error prone built against and shipped


👍 I dream of a day they once again are both one and the same, but, for now, thanks for this :)

jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParams.java

lazaroclapp · 2019-04-05T15:07:56Z

nullaway/src/main/java/com/uber/nullaway/NullAway.java

@@ -919,7 +917,7 @@ private boolean fieldAlwaysInitializedBeforeRead(
      if (classTreePath == null) {
        throw new IllegalStateException(
            "could not find enclosing class / enum / interface for "
-                + enclosingBlockPath.getLeaf());
+                + state.getSourceForNode(enclosingBlockPath.getLeaf()));


Minor concern here: state. getSourceForNode performs file IO to retrieve the code, where JTree.toString() uses a visitor to reconstruct the string representation from the in-memory AST. This means toString() is faster. As long as we are only calling getSourceForNode for reporting actual errors (with imply IO anyways), it's probably fine to call this API, but we don't want to use it anywhere else, as it might cause a performance regression.

Related: getSourceForNode uses CompilationUnitTree.getSourceFile, which just reads JCCompilationUnit.sourcefile in JavaC. Do we know if that field is always set? Can javac operate on streams of java sources that are not files? (The JVM certainly can when loading bytecode, so... )

Either way, I think this is all fine for our use cases until we see an issue from this (either performance related or with getting the file) but well... any thoughts, @msridhar ?

This is fine I think. We should not be turning ASTs into Strings in any perf-critical computation.

Yep agreed. Double checked and this is only used in exception messaging

lazaroclapp · 2019-04-05T15:09:11Z

nullaway/src/main/java/com/uber/nullaway/NullAway.java

    }
+    ImmutableSet.Builder<Element> resultBuilder = ImmutableSet.builder();


nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java

lazaroclapp · 2019-04-05T15:29:44Z

nullaway/src/test/java/com/uber/nullaway/NullAwayAndroidTest.java

@@ -17,6 +17,7 @@

  private CompilationTestHelper compilationHelper;

+  @SuppressWarnings("CheckReturnValue")


The documentation for "CheckReturnValue" is unusually unhelpful. But from what I understand, this is complaining that we are not "using" the result from .newInstance(...), even though we are assigning it to a field, correct?

The weird thing is, this seems like perfectly idiomatic Error Prone test code, e.g.: https://github.com/google/error-prone/blob/master/test_helpers/src/test/java/com/google/errorprone/CompilationTestHelperTest.java#L45

Yeah I don't fully understand why we need to suppress here either

It's because setArgs is annoyingly marked as @CheckReturnValue, as they marked the entire package with that annotation

msridhar

Thanks for the great cleanup!

PR #300 introduces a code pattern that is linted against after #295.

lazaroclapp requested changes Apr 1, 2019

View reviewed changes

jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParams.java Show resolved Hide resolved

nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java Show resolved Hide resolved

Separate error prone API version from error prone build version

d01f759

lazaroclapp approved these changes Apr 5, 2019

View reviewed changes

msridhar approved these changes Apr 5, 2019

View reviewed changes

msridhar mentioned this pull request Apr 8, 2019

Update Gradle, Error Prone plugin, and Android Gradle Plugin #294

Merged

lazaroclapp merged commit 26b3f55 into uber:master Apr 8, 2019

lazaroclapp added a commit that referenced this pull request Apr 8, 2019

Fix merge conflict between PRs #300 and #295 (last two commits)

a358cb6

lazaroclapp added a commit that referenced this pull request Apr 8, 2019

Fix merge conflict between the two latest commits (#302)

244a587

PR #300 introduces a code pattern that is linted against after #295.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Error Prone 2.3.3 #295

Upgrade to Error Prone 2.3.3 #295

ZacSweers commented Apr 1, 2019

lazaroclapp left a comment

ZacSweers commented Apr 1, 2019

msridhar commented Apr 1, 2019

ZacSweers commented Apr 2, 2019

msridhar commented Apr 2, 2019

ZacSweers commented Apr 5, 2019

lazaroclapp left a comment

lazaroclapp Apr 5, 2019

lazaroclapp Apr 5, 2019

msridhar Apr 5, 2019

ZacSweers Apr 6, 2019

lazaroclapp Apr 5, 2019

lazaroclapp Apr 5, 2019

msridhar Apr 5, 2019

ZacSweers Apr 5, 2019

msridhar left a comment

		}
		ImmutableSet.Builder<Element> resultBuilder = ImmutableSet.builder();

		@@ -17,6 +17,7 @@

		private CompilationTestHelper compilationHelper;

		@SuppressWarnings("CheckReturnValue")

Upgrade to Error Prone 2.3.3 #295

Upgrade to Error Prone 2.3.3 #295

Conversation

ZacSweers commented Apr 1, 2019

lazaroclapp left a comment

Choose a reason for hiding this comment

ZacSweers commented Apr 1, 2019

msridhar commented Apr 1, 2019

ZacSweers commented Apr 2, 2019

msridhar commented Apr 2, 2019

ZacSweers commented Apr 5, 2019

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment