Fix compatibility with error-prone 2.38.0 #3101
Conversation
Generate changelog in
|
...rone/src/main/java/com/palantir/baseline/errorprone/ExecutorSubmitRunnableFutureIgnored.java
Show resolved
Hide resolved
| return switch (tree.getKind()) { | ||
| case IDENTIFIER -> ((IdentifierTree) tree).getName().contentEquals("this"); | ||
| case MEMBER_SELECT -> ((MemberSelectTree) tree).getIdentifier().contentEquals("this"); | ||
| default -> false; | ||
| }; |
There was a problem hiding this comment.
Can we please save unrelated refactors for a separate PR? It makes it hard to review this PR.
There was a problem hiding this comment.
This was required by error prone checks on the repo. Would you prefer I suppress in this PR?
There was a problem hiding this comment.
Oh this is a new one introduced in Error Prone.
https://errorprone.info/bugpattern/StatementSwitchToExpressionSwitch
There was a problem hiding this comment.
We should work out if this (and any other introduced check) is something we want to disable, or are comfortable will a load of automated suppressions for them. Perhaps it has autofixes?
There was a problem hiding this comment.
Perhaps it has autofixes?
This check only reports a match if it has an auto-fix. So I think it should be fine to roll this out.
There was a problem hiding this comment.
StringConcatToTextBlock also feels a bit 🌶️, but there are automated fixes for that too. And given that text blocks seem objectively better than string concatenation, I'm inclined to leave that one also.
jmongeluzzi
changed the title
| '''.stripIndent() | ||
| } | ||
| // As of https://github.com/google/error-prone/pull/4699, error prone no longer applies patches to disabled checks. | ||
| // Disabling this test since it is no longer valid and will require significant changes to restore the prior behavior. |
There was a problem hiding this comment.
Can you elaborate on this? Why do we need to disable this test?
There was a problem hiding this comment.
Quoting from google/error-prone#4699 (comment)
❗ That said, my changes impact semantics ❗. Before the changes in this PR, Error Prone would ignore the -Xep:AssignmentUpdater:OFF flag and apply any suggestions by AssignmentUpdater. After my changes, AssignmentUpdater is ignored altogether.
Disabling a check results in it being set to OFF, so suggestions do not get applied.
There was a problem hiding this comment.
This feels like something we need to address before merging this PR. A lot of our internal automation is built on Error Prone and applying suggested fixes.
|
Closing this as the scope of what is needed to merge this has expanded to include changes to other gradle plugins (see |
3 participants
There were several breaking changes between error-prone 2.31.0 and 2.38.0. This change should fix compatibility without affecting functionality.
Because this repo applies the gradle-baseline plugin to itself, error-prone fails on this change because of the same issue it is intended to fix. To work around this, I have disabled the affected error-prone checks temporarily and will re-enable them in a subsequent PR that bumps gradle-baseline to the latest version.
Also, a change in error-prone (google/error-prone#4699) means that patches are no longer applied to disabled checks, so I have commented out the test for this behavior. Restoring this behavior could be possible in the future via argument manipulation in https://github.com/palantir/suppressible-error-prone, but that is beyond the scope of this change.
==COMMIT_MSG==
Fix compatibility with error-prone 2.38.0.
==COMMIT_MSG==