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

Fix another bug with Lombok equals() parameter annotations #880

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
26 changes: 22 additions & 4 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,30 @@ 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",
"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)) {
// 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
}
} else {
project.dependencies {
errorprone deps.build.errorProneCore
}
}
project.tasks.withType(JavaCompile) {
dependsOn(installGitHooks)
Expand All @@ -60,8 +80,6 @@ subprojects { project ->
disableWarningsInGeneratedCode = true
// this check is too noisy
check("StringSplitter", CheckSeverity.OFF)
// https://github.com/google/error-prone/issues/3366
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not required with the latest Error Prone version (the issue was fixed), and the check did not exist in the oldest Error Prone version we support, so just delete

check("CanIgnoreReturnValueSuggester", CheckSeverity.OFF)
// turn up various checks
check("WildcardImport", CheckSeverity.ERROR)
check("MissingBraces", CheckSeverity.ERROR)
Expand Down
7 changes: 4 additions & 3 deletions nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,10 @@ 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. We
// can get rid of the check for "all" once we require Error Prone 2.22.0 or higher.
Comment on lines +132 to +133
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I was talking about on the EP support policy, we probably want to remove this once we drop support for older EP versions (and we can have an issue to track it due to performance implications)

Copy link
Collaborator Author

@msridhar msridhar Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good question. Right now, we are stuck supporting a very old EP version (2.10.0) since we still support running NullAway on JDK 8, and that was the last EP version that supported running on JDK 8. At this point, I think we should make a plan for dropping JDK 8 support in the near-ish future, and then we could bump our minimum EP version to something much more recent. As to what policy we should adhere to after that, I'm thinking something like we will typically support an EP version for X months after it was released. For reference, 2.10.0 was released on Nov. 4, 2021, so we are just past two years since it came out. Maybe we could generally aim for supporting an EP release for 12-18 months? Not sure of the right number, and we don't need a hard-and-fast rule. But I agree we should have something in mind.

I've added a comment on #634 regarding dropping JDK 8 support. And I've opened #882 regarding support for older Error Prone versions.

TreePath path = state.getPath();
if (hasPathSuppression(path, checkName) || hasPathSuppression(path, "all")) {
return Description.NO_MATCH;
}

Expand Down
7 changes: 7 additions & 0 deletions test-java-lib-lombok/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ tasks.withType(JavaCompile) {
check("NullAway", CheckSeverity.ERROR)
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)
check("MissingSummary", CheckSeverity.OFF)
}
}
}
// We need to fork on JDK 16+ since Lombok accesses internal compiler APIs
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package com.uber.lombok;

import javax.annotation.Nullable;
import lombok.Data;

@Data
public class SimpleDataSuper {
@Nullable private String field1;
}