From 9adca4c0698127c4588dabfe9e56f0db6e031ff9 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Tue, 29 Oct 2024 11:03:58 -0700 Subject: [PATCH] Discourage conditional expressions and if statements where both branches are the same PiperOrigin-RevId: 691086580 --- .../bugpatterns/DuplicateBranches.java | 100 ++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../bugpatterns/DuplicateBranchesTest.java | 218 ++++++++++++++++++ docs/bugpattern/DuplicateBranches.md | 32 +++ 4 files changed, 352 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/DuplicateBranches.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/DuplicateBranchesTest.java create mode 100644 docs/bugpattern/DuplicateBranches.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DuplicateBranches.java b/core/src/main/java/com/google/errorprone/bugpatterns/DuplicateBranches.java new file mode 100644 index 00000000000..3936c3fc9ad --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/DuplicateBranches.java @@ -0,0 +1,100 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * 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.google.errorprone.bugpatterns; + +import static com.google.common.collect.Iterables.getLast; +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.getStartPosition; +import static java.util.stream.Collectors.joining; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.ConditionalExpressionTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.IfTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ErrorProneTokens; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.ConditionalExpressionTree; +import com.sun.source.tree.IfTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.Tree.Kind; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern(summary = "Both branches contain identical code", severity = ERROR) +public class DuplicateBranches extends BugChecker + implements IfTreeMatcher, ConditionalExpressionTreeMatcher { + @Override + public Description matchConditionalExpression( + ConditionalExpressionTree tree, VisitorState state) { + return match(tree, tree.getTrueExpression(), tree.getFalseExpression(), state); + } + + @Override + public Description matchIf(IfTree tree, VisitorState state) { + if (tree.getElseStatement() == null) { + return NO_MATCH; + } + return match(tree, tree.getThenStatement(), tree.getElseStatement(), state); + } + + // The comparison relies on Tree#toString, which isn't free for very long trees. Only check + // relatively short trees. + private static final int MAX_LENGTH_TO_COMPARE = 750; + + @SuppressWarnings("TreeToString") + private Description match(Tree tree, Tree thenTree, Tree elseTree, VisitorState state) { + if (state.getSourceForNode(thenTree).length() > MAX_LENGTH_TO_COMPARE + || state.getSourceForNode(elseTree).length() > MAX_LENGTH_TO_COMPARE) { + return NO_MATCH; + } + // This could do something similar to com.sun.tools.javac.comp.TreeDiffer. That doesn't + // do exactly what we want here, which is to compare the syntax including of identifiers and + // not their underlying symbols, and it would require a lot of case work to implement for all + // AST nodes. + if (!thenTree.toString().equals(elseTree.toString())) { + return NO_MATCH; + } + int start = getStartPosition(elseTree); + int end = state.getEndPosition(elseTree); + boolean needsBraces = false; + if (elseTree instanceof BlockTree) { + needsBraces = !state.getPath().getParentPath().getLeaf().getKind().equals(Kind.BLOCK); + var statements = ((BlockTree) elseTree).getStatements(); + start = getStartPosition(statements.get(0)); + end = state.getEndPosition(getLast(statements)); + } + String comments = + ErrorProneTokens.getTokens( + state.getSourceCode().subSequence(getStartPosition(tree), start).toString(), + getStartPosition(tree), + state.context) + .stream() + .flatMap(errorProneToken -> errorProneToken.comments().stream()) + .map(c -> c.getText()) + .collect(joining("\n")); + if (!comments.isEmpty()) { + comments += "\n"; + } + String replacement = comments + state.getSourceCode().subSequence(start, end); + if (needsBraces) { + replacement = "{\n" + replacement + "}"; + } + return describeMatch(tree, SuggestedFix.replace(tree, replacement)); + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index f9b7bed5964..49c549b9d59 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -121,6 +121,7 @@ import com.google.errorprone.bugpatterns.DoNotMockAutoValue; import com.google.errorprone.bugpatterns.DoNotMockChecker; import com.google.errorprone.bugpatterns.DoubleBraceInitialization; +import com.google.errorprone.bugpatterns.DuplicateBranches; import com.google.errorprone.bugpatterns.DuplicateDateFormatField; import com.google.errorprone.bugpatterns.DuplicateMapKeys; import com.google.errorprone.bugpatterns.EmptyCatch; @@ -706,6 +707,7 @@ public static ScannerSupplier warningChecks() { DoNotCallChecker.class, DoNotMockChecker.class, DoubleBraceInitialization.class, + DuplicateBranches.class, DuplicateMapKeys.class, DurationFrom.class, DurationGetTemporalUnit.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/DuplicateBranchesTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/DuplicateBranchesTest.java new file mode 100644 index 00000000000..bf9c8eb87bb --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/DuplicateBranchesTest.java @@ -0,0 +1,218 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * 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.google.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class DuplicateBranchesTest { + + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(DuplicateBranches.class, getClass()); + + @Test + public void positive() { + compilationHelper + .addSourceLines( + "Test.java", + """ + class Test { + String f(boolean a, String b, String c) { + // BUG: Diagnostic contains: + return a ? b : b; + } + + String g(boolean a, String b, String c) { + // BUG: Diagnostic contains: + if (a) { + return b; + } else { + return b; + } + } + } + """) + .doTest(); + } + + @Test + public void negative() { + compilationHelper + .addSourceLines( + "Test.java", + """ + class Test { + String f(boolean a, String b, String c) { + return a ? b : c; + } + + String g(boolean a, String b, String c) { + if (a) { + return b; + } else { + return c; + } + } + + String h(boolean a, String b, String c) { + if (a) { + return b; + } + return ""; + } + } + """) + .doTest(); + } + + @Test + public void statementRefactoring() { + BugCheckerRefactoringTestHelper.newInstance(DuplicateBranches.class, getClass()) + .addInputLines( + "Test.java", + """ + class Test { + String g(boolean a, String b, String c) { + if (a) { + return b; + } else { + return b; + } + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + String g(boolean a, String b, String c) { + return b; + } + } + """) + .doTest(TestMode.TEXT_MATCH); + } + + @Test + public void statementRefactoringChain() { + BugCheckerRefactoringTestHelper.newInstance(DuplicateBranches.class, getClass()) + .addInputLines( + "Test.java", + """ + class Test { + String g(boolean a, String b, String c) { + if (a) { + return c; + } else if (a) { + return b; + } else { + return b; + } + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + String g(boolean a, String b, String c) { + if (a) { + return c; + } else { + return b; + } + } + } + """) + .doTest(TestMode.TEXT_MATCH); + } + + @Test + public void commentRefactoring() { + BugCheckerRefactoringTestHelper.newInstance(DuplicateBranches.class, getClass()) + .addInputLines( + "Test.java", + """ + class Test { + String g(boolean a, String b, String c) { + if (a) { + // foo + return b; + } else { + // bar + return b; + } + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + String g(boolean a, String b, String c) { + // foo + // bar + return b; + } + } + """) + .doTest(TestMode.TEXT_MATCH); + } + + @Test + public void commentRefactoringIfElse() { + BugCheckerRefactoringTestHelper.newInstance(DuplicateBranches.class, getClass()) + .addInputLines( + "Test.java", + """ + class Test { + boolean g(boolean a, boolean b) { + if (a) { + return true; + } else if (a) { + // foo + return b; + } else { + // bar + return b; + } + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + boolean g(boolean a, boolean b) { + if (a) { + return true; + } else { + // foo + // bar + return b; + } + } + } + """) + .doTest(TestMode.TEXT_MATCH); + } +} diff --git a/docs/bugpattern/DuplicateBranches.md b/docs/bugpattern/DuplicateBranches.md new file mode 100644 index 00000000000..19ace08b1ae --- /dev/null +++ b/docs/bugpattern/DuplicateBranches.md @@ -0,0 +1,32 @@ +Branching constructs (`if` statements, `conditional` expressions) should contain +difference code in the two branches. Repeating identical code in both branches +is usually a bug. + +For example: + +```java +condition ? same : same +``` + +```java +if (condition) { + same(); +} else { + same(); +} +``` + +this usually indicates a typo where one of the branches was supposed to contain +different logic: + +```java +condition ? something : somethingElse +``` + +```java +if (condition) { + doSomething(); +} else { + doSomethingElse(); +} +```