Fix another bug with Lombok equals() parameter annotations #880
Conversation
…or 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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #880 +/- ##
============================================
- Coverage 87.09% 87.07% -0.02%
Complexity 1990 1990
============================================
Files 77 77
Lines 6430 6431 +1
Branches 1245 1245
============================================
Hits 5600 5600
Misses 422 422
- Partials 408 409 +1 ☔ View full report in Codecov by Sentry. |
msridhar
changed the title
This reverts commit e3d53ba.
| @@ -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 | |||
There was a problem hiding this comment.
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
| // 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. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
|
FYI I am going to hold off on landing this, as I realized it doesn't completely fix the problems we were seeing. Once we figure out a plan for moving forward, I will land this PR (assuming it's part of that plan). |
#886) …arameter (#874)" This reverts commit 5fbee1f. It turns out that this change requires a couple of other changes along with it, including #880 and better overall checking of overriding of `equals()` methods. We want to get a release out soon, so temporarily revert this change; we will restore it after cutting the release.
We were getting override errors in relation to the parameter of some Lombok-generated
equals()method due to the fix in #874. This error is not reported with the latest version of Error Prone since it properly handles@SuppressWarnings("all"), which is generated by Lombok (see google/error-prone#4076). For compatibility with older Error Prone versions we add our own check for@SuppressWarnings("all").Getting a test case here required some work since this issue can only be reproduced on older Error Prone versions. We re-jigger some test projects to build against whatever Error Prone version is specified with
epApiVersion, so that we catch issues like these in the future.