From 7aaf0619a9a22c51ed957574eec287a013b515df Mon Sep 17 00:00:00 2001 From: Error Prone Team Date: Mon, 19 Dec 2022 09:27:22 -0800 Subject: [PATCH] return switch conversion FUTURE_COPYBARA_INTEGRATE_REVIEW=https://github.com/google/error-prone/pull/3690 from msridhar:make-memoizeconstantvisitorstatelookups-suppressible 0adb67bb80c49478e7d45fbacdde5bdd7a1e7546 PiperOrigin-RevId: 496418664 --- .../MemoizeConstantVisitorStateLookups.java | 7 +- .../StatementSwitchToExpressionSwitch.java | 628 +++++++++++++- ...emoizeConstantVisitorStateLookupsTest.java | 27 + ...StatementSwitchToExpressionSwitchTest.java | 790 +++++++++++++++++- 4 files changed, 1434 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MemoizeConstantVisitorStateLookups.java b/core/src/main/java/com/google/errorprone/bugpatterns/MemoizeConstantVisitorStateLookups.java index 506dce96fb82..80aa91d5cee3 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MemoizeConstantVisitorStateLookups.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MemoizeConstantVisitorStateLookups.java @@ -42,7 +42,6 @@ import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.util.TreeScanner; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.TypeSymbol; import com.sun.tools.javac.tree.JCTree.JCFieldAccess; @@ -157,9 +156,9 @@ private static final class CallSite { } } - private static ImmutableList findConstantLookups(ClassTree tree, VisitorState state) { + private ImmutableList findConstantLookups(ClassTree tree, VisitorState state) { ImmutableList.Builder result = ImmutableList.builder(); - new TreeScanner() { + new SuppressibleTreePathScanner(state) { @Override public Void visitMethodInvocation(MethodInvocationTree tree, Void unused) { if (CONSTANT_LOOKUP.matches(tree, state)) { @@ -186,7 +185,7 @@ private void handleConstantLookup(MethodInvocationTree tree) { } } } - }.scan(tree, null); + }.scan(state.getPath(), null); return result.build(); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java index fd490cbc95ea..c37f8b4a6c47 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java @@ -17,12 +17,16 @@ package com.google.errorprone.bugpatterns; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.Iterables.getLast; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.isLastStatementInBlock; import static com.google.errorprone.util.ASTHelpers.getStartPosition; +import static com.google.errorprone.util.ASTHelpers.getSymbol; import static com.sun.source.tree.Tree.Kind.BREAK; import static com.sun.source.tree.Tree.Kind.EXPRESSION_STATEMENT; +import static com.sun.source.tree.Tree.Kind.RETURN; import static com.sun.source.tree.Tree.Kind.THROW; import static java.util.stream.Collectors.joining; @@ -30,6 +34,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Sets; import com.google.common.collect.Streams; import com.google.errorprone.BugPattern; import com.google.errorprone.ErrorProneFlags; @@ -37,26 +42,40 @@ import com.google.errorprone.bugpatterns.BugChecker.SwitchTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.util.ASTHelpers; import com.google.errorprone.util.Reachability; import com.google.errorprone.util.RuntimeVersion; import com.google.errorprone.util.SourceVersion; +import com.sun.source.tree.AssignmentTree; +import com.sun.source.tree.BlockTree; import com.sun.source.tree.BreakTree; import com.sun.source.tree.CaseTree; +import com.sun.source.tree.CompoundAssignmentTree; +import com.sun.source.tree.ExpressionStatementTree; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.ReturnTree; import com.sun.source.tree.StatementTree; import com.sun.source.tree.SwitchTree; +import com.sun.source.tree.Tree; import com.sun.source.tree.Tree.Kind; +import com.sun.source.util.TreePath; +import com.sun.tools.javac.code.Type; import com.sun.tools.javac.tree.JCTree; import java.io.BufferedReader; import java.io.CharArrayReader; import java.io.IOException; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.regex.Pattern; import java.util.stream.Stream; import javax.inject.Inject; +import javax.lang.model.element.ElementKind; /** Checks for statement switches that can be expressed as an equivalent expression switch. */ @BugPattern( @@ -68,6 +87,11 @@ public final class StatementSwitchToExpressionSwitch extends BugChecker // it's either an ExpressionStatement or a Throw. Refer to JLS 14 ยง14.11.1 private static final ImmutableSet KINDS_CONVERTIBLE_WITHOUT_BRACES = ImmutableSet.of(THROW, EXPRESSION_STATEMENT); + + private static final ImmutableSet KINDS_RETURN_OR_THROW = ImmutableSet.of(THROW, RETURN); + private static final ImmutableSet KINDS_EXPRESSIONSTATEMENT_OR_THROW = + ImmutableSet.of(THROW, EXPRESSION_STATEMENT); + private static final Pattern FALL_THROUGH_PATTERN = Pattern.compile("\\bfalls?.?through\\b", Pattern.CASE_INSENSITIVE); // Tri-state to represent the fall-thru control flow of a particular case of a particular @@ -79,11 +103,21 @@ private static enum CaseFallThru { }; private final boolean enableDirectConversion; + private final boolean enableReturnSwitchConversion; + private final boolean enableAssignmentSwitchConversion; @Inject public StatementSwitchToExpressionSwitch(ErrorProneFlags flags) { this.enableDirectConversion = flags.getBoolean("StatementSwitchToExpressionSwitch:EnableDirectConversion").orElse(false); + this.enableReturnSwitchConversion = + flags + .getBoolean("StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion") + .orElse(false); + this.enableAssignmentSwitchConversion = + flags + .getBoolean("StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion") + .orElse(false); } @Override @@ -92,13 +126,33 @@ public Description matchSwitch(SwitchTree switchTree, VisitorState state) { return NO_MATCH; } - AnalysisResult analysisResult = analyzeSwitchTree(switchTree); + // FJ.incrementCounter("matchSwitch"); + + AnalysisResult analysisResult = analyzeSwitchTree(switchTree, state); + System.out.println( + "analysisResults: canConvertToReturn " + + analysisResult.canConvertToReturnSwitch() + + " canConvDirect " + + analysisResult.canConvertDirectlyToExpressionSwitch() + + " combinedWithNext: " + + analysisResult.groupedWithNextCase()); + + List suggestedFixes = new ArrayList<>(); + if ((true || enableReturnSwitchConversion) && analysisResult.canConvertToReturnSwitch()) { + suggestedFixes.add(convertToReturnSwitch(switchTree, state, analysisResult)); + } + if ((true || enableAssignmentSwitchConversion) + && analysisResult.canConvertToAssignmentSwitch()) { + suggestedFixes.add(convertToAssignmentSwitch(switchTree, state, analysisResult)); + } if (enableDirectConversion && analysisResult.canConvertDirectlyToExpressionSwitch()) { - return convertDirectlyToExpressionSwitch(switchTree, state, analysisResult); + suggestedFixes.add(convertDirectlyToExpressionSwitch(switchTree, state, analysisResult)); } - return NO_MATCH; + return suggestedFixes.isEmpty() + ? NO_MATCH + : buildDescription(switchTree).addAllFixes(suggestedFixes).build(); } /** @@ -106,7 +160,12 @@ public Description matchSwitch(SwitchTree switchTree, VisitorState state) { * to expression switches that can be made. Does not report any findings or suggested fixes up to * the Error Prone framework. */ - private static AnalysisResult analyzeSwitchTree(SwitchTree switchTree) { + private static AnalysisResult analyzeSwitchTree(SwitchTree switchTree, VisitorState state) { + // Don't convert expression switch within switch + if (ASTHelpers.findEnclosingNode(state.getPath(), SwitchTree.class) != null) { + return AnalysisResult.of(false, false, false, ImmutableList.of(), Optional.empty()); + } + List cases = switchTree.getCases(); // A given case is said to have definite control flow if we are sure it always or never falls // thru at the end of its statement block @@ -116,10 +175,58 @@ private static AnalysisResult analyzeSwitchTree(SwitchTree switchTree) { // example "case A,B -> ..." List groupedWithNextCase = new ArrayList<>(Collections.nCopies(cases.size(), false)); + // Set of all enum values (names) explicitly listed in a case + Set handledEnumValues = new HashSet<>(); + // Does at least one case consist solely of returning a (non-void) expression? + boolean hasSimpleReturnExpressionCase = false; + // Does every case consist simply of a throw or returning a (non-void) expression? + boolean allCasesAreSimpleReturnOrThrow = true; + // Does at least one case consist of a simple assignment? + boolean hasSimpleAssignmentExpressionCase = false; + // Does every case consist simply of a throw or assignment? + boolean allCasesAreSimpleAssignmentOrThrow = true; + Optional assignmentExpressionTreeOptional = Optional.empty(); + + boolean hasDefaultCase = false; // One-pass scan to identify whether statement switch can be converted to expression switch for (int caseIndex = 0; caseIndex < cases.size(); caseIndex++) { CaseTree caseTree = cases.get(caseIndex); - boolean isDefaultCase = caseTree.getExpression() == null; + System.out.println( + String.format( + "DEB case %d (coming in allCasesAreSimpleReturnOrThrow %b) old is %s ", + caseIndex, + allCasesAreSimpleReturnOrThrow, + caseTree.getExpression() == null + ? "null" + : state.getSourceForNode(caseTree.getExpression()))); + + System.out.println( + String.format( + "DEB new is null? %b count %d caseTree %s ", + getExpressions(caseTree) == null, + getExpressions(caseTree).count(), + caseTree.getExpression() == null + ? "null" + : state.getSourceForNode(caseTree.getExpression()))); + + getExpressions(caseTree) + .forEach( + c -> { + if (c != null) { + System.out.println(String.format("Case is %s", state.getSourceForNode(c))); + } else { + System.out.println("Case is null"); + } + }); + + boolean isDefaultCase = (getExpressions(caseTree).count() == 0); + hasDefaultCase |= isDefaultCase; + // enum values included in a case statement + handledEnumValues.addAll( + getExpressions(caseTree) + .filter(IdentifierTree.class::isInstance) + .map(expressionTree -> ((IdentifierTree) expressionTree).getName().toString()) + .collect(toImmutableSet())); boolean isLastCaseInSwitch = caseIndex == cases.size() - 1; List statements = caseTree.getStatements(); @@ -128,15 +235,101 @@ private static AnalysisResult analyzeSwitchTree(SwitchTree switchTree) { // This case must be of kind CaseTree.CaseKind.RULE, and thus this is already an expression // switch; no need to continue analysis. return AnalysisResult.of( - /* canConvertDirectlyToExpressionSwitch= */ false, ImmutableList.of()); + /* canConvertDirectlyToExpressionSwitch= */ false, + /* canConvertToReturnSwitch= */ false, + /* canConvertToAssignmentSwitch= */ false, + ImmutableList.of(), + /* assignmentTargetOptional= */ Optional.empty()); } else if (statements.isEmpty()) { // If the code for this case is just an empty block, then it must fall thru caseFallThru = CaseFallThru.DEFINITELY_DOES_FALL_THRU; // Can group with the next case (unless this is the last case) - groupedWithNextCase.set(caseIndex, caseIndex < cases.size() - 1); + boolean notLastCase = caseIndex < cases.size() - 1; + groupedWithNextCase.set(caseIndex, notLastCase); + allCasesAreSimpleReturnOrThrow &= notLastCase; + allCasesAreSimpleAssignmentOrThrow &= notLastCase; } else { groupedWithNextCase.set(caseIndex, false); + if (statements.size() == 1 && KINDS_RETURN_OR_THROW.contains(statements.get(0).getKind())) { + System.out.println( + String.format( + "Simple case block with return or throw. kind %s", statements.get(0).getKind())); + // Is the case's statement block just the return of an expression? + if (RETURN.equals(statements.get(0).getKind())) { + System.out.println( + String.format( + "Cast to return tree: %s ", state.getSourceForNode(statements.get(0)))); + Type returnType = ASTHelpers.getType(((ReturnTree) statements.get(0)).getExpression()); + System.out.println(String.format("Return type is %s", returnType)); + hasSimpleReturnExpressionCase |= (returnType != null); + } + } else { + allCasesAreSimpleReturnOrThrow = false; + } + + if (!statements.isEmpty()) { + System.out.println("First statement kind: " + statements.get(0).getKind()); + } + // Can be one statement or two with the second being and unconditional break + if ((statements.size() == 1 + && KINDS_EXPRESSIONSTATEMENT_OR_THROW.contains(statements.get(0).getKind())) + || (statements.size() == 2 + && KINDS_EXPRESSIONSTATEMENT_OR_THROW.contains(statements.get(0).getKind()) + && BREAK.equals(statements.get(1).getKind()) + && ((BreakTree) statements.get(1)).getLabel() == null)) { + System.out.println( + String.format( + "Simple case block with expressionstatement or throw. kind %s", + statements.get(0).getKind())); + + if (EXPRESSION_STATEMENT.equals(statements.get(0).getKind())) { + ExpressionTree expression = + ((ExpressionStatementTree) statements.get(0)).getExpression(); + Optional assignmentTarget = Optional.empty(); + if (expression instanceof CompoundAssignmentTree) { + + assignmentTarget = Optional.of(((CompoundAssignmentTree) expression).getVariable()); + + } else if (expression instanceof AssignmentTree) { + assignmentTarget = Optional.of(((AssignmentTree) expression).getVariable()); + } else { + System.out.println( + "Not any kind of assignment tree " + + expression.getKind() + + ": Src=" + + state.getSourceForNode(expression)); + } + boolean assignable = + (assignmentExpressionTreeOptional.isEmpty() && assignmentTarget.isPresent()) + || (assignmentExpressionTreeOptional.isPresent() + && assignmentTarget.isPresent() + && getSymbol(assignmentExpressionTreeOptional.get()) + .equals(getSymbol(assignmentTarget.get()))); + + if (!assignable) { + System.out.println( + "All cases NOT simple assignment or throw due to incompatibility."); + allCasesAreSimpleAssignmentOrThrow = false; + } + + hasSimpleAssignmentExpressionCase |= assignable; + + System.out.println( + "oldAssignmentSymbol= " + + assignmentExpressionTreeOptional + + " assignmentTarget=" + + assignmentTarget + + " assignable=" + + assignable); + if (assignmentExpressionTreeOptional.isEmpty()) { + assignmentExpressionTreeOptional = assignmentTarget; + } + } + } else { + allCasesAreSimpleAssignmentOrThrow = false; + } + // Code for this case is non-empty if (areStatementsConvertibleToExpressionSwitch(statements, isLastCaseInSwitch)) { caseFallThru = CaseFallThru.DEFINITELY_DOES_NOT_FALL_THRU; @@ -158,10 +351,90 @@ private static AnalysisResult analyzeSwitchTree(SwitchTree switchTree) { // Cases other than default allCasesHaveDefiniteControlFlow &= !CaseFallThru.MAYBE_FALLS_THRU.equals(caseFallThru); } + System.out.println( + "CaseFallThru = " + + caseFallThru + + " allCasesHaveDefiniteControlFlow " + + allCasesHaveDefiniteControlFlow + + " hasSimpleAssignmentExpressioncase " + + hasSimpleAssignmentExpressionCase + + " allCasesAreSimpleAssignmentOrThrow " + + allCasesAreSimpleAssignmentOrThrow); } + boolean canConvertToReturnSwitch = + // All restrictions for direct conversion apply + allCasesHaveDefiniteControlFlow + // Each expression switch block must be a single throw or return + && allCasesAreSimpleReturnOrThrow + // At least one return is needed + && hasSimpleReturnExpressionCase + // The switch must be exhaustive + && isSwitchExhaustive( + hasDefaultCase, handledEnumValues, ASTHelpers.getType(switchTree.getExpression())); + + boolean canConvertToAssignmentSwitch = + // All restrictions for direct conversion apply + allCasesHaveDefiniteControlFlow + // Each expression switch block must be a single throw or return + && allCasesAreSimpleAssignmentOrThrow + // At least one assignment is needed + && hasSimpleAssignmentExpressionCase + // The switch must be exhaustive + && isSwitchExhaustive( + hasDefaultCase, handledEnumValues, ASTHelpers.getType(switchTree.getExpression())); + + // The switch must be exhaustive (default case implies this) + // && hasDefaultCase; + + /* + if (isSwitchExhaustive( + hasDefaultCase, handledEnumValues, ASTHelpers.getType(switchTree.getExpression()))) { + FJ.incrementCounter("matchSwitchIsExhaustive"); + } + if (hasDefaultCase) { + FJ.incrementCounter("matchHasDefaultCase"); + } + if (allCasesAreSimpleReturnOrThrow) { + FJ.incrementCounter("matchAllCasesAreSimpleReturnOrThrow"); + } + if (hasSimpleReturnExpressionCase) { + FJ.incrementCounter("matchHasSimpleReturnExpressionCase"); + } + if (allCasesHaveDefiniteControlFlow) { + FJ.incrementCounter("matchAllCasesHaveDefiniteControlFlow"); + } + if (canConvertToReturnSwitch) { + FJ.incrementCounter("matchCanConvertToReturnSwitch"); + } else { + FJ.incrementCounter("matchCannotConvertToReturnSwitch"); + } + if (canConvertToReturnSwitch && hasDefaultCase) { + FJ.incrementCounter("matchCanConvertToReturnSwitchAndHasDefaultCase"); + } + if (canConvertToAssignmentSwitch) { + FJ.incrementCounter("matchCanConvertToAssignmentSwitch"); + } + */ + System.out.println( + String.format( + "canConvertToReturnSwitch %b. canConvertToAssignmentSwitch %b. hasDef %b" + + " allCasesHaveDefiniteControlFlow %b allCasesSimpleRetThrow %b" + + " hasSimpleREturnExpression %b handledEnumValues %s hasSimpleReturnExCase %b", + canConvertToReturnSwitch, + canConvertToAssignmentSwitch, + hasDefaultCase, + allCasesHaveDefiniteControlFlow, + allCasesAreSimpleReturnOrThrow, + hasSimpleReturnExpressionCase, + handledEnumValues, + hasSimpleReturnExpressionCase)); return AnalysisResult.of( - allCasesHaveDefiniteControlFlow, ImmutableList.copyOf(groupedWithNextCase)); + /* canConvertDirectlyToExpressionSwitch= */ allCasesHaveDefiniteControlFlow, + canConvertToReturnSwitch, + canConvertToAssignmentSwitch, + /* groupedWithNextCase= */ ImmutableList.copyOf(groupedWithNextCase), + assignmentExpressionTreeOptional); } /** @@ -193,7 +466,7 @@ private static boolean areStatementsConvertibleToExpressionSwitch( * conversion, each nontrivial statement block is mapped one-to-one to a new {@code Expression} or * {@code StatementBlock} on the right-hand side. Comments are presevered wherever possible. */ - private Description convertDirectlyToExpressionSwitch( + private SuggestedFix convertDirectlyToExpressionSwitch( SwitchTree switchTree, VisitorState state, AnalysisResult analysisResult) { List cases = switchTree.getCases(); @@ -279,9 +552,237 @@ private Description convertDirectlyToExpressionSwitch( // Close the switch statement replacementCodeBuilder.append("\n}"); + return SuggestedFix.builder().replace(switchTree, replacementCodeBuilder.toString()).build(); + } + + private SuggestedFix convertToReturnSwitch( + SwitchTree switchTree, VisitorState state, AnalysisResult analysisResult) { + + List statementsToDelete = new ArrayList<>(); + List cases = switchTree.getCases(); + StringBuilder replacementCodeBuilder = new StringBuilder(); + replacementCodeBuilder + .append("return switch ") + .append(state.getSourceForNode(switchTree.getExpression())) + .append(" {"); + + StringBuilder groupedCaseCommentsAccumulator = null; + boolean firstCaseInGroup = true; + for (int caseIndex = 0; caseIndex < cases.size(); caseIndex++) { + CaseTree caseTree = cases.get(caseIndex); + boolean isDefaultCase = caseTree.getExpression() == null; + + String transformedBlockSource = + transformReturnOrThrowBlock(caseTree, state, cases, caseIndex, caseTree.getStatements()); + + if (firstCaseInGroup) { + groupedCaseCommentsAccumulator = new StringBuilder(); + replacementCodeBuilder.append("\n "); + if (!isDefaultCase) { + replacementCodeBuilder.append("case "); + } + } + replacementCodeBuilder.append( + isDefaultCase ? "default" : printCaseExpressions(caseTree, state)); + + if (analysisResult.groupedWithNextCase().get(caseIndex)) { + firstCaseInGroup = false; + replacementCodeBuilder.append(", "); + // Capture comments from this case so they can be added to the group's transformed case + if (!transformedBlockSource.trim().isEmpty()) { + groupedCaseCommentsAccumulator.append(removeFallThruLines(transformedBlockSource)); + } + // Add additional cases to the list on the lhs of the arrow + continue; + } else { + // This case is the last case in its group, so insert the collected comments from the lhs of + // the colon here + transformedBlockSource = groupedCaseCommentsAccumulator + transformedBlockSource; + } + + replacementCodeBuilder.append(" -> "); + + // Single statement with no comments - no braces needed + replacementCodeBuilder.append(transformedBlockSource); + + firstCaseInGroup = true; + } // case loop + + // Close the switch statement + replacementCodeBuilder.append("\n};"); + + System.out.println( + String.format( + "Synth: %s which will replace %s", + replacementCodeBuilder, state.getSourceForNode(switchTree))); + + // Experiment with removing stuff after the switch. + TreePath pathToEnclosing = state.findPathToEnclosing(BlockTree.class); + if (pathToEnclosing != null) { + Tree enclosing = pathToEnclosing.getLeaf(); + // What does this do ????????? + state = state.withPath(pathToEnclosing); + if (enclosing instanceof BlockTree) { + System.out.println( + "Enclosing is a block tree. The source is " + state.getSourceForNode(enclosing)); + BlockTree blockTree = (BlockTree) enclosing; + System.out.println( + "The block tree has " + + blockTree.getStatements().size() + + " statements. Static? " + + blockTree.isStatic()); + + if (Matchers.nextStatement(Matchers.anything()).matches(switchTree, state)) { + System.out.println("Next statement matches (exist)"); + } else { + System.out.println("Next statement does not match (exist)"); + } + + boolean last = + isLastStatementInBlock().matches(switchTree, state.withPath(pathToEnclosing)); + System.out.println("Last? " + last); + + TreePath p = state.getPath(); + System.out.println( + "Current path " + + p + + " which has leaf " + + state.getSourceForNode(p.getLeaf()) + + " parent path " + + p.getParentPath() + + state.getSourceForNode(p.getParentPath().getLeaf())); + + // Path from root -> switchTree + TreePath rootToSwitchPath = TreePath.getPath(pathToEnclosing, switchTree); + + System.out.println( + "rootToSwitchPath " + + rootToSwitchPath + + " " + + state.getSourceForNode(rootToSwitchPath.getLeaf())); + + System.out.println( + "Computed statement number: " + + computeStatementNumber(pathToEnclosing, blockTree) + + ", and that block is " + + state.getSourceForNode( + blockTree + .getStatements() + .get(computeStatementNumber(pathToEnclosing, blockTree)))); + + int csn = computeStatementNumber(rootToSwitchPath, blockTree); + for (int i = csn + 1; i < blockTree.getStatements().size(); i++) { + statementsToDelete.add(blockTree.getStatements().get(i)); + System.out.println( + "CSN purge " + + i + + " which is: " + + state.getSourceForNode(blockTree.getStatements().get(i))); + } + + } else if (enclosing instanceof CaseTree) { + System.out.println("Enclosing is a case tree, ignore it."); + } else { + // findEnclosing given two types must return something of one of those types + throw new IllegalStateException("enclosing tree not a BlockTree or CaseTree"); + } + } + SuggestedFix.Builder suggestedFixBuilder = SuggestedFix.builder().replace(switchTree, replacementCodeBuilder.toString()); - return describeMatch(switchTree, suggestedFixBuilder.build()); + // Delete redundant statements, leaving comments where feasible + statementsToDelete.stream().forEach(deleteMe -> suggestedFixBuilder.replace(deleteMe, "")); + + return suggestedFixBuilder.build(); + } + + private int computeStatementNumber(TreePath p, BlockTree blockTree) { + + for (int i = 0; i < blockTree.getStatements().size(); i++) { + + StatementTree thisStatement = blockTree.getStatements().get(i); + + // Look for thisStatement along the path from the root to the switch tree + TreePath pp = TreePath.getPath(p, thisStatement); + if (pp != null) { + return i; + } + } + return -1; + } + + private SuggestedFix convertToAssignmentSwitch( + SwitchTree switchTree, VisitorState state, AnalysisResult analysisResult) { + + List cases = switchTree.getCases(); + StringBuilder replacementCodeBuilder = + new StringBuilder(state.getSourceForNode(analysisResult.assignmentTargetOptional().get())); + replacementCodeBuilder + .append(" = switch ") + .append(state.getSourceForNode(switchTree.getExpression())) + .append(" {"); + + StringBuilder groupedCaseCommentsAccumulator = null; + boolean firstCaseInGroup = true; + for (int caseIndex = 0; caseIndex < cases.size(); caseIndex++) { + CaseTree caseTree = cases.get(caseIndex); + boolean isDefaultCase = caseTree.getExpression() == null; + ImmutableList filteredStatements = filterOutRedundantBreak(caseTree); + + String transformedBlockSource = + transformAssignOrThrowBlock(caseTree, state, cases, caseIndex, filteredStatements); + + if (firstCaseInGroup) { + groupedCaseCommentsAccumulator = new StringBuilder(); + replacementCodeBuilder.append("\n "); + if (!isDefaultCase) { + replacementCodeBuilder.append("case "); + } + } + replacementCodeBuilder.append( + isDefaultCase ? "default" : printCaseExpressions(caseTree, state)); + + if (analysisResult.groupedWithNextCase().get(caseIndex)) { + firstCaseInGroup = false; + replacementCodeBuilder.append(", "); + // Capture comments from this case so they can be added to the group's transformed case + if (!transformedBlockSource.trim().isEmpty()) { + groupedCaseCommentsAccumulator.append(removeFallThruLines(transformedBlockSource)); + } + // Add additional cases to the list on the lhs of the arrow + continue; + } else { + // This case is the last case in its group, so insert the collected comments from the lhs of + // the colon here + transformedBlockSource = groupedCaseCommentsAccumulator + transformedBlockSource; + } + + replacementCodeBuilder.append(" -> "); + + // Extract comments (if any) for break that was removed as redundant + Optional commentsBeforeRemovedBreak = + extractCommentsBeforeRemovedBreak(caseTree, state, filteredStatements); + if (commentsBeforeRemovedBreak.isPresent()) { + transformedBlockSource = transformedBlockSource + "\n" + commentsBeforeRemovedBreak.get(); + } + + // Single statement with no comments - no braces needed + replacementCodeBuilder.append(transformedBlockSource); + + firstCaseInGroup = true; + } // case loop + + // Close the switch statement + replacementCodeBuilder.append("\n};"); + + System.out.println( + String.format( + "Synth: %s which will replace %s", + replacementCodeBuilder, state.getSourceForNode(switchTree))); + + SuggestedFix.Builder suggestedFixBuilder = + SuggestedFix.builder().replace(switchTree, replacementCodeBuilder.toString()); + return suggestedFixBuilder.build(); } /** @@ -446,23 +947,124 @@ private static Stream getExpressions(CaseTree caseTree CaseTree.class.getMethod("getExpressions").invoke(caseTree)) .stream(); } else { - return Stream.of(caseTree.getExpression()); + // Conform to behavior of CaseTree.getExpressions() API + return caseTree.getExpression() == null + ? Stream.empty() + : Stream.of(caseTree.getExpression()); } } catch (ReflectiveOperationException e) { throw new LinkageError(e.getMessage(), e); } } + /** + * Ad-hoc algorithm to search for a surjective map from (non-null) values of the {@code switch} + * expression to a {@code CaseTree}. This algorithm does not compute whether such a map exists, + * but rather only whether it can find such a map. + */ + private static boolean isSwitchExhaustive( + boolean hasDefaultCase, Set handledEnumValues, Type switchType) { + if (hasDefaultCase) { + // Anything not included in a case can be mapped to the default CaseTree + return true; + } + + // Handles switching on enum (map is bijective) + if (switchType.asElement().getKind() != ElementKind.ENUM) { + return false; + } + return Sets.difference(ASTHelpers.enumValues(switchType.asElement()), handledEnumValues) + .isEmpty(); + } + + private static String transformReturnOrThrowBlock( + CaseTree caseTree, + VisitorState state, + List cases, + int caseIndex, + List statements) { + + StringBuilder transformedBlockBuilder = new StringBuilder(); + int codeBlockStart; + int codeBlockEnd = + statements.isEmpty() + ? getBlockEnd(state, caseTree, cases, caseIndex) + : state.getEndPosition(Streams.findLast(statements.stream()).get()); + + if (statements.size() == 1 && statements.get(0).getKind().equals(RETURN)) { + // For "return x;", we want to take source starting after the "return" + int unused = extractLhsComments(caseTree, state, transformedBlockBuilder); + ReturnTree returnTree = (ReturnTree) statements.get(0); + codeBlockStart = getStartPosition(returnTree.getExpression()); + // TODO : what about comments after the return but before the expression ??? + } else { + codeBlockStart = extractLhsComments(caseTree, state, transformedBlockBuilder); + } + transformedBlockBuilder.append(state.getSourceCode(), codeBlockStart, codeBlockEnd); + + return transformedBlockBuilder.toString(); + } + + private static String transformAssignOrThrowBlock( + CaseTree caseTree, + VisitorState state, + List cases, + int caseIndex, + List statements) { + + // TODO : comment scenarios. + StringBuilder transformedBlockBuilder = new StringBuilder(); + int codeBlockStart; + int codeBlockEnd = + statements.isEmpty() + ? getBlockEnd(state, caseTree, cases, caseIndex) + : state.getEndPosition(Streams.findLast(statements.stream()).get()); + + if (!statements.isEmpty() && statements.get(0).getKind().equals(EXPRESSION_STATEMENT)) { + // For "x = foo", we want to take source starting after the "x =" + int unused = extractLhsComments(caseTree, state, transformedBlockBuilder); + // ReturnTree returnTree = (ReturnTree) statements.get(0); + ExpressionTree expression = ((ExpressionStatementTree) statements.get(0)).getExpression(); + Optional rhs = Optional.empty(); + if (expression instanceof CompoundAssignmentTree) { + rhs = Optional.of(((CompoundAssignmentTree) expression).getExpression()); + } else if (expression instanceof AssignmentTree) { + rhs = Optional.of(((AssignmentTree) expression).getExpression()); + } + codeBlockStart = getStartPosition(rhs.get()); + + } else { + codeBlockStart = extractLhsComments(caseTree, state, transformedBlockBuilder); + } + transformedBlockBuilder.append(state.getSourceCode(), codeBlockStart, codeBlockEnd); + + return transformedBlockBuilder.toString(); + } + @AutoValue abstract static class AnalysisResult { abstract boolean canConvertDirectlyToExpressionSwitch(); + abstract boolean canConvertToReturnSwitch(); + + abstract boolean canConvertToAssignmentSwitch(); + abstract ImmutableList groupedWithNextCase(); + abstract Optional assignmentTargetOptional(); + static AnalysisResult of( - boolean canConvertDirectlyToExpressionSwitch, ImmutableList groupedWithNextCase) { + boolean canConvertDirectlyToExpressionSwitch, + boolean canConvertToReturnSwitch, + boolean canConvertToAssignmentSwitch, + ImmutableList groupedWithNextCase, + Optional assignmentTargetOptional) { return new AutoValue_StatementSwitchToExpressionSwitch_AnalysisResult( - canConvertDirectlyToExpressionSwitch, groupedWithNextCase); + canConvertDirectlyToExpressionSwitch, + canConvertToReturnSwitch, + canConvertToAssignmentSwitch, + groupedWithNextCase, + assignmentTargetOptional); } } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/MemoizeConstantVisitorStateLookupsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/MemoizeConstantVisitorStateLookupsTest.java index bb00fbc96cc5..216fc5f088ad 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/MemoizeConstantVisitorStateLookupsTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/MemoizeConstantVisitorStateLookupsTest.java @@ -162,4 +162,31 @@ public void negative_doesntMemoizeTwice() { .expectUnchanged() .doTest(); } + + @Test + public void testSuppressWarnings() { + compilationTestHelper + .addSourceLines( + "Test.java", + "import com.google.errorprone.VisitorState;", + "import com.sun.tools.javac.code.Type;", + "import com.sun.tools.javac.util.Name;", + "class Test {", + " @SuppressWarnings(\"MemoizeConstantVisitorStateLookups\")", + " public Test(VisitorState state) {", + " Name className = state.getName(\"java.lang.Class\");", + " }", + " @SuppressWarnings(\"MemoizeConstantVisitorStateLookups\")", + " public void testMethod(VisitorState state) {", + " Name className = state.getName(\"java.lang.Class\");", + " }", + " @SuppressWarnings(\"MemoizeConstantVisitorStateLookups\")", + " class InnerClass {", + " void innerMethod(VisitorState state) {", + " Name className = state.getName(\"java.lang.Class\");", + " }", + " }", + "}") + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java index 9bc9ecfce43a..ab601515d83b 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java @@ -285,6 +285,7 @@ public void switchByEnumCard_combinesCaseComments_error() { @Test public void switchByEnumCard2_removesRedundantBreaks_error() { + System.out.println("RRRRRRRRRRRRRRRRRRRRRR"); assumeTrue(RuntimeVersion.isAtLeast14()); helper .addSourceLines( @@ -365,6 +366,8 @@ public void switchByEnumCard2_removesRedundantBreaks_error() { .setArgs( ImmutableList.of("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")) .doTest(); + + System.out.println("rrrrrrrrrrrrrrrrrrrrr"); } @Test @@ -789,6 +792,7 @@ public void switchByEnumCardWithThrow_error() { @Test public void switchInSwitch_error() { + // Only the outer "switch" should generate a finding assumeTrue(RuntimeVersion.isAtLeast14()); helper .addSourceLines( @@ -802,7 +806,6 @@ public void switchInSwitch_error() { " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", " switch(side) {", " case HEART:", - " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", " switch(side) {", " case HEART: ", " case SPADE: ", @@ -1069,4 +1072,789 @@ public void dynamicWithThrowableDuringInitializationFromMethod_noMatch() { ImmutableList.of("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")) .doTest(); } + + //////////////// + /////////////// Return switch () ... + ////////////// + ///////////// + + @Test + public void switchByEnum_returnSwitch_error() { + System.out.println("RRRRRRRR"); + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(side) {", + " case HEART:", + " case DIAMOND:", + " return invoke();", + " case SPADE:", + " throw new RuntimeException();", + " default:", + " throw new NullPointerException();", + " }", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .doTest(); + + // Check correct generated code + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " switch(side) {", + " case HEART:", + " case DIAMOND:", + " return invoke();", + " case SPADE:", + " throw new RuntimeException();", + " default:", + " throw new NullPointerException();", + " }", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " return switch(side) {", + " case HEART, DIAMOND -> invoke();", + " case SPADE -> throw new RuntimeException();", + " default -> throw new NullPointerException();", + " };", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .doTest(); + } + + @Test + public void switchByEnum_returnSwitchWithShouldNeverHappen_error() { + System.out.println("DDDDDDDD"); + assumeTrue(RuntimeVersion.isAtLeast14()); + + // TODO Add the first part + + // Check correct generated code + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " switch(side) {", + " case HEART:", + " case DIAMOND:", + " return invoke();", + " case SPADE:", + " throw new RuntimeException();", + " case CLUB:", + " throw new NullPointerException();", + " }", + " // This should never happen", + " int z = invoke();", + " z++;", + " throw new RuntimeException(\"Switch was not exhaustive at runtime \" + z);", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " return switch(side) {", + " case HEART, DIAMOND -> invoke();", + " case SPADE -> throw new RuntimeException();", + " case CLUB -> throw new NullPointerException();", + " };", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .doTest(); + } + + @Test + public void switchByEnum_switchInReturnSwitchWithShouldNeverHappen_error() { + System.out.println("QQQQQQQQQQQQ"); + assumeTrue(RuntimeVersion.isAtLeast14()); + + // No error because the inner switch is the only fixable one + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " switch(side) {", + " case HEART:", + " System.out.println(\"hi\");", + " switch(side) {", + " case HEART:", + " case DIAMOND:", + " return invoke();", + " case SPADE:", + " throw new RuntimeException();", + " case CLUB:", + " throw new NullPointerException();", + " }", + " // This should never happen", + " int z = invoke();", + " z++;", + " throw new RuntimeException(\"Switch was not exhaustive at runtime \" + z);", + " ", + " default: ", + " System.out.println(\"default\");", + " return 0;", + " }", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .doTest(); + } + + @Test + public void switchByEnum_exhaustiveWithDefault_error() { + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " String z = \"dkfj\";", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(z) {", + " case \"\":", + " case \"DIAMOND\":", + " // Custom comment", + " case \"SPADE\":", + " return invoke();", + " default:", + " return 2;", + " }", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .doTest(); + + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " String z = \"dkfj\";", + " switch(z) {", + " case \"\":", + " case \"DIAMOND\":", + " // Custom comment", + " case \"SPADE\":", + " return invoke();", + " default:", + " return 2;", + " }", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " String z = \"dkfj\";", + " return switch(z) {", + " case \"\", \"DIAMOND\", \"SPADE\" ->", + " // Custom comment", + " invoke();", + " default -> 2;", + " };", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .doTest(); + } + + @Test + public void switchByEnum_defaultFallThru_error() { + // No error because default doesn't return anything within its block + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " switch(side) {", + " case HEART:", + " case DIAMOND:", + " return invoke();", + " case SPADE:", + " throw new RuntimeException();", + " default:", + " // Fall through", + " }", + " return -2;", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .doTest(); + } + + @Test + public void switchByEnum_returnSwitchWithShouldNeverHappen_errorAndRemoveShouldNeverHappen() { + // The switch has a case for each enum and "should never happen" error handling + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " System.out.println(\"don't delete 0\");", + " if (invoke() > 0) {", + " System.out.println(\"don't delete 1\");", + " // Preceding comment", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(side) {", + " case HEART:", + " // Fall through", + " case DIAMOND:", + " return invoke();", + " case SPADE:", + " throw new RuntimeException();", + " case CLUB:", + " throw new NullPointerException();", + " }", + " // Custom comment - should never happen", + " int z = invoke(/* block comment 0 */);", + " z++;", + " throw new RuntimeException(\"Switch was not exhaustive at runtime \" + z);", + " }", + " System.out.println(\"don't delete 2\");", + " return 0;", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .doTest(); + + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " System.out.println(\"don't delete 0\");", + " if (invoke() > 0) {", + " System.out.println(\"don't delete 1\");", + " // Preceding comment", + " switch(side) {", + " case HEART /* lhs comment */: // rhs comment", + " // Fall through", + " case DIAMOND:", + " return invoke();", + " case SPADE:", + " throw new RuntimeException();", + " case CLUB:", + " throw new NullPointerException();", + " }", + " // Custom comment - should never happen", + " int z = invoke(/* block comment 0 */);", + " z++;", + " throw new RuntimeException(\"Switch was not exhaustive at runtime \" + z);", + " }", + " System.out.println(\"don't delete 2\");", + " return 0;", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " System.out.println(\"don't delete 0\");", + " if (invoke() > 0) {", + " System.out.println(\"don't delete 1\");", + " // Preceding comment", + " return switch(side) {", + " case HEART, DIAMOND -> ", + " /* lhs comment */", + " // rhs comment", + " invoke();", + " case SPADE -> throw new RuntimeException();", + " case CLUB -> throw new NullPointerException();", + " };", + " // Custom comment - should never happen", + " }", + " System.out.println(\"don't delete 2\");", + " return 0;", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .doTest(); + } + + @Test + public void + switchByEnum_returnSwitchWithShouldNeverHappenInLambda_errorAndRemoveShouldNeverHappen() { + // Conversion to return switch within a lambda + assumeTrue(RuntimeVersion.isAtLeast14()); + + refactoringHelper + .addInputLines( + "Test.java", + "import java.util.function.Supplier;", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " Supplier lambda = () -> {", + " // Preceding comment", + " switch(side) {", + " case HEART :", + " // Fall through", + " case DIAMOND:", + " return invoke();", + " case SPADE:", + " throw new RuntimeException();", + " case CLUB:", + " throw new NullPointerException();", + " }", + " // Custom comment - should never happen", + " int z = invoke(/* block comment 0 */);", + " z++;", + " throw new RuntimeException(\"Switch was not exhaustive at runtime \" + z);", + " };", + " System.out.println(\"don't delete 2\");", + " return lambda.get();", + " }", + "}") + .addOutputLines( + "Test.java", + "import java.util.function.Supplier;", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " Supplier lambda = () -> {", + " // Preceding comment", + " return switch(side) {", + " case HEART, DIAMOND -> invoke();", + " case SPADE -> throw new RuntimeException();", + " case CLUB -> throw new NullPointerException();", + " };", + " // Custom comment - should never happen", + " };", + " System.out.println(\"don't delete 2\");", + " return lambda.get();", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .doTest(); + } + + //////////////// + /////////////// ASSIGNMENT switch () ... + ////////////// + ///////////// + + @Test + public void switchByEnum_assignmentSwitchToLocalHasDefault_error() { + System.out.println("AAA123"); + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int foo(Side side) { ", + " int x = 0;", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(side) {", + " case HEART:", + " case DIAMOND:", + " x = (((x+1) * (x*x)) << 1);", + " break;", + " case SPADE:", + " throw new RuntimeException();", + " default:", + " throw new NullPointerException();", + " }", + " return x;", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion")) + .doTest(); + + // Check correct generated code + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int foo(Side side) { ", + " int x = 0;", + " switch(side) {", + " case HEART:", + " case DIAMOND:", + " x = ((x+1) * (x*x)) << 1;", + " break;", + " case SPADE:", + " throw new RuntimeException();", + " default:", + " throw new NullPointerException();", + " }", + " return x;", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int foo(Side side) { ", + " int x = 0;", + " x = switch(side) {", + " case HEART, DIAMOND ->", + " ((x+1) * (x*x)) << 1;", + " case SPADE ->", + " throw new RuntimeException();", + " default ->", + " throw new NullPointerException();", + " };", + " return x;", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion")) + .doTest(); + } + + @Test + public void switchByEnum_assignmentSwitchMixedReferences_error() { + // "x" and "this.x" are equivalent here + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " int x;", + " public Test(int foo) {", + " x = -1;", + " }", + " ", + " public int foo(Side side) { ", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(side) {", + " case HEART:", + " x = 2;", + " break;", + " case DIAMOND:", + " this.x = (((x+1) * (x*x)) << 1);", + " break;", + " case SPADE:", + " throw new RuntimeException();", + " default:", + " throw new NullPointerException();", + " }", + " return x;", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion")) + .doTest(); + + // Check correct generated code. + // Note "x" and "this.x" are swapped relative to above. It uses the style of the first in code + // order. + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " int x;", + " public Test(int foo) {", + " x = -1;", + " }", + " ", + " public int foo(Side side) { ", + " switch(side) {", + " case HEART:", + " this.x = 2;", + " break;", + " case DIAMOND:", + " x = (((x+1) * (x*x)) << 1);", + " break;", + " case SPADE:", + " throw new RuntimeException();", + " default:", + " throw new NullPointerException();", + " }", + " return x;", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " int x;", + " public Test(int foo) {", + " x = -1;", + " }", + " ", + " public int foo(Side side) { ", + " this.x = switch(side) {", + " case HEART -> 2;", + " case DIAMOND -> (((x+1) * (x*x)) << 1);", + " case SPADE -> throw new RuntimeException();", + " default -> throw new NullPointerException();", + " };", + " return x;", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion")) + .doTest(); + } + + @Test + public void switchByEnum_exhaustiveAssignmentSwitch_error() { + + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int foo(Side side) { ", + " int x = 0;", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(side) {", + " case HEART:", + " case DIAMOND:", + " x = (((x+1) * (x*x)) << 1);", + " break;", + " case SPADE:", + " throw new RuntimeException();", + " case CLUB:", + " throw new NullPointerException();", + " }", + " return x;", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion")) + .doTest(); + + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int foo(Side side) { ", + " int x = 0;", + " switch(side) {", + " case HEART:", + " case DIAMOND:", + " x = (((x+1) * (x*x)) << 1);", + " break;", + " case SPADE:", + " throw new RuntimeException();", + " case CLUB:", + " throw new NullPointerException();", + " }", + " return x;", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int foo(Side side) { ", + " int x = 0;", + " x = switch(side) {", + " case HEART, DIAMOND -> (((x+1) * (x*x)) << 1);", + " case SPADE -> throw new RuntimeException();", + " case CLUB -> throw new NullPointerException();", + " };", + " return x;", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion")) + .doTest(); + } + + @Test + public void switchByEnum_nonExhaustiveAssignmentSwitch_noError() { + + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int foo(Side side) { ", + " int x = 0;", + " switch(side) {", + " // No HEART case", + " case DIAMOND:", + " x = (((x+1) * (x*x)) << 1);", + " break;", + " case SPADE:", + " throw new RuntimeException();", + " case CLUB:", + " throw new NullPointerException();", + " }", + " return x;", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion")) + .doTest(); + } }