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

Error prone validation to avoid redundant modifiers #1010

Merged
merged 5 commits into from
Nov 1, 2019
+305 −4
Merged

Error prone validation to avoid redundant modifiers #1010

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ab9b2b2
Error prone validation to avoid redundant modifiers
carterkozak Nov 1, 2019
320468d
Add generated changelog entries
carterkozak Nov 1, 2019
c2303f7
Upgrade RedundantModifier to ERROR, remove redundant checkstyle regex…
carterkozak Nov 1, 2019
f4a4d4e
future work
carterkozak Nov 1, 2019
2c9d3b6
Merge branch 'ckozak/RedundantModifier' of github.com:palantir/gradle…
carterkozak Nov 1, 2019
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
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c
- `ReverseDnsLookup`: Calling address.getHostName may result in an unexpected DNS lookup.
- `ReadReturnValueIgnored`: The result of a read call must be checked to know if EOF has been reached or the expected number of bytes have been consumed.
- `FinalClass`: A class should be declared final if all of its constructors are private.
- `RedundantModifier`: Avoid using redundant modifiers.

### Programmatic Application

Expand Down
113 changes: 113 additions & 0 deletions 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
@@ -0,0 +1,113 @@
/*
* (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.baseline.errorprone;

import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.Tree;
import java.util.Locale;
import javax.lang.model.element.Modifier;

@AutoService(BugChecker.class)
@BugPattern(
name = "RedundantModifier",
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = BugPattern.LinkType.CUSTOM,
providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION,
severity = BugPattern.SeverityLevel.WARNING,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we upgrade this to ERROR and remove the checkstyle regex?

        <module name="RegexpSinglelineJava">
            <property name="format" value="static enum"/>
            <property name="message" value="Redundant ''static'' modifier."/>
        </module>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't realize we also have a checkstyle rule with the same name in addition to this regex check. I'll add support for the same suppressions, and bump to error.

summary = "Avoid using redundant modifiers")
public final class RedundantModifier extends BugChecker
implements BugChecker.ClassTreeMatcher, BugChecker.MethodTreeMatcher {

private static final Matcher<ClassTree> STATIC_ENUM_OR_INTERFACE = Matchers.allOf(
Matchers.anyOf(Matchers.kindIs(Tree.Kind.ENUM), Matchers.kindIs(Tree.Kind.INTERFACE)),
Matchers.hasModifier(Modifier.STATIC));

private static final Matcher<MethodTree> PRIVATE_ENUM_CONSTRUCTOR = Matchers.allOf(
Matchers.methodIsConstructor(),
Matchers.enclosingClass(Matchers.kindIs(Tree.Kind.ENUM)),
Matchers.hasModifier(Modifier.PRIVATE));

private static final Matcher<MethodTree> STATIC_FINAL_METHOD = Matchers.allOf(
Matchers.isStatic(),
Matchers.hasModifier(Modifier.FINAL));

private static final Matcher<MethodTree> UNNECESSARY_INTERFACE_METHOD_MODIFIERS = Matchers.allOf(
Matchers.enclosingClass(Matchers.kindIs(Tree.Kind.INTERFACE)),
Matchers.not(Matchers.isStatic()),
Matchers.not(Matchers.hasModifier(Modifier.DEFAULT)),
Matchers.anyOf(Matchers.hasModifier(Modifier.PUBLIC), Matchers.hasModifier(Modifier.ABSTRACT)));

private static final Matcher<MethodTree> UNNECESSARY_FINAL_METHOD_ON_FINAL_CLASS = Matchers.allOf(
Matchers.not(Matchers.isStatic()),
Matchers.enclosingClass(Matchers.allOf(
Matchers.kindIs(Tree.Kind.CLASS),
Matchers.hasModifier(Modifier.FINAL))),
Matchers.allOf(
Matchers.hasModifier(Modifier.FINAL),
Matchers.not(Matchers.hasAnnotation(SafeVarargs.class))));

@Override
public Description matchClass(ClassTree tree, VisitorState state) {
if (STATIC_ENUM_OR_INTERFACE.matches(tree, state)) {
return buildDescription(tree)
.setMessage(tree.getKind().name().toLowerCase(Locale.ENGLISH)
+ "s are static by default. The 'static' modifier is unnecessary.")
.addFix(SuggestedFixes.removeModifiers(tree, state, Modifier.STATIC))
.build();
}
return Description.NO_MATCH;
}

@Override
public Description matchMethod(MethodTree tree, VisitorState state) {
if (PRIVATE_ENUM_CONSTRUCTOR.matches(tree, state)) {
return buildDescription(tree)
.setMessage("Enum constructors are private by default. The 'private' modifier is unnecessary.")
.addFix(SuggestedFixes.removeModifiers(tree, state, Modifier.PRIVATE))
.build();
}
if (STATIC_FINAL_METHOD.matches(tree, state)) {
return buildDescription(tree)
.setMessage("Static methods cannot be overridden. The 'final' modifier is unnecessary.")
.addFix(SuggestedFixes.removeModifiers(tree, state, Modifier.FINAL))
.build();
}
if (UNNECESSARY_INTERFACE_METHOD_MODIFIERS.matches(tree, state)) {
return buildDescription(tree)
.setMessage("Interface methods are public and abstract by default. "
+ "The 'public' and 'abstract' modifiers are unnecessary.")
.addFix(SuggestedFixes.removeModifiers(tree, state, Modifier.PUBLIC, Modifier.ABSTRACT))
.build();
}
if (UNNECESSARY_FINAL_METHOD_ON_FINAL_CLASS.matches(tree, state)) {
return buildDescription(tree)
.setMessage("Redundant 'final' modifier on an instance method of a final class.")
.addFix(SuggestedFixes.removeModifiers(tree, state, Modifier.FINAL))
.build();
}
return Description.NO_MATCH;
}
}
180 changes: 180 additions & 0 deletions ...ine-error-prone/src/test/java/com/palantir/baseline/errorprone/RedundantModifierTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
/*
* (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.baseline.errorprone;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.parallel.Execution;
import org.junit.jupiter.api.parallel.ExecutionMode;

@Execution(ExecutionMode.CONCURRENT)
class RedundantModifierTest {

@Test
void fixEnumConstructor() {
fix()
.addInputLines(
"Test.java",
"public enum Test {",
" INSTANCE(\"str\");",
" private final String str;",
" private Test(String str) {",
" this.str = str;",
" }",
"}"
)
.addOutputLines(
"Test.java",
"public enum Test {",
" INSTANCE(\"str\");",
" private final String str;",
" Test(String str) {",
" this.str = str;",
" }",
"}"
).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}

@Test
@SuppressWarnings("checkstyle:RegexpSinglelineJava")
void fixStaticEnum() {
fix()
.addInputLines(
"Enclosing.java",
"public class Enclosing {",
" public static enum Test {",
" INSTANCE",
" }",
"}"
)
.addOutputLines(
"Enclosing.java",
"public class Enclosing {",
" public enum Test {",
" INSTANCE",
" }",
"}"
).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}

@Test
void fixStaticInterface() {
fix()
.addInputLines(
"Enclosing.java",
"public class Enclosing {",
" public static interface Test {",
" }",
"}"
)
.addOutputLines(
"Enclosing.java",
"public class Enclosing {",
" public interface Test {",
" }",
"}"
).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}

@Test
void fixInterfaceMethods() {
fix()
.addInputLines(
"Enclosing.java",
"public class Enclosing {",
" public interface Test {",
" public int a();",
" int b();",
" abstract int c();",
" public abstract int d();",
" }",
"}"
)
.addOutputLines(
"Enclosing.java",
"public class Enclosing {",
" public interface Test {",
" int a();",
" int b();",
" int c();",
" int d();",
" }",
"}"
).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}

@Test
void fixFinalClassModifiers() {
fix()
.addInputLines(
"Test.java",
"public final class Test {",
" public final void a() {}",
" private final void b() {}",
" final void c() {}",
" @SafeVarargs public final void d(Object... value) {}",
"}"
)
.addOutputLines(
"Test.java",
"public final class Test {",
" public void a() {}",
" private void b() {}",
" void c() {}",
// SafeVarargs is a special case
" @SafeVarargs public final void d(Object... value) {}",
"}"
).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}

@Test
void fixStaticFinalMethod() {
fix()
.addInputLines(
"Test.java",
"public class Test {",
" public static final int a() {",
" return 1;",
" }",
" private static final int b() {",
" return 1;",
" }",
" static final int c() {",
" return 1;",
" }",
"}"
)
.addOutputLines(
"Test.java",
"public class Test {",
" public static int a() {",
" return 1;",
" }",
" private static int b() {",
" return 1;",
" }",
" static int c() {",
" return 1;",
" }",
"}"
).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}

private BugCheckerRefactoringTestHelper fix() {
return BugCheckerRefactoringTestHelper.newInstance(new RedundantModifier(), getClass());
}
}
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-1010.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Error prone validation to avoid redundant modifiers
links:
- https://github.com/palantir/gradle-baseline/pull/1010
1 change: 1 addition & 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 @@ -34,6 +34,7 @@ public class BaselineErrorProneExtension {
"PreferSafeLoggableExceptions",
"PreferSafeLoggingPreconditions",
"ReadReturnValueIgnored",
"RedundantModifier",
"Slf4jLevelCheck",
"Slf4jLogsafeArgs",
"StrictUnusedVariable",
Expand Down