From a0f9d09687a948a69a926f840cf4da772b1c3188 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 18 Dec 2019 13:51:52 -0500 Subject: [PATCH] Implement BracesRequired error-prone check with suggested fixes (#1130) Implement BracesRequired error-prone check with suggested fixes ```diff - if (condition) statement; + if (condition) { + statement; + } ``` --- README.md | 1 + .../baseline/errorprone/BracesRequired.java | 94 +++++++ .../errorprone/BracesRequiredTest.java | 247 ++++++++++++++++++ changelog/@unreleased/pr-1130.v2.yml | 12 + .../BaselineErrorProneExtension.java | 1 + 5 files changed, 355 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/BracesRequired.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/BracesRequiredTest.java create mode 100644 changelog/@unreleased/pr-1130.v2.yml diff --git a/README.md b/README.md index f9287798c..c8f53160e 100644 --- a/README.md +++ b/README.md @@ -184,6 +184,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `ExceptionSpecificity`: Prefer more specific catch types than Exception and Throwable. - `ThrowSpecificity`: Prefer to declare more specific `throws` types than Exception and Throwable. - `UnsafeGaugeRegistration`: Use TaggedMetricRegistry.registerWithReplacement over TaggedMetricRegistry.gauge. +- `BracesRequired`: Require braces for loops and if expressions. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/BracesRequired.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/BracesRequired.java new file mode 100644 index 000000000..e87885958 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/BracesRequired.java @@ -0,0 +1,94 @@ +/* + * (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.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.DoWhileLoopTree; +import com.sun.source.tree.EnhancedForLoopTree; +import com.sun.source.tree.ForLoopTree; +import com.sun.source.tree.IfTree; +import com.sun.source.tree.StatementTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.WhileLoopTree; +import javax.annotation.Nullable; + +@AutoService(BugChecker.class) +@BugPattern( + name = "BracesRequired", + 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, + summary = "Braces are required for readability") +public final class BracesRequired extends BugChecker implements + BugChecker.DoWhileLoopTreeMatcher, + BugChecker.ForLoopTreeMatcher, + BugChecker.EnhancedForLoopTreeMatcher, + BugChecker.IfTreeMatcher, + BugChecker.WhileLoopTreeMatcher { + + @Override + public Description matchIf(IfTree tree, VisitorState state) { + check(tree.getThenStatement(), state); + StatementTree elseStatement = tree.getElseStatement(); + if (elseStatement != null + // Additional check for else if + && elseStatement.getKind() != Tree.Kind.IF) { + check(elseStatement, state); + } + return Description.NO_MATCH; + } + + @Override + public Description matchWhileLoop(WhileLoopTree tree, VisitorState state) { + check(tree.getStatement(), state); + return Description.NO_MATCH; + } + + @Override + public Description matchDoWhileLoop(DoWhileLoopTree tree, VisitorState state) { + check(tree.getStatement(), state); + return Description.NO_MATCH; + } + + @Override + public Description matchEnhancedForLoop(EnhancedForLoopTree tree, VisitorState state) { + check(tree.getStatement(), state); + return Description.NO_MATCH; + } + + @Override + public Description matchForLoop(ForLoopTree tree, VisitorState state) { + check(tree.getStatement(), state); + return Description.NO_MATCH; + } + + private void check(@Nullable StatementTree tree, VisitorState state) { + if (tree != null + && tree.getKind() != Tree.Kind.BLOCK + && tree.getKind() != Tree.Kind.EMPTY_STATEMENT) { + state.reportMatch(buildDescription(tree) + .addFix(SuggestedFix.replace(tree, "{" + state.getSourceForNode(tree) + "}")) + .build()); + } + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/BracesRequiredTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/BracesRequiredTest.java new file mode 100644 index 000000000..9391d5497 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/BracesRequiredTest.java @@ -0,0 +1,247 @@ +/* + * (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; + +public class BracesRequiredTest { + + @Test + void testFix_if_then() { + fix() + .addInputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " if (param) System.out.println();", + " }", + "}") + .addOutputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " if (param) {", + " System.out.println();", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_if_else() { + fix() + .addInputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " if (param) {", + " System.out.println(\"if\");", + " } else", + " System.out.println(\"else\");", + " }", + "}") + .addOutputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " if (param) {", + " System.out.println(\"if\");", + " } else {", + " System.out.println(\"else\");", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_if_both() { + fix() + .addInputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " if (param)", + " System.out.println(\"if\");", + " else", + " System.out.println(\"else\");", + " }", + "}") + .addOutputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " if (param) {", + " System.out.println(\"if\");", + " } else {", + " System.out.println(\"else\");", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_if_emptyThen() { + fix() + .addInputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " if (param); else", + " System.out.println(\"else\");", + " }", + "}") + .addOutputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " if (param); else {", + " System.out.println(\"else\");", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_while() { + fix() + .addInputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " while (param) System.out.println();", + " }", + "}") + .addOutputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " while (param) {", + " System.out.println();", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_doWhile() { + fix() + .addInputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " do System.out.println(); while (param);", + " }", + "}") + .addOutputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " do {", + " System.out.println();", + " } while (param);", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_for() { + fix() + .addInputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " for (int i = 0; i < 5; i++) System.out.println();", + " }", + "}") + .addOutputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " for (int i = 0; i < 5; i++) {", + " System.out.println();", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_enhancedFor() { + fix() + .addInputLines("Test.java", + "import java.util.List;", + "class Test {", + " void f(List list) {", + " for (String item : list) System.out.println(item);", + " }", + "}") + .addOutputLines("Test.java", + "import java.util.List;", + "class Test {", + " void f(List list) {", + " for (String item : list) {", + " System.out.println(item);", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_nested() { + fix() + .addInputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " if (param) if (param) {", + " System.out.println();", + " }", + " }", + "}") + .addOutputLines("Test.java", + "class Test {", + " void f(boolean param) {", + " if (param) {", + " if (param) {", + " System.out.println();", + " }", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_elseIf() { + fix() + .addInputLines("Test.java", + "class Test {", + " void f(boolean p0, boolean p1) {", + " if (p0) System.out.println();", + " else if (p1) System.out.println();", + " }", + "}") + .addOutputLines("Test.java", + "class Test {", + " void f(boolean p0, boolean p1) {", + " if (p0) {", + " System.out.println();", + " } else if (p1) {", + " System.out.println();", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + private RefactoringValidator fix() { + return RefactoringValidator.of(new BracesRequired(), getClass()); + } +} diff --git a/changelog/@unreleased/pr-1130.v2.yml b/changelog/@unreleased/pr-1130.v2.yml new file mode 100644 index 000000000..4df174623 --- /dev/null +++ b/changelog/@unreleased/pr-1130.v2.yml @@ -0,0 +1,12 @@ +type: improvement +improvement: + description: |- + Implement BracesRequired error-prone check with suggested fixes + ```diff + - if (condition) statement; + + if (condition) { + + statement; + + } + ``` + links: + - https://github.com/palantir/gradle-baseline/pull/1130 diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index ea90e1bcf..332435d78 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -23,6 +23,7 @@ public class BaselineErrorProneExtension { private static final ImmutableList DEFAULT_PATCH_CHECKS = ImmutableList.of( // Baseline checks + "BracesRequired", "CatchBlockLogException", // TODO(ckozak): re-enable pending scala check //"CatchSpecificity",