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

Don't include SUGGESTION checks in default checks #1380

Merged
merged 3 commits into from
Jun 2, 2020
Merged

Don't include SUGGESTION checks in default checks #1380

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ca404d
Don't include SUGGESTION checks in default checks
pkoenig10 Jun 2, 2020
72d40e2
Comments
pkoenig10 Jun 2, 2020
60f9efc
Add generated changelog entries
pkoenig10 Jun 2, 2020
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
3 changes: 2 additions & 1 deletion ...ine-error-prone/src/main/java/com/palantir/baseline/errorprone/LambdaMethodReference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.fixes.SuggestedFix;
Expand Down Expand Up @@ -53,7 +54,7 @@
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = BugPattern.LinkType.CUSTOM,
providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION,
severity = BugPattern.SeverityLevel.SUGGESTION,
severity = SeverityLevel.WARNING,
summary = "Lambda should be a method reference")
@SuppressWarnings("checkstyle:CyclomaticComplexity")
public final class LambdaMethodReference extends BugChecker implements BugChecker.LambdaExpressionTreeMatcher {
Expand Down
3 changes: 2 additions & 1 deletion ...r-prone/src/main/java/com/palantir/baseline/errorprone/PreferBuiltInConcurrentKeySet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.fixes.SuggestedFix;
Expand All @@ -34,7 +35,7 @@
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = BugPattern.LinkType.CUSTOM,
providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION,
severity = BugPattern.SeverityLevel.SUGGESTION,
severity = SeverityLevel.WARNING,
summary = "Prefer Java's built-in Concurrent Set implementation over Guava's ConcurrentHashSet, as it does "
+ "the same thing with less indirection and doesn't rely on Guava")
public final class PreferBuiltInConcurrentKeySet extends BugChecker implements BugChecker.MethodInvocationTreeMatcher {
Expand Down
3 changes: 2 additions & 1 deletion ...-error-prone/src/main/java/com/palantir/baseline/errorprone/RedundantMethodReference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.fixes.SuggestedFix;
Expand All @@ -35,7 +36,7 @@
name = "RedundantMethodReference",
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = BugPattern.LinkType.CUSTOM,
severity = BugPattern.SeverityLevel.SUGGESTION,
severity = SeverityLevel.WARNING,
summary = "Redundant method reference to the same type")
public final class RedundantMethodReference extends BugChecker implements BugChecker.MemberReferenceTreeMatcher {

Expand Down
3 changes: 2 additions & 1 deletion baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/RedundantModifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.fixes.SuggestedFixes;
Expand All @@ -41,7 +42,7 @@
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = BugPattern.LinkType.CUSTOM,
providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION,
severity = BugPattern.SeverityLevel.SUGGESTION,
severity = SeverityLevel.WARNING,
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like this should be an error

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want WARN here, iirc we use a flag to ignore error-prone warnings in generated code but it doesn't apply to errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we explicitly opt out of errorprone on generated source sets:

gradle-baseline/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java

Line 210 in a63a0ad

.set(String.format(

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we change this in a FLUP? I'd prefer not to accidentally break things in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for handling warn->error separately. I don't think our regex catches 100% of generated code and I'm not sure it's worth the risk breaking some folks in order to fail builds in repos that don't fail on errors

summary = "Avoid using redundant modifiers")
public final class RedundantModifier extends BugChecker
implements BugChecker.ClassTreeMatcher, BugChecker.MethodTreeMatcher, BugChecker.VariableTreeMatcher {
Expand Down
3 changes: 2 additions & 1 deletion baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOfEmpty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.fixes.SuggestedFixes;
Expand All @@ -33,7 +34,7 @@
name = "StreamOfEmpty",
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = BugPattern.LinkType.CUSTOM,
severity = BugPattern.SeverityLevel.SUGGESTION,
severity = SeverityLevel.WARNING,
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, definitely a bug

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the risk with making some of these errors is that repos which have a bunch of existing violation will be blocked. WARNING seems a bit safer because it's won't fail compilation unless you have -Werror.

Happy to change it if you would still prefer ERROR, just wanted to point this out.

Copy link
Contributor

Choose a reason for hiding this comment

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

in practice these rules will be auto applied on upgrade so I'm not particularly worried about blocking upgrades.

summary = "Stream.of() should be replaced with Stream.empty() to avoid unnecessary varargs allocation.")
public final class StreamOfEmpty extends BugChecker implements BugChecker.MethodInvocationTreeMatcher {

Expand Down
5 changes: 5 additions & 0 deletions ...ne-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@
import org.gradle.api.provider.ListProperty;

public class BaselineErrorProneExtension {

/*
* Do not add SUGGESTION checks here. Instead either increase the severity to WARNING or do not apply them by
* default.
*/
private static final ImmutableList<String> DEFAULT_PATCH_CHECKS = ImmutableList.of(
// Baseline checks
"BracesRequired",
Expand Down