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.

Sign up for GitHub

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

Upgrade Error Prone 2.17.0 -> 2.18.0 #455

Merged
merged 1 commit into from
Jan 11, 2023
Merged

Upgrade Error Prone 2.17.0 -> 2.18.0 #455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@
<version.auto-value>1.10.1</version.auto-value>
<version.error-prone>${version.error-prone-orig}</version.error-prone>
<version.error-prone-fork>v${version.error-prone-orig}-picnic-1</version.error-prone-fork>
<version.error-prone-orig>2.17.0</version.error-prone-orig>
<version.error-prone-orig>2.18.0</version.error-prone-orig>
<version.error-prone-slf4j>0.1.17</version.error-prone-slf4j>
<version.guava-beta-checker>1.0</version.guava-beta-checker>
<version.jdk>11</version.jdk>
Expand Down Expand Up @@ -1575,6 +1575,10 @@
<!-- XXX: Enable this once we open-source
this library. -->
-Xep:BetaApi:OFF
<!-- XXX: Enable once compatible with
`UnnecessarilyVisible`. See
https://github.com/google/error-prone/issues/3706. -->
-Xep:InjectOnBugCheckers:OFF
<!-- We don't target JDK 7. -->
-Xep:Java7ApiChecker:OFF
<!-- We don't target JDK 8. -->
Expand All @@ -1583,6 +1587,9 @@
-Xep:StaticOrDefaultInterfaceMethod:OFF
<!-- We generally discourage `var` use. -->
-Xep:Varifier:OFF
<!-- Yoda conditions are not always more
readable than the alternative. -->
-Xep:YodaCondition:OFF
Comment on lines +1590 to +1592
Copy link
Member Author

Choose a reason for hiding this comment

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

To be sure: we can discuss enabling this, of course. Maybe I should just get used to it. Even so: the resultant changes also require careful null analysis.

Copy link
Member

Choose a reason for hiding this comment

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

I'm on the fence about this. Enabling this would result in better/easier readable code IMO. What concerns do you have with enabling this 👀? (Other than getting used to it 😉)

In any case, it would make sense to enable this in a separate PR (especially if we want to make the next internal PSM release).

Copy link
Member Author

Choose a reason for hiding this comment

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

What concerns do you have with enabling this 👀 (Other than getting used to it 😉 )

Well the question is whether it's just "getting used to", or that in some cases readability actually suffers. Separately from that it might force us to introduce additional explicit null checks, which would be plain awkward.

Copy link
Member

Choose a reason for hiding this comment

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

We could open a PR where we enable the check and go over the results and make a decision for EPS there? It is hard to reason about it now if we don't know how "bad" it'll be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Draft PRs are free ;)

That said, I'd also like to see the arguments brought up during the internal discussion on this topic. 🧑‍🎓

-XepOpt:CheckReturnValue:CheckAllConstructors=true
<!-- XXX: Enable once there are fewer
false-positives.
Expand Down
1 change: 1 addition & 0 deletions refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ private static Optional<SeverityLevel> toSeverityLevel(Severity severity) {
* that could cause {@link VisitorState#reportMatch(Description)}} to override the reported
* severity).
*/
@SuppressWarnings("RestrictedApiChecker" /* We create a heavily customized `Description` here. */)
private static Description augmentDescription(
Description description, Optional<SeverityLevel> severityOverride) {
return Description.builder(
Expand Down
1 change: 1 addition & 0 deletions ...port/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public final void apply(TreePath path, Context context, DescriptionListener list
}
}

@SuppressWarnings("RestrictedApiChecker" /* We create a heavily customized `Description` here. */)
private Description augmentDescription(
Description description, CodeTransformer delegate, Context context) {
String shortCheckName = getShortCheckName(description.checkName);
Expand Down
1 change: 1 addition & 0 deletions .../src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ private static Description refasterDescription(String name) {
return description(name, Optional.of(""), WARNING, "");
}

@SuppressWarnings("RestrictedApiChecker" /* We create a heavily customized `Description` here. */)
private static Description description(
String name, Optional<String> link, SeverityLevel severityLevel, String message) {
return Description.builder(DUMMY_POSITION, name, link.orElse(null), severityLevel, message)
Expand Down
4 changes: 1 addition & 3 deletions ...rt/src/test/java/tech/picnic/errorprone/refaster/matchers/AbstractMatcherTestChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ public Description matchCompilationUnit(CompilationUnitTree compilationUnit, Vis
@Override
public @Nullable Void scan(Tree tree, @Nullable Void unused) {
if (tree instanceof ExpressionTree && delegate.matches((ExpressionTree) tree, state)) {
state.reportMatch(
Description.builder(tree, canonicalName(), null, defaultSeverity(), message())
.build());
state.reportMatch(describeMatch(tree));
}

return super.scan(tree, unused);
Expand Down