From 1ccfab4672e3260e0e1e3a7e019783944223e262 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 18 Dec 2023 17:17:59 -0800 Subject: [PATCH 1/7] Test to reproduce the issue. But it only reproduces on an earlier Error Prone version. So we need to hack dependencies.gradle, and also you need to compile with -PepApiVersion=2.18.0 or some old version to get it to repro. On the latest Error Prone version, 2.23.0, @SuppressWarnings("all") is better respected, so the NullAway error goes away. --- gradle/dependencies.gradle | 2 +- test-java-lib-lombok/build.gradle | 1 + .../src/main/java/com/uber/lombok/SimpleDataSub.java | 11 +++++++++++ .../main/java/com/uber/lombok/SimpleDataSuper.java | 9 +++++++++ 4 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 test-java-lib-lombok/src/main/java/com/uber/lombok/SimpleDataSub.java create mode 100644 test-java-lib-lombok/src/main/java/com/uber/lombok/SimpleDataSuper.java diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 26fc151bd8..7ddcbb2581 100755 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -67,7 +67,7 @@ def build = [ asm : "org.ow2.asm:asm:${versions.asm}", asmTree : "org.ow2.asm:asm-tree:${versions.asm}", errorProneCheckApi : "com.google.errorprone:error_prone_check_api:${versions.errorProneApi}", - errorProneCore : "com.google.errorprone:error_prone_core:${versions.errorProne}", + errorProneCore : "com.google.errorprone:error_prone_core:${versions.errorProneApi}", errorProneCoreForApi : "com.google.errorprone:error_prone_core:${versions.errorProneApi}", errorProneJavac : "com.google.errorprone:javac:9+181-r4173-1", errorProneTestHelpers : "com.google.errorprone:error_prone_test_helpers:${versions.errorProneApi}", diff --git a/test-java-lib-lombok/build.gradle b/test-java-lib-lombok/build.gradle index c70f54828d..ab4468401d 100644 --- a/test-java-lib-lombok/build.gradle +++ b/test-java-lib-lombok/build.gradle @@ -36,6 +36,7 @@ tasks.withType(JavaCompile) { check("NullAway", CheckSeverity.ERROR) option("NullAway:AnnotatedPackages", "com.uber") option("NullAway:UnannotatedSubPackages", "com.uber.lib.unannotated") + check("MissingBraces", CheckSeverity.OFF) } } // We need to fork on JDK 16+ since Lombok accesses internal compiler APIs diff --git a/test-java-lib-lombok/src/main/java/com/uber/lombok/SimpleDataSub.java b/test-java-lib-lombok/src/main/java/com/uber/lombok/SimpleDataSub.java new file mode 100644 index 0000000000..1fab1b8397 --- /dev/null +++ b/test-java-lib-lombok/src/main/java/com/uber/lombok/SimpleDataSub.java @@ -0,0 +1,11 @@ +package com.uber.lombok; + +import javax.annotation.Nullable; +import lombok.Data; +import lombok.EqualsAndHashCode; + +@Data +@EqualsAndHashCode(callSuper = false) +public class SimpleDataSub extends SimpleDataSuper { + @Nullable private String field2; +} diff --git a/test-java-lib-lombok/src/main/java/com/uber/lombok/SimpleDataSuper.java b/test-java-lib-lombok/src/main/java/com/uber/lombok/SimpleDataSuper.java new file mode 100644 index 0000000000..d3241c6f67 --- /dev/null +++ b/test-java-lib-lombok/src/main/java/com/uber/lombok/SimpleDataSuper.java @@ -0,0 +1,9 @@ +package com.uber.lombok; + +import javax.annotation.Nullable; +import lombok.Data; + +@Data +public class SimpleDataSuper { + @Nullable private String field1; +} From 5d50e8d87b8230d12e521eb19a20a3b65cfd33cb Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 18 Dec 2023 17:42:40 -0800 Subject: [PATCH 2/7] remove Gradle hack; now this failure should show up on CI --- build.gradle | 23 +++++++++++++++++++---- gradle/dependencies.gradle | 2 +- test-java-lib-lombok/build.gradle | 7 ++++++- 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/build.gradle b/build.gradle index 729ffc2539..a57c3b0ea4 100644 --- a/build.gradle +++ b/build.gradle @@ -42,10 +42,27 @@ repositories { apply from: "gradle/dependencies.gradle" +boolean isTestProject(Project p) { + def testProjs = [ + "test-java-lib", + "test-java-lib-lombok", + "test-library-models", + "sample", + "sample-app" + ] + return testProjs.contains(p.name) +} + subprojects { project -> project.apply plugin: "net.ltgt.errorprone" - project.dependencies { - errorprone deps.build.errorProneCore + if (isTestProject(project)) { + project.dependencies { + errorprone deps.build.errorProneCoreForApi + } + } else { + project.dependencies { + errorprone deps.build.errorProneCore + } } project.tasks.withType(JavaCompile) { dependsOn(installGitHooks) @@ -60,8 +77,6 @@ subprojects { project -> disableWarningsInGeneratedCode = true // this check is too noisy check("StringSplitter", CheckSeverity.OFF) - // https://github.com/google/error-prone/issues/3366 - check("CanIgnoreReturnValueSuggester", CheckSeverity.OFF) // turn up various checks check("WildcardImport", CheckSeverity.ERROR) check("MissingBraces", CheckSeverity.ERROR) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 7ddcbb2581..26fc151bd8 100755 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -67,7 +67,7 @@ def build = [ asm : "org.ow2.asm:asm:${versions.asm}", asmTree : "org.ow2.asm:asm-tree:${versions.asm}", errorProneCheckApi : "com.google.errorprone:error_prone_check_api:${versions.errorProneApi}", - errorProneCore : "com.google.errorprone:error_prone_core:${versions.errorProneApi}", + errorProneCore : "com.google.errorprone:error_prone_core:${versions.errorProne}", errorProneCoreForApi : "com.google.errorprone:error_prone_core:${versions.errorProneApi}", errorProneJavac : "com.google.errorprone:javac:9+181-r4173-1", errorProneTestHelpers : "com.google.errorprone:error_prone_test_helpers:${versions.errorProneApi}", diff --git a/test-java-lib-lombok/build.gradle b/test-java-lib-lombok/build.gradle index ab4468401d..4e939f753a 100644 --- a/test-java-lib-lombok/build.gradle +++ b/test-java-lib-lombok/build.gradle @@ -36,7 +36,12 @@ tasks.withType(JavaCompile) { check("NullAway", CheckSeverity.ERROR) option("NullAway:AnnotatedPackages", "com.uber") option("NullAway:UnannotatedSubPackages", "com.uber.lib.unannotated") - check("MissingBraces", CheckSeverity.OFF) + if (deps.versions.errorProneApi != deps.versions.errorProneLatest) { + check("MissingBraces", CheckSeverity.OFF) + check("SameNameButDifferent", CheckSeverity.OFF) + check("MissingOverride", CheckSeverity.OFF) + check("MissingSummary", CheckSeverity.OFF) + } } } // We need to fork on JDK 16+ since Lombok accesses internal compiler APIs From bad9d8a0f2b6eb1ef35936d5eaee08ef8a26434b Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 18 Dec 2023 18:18:38 -0800 Subject: [PATCH 3/7] Detect @SuppressWarnings("all") --- nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index e60b798a9d..1409e47f62 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -129,9 +129,9 @@ public Description createErrorDescription( checkName = INITIALIZATION_CHECK_NAME; } - // Mildly expensive state.getPath() traversal, occurs only once per potentially - // reported error. - if (hasPathSuppression(state.getPath(), checkName)) { + // Mildly expensive state.getPath() traversal, occurs twice per potentially reported error. + TreePath path = state.getPath(); + if (hasPathSuppression(path, checkName) || hasPathSuppression(path, "all")) { return Description.NO_MATCH; } From ddb321cd4d3aa335dd07abd7b0b22ede9e90eea6 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 18 Dec 2023 18:21:27 -0800 Subject: [PATCH 4/7] improve comment --- nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index 1409e47f62..25a08d7cb2 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -129,7 +129,8 @@ public Description createErrorDescription( checkName = INITIALIZATION_CHECK_NAME; } - // Mildly expensive state.getPath() traversal, occurs twice per potentially reported error. + // Mildly expensive state.getPath() traversal, occurs twice per potentially reported error. We + // can get rid of the check for "all" once we require Error Prone 2.22.0 or higher. TreePath path = state.getPath(); if (hasPathSuppression(path, checkName) || hasPathSuppression(path, "all")) { return Description.NO_MATCH; From d5ae980e5beadcc755c6cc7f87783dd7bf679043 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 18 Dec 2023 18:27:11 -0800 Subject: [PATCH 5/7] comments --- build.gradle | 3 +++ test-java-lib-lombok/build.gradle | 1 + 2 files changed, 4 insertions(+) diff --git a/build.gradle b/build.gradle index a57c3b0ea4..62d0981b5c 100644 --- a/build.gradle +++ b/build.gradle @@ -42,6 +42,7 @@ repositories { apply from: "gradle/dependencies.gradle" +// Returns true if the project's purpose is to test a NullAway build outside of unit tests or is a sample project boolean isTestProject(Project p) { def testProjs = [ "test-java-lib", @@ -56,6 +57,8 @@ boolean isTestProject(Project p) { subprojects { project -> project.apply plugin: "net.ltgt.errorprone" if (isTestProject(project)) { + // Compile the project using the same Error Prone version as the API dependence. This way, we test that we can + // still compile the project with old Error Prone versions. project.dependencies { errorprone deps.build.errorProneCoreForApi } diff --git a/test-java-lib-lombok/build.gradle b/test-java-lib-lombok/build.gradle index 4e939f753a..a590c7f42e 100644 --- a/test-java-lib-lombok/build.gradle +++ b/test-java-lib-lombok/build.gradle @@ -37,6 +37,7 @@ tasks.withType(JavaCompile) { option("NullAway:AnnotatedPackages", "com.uber") option("NullAway:UnannotatedSubPackages", "com.uber.lib.unannotated") if (deps.versions.errorProneApi != deps.versions.errorProneLatest) { + // These checks are incompatible with Lombok in older Error Prone versions, so disable them check("MissingBraces", CheckSeverity.OFF) check("SameNameButDifferent", CheckSeverity.OFF) check("MissingOverride", CheckSeverity.OFF) From e3d53bad1429e4439429d611471ce31c85dfd9b1 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 18 Dec 2023 18:44:46 -0800 Subject: [PATCH 6/7] undo change --- build.gradle | 2 ++ 1 file changed, 2 insertions(+) diff --git a/build.gradle b/build.gradle index 62d0981b5c..f73c6a5d52 100644 --- a/build.gradle +++ b/build.gradle @@ -80,6 +80,8 @@ subprojects { project -> disableWarningsInGeneratedCode = true // this check is too noisy check("StringSplitter", CheckSeverity.OFF) + // https://github.com/google/error-prone/issues/3366 + check("CanIgnoreReturnValueSuggester", CheckSeverity.OFF) // turn up various checks check("WildcardImport", CheckSeverity.ERROR) check("MissingBraces", CheckSeverity.ERROR) From 7091a253d7901a737a1442d2c07f927888ab0a4e Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 18 Dec 2023 18:51:38 -0800 Subject: [PATCH 7/7] Revert "undo change" This reverts commit e3d53bad1429e4439429d611471ce31c85dfd9b1. --- build.gradle | 2 -- 1 file changed, 2 deletions(-) diff --git a/build.gradle b/build.gradle index f73c6a5d52..62d0981b5c 100644 --- a/build.gradle +++ b/build.gradle @@ -80,8 +80,6 @@ subprojects { project -> disableWarningsInGeneratedCode = true // this check is too noisy check("StringSplitter", CheckSeverity.OFF) - // https://github.com/google/error-prone/issues/3366 - check("CanIgnoreReturnValueSuggester", CheckSeverity.OFF) // turn up various checks check("WildcardImport", CheckSeverity.ERROR) check("MissingBraces", CheckSeverity.ERROR)