From c90d11e51db2f3d595ca6a10a92c92c3b569c4a2 Mon Sep 17 00:00:00 2001 From: markbrady Date: Tue, 11 Nov 2025 12:50:16 -0800 Subject: [PATCH] [IfChainToSwitch] Create a new checker to suggest converting chains of if-statements into arrow switches, where feasible and helpful to readability PiperOrigin-RevId: 831031548 --- .../bugpatterns/IfChainToSwitch.java | 1611 +++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../bugpatterns/IfChainToSwitchTest.java | 1992 +++++++++++++++++ docs/bugpattern/IfChainToSwitch.md | 120 + 4 files changed, 3725 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java create mode 100644 docs/bugpattern/IfChainToSwitch.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java b/core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java new file mode 100644 index 00000000000..1cc2a370d1f --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java @@ -0,0 +1,1611 @@ +/* + * Copyright 2025 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.base.Preconditions.checkArgument; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.constValue; +import static com.google.errorprone.util.ASTHelpers.getStartPosition; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.isSubtype; +import static com.google.errorprone.util.ASTHelpers.sameVariable; +import static com.sun.source.tree.Tree.Kind.EXPRESSION_STATEMENT; +import static com.sun.source.tree.Tree.Kind.THROW; +import static java.lang.Math.max; +import static java.lang.Math.min; +import static java.util.stream.Collectors.joining; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Range; +import com.google.errorprone.BugPattern; +import com.google.errorprone.ErrorProneFlags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.IfTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.CompileTimeConstantExpressionMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.suppliers.Suppliers; +import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.util.ErrorProneComment; +import com.google.errorprone.util.Reachability; +import com.google.errorprone.util.SourceVersion; +import com.sun.source.tree.BinaryTree; +import com.sun.source.tree.BindingPatternTree; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.BreakTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IfTree; +import com.sun.source.tree.InstanceOfTree; +import com.sun.source.tree.ParenthesizedTree; +import com.sun.source.tree.StatementTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.Tree.Kind; +import com.sun.source.tree.VariableTree; +import com.sun.source.tree.YieldTree; +import com.sun.source.util.TreePath; +import com.sun.source.util.TreeScanner; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.TypeVariableSymbol; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.TypeTag; +import com.sun.tools.javac.code.Types; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Stream; +import javax.inject.Inject; +import javax.lang.model.element.ElementKind; +import org.jspecify.annotations.Nullable; + +/** Checks for chains of if statements that may be converted to a switch. */ +@BugPattern(severity = WARNING, summary = "This if-chain may be converted into a switch") +public final class IfChainToSwitch extends BugChecker implements IfTreeMatcher { + // Braces are not required if there is exactly one statement on the right hand of the arrow, and + // 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 Matcher COMPILE_TIME_CONSTANT_MATCHER = + CompileTimeConstantExpressionMatcher.instance(); + + /** + * Tri-state of whether the if-chain is valid, invalid, or possibly valid for conversion to a + * switch. + */ + enum Validity { + MAYBE_VALID, + INVALID, + VALID + } + + private final boolean enableMain; + private final int maxChainLength; + + @Inject + IfChainToSwitch(ErrorProneFlags flags) { + enableMain = flags.getBoolean("IfChainToSwitch:EnableMain").orElse(false); + maxChainLength = flags.getInteger("IfChainToSwitch:MaxChainLength").orElse(50); + } + + @Override + public Description matchIf(IfTree ifTree, VisitorState visitorState) { + if (!enableMain) { + return NO_MATCH; + } + + if (!SourceVersion.supportsPatternMatchingSwitch(visitorState.context)) { + return NO_MATCH; + } + + return analyzeIfTree(ifTree, visitorState); + } + + /** Analyzes the tree rooted at {@code IfTree} for conversion to a switch. */ + private Description analyzeIfTree(IfTree ifTree, VisitorState state) { + // Match only at start of chain; no if-within-if + for (Tree tree : state.getPath().getParentPath()) { + if (tree instanceof IfTree) { + return NO_MATCH; + } + } + + Range ifTreeSourceRange = computeIfTreeSourceRange(ifTree, state); + IfChainAnalysisState ifChainAnalysisState = + new IfChainAnalysisState( + /* subjectOptional= */ Optional.empty(), + /* depth= */ 1, + Validity.MAYBE_VALID, + /* at= */ ifTree, + /* allConditionalBlocksReturnOrThrow= */ true, + /* handledEnumValues= */ ImmutableSet.of()); + List cases = new ArrayList<>(); + + // Walk down the if-chain, performing quick analysis at each "if" to see whether it's + // potentially convertible + for (int iteration = 0; + ifChainAnalysisState.validity().equals(Validity.MAYBE_VALID); + iteration++) { + + ifChainAnalysisState = + analyzeIfStatement( + ifChainAnalysisState, + ifChainAnalysisState.at().getCondition(), + ifChainAnalysisState.at().getThenStatement(), + ifChainAnalysisState.at().getElseStatement(), + cases, + state, + ifTreeSourceRange); + + // If the if-chain is long, then the worst-case runtime of the deep analysis can grow. + // For runtime performance reasons, limit the maximum length of the if-chain + if (iteration > maxChainLength) { + return NO_MATCH; + } + } + + List suggestedFixes = new ArrayList<>(); + if (ifChainAnalysisState.validity().equals(Validity.VALID) + // Exclude short if-chains, since they may be more readable as-is + && ifChainAnalysisState.depth() >= 3) { + + suggestedFixes = + deepAnalysisOfIfChain( + cases, ifChainAnalysisState, ifTree, state, ifTreeSourceRange, suggestedFixes); + } + + return suggestedFixes.isEmpty() + ? NO_MATCH + : buildDescription(ifTree).addAllFixes(suggestedFixes).build(); + } + + /** Compute the position range of the source code for the {@code IfTree} */ + private static Range computeIfTreeSourceRange(IfTree ifTree, VisitorState state) { + ImmutableList subsequentIfStatements = getSubsequentStatementsInBlock(state); + if (subsequentIfStatements.isEmpty()) { + return Range.closedOpen(getStartPosition(ifTree), state.getEndPosition(ifTree)); + } + return Range.closedOpen( + getStartPosition(ifTree), state.getEndPosition(subsequentIfStatements.get(0))); + } + + /** + * Within the context of the right-hand-side of the arrow of a `case`, strips any redundant + * surrounding braces. The returned value is either the input {@code StatementTree}, or a subtree + * of the input that excludes the unnecessary braces. + */ + private static StatementTree stripUnnecessaryBracesForArrowRhs(StatementTree statementTree) { + + StatementTree at = statementTree; + + while (at instanceof BlockTree blockTree && (blockTree.getStatements().size() == 1)) { + StatementTree firstStatement = blockTree.getStatements().get(0); + Kind firstStatementKind = firstStatement.getKind(); + + if (KINDS_CONVERTIBLE_WITHOUT_BRACES.contains(firstStatementKind)) { + // Remove braces + at = firstStatement; + break; + } + + if (firstStatement instanceof BlockTree) { + // Descend + at = firstStatement; + continue; + } + + // We're neither over another block nor a statement that can be rendered without braces. + // Thus, these braces are necessary. + break; + } + return at; + } + + /** + * Determines whether the given statement tree needs to be wrapped in braces when used on the + * right hand side of the arrow of a `case`. + */ + public static boolean needsBracesForArrowRhs(StatementTree statementTree) { + Kind statementTreeKind = statementTree.getKind(); + + if (statementTree instanceof BlockTree) { + // Already wrapped in braces + return false; + } + + if (KINDS_CONVERTIBLE_WITHOUT_BRACES.contains(statementTreeKind)) { + return false; + } + + return true; + } + + /** + * Renders Java source code for a {@code switch} statement, as specified by the supplied internal + * representations. Context is extracted from the {@code VisitorState} and {@code + * SuggestedFix.Builder} where needed. + */ + private static String prettyPrint( + List cases, + ExpressionTree subject, + VisitorState state, + SuggestedFix.Builder suggestedFixBuilder, + Range ifTreeSourceRange, + ImmutableList allComments) { + + StringBuilder sb = new StringBuilder(); + sb.append("switch (").append(state.getSourceForNode(subject)).append(") {\n"); + for (CaseIr caseIr : cases) { + if (caseIr.hasCaseNull()) { + sb.append("case null"); + } else if (caseIr.hasDefault()) { + sb.append("default"); + } else if (caseIr.expressionsOptional().isPresent()) { + sb.append("case "); + sb.append( + caseIr.expressionsOptional().get().stream() + .map(state::getSourceForNode) + .collect(joining(","))); + sb.append(" "); + } else if (caseIr.instanceOfOptional().isPresent()) { + InstanceOfIr instanceOfIr = caseIr.instanceOfOptional().get(); + sb.append("case "); + if (instanceOfIr.patternVariable() != null) { + sb.append( + printRawTypesAsWildcards( + getType(instanceOfIr.patternVariable()), state, suggestedFixBuilder)); + + Symbol sym = ASTHelpers.getSymbol(instanceOfIr.patternVariable()); + sb.append(" ").append(sym.getSimpleName()).append(" "); + } else if (instanceOfIr.type() != null && instanceOfIr.expression() != null) { + sb.append( + printRawTypesAsWildcards(getType(instanceOfIr.type()), state, suggestedFixBuilder)); + // It's possible that "unused" could conflict with an existing local variable name; + // support for unnamed variables gets around this issue, but requires later Java versions + sb.append(" unused "); + } + if (caseIr.guardOptional().isPresent()) { + sb.append("when ").append(state.getSourceForNode(caseIr.guardOptional().get())); + } + } + + sb.append(" -> "); + + int ifTreeStart = ifTreeSourceRange.lowerEndpoint(); + + if (caseIr.arrowRhsOptional().isEmpty()) { + // Empty braces on RHS; block is synthesized, so no comments to render + sb.append("{}"); + } else { + StatementTree stripped = stripUnnecessaryBracesForArrowRhs(caseIr.arrowRhsOptional().get()); + Range printedRange = + Range.closedOpen(getStartPosition(stripped), state.getEndPosition(stripped)); + ImmutableList orphanedComments = + allComments.stream() + // Within this case's source code + .filter( + comment -> + caseIr + .caseSourceCodeRange() + .encloses(buildCommentRange(comment, ifTreeStart))) + // But outside the printed range for the RHS + .filter( + comment -> !buildCommentRange(comment, ifTreeStart).isConnected(printedRange)) + .collect(toImmutableList()); + + boolean needsBraces = needsBracesForArrowRhs(stripped); + if (needsBraces) { + sb.append("{"); + } + + String renderedOrphans = renderComments(orphanedComments); + if (!renderedOrphans.isEmpty()) { + sb.append("\n").append(renderedOrphans).append("\n"); + } + + sb.append(state.getSourceForNode(stripped)); + if (needsBraces) { + sb.append("}"); + } + } + sb.append("\n"); + } + sb.append("}"); + + return sb.toString(); + } + + /** + * Returns a range that encloses the given comment, offset by the given start position of the if + * tree. + */ + private static Range buildCommentRange(ErrorProneComment comment, int ifTreeStart) { + return Range.closedOpen(comment.getPos() + ifTreeStart, comment.getEndPos() + ifTreeStart); + } + + /** Render the supplied comments, separated by newlines. */ + private static String renderComments(ImmutableList comments) { + return comments.stream() + .map(ErrorProneComment::getText) + .filter(commentText -> !commentText.isEmpty()) + .collect(joining("\n")); + } + + /** + * Renders Java source code representation of the supplied {@code Type} that is suitable for use + * in fixes, where any raw types are replaced with wildcard types. For example, `List` becomes + * `List`. + */ + private static String printRawTypesAsWildcards( + Type type, VisitorState state, SuggestedFix.Builder suggestedFixBuilder) { + StringBuilder sb = new StringBuilder(); + List typeParameters = type.tsym.getTypeParameters(); + List typeArguments = type.getTypeArguments(); + Types types = state.getTypes(); + + // Unwrap array-of's + if (types.isArray(type)) { + Type at = type; + StringBuilder suffix = new StringBuilder(); + while (types.isArray(at)) { + suffix.append("[]"); + at = types.elemtype(at); + } + // Primitive types are always in context and don't need qualification + sb.append( + at.isPrimitive() + ? SuggestedFixes.prettyType(at, state) + : SuggestedFixes.qualifyType(state, suggestedFixBuilder, at.tsym)); + sb.append(suffix); + } else { + sb.append(SuggestedFixes.qualifyType(state, suggestedFixBuilder, type.tsym)); + } + + if (!typeParameters.isEmpty()) { + if (typeArguments.isEmpty()) { + sb.append("<"); + sb.repeat("?,", max(0, typeParameters.size() - 1)) + .repeat("?", min(1, typeParameters.size())); + sb.append(">"); + } else { + sb.append("<"); + sb.append( + typeArguments.stream() + .map(t -> SuggestedFixes.prettyType(t, state)) + .collect(joining(","))); + sb.append(">"); + } + } + + return sb.toString(); + } + + /** + * Analyzes the supplied case IRs for a switch statement for issues related default/unconditional + * cases. If deemed necessary, this method injects a `default` case into the supplied case IRs. If + * the supplied case IRs cannot be used to form a syntactically valid switch statement, returns + * `Optional.empty()`. + */ + private static Optional> maybeFixDefaultAndUnconditional( + List cases, + ExpressionTree subject, + StatementTree ifTree, + VisitorState state, + Optional suggestedFixBuilder, + int numberPulledUp, + Set handledEnumValues, + Type switchType, + Range ifTreeSourceRange) { + + boolean hasDefault = cases.stream().anyMatch(CaseIr::hasDefault); + + // Has an unconditional case, meaning that any non-null value of the subject will be matched + long unconditionalCount = + cases.stream() + .filter( + caseIr -> + caseIr.instanceOfOptional().isPresent() + && caseIr.guardOptional().isEmpty() + && isSubtype( + getType(subject), + getType(caseIr.instanceOfOptional().get().type()), + state)) + .count(); + + boolean hasUnconditional = unconditionalCount > 0; + boolean hasMultipleUnconditional = unconditionalCount > 1; + // Has at least once case with a pattern + boolean hasPattern = cases.stream().anyMatch(x -> x.instanceOfOptional().isPresent()); + + boolean allEnumValuesPresent = + isEnum(subject, state) + && handledEnumValues.containsAll(ASTHelpers.enumValues(switchType.asElement())); + + if (hasDefault && hasUnconditional) { + // The explicit default cases conflicts with the unconditional case + return Optional.empty(); + } + + if (hasMultipleUnconditional) { + // Multiple unconditional cases conflicts with each other + return Optional.empty(); + } + + if (hasPattern && !(hasDefault || hasUnconditional)) { + // If there's a pattern, then the switch must be exhaustive, but it's not + return Optional.empty(); + } + + // Although not required by Java itself, Error Prone will generate a finding if there is no + // default for the switch. We conform to that convention here, too. + if (!hasPattern && !hasDefault && !allEnumValuesPresent) { + List casesCopy = new ArrayList<>(cases); + int previousCaseEndIndex = + cases.stream() + .map(CaseIr::caseSourceCodeRange) + .mapToInt(Range::upperEndpoint) + .max() + .orElse(ifTreeSourceRange.lowerEndpoint()); + casesCopy.add( + new CaseIr( + /* hasCaseNull= */ false, + /* hasDefault= */ true, + /* instanceOfOptional= */ Optional.empty(), + /* guardOptional= */ Optional.empty(), + /* expressionsOptional= */ Optional.empty(), + /* arrowRhsOptional= */ Optional.empty(), + /* caseSourceCodeRange= */ Range.closedOpen( + previousCaseEndIndex, previousCaseEndIndex))); + cases = casesCopy; + } + + // Given any possible changes, is the code after the switch reachable? + if (suggestedFixBuilder.isPresent()) { + long emptyRhsBlockCount = + cases.stream().filter(caseIr -> caseIr.arrowRhsOptional().isEmpty()).count(); + long canCompleteNormallyBlockCount = + cases.stream() + .filter( + caseIr -> + caseIr.arrowRhsOptional().isPresent() + && Reachability.canCompleteNormally( + caseIr.arrowRhsOptional().get(), ImmutableMap.of())) + .count(); + + if (emptyRhsBlockCount + canCompleteNormallyBlockCount == 0) { + // All cases cannot complete normally, so we need to do reachability analysis + Tree cannotCompleteNormallyTree = ifTree; + // Search up the AST for enclosing statement blocks, marking any newly-dead code for + // deletion along the way + Tree prev = state.getPath().getLeaf(); + for (Tree tree : state.getPath().getParentPath()) { + if (tree instanceof BlockTree blockTree) { + var statements = blockTree.getStatements(); + int indexInBlock = statements.indexOf(prev); + // A single mock of the immediate child statement block (or switch) is sufficient to + // analyze reachability here; deeper-nested statements are not relevant. + boolean nextStatementReachable = + Reachability.canCompleteNormally( + statements.get(indexInBlock), + ImmutableMap.of(cannotCompleteNormallyTree, false)); + // If we continue to the ancestor statement block, it will be because the end of this + // statement block is not reachable + cannotCompleteNormallyTree = blockTree; + if (nextStatementReachable) { + break; + } + // If a next statement in this block exists, then it is not reachable. + if (indexInBlock + numberPulledUp < statements.size() - 1) { + String deletedRegion = + state + .getSourceCode() + .subSequence( + state.getEndPosition(statements.get(indexInBlock + numberPulledUp)), + state.getEndPosition(blockTree)) + .toString(); + // If the region we would delete looks interesting, bail out and just delete the + // orphaned statements. + if (deletedRegion.contains("LINT.")) { + statements + .subList(indexInBlock + 1, statements.size()) + .forEach(suggestedFixBuilder.get()::delete); + } else { + // If the region doesn't seem to contain interesting comments, delete it along with + // comments: those comments are often just of the form "Unreachable code". + suggestedFixBuilder + .get() + .replace( + state.getEndPosition(statements.get(indexInBlock)), + state.getEndPosition(blockTree), + "}"); + } + } + } + // Only applies in lowest block + numberPulledUp = 0; + + prev = tree; + } + } + } + + return Optional.of(cases); + } + + /** + * Analyzes the supplied case IRs for a switch statement. If deemed likely possible, this method + * pulls up the statement subsequent to the if statement into the switch under a default case, and + * removes it in the {@code SuggestedFix.Builder}. If deemed not likely possible, returns the + * original case IRs. Note that this method does not fully validate the resulting case IRs, but + * rather only partially validates them with respect to pull-up. + */ + private static Optional> maybePullUp( + List cases, + VisitorState state, + IfChainAnalysisState ifChainAnalysisState, + Optional suggestedFixBuilder, + Range ifTreeRange) { + + if (ifChainAnalysisState.subjectOptional().isEmpty()) { + // Nothing to analyze + return Optional.empty(); + } + + List casesCopy = new ArrayList<>(cases); + + if (ifChainAnalysisState.at().getElseStatement() == null + && ifChainAnalysisState.allConditionalBlocksReturnOrThrow()) { + ImmutableList subsequentIfStatements = getSubsequentStatementsInBlock(state); + // If there are, say, ten subsequent statements, pulling up one and leaving the other nine + // wouldn't necessarily be a readability win. + int subsequentStatementsLimit = 1; + if (subsequentIfStatements.size() <= subsequentStatementsLimit) { + for (StatementTree statement : subsequentIfStatements) { + if (hasBreakOrYieldInTree(statement)) { + // Statements containing break or yield cannot be pulled up + break; + } + + int startPos = + casesCopy.isEmpty() + ? ifTreeRange.lowerEndpoint() + : casesCopy.getLast().caseSourceCodeRange().upperEndpoint(); + int endPos = state.getEndPosition(statement); + casesCopy.add( + new CaseIr( + /* hasCaseNull= */ false, + /* hasDefault= */ true, + /* instanceOfOptional= */ Optional.empty(), + /* guardOptional= */ Optional.empty(), + /* expressionsOptional= */ Optional.empty(), + /* arrowRhsOptional= */ Optional.of(statement), + /* caseSourceCodeRange= */ Range.closedOpen(startPos, endPos))); + if (suggestedFixBuilder.isPresent()) { + suggestedFixBuilder.get().delete(statement); + } + } + } + } + return Optional.of(casesCopy); + } + + /** + * Analyzes the supplied case IRs for duplicate constants (either primitives or enum values). If + * any duplicates are found, returns {@code Optional.empty()}. + */ + private static Optional> maybeDetectDuplicateConstants(List cases) { + + Set seenConstants = new HashSet<>(); + + for (CaseIr caseIr : cases) { + if (caseIr.expressionsOptional().isPresent()) { + for (ExpressionTree expression : caseIr.expressionsOptional().get()) { + // Check for compile-time constants + Object constant = constValue(expression); + if (constant != null) { + if (seenConstants.contains(constant)) { + return Optional.empty(); + } + seenConstants.add(constant); + } + + // Check for enums + var sym = ASTHelpers.getSymbol(expression); + if (sym != null && sym.getKind() == ElementKind.ENUM_CONSTANT) { + if (seenConstants.contains(sym)) { + return Optional.empty(); + } + seenConstants.add(sym); + } + } + } + } + + return Optional.of(cases); + } + + /** + * Analyzes the supplied case IRs for dominance violations. If a dominance violation is detected, + * attempts to fix the problem by reordering them so that the violation does not occur. If the + * dominance violation cannot be corrected by this algorithm, returns {@code Optional.empty()}. + */ + private static Optional> maybeFixDominance( + List cases, VisitorState state, ExpressionTree subject) { + + List casesCopy = new ArrayList<>(cases); + List casesToInsert = new ArrayList<>(); + + for (int insert = 0; insert < casesCopy.size(); insert++) { + // Happy path: try to insert at the end, in its original code order (i.e. the order in which + // they were encountered in the if-chain) + casesToInsert.add(casesCopy.get(insert)); + + if (hasDominanceViolation(casesToInsert, state, subject)) { + // Violation found when trying to insert @insert + casesToInsert.remove(casesToInsert.size() - 1); + + // Slow path: try to clean up by moving insertion around. The idea is a bit like an + // insertion sort, although not entirely, since dominance is not transitive, thus it does + // not constitute a partial order + List temp = new ArrayList<>(); + boolean cleanedUp = false; + for (int insertPos = casesToInsert.size() - 1; insertPos >= 0; insertPos--) { + temp.clear(); + int readIndex = 0; + + for (int i = 0; i < insertPos; i++) { + temp.add(casesToInsert.get(readIndex++)); + } + temp.add(casesCopy.get(insert)); + for (; readIndex < casesToInsert.size(); readIndex++) { + temp.add(casesToInsert.get(readIndex)); + } + if (!hasDominanceViolation(temp, state, subject)) { + cleanedUp = true; + // Successfully cleaned up by inserting @insertPos + break; + } + } + + if (cleanedUp) { + casesToInsert = temp; + } else { + // Cleanup failed + return Optional.empty(); + } + } + } + + return Optional.of(casesToInsert); + } + + /** + * Checks whether the supplied case IRs contain a dominance violation, returning {@code true} if + * so. + */ + private static boolean hasDominanceViolation( + List cases, VisitorState state, ExpressionTree subject) { + List casesCopy = new ArrayList<>(cases); + + for (int r = 1; r < casesCopy.size(); r++) { + for (int l = 0; l < r; l++) { + CaseIr caseR = casesCopy.get(r); + CaseIr caseL = casesCopy.get(l); + boolean violation = isDominatedBy(caseL, caseR, state, subject); + if (violation) { + return true; + } + } + } + return false; + } + + /** + * Analyzes the supplied if statement (current level only, no recursion) for the purposes of + * converting it to a switch statement. Returns the analysis state following the analysis of the + * if statement at this level. + */ + private static IfChainAnalysisState analyzeIfStatement( + IfChainAnalysisState ifChainAnalysisState, + ExpressionTree condition, + StatementTree conditionalBlock, + StatementTree elseStatement, + List cases, + VisitorState state, + Range ifTreeRange) { + + if (ifChainAnalysisState.validity().equals(Validity.INVALID)) { + return ifChainAnalysisState; + } + + boolean conditionalBlockCanCompleteNormally = + conditionalBlock == null || Reachability.canCompleteNormally(conditionalBlock); + + // Is the predicate sensible? + Set handledEnumValues = new HashSet<>(ifChainAnalysisState.handledEnumValues()); + int caseStartIndex = + cases.isEmpty() + ? ifTreeRange.lowerEndpoint() + : cases.getLast().caseSourceCodeRange().upperEndpoint(); + Optional newSubjectOptional = + validatePredicateForSubject( + condition, + ifChainAnalysisState.subjectOptional(), + state, + /* mustBeInstanceOf= */ false, + cases, + /* elseOptional= */ Optional.ofNullable(elseStatement), + /* arrowRhsOptional= */ Optional.ofNullable(conditionalBlock), + handledEnumValues, + ifTreeRange, + caseStartIndex); + if (newSubjectOptional.isEmpty()) { + // Not sensible, return invalid state + return new IfChainAnalysisState( + Optional.empty(), + ifChainAnalysisState.depth() + 1, + Validity.INVALID, + ifChainAnalysisState.at(), + ifChainAnalysisState.allConditionalBlocksReturnOrThrow() + && !conditionalBlockCanCompleteNormally, + ImmutableSet.copyOf(handledEnumValues)); + } + + if (!(elseStatement instanceof IfTree elseStatementIfTree)) { + // We reached the final else in the chain (if non-null), or reached the end of the chain and + // there's no else (if null) + return new IfChainAnalysisState( + ifChainAnalysisState.subjectOptional(), + ifChainAnalysisState.depth(), + Validity.VALID, + ifChainAnalysisState.at(), + ifChainAnalysisState.allConditionalBlocksReturnOrThrow() + && !conditionalBlockCanCompleteNormally, + ImmutableSet.copyOf(handledEnumValues)); + } + + // Invariant:The next level of the chain exists + return new IfChainAnalysisState( + newSubjectOptional, + ifChainAnalysisState.depth() + 1, + ifChainAnalysisState.validity(), + elseStatementIfTree, + ifChainAnalysisState.allConditionalBlocksReturnOrThrow() + && !conditionalBlockCanCompleteNormally, + ImmutableSet.copyOf(handledEnumValues)); + } + + private static boolean isEnum(ExpressionTree tree, VisitorState state) { + return isSubtype(getType(tree), state.getSymtab().enumSym.type, state); + } + + /** Determines whether any yield or break statements are present in the tree. */ + private static boolean hasBreakOrYieldInTree(Tree tree) { + Boolean result = + new TreeScanner() { + @Override + public Boolean visitBreak(BreakTree breakTree, Void unused) { + return true; + } + + @Override + public Boolean visitYield(YieldTree continueTree, Void unused) { + return true; + } + + @Override + public Boolean reduce(@Nullable Boolean left, @Nullable Boolean right) { + return Objects.equals(left, true) || Objects.equals(right, true); + } + }.scan(tree, null); + + return result != null && result; + } + + /** + * Validates whether the predicate of the if statement can be converted by this checker to a + * switch, returning the expression being switched on if so. If it cannot be converted by this + * checker, returns {@code Optional.empty()}. Note that this checker's conversion is best-effort, + * so even if this method returns {@code Optional.empty()} the predicate may still be convertible + * by some other (unsupported) means. + */ + private static Optional validatePredicateForSubject( + ExpressionTree predicate, + Optional subject, + VisitorState state, + boolean mustBeInstanceOf, + List cases, + Optional elseOptional, + Optional arrowRhsOptional, + Set handledEnumValues, + Range ifTreeRange, + int caseStartIndex) { + + int caseEndIndex = + elseOptional.isPresent() + ? getStartPosition(elseOptional.get()) + : (arrowRhsOptional.isPresent() + ? state.getEndPosition(arrowRhsOptional.get()) + : state.getEndPosition(predicate)); + + boolean hasElse = elseOptional.isPresent(); + boolean hasElseIf = hasElse && (elseOptional.get() instanceof IfTree); + ExpressionTree at = predicate; + + // Strip any surrounding parentheses e.g. `if(((x == 1)))` + while (at instanceof ParenthesizedTree) { + at = ((ParenthesizedTree) at).getExpression(); + } + + InstanceOfTree instanceOfTree = null; + if (at instanceof InstanceOfTree iot) { + instanceOfTree = iot; + at = iot.getExpression(); + } + + if (at instanceof BinaryTree binaryTree) { + ExpressionTree lhs = binaryTree.getLeftOperand(); + ExpressionTree rhs = binaryTree.getRightOperand(); + boolean predicateIsEquality = binaryTree.getKind().equals(Kind.EQUAL_TO); + boolean predicateIsConditionalAnd = binaryTree.getKind().equals(Kind.CONDITIONAL_AND); + + if (!mustBeInstanceOf && predicateIsEquality) { + // Either lhs or rhs must be a compile-time constant. + if (COMPILE_TIME_CONSTANT_MATCHER.matches(lhs, state) + || COMPILE_TIME_CONSTANT_MATCHER.matches(rhs, state)) { + return validateCompileTimeConstantForSubject( + lhs, + rhs, + subject, + state, + cases, + elseOptional, + arrowRhsOptional, + ifTreeRange, + caseEndIndex, + hasElse, + hasElseIf); + } else { + // Predicate is a binary tree, but neither side is a constant. + if (isEnum(lhs, state) || isEnum(rhs, state)) { + return validateEnumPredicateForSubject( + lhs, + rhs, + subject, + state, + cases, + elseOptional, + arrowRhsOptional, + handledEnumValues, + ifTreeRange, + caseEndIndex, + hasElse, + hasElseIf); + } + + return Optional.empty(); + } + } else if (predicateIsConditionalAnd) { + // Maybe the predicate is something like `a instanceof Foo && predicate`. If so, recurse on + // the left side, and attach the right side of the conditional and as a guard to the + // resulting case. + if (!mustBeInstanceOf && binaryTree.getKind().equals(Kind.CONDITIONAL_AND)) { + int currentCasesSize = cases.size(); + var rv = + validatePredicateForSubject( + binaryTree.getLeftOperand(), + subject, + state, + /* mustBeInstanceOf= */ true, + cases, + elseOptional, + arrowRhsOptional, + handledEnumValues, + ifTreeRange, + /* caseStartIndex= */ caseStartIndex); + if (rv.isPresent()) { + CaseIr oldLastCase = cases.get(currentCasesSize); + // Update last case to attach the guard + cases.set( + currentCasesSize, + new CaseIr( + /* hasCaseNull= */ oldLastCase.hasCaseNull(), + /* hasDefault= */ oldLastCase.hasDefault(), + /* instanceOfOptional= */ oldLastCase.instanceOfOptional(), + /* guardOptional= */ Optional.of(binaryTree.getRightOperand()), + /* expressionsOptional= */ oldLastCase.expressionsOptional(), + /* arrowRhsOptional= */ oldLastCase.arrowRhsOptional(), + /* caseSourceCodeRange= */ oldLastCase.caseSourceCodeRange())); + return rv; + } + } + } + } + + if (instanceOfTree != null) { + return validateInstanceofForSubject( + at, + instanceOfTree, + subject, + state, + cases, + elseOptional, + arrowRhsOptional, + ifTreeRange, + caseEndIndex, + hasElse, + hasElseIf); + } + + // Predicate not a supported style + return Optional.empty(); + } + + private static Optional validateInstanceofForSubject( + ExpressionTree at, + InstanceOfTree instanceOfTree, + Optional subject, + VisitorState state, + List cases, + Optional elseOptional, + Optional arrowRhsOptional, + Range ifTreeRange, + int caseEndIndex, + boolean hasElse, + boolean hasElseIf) { + + ExpressionTree expression = at; + // Does this expression and the subject (if present) refer to the same thing? + if (subject.isPresent() + && !(sameVariable(subject.get(), expression) + || subject.get().equals(expression) + || expressionSourceMatches(subject, expression, state))) { + return Optional.empty(); + } + + if (instanceOfTree.getPattern() instanceof BindingPatternTree bpt) { + boolean addDefault = hasElse && !hasElseIf; + int previousCaseEndIndex = + cases.isEmpty() + ? ifTreeRange.lowerEndpoint() + : cases.getLast().caseSourceCodeRange().upperEndpoint(); + cases.add( + new CaseIr( + /* hasCaseNull= */ false, + /* hasDefault= */ false, + /* instanceOfOptional= */ Optional.of( + new InstanceOfIr( + instanceOfTree.getExpression(), bpt.getVariable(), instanceOfTree.getType())), + /* guardOptional= */ Optional.empty(), + /* expressionsOptional= */ Optional.empty(), + /* arrowRhsOptional= */ arrowRhsOptional, + /* caseSourceCodeRange= */ Range.closedOpen(previousCaseEndIndex, caseEndIndex))); + + if (addDefault) { + previousCaseEndIndex = + cases.isEmpty() + ? ifTreeRange.lowerEndpoint() + : cases.getLast().caseSourceCodeRange().upperEndpoint(); + cases.add( + new CaseIr( + /* hasCaseNull= */ false, + /* hasDefault= */ true, + /* instanceOfOptional= */ Optional.empty(), + /* guardOptional= */ Optional.empty(), + /* expressionsOptional= */ Optional.empty(), + /* arrowRhsOptional= */ elseOptional, + /* caseSourceCodeRange= */ Range.closedOpen( + caseEndIndex, + elseOptional.isPresent() + ? getStartPosition(elseOptional.get()) + : caseEndIndex))); + } + } else if (instanceOfTree.getType() != null) { + boolean addDefault = hasElse && !hasElseIf; + int previousCaseEndIndex = + cases.isEmpty() + ? ifTreeRange.lowerEndpoint() + : cases.getLast().caseSourceCodeRange().upperEndpoint(); + cases.add( + new CaseIr( + /* hasCaseNull= */ false, + /* hasDefault= */ false, + /* instanceOfOptional= */ Optional.of( + new InstanceOfIr(instanceOfTree.getExpression(), null, instanceOfTree.getType())), + /* guardOptional= */ Optional.empty(), + /* expressionsOptional= */ Optional.empty(), + /* arrowRhsOptional= */ arrowRhsOptional, + /* caseSourceCodeRange= */ Range.closedOpen(previousCaseEndIndex, caseEndIndex))); + if (addDefault) { + cases.add( + new CaseIr( + /* hasCaseNull= */ false, + /* hasDefault= */ true, + /* instanceOfOptional= */ Optional.empty(), + /* guardOptional= */ Optional.empty(), + /* expressionsOptional= */ Optional.empty(), + /* arrowRhsOptional= */ elseOptional, + /* caseSourceCodeRange= */ Range.closedOpen( + caseEndIndex, + elseOptional.isPresent() + ? state.getEndPosition(elseOptional.get()) + : caseEndIndex))); + } + } else { + // Neither a binding pattern tree nor a type (possibly a record); unsupported + return Optional.empty(); + } + + return Optional.of(expression); + } + + private static Optional validateCompileTimeConstantForSubject( + ExpressionTree lhs, + ExpressionTree rhs, + Optional subject, + VisitorState state, + List cases, + Optional elseOptional, + Optional arrowRhsOptional, + Range ifTreeRange, + int caseEndIndex, + boolean hasElse, + boolean hasElseIf) { + boolean compileTimeConstantOnLhs = COMPILE_TIME_CONSTANT_MATCHER.matches(lhs, state); + ExpressionTree testExpression = compileTimeConstantOnLhs ? rhs : lhs; + ExpressionTree compileTimeConstant = compileTimeConstantOnLhs ? lhs : rhs; + + if (subject.isPresent() + && !(sameVariable(subject.get(), testExpression) + || subject.get().equals(testExpression) + || expressionSourceMatches(subject, testExpression, state))) { + // Predicate not compatible with predicate of preceding if statement + return Optional.empty(); + } + + // Don't support the use of Booleans as switch conditions + if (isSubtype(getType(testExpression), Suppliers.JAVA_LANG_BOOLEAN_TYPE.get(state), state)) { + return Optional.empty(); + } + + // Don't support the use of String as switch conditions + if (isSubtype(getType(testExpression), state.getSymtab().stringType, state)) { + return Optional.empty(); + } + + // Switching on primitive long requires later Java version (we don't currently support) + if (state.getTypes().isSameType(getType(testExpression), state.getSymtab().longType)) { + return Optional.empty(); + } + + boolean addDefault = hasElse && !hasElseIf; + int previousCaseEndIndex = + cases.isEmpty() + ? ifTreeRange.lowerEndpoint() + : cases.getLast().caseSourceCodeRange().upperEndpoint(); + cases.add( + new CaseIr( + /* hasCaseNull= */ compileTimeConstant.getKind() == Kind.NULL_LITERAL, + /* hasDefault= */ false, + /* instanceOfOptional= */ Optional.empty(), + /* guardOptional= */ Optional.empty(), + /* expressionsOptional= */ Optional.of(ImmutableList.of(compileTimeConstant)), + /* arrowRhsOptional= */ arrowRhsOptional, + /* caseSourceCodeRange= */ Range.openClosed(previousCaseEndIndex, caseEndIndex))); + if (addDefault) { + previousCaseEndIndex = + cases.isEmpty() + ? ifTreeRange.lowerEndpoint() + : cases.getLast().caseSourceCodeRange().upperEndpoint(); + cases.add( + new CaseIr( + /* hasCaseNull= */ false, + /* hasDefault= */ true, + /* instanceOfOptional= */ Optional.empty(), + /* guardOptional= */ Optional.empty(), + /* expressionsOptional= */ Optional.empty(), + /* arrowRhsOptional= */ elseOptional, + /* caseSourceCodeRange= */ Range.openClosed( + previousCaseEndIndex, + elseOptional.isPresent() ? getStartPosition(elseOptional.get()) : caseEndIndex))); + } + + return Optional.of(testExpression); + } + + private static Optional validateEnumPredicateForSubject( + ExpressionTree lhs, + ExpressionTree rhs, + Optional subject, + VisitorState state, + List cases, + Optional elseOptional, + Optional arrowRhsOptional, + Set handledEnumValues, + Range ifTreeRange, + int caseEndIndex, + boolean hasElse, + boolean hasElseIf) { + boolean lhsIsEnumConstant = + isEnum(lhs, state) + && (ASTHelpers.getSymbol(lhs) != null) + && (ASTHelpers.getSymbol(lhs).getKind() == ElementKind.ENUM_CONSTANT); + boolean rhsIsEnumConstant = + isEnum(rhs, state) + && (ASTHelpers.getSymbol(rhs) != null) + && (ASTHelpers.getSymbol(rhs).getKind() == ElementKind.ENUM_CONSTANT); + + if (lhsIsEnumConstant && rhsIsEnumConstant) { + // Comparing enum const to enum const, cannot convert + return Optional.empty(); + } + + if (!lhsIsEnumConstant && !rhsIsEnumConstant) { + // Can't find an enum const, cannot convert + return Optional.empty(); + } + + // Invariant: exactly one of lhs or rhs is an enum constant. + ExpressionTree compileTimeConstant = lhsIsEnumConstant ? lhs : rhs; + ExpressionTree testExpression = lhsIsEnumConstant ? rhs : lhs; + + if (subject.isPresent() + && !(sameVariable(subject.get(), testExpression) + || subject.get().equals(testExpression) + || expressionSourceMatches(subject, testExpression, state))) { + return Optional.empty(); + } + + // Record this enum const as handled + handledEnumValues.addAll( + Stream.of(compileTimeConstant) + .map(ASTHelpers::getSymbol) + .filter(x -> x != null) + .map(symbol -> symbol.getSimpleName().toString()) + .collect(toImmutableSet())); + + boolean addDefault = hasElse && !hasElseIf; + int previousCaseEndIndex = + cases.isEmpty() + ? ifTreeRange.lowerEndpoint() + : cases.getLast().caseSourceCodeRange().upperEndpoint(); + cases.add( + new CaseIr( + /* hasCaseNull= */ false, + /* hasDefault= */ false, + /* instanceOfOptional= */ Optional.empty(), + /* guardOptional= */ Optional.empty(), + /* expressionsOptional= */ Optional.of(ImmutableList.of(compileTimeConstant)), + /* arrowRhsOptional= */ arrowRhsOptional, + /* caseSourceCodeRange= */ Range.closedOpen(previousCaseEndIndex, caseEndIndex))); + + if (addDefault) { + previousCaseEndIndex = + cases.isEmpty() + ? ifTreeRange.lowerEndpoint() + : cases.getLast().caseSourceCodeRange().upperEndpoint(); + cases.add( + new CaseIr( + /* hasCaseNull= */ false, + /* hasDefault= */ true, + /* instanceOfOptional= */ Optional.empty(), + /* guardOptional= */ Optional.empty(), + /* expressionsOptional= */ Optional.empty(), + /* arrowRhsOptional= */ elseOptional, + /* caseSourceCodeRange= */ Range.closedOpen( + caseEndIndex, + elseOptional.isPresent() + ? state.getEndPosition(elseOptional.get()) + : caseEndIndex))); + } + return Optional.of(testExpression); + } + + /** + * Determines whether the supplied expression and subject (if present) have the same Java source + * code. Note that this is just a textual comparison, and does not consider code structure, + * comments, etc. + */ + private static boolean expressionSourceMatches( + Optional subject, ExpressionTree expression, VisitorState state) { + + return subject.isPresent() + && state.getSourceForNode(subject.get()).equals(state.getSourceForNode(expression)); + } + + /** Retrieves a list of all statements (if any) following the current path, if any. */ + private static ImmutableList getSubsequentStatementsInBlock(VisitorState state) { + TreePath path = state.getPath(); + if (!(path.getParentPath().getLeaf() instanceof BlockTree blockTree)) { + return ImmutableList.of(); + } + var statements = blockTree.getStatements(); + return ImmutableList.copyOf( + statements.subList(statements.indexOf(path.getLeaf()) + 1, statements.size())); + } + + /** + * Unboxes the given type, if it is a reference type which can be unboxed. Returns {@code + * Optional.empty()} if cannot be unboxed. + */ + private static Optional unboxed(Tree tree, VisitorState state) { + Type type = ASTHelpers.getType(tree); + if (type == null || !type.isReference()) { + return Optional.empty(); + } + Type unboxed = state.getTypes().unboxedType(type); + if (unboxed == null + || unboxed.getTag() == TypeTag.NONE + // Don't match java.lang.Void. + || unboxed.getTag() == TypeTag.VOID) { + return Optional.empty(); + } + return Optional.of(unboxed); + } + + /** + * Finds the intersection of two types, or {@code null} if there is no such intersection. This is + * not quite the same thing as the "Intersection Types" defined JLS 21 § 4.9 (it is not a distinct + * type; there is no {@code IntersectionTypeTree}) although they are similar in that the + * (non-null) return value can be assigned to both types. + */ + public static @Nullable Type intersectTypes(Type type1, Type type2, VisitorState state) { + com.sun.tools.javac.code.Type lower = null; + if (isSubtype(type1, type2, state)) { + lower = type1; + } + + if (isSubtype(type2, type1, state)) { + lower = type2; + } + + return lower; + } + + /** Performs a detailed analysis of the if-chain, generating suggested fixes as needed. */ + private static List deepAnalysisOfIfChain( + List cases, + IfChainAnalysisState finalIfChainAnalysisState, + IfTree ifTree, + VisitorState state, + Range ifTreeSourceRange, + List suggestedFixes) { + + // Wrapping break/yield in a switch can potentially change its semantics. A deeper analysis of + // whether semantics are preserved is not attempted here + if (hasBreakOrYieldInTree(ifTree)) { + return new ArrayList<>(); + } + + ExpressionTree subject = finalIfChainAnalysisState.subjectOptional().get(); + + SuggestedFix.Builder suggestedFixBuilderWithPullupEnabled = SuggestedFix.builder(); + Optional> casesWithPullupMaybeApplied = + Optional.of(cases) + .flatMap( + caseList -> + maybePullUp( + caseList, + state, + finalIfChainAnalysisState, + Optional.of(suggestedFixBuilderWithPullupEnabled), + ifTreeSourceRange)); + + int numberPulledUp = + casesWithPullupMaybeApplied.isEmpty() + ? 0 + : (casesWithPullupMaybeApplied.get().size() - cases.size()); + Type switchType = getType(finalIfChainAnalysisState.subjectOptional().get()); + + Optional> fixedCasesOptional = + casesWithPullupMaybeApplied + .flatMap(caseList -> maybeDetectDuplicateConstants(caseList)) + .flatMap(caseList -> maybeFixDominance(caseList, state, subject)) + .flatMap( + x -> + maybeFixDefaultAndUnconditional( + x, + subject, + ifTree, + state, + Optional.of(suggestedFixBuilderWithPullupEnabled), + numberPulledUp, + finalIfChainAnalysisState.handledEnumValues(), + switchType, + ifTreeSourceRange)); + + SuggestedFix.Builder suggestedFixBuilderWithoutPullup = SuggestedFix.builder(); + boolean pullupDisabled = false; + if (fixedCasesOptional.isEmpty()) { + // Try again with pull-up disabled + pullupDisabled = true; + fixedCasesOptional = + Optional.of(cases) + .flatMap(caseList -> maybeDetectDuplicateConstants(caseList)) + .flatMap(caseList -> maybeFixDominance(caseList, state, subject)) + .flatMap( + caseList -> + maybeFixDefaultAndUnconditional( + caseList, + subject, + ifTree, + state, + Optional.of(suggestedFixBuilderWithoutPullup), + /* numberPulledUp= */ 0, + finalIfChainAnalysisState.handledEnumValues(), + switchType, + ifTreeSourceRange)); + } + + return maybeBuildAndAddSuggestedFix( + fixedCasesOptional, + pullupDisabled ? suggestedFixBuilderWithoutPullup : suggestedFixBuilderWithPullupEnabled, + finalIfChainAnalysisState, + ifTree, + state, + ifTreeSourceRange, + suggestedFixes); + } + + /** + * If a finding is available, build a {@code SuggestedFix} for it and add to the suggested fixes. + */ + private static List maybeBuildAndAddSuggestedFix( + Optional> fixedCasesOptional, + SuggestedFix.Builder suggestedFixBuilder, + IfChainAnalysisState ifChainAnalysisState, + IfTree ifTree, + VisitorState state, + Range ifTreeSourceRange, + List suggestedFixes) { + + if (fixedCasesOptional.isPresent()) { + List fixedCases = fixedCasesOptional.get(); + ImmutableList allComments = + state.getTokensForNode(ifTree).stream() + .flatMap(errorProneToken -> errorProneToken.comments().stream()) + .collect(toImmutableList()); + + suggestedFixBuilder.replace( + ifTree, + prettyPrint( + fixedCases, + ifChainAnalysisState.subjectOptional().get(), + state, + suggestedFixBuilder, + ifTreeSourceRange, + allComments)); + + // Defensive copy + List suggestedFixesCopy = new ArrayList<>(suggestedFixes); + suggestedFixesCopy.add(suggestedFixBuilder.build()); + suggestedFixes = suggestedFixesCopy; + } + + return suggestedFixes; + } + + /** + * Compute whether the RHS is dominated by the LHS. + * + *

Domination refers to the notion of "is dominated" defined in e.g. JLS 21 § 14.11.1. Note + * that this method does not support record types, which simplifies implementation. + */ + public static boolean isDominatedBy( + CaseIr lhs, CaseIr rhs, VisitorState state, ExpressionTree subject) { + + // Nothing dominates the default case + if (rhs.hasDefault()) { + return false; + } + + // Only default dominates case null + if (rhs.hasCaseNull()) { + return lhs.hasDefault(); + } + + // Constants are dominated by patterns that can match them + if (rhs.instanceOfOptional().isEmpty()) { + for (ExpressionTree constantExpression : rhs.expressionsOptional().get()) { + // A "constant expression" (e.g. JLS 21 § 15.29) can be a primitive type or a String. + // However, constant expressions as used here refer to the usage as in e.g. + // ConstantCaseLabelTree, which can include enum values. + boolean isPrimitive = getType(constantExpression).isPrimitive(); + if (isPrimitive) { + // Guarded patterns cannot dominate primitives + if (lhs.guardOptional().isPresent()) { + continue; + } + if (lhs.instanceOfOptional().isPresent()) { + InstanceOfIr instanceOfIr = lhs.instanceOfOptional().get(); + if (instanceOfIr.type() != null) { + Optional unboxedInstanceOfType = unboxed(instanceOfIr.type(), state); + if (unboxedInstanceOfType.isPresent()) { + if (isSubtype(getType(constantExpression), unboxedInstanceOfType.get(), state)) { + // RHS constant can be assigned to LHS unboxed instanceof's type + return true; + } + } else { + // Cannot unbox LHS pattern, so RHS primitive constant should come before it + return true; + } + } else if (instanceOfIr.patternVariable() != null) { + VariableTree patternVariable = instanceOfIr.patternVariable(); + Type patternType = getType(patternVariable); + if (isSubtype(getType(constantExpression), patternType, state)) { + // RHS constant can be assigned to LHS pattern + return true; + } + } + continue; + } else { + // LHS must be a constant; constants don't dominate other constants. + continue; + } + } + boolean isEnum = isEnum(constantExpression, state); + if (isEnum) { + if (lhs.guardOptional().isPresent()) { + // Guarded patterns cannot dominate enum values + continue; + } + + if (lhs.instanceOfOptional().isPresent()) { + InstanceOfIr instanceOfIr = lhs.instanceOfOptional().get(); + if (isSubtype(getType(constantExpression), getType(instanceOfIr.type()), state)) { + // RHS enum value can be assigned to LHS instanceof's type + return true; + } + } else { + // LHS must be a constant + continue; + } + continue; + } + + // RHS must be a reference + // The rhs-reference code would be needed to support e.g. String literals. It is included + // for completeness. + if (lhs.guardOptional().isPresent()) { + // Guarded patterns cannot dominate references + continue; + } + + if (lhs.instanceOfOptional().isPresent()) { + InstanceOfIr instanceOfIr = lhs.instanceOfOptional().get(); + Type subjectType = getType(subject); + if (instanceOfIr.type() != null) { + var instanceOfType = instanceOfIr.type(); + if (isSubtype( + intersectTypes(getType(constantExpression), subjectType, state), + intersectTypes(getType(instanceOfType), subjectType, state), + state)) { + return true; + } + } + } else { + // LHS must be a constant + } + } // for loop + return false; + } + + // The RHS must be a pattern + if (lhs.hasDefault() || lhs.hasCaseNull()) { + // LHS has a default or case null, which dominates the RHS + return true; + } + + // RHS must be a pattern + if (lhs.guardOptional().isPresent()) { + // LHS has a guard, so cannot dominate RHS + return false; + } + + // Is LHS a pattern? + if (lhs.instanceOfOptional().isPresent()) { + Type lhsType = + lhs.instanceOfOptional().get().type() != null + ? getType(lhs.instanceOfOptional().get().type()) + : getType(lhs.instanceOfOptional().get().patternVariable().getType()); + var rhsInstanceOf = rhs.instanceOfOptional().get(); + Type rhsType = + rhsInstanceOf.type() != null + ? getType(rhsInstanceOf.type()) + : getType(rhsInstanceOf.patternVariable().getType()); + if (isSubtype(rhsType, lhsType, state)) { + // The LHS type is a subtype of the RHS type, so the LHS dominates the RHS + return true; + } + } + + // RHS is a pattern; LHS constant cannot dominate this pattern + return false; + } + + /** + * This record is an intermediate representation of a single `x instanceof Y` or `x instanceof Y + * y` expression. + */ + record InstanceOfIr( + // In the example above, the expression would be `y`. + @Nullable ExpressionTree expression, + // In the example above, the variable tree would be `Y y`. + @Nullable VariableTree patternVariable, + // In the example above, the type would be `Y`. + Tree type) { + + InstanceOfIr { + checkArgument(type != null); + } + } + + /** + * This record is an intermediate representation of a single case in a switch statement that is + * being synthesized. Its scope is roughly equivalent to a `CaseTree` in Java's AST, although does + * not cover all of the same functionality. + */ + record CaseIr( + boolean hasCaseNull, + boolean hasDefault, + // The pattern, if any + Optional instanceOfOptional, + // The guard predicate, if any + Optional guardOptional, + // Constants appearing before the arrow in the case + Optional> expressionsOptional, + // Code appearing after the arrow in the case + Optional arrowRhsOptional, + // Range of source code covered by the case + Range caseSourceCodeRange) { + + CaseIr { + checkArgument( + hasCaseNull + || hasDefault + || instanceOfOptional.isPresent() + || (expressionsOptional.isPresent() && !expressionsOptional.get().isEmpty()), + "CaseIr must have at least one of case null, default, instanceof, or expressions"); + checkArgument( + !(hasDefault && (instanceOfOptional.isPresent() || expressionsOptional.isPresent())), + "Default and instanceof/expressions cannot both be present"); + } + } + + /** This record represents the current state of the analysis of an if-chain. */ + record IfChainAnalysisState( + // The expression to be switched on (if known) + Optional subjectOptional, + // Depth of the if statement being analyzed (relative to the start of the if-chain) + int depth, + // Whether the if-chain is eligible for conversion (so far) + Validity validity, + // Current if statement being analyzed in the if-chain + IfTree at, + // Do all RHS blocks complete abruptly (so far)? + boolean allConditionalBlocksReturnOrThrow, + // All enum values seen so far (in condition expressions) + ImmutableSet handledEnumValues) {} +} 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 908c715c68d..bd364f65181 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -174,6 +174,7 @@ import com.google.errorprone.bugpatterns.IdentityBinaryExpression; import com.google.errorprone.bugpatterns.IdentityHashMapBoxing; import com.google.errorprone.bugpatterns.IdentityHashMapUsage; +import com.google.errorprone.bugpatterns.IfChainToSwitch; import com.google.errorprone.bugpatterns.IgnoredPureGetter; import com.google.errorprone.bugpatterns.ImmutableMemberCollection; import com.google.errorprone.bugpatterns.ImmutableSetForContains; @@ -978,6 +979,7 @@ public static ScannerSupplier warningChecks() { HidingField.class, ICCProfileGetInstance.class, IdentityHashMapUsage.class, + IfChainToSwitch.class, IgnoredPureGetter.class, ImmutableAnnotationChecker.class, ImmutableEnumChecker.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java new file mode 100644 index 00000000000..baa80c03800 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java @@ -0,0 +1,1992 @@ +/* + * Copyright 2025 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.truth.Truth.assertThat; +import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.fixes.Fix; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link StatementSwitchToExpressionSwitch}. */ +@RunWith(JUnit4.class) +public final class IfChainToSwitchTest { + private static final String SUIT = + """ + enum Suit { + HEART, + SPADE, + DIAMOND, + CLUB + }; + """; + private final CompilationTestHelper helper = + CompilationTestHelper.newInstance(IfChainToSwitch.class, getClass()) + .addSourceLines("Suit.java", SUIT); + private final BugCheckerRefactoringTestHelper refactoringHelper = + BugCheckerRefactoringTestHelper.newInstance(IfChainToSwitch.class, getClass()) + .addInputLines("Suit.java", SUIT) + .expectUnchanged(); + + @Test + public void ifChain_nameInvariance_error() { + // Different ways of referencing the same symbol should not matter + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = s; + System.out.println("yo"); + if (this.suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit instanceof Number) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Number unused -> System.out.println("It's a number!"); + case Suit unused -> System.out.println("It's a Suit!"); + default -> throw new AssertionError(); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_removesTrailing_error() { + // Removal of unreachable code after the final branch + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = s; + System.out.println("yo"); + if (this.suit instanceof String) { + throw new RuntimeException("It's a string!"); + } else if (suit instanceof Number) { + return; + } else if (suit instanceof Object) { + throw new RuntimeException("It's an object!"); + } + // Farewell to this comment + System.out.println("Delete me"); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> throw new RuntimeException("It's a string!"); + case Number unused -> { + return; + } + case Object unused -> throw new RuntimeException("It's an object!"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_removesTrailingButKeepsInterestingComments_error() { + // Interesting comments should be preserved + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = s; + System.out.println("yo"); + if (this.suit instanceof String) { + throw new RuntimeException("It's a string!"); + } else if (suit instanceof Number) { + return; + } else if (suit instanceof Object) { + throw new RuntimeException("It's an object!"); + } + // LINT.Something(...) + + System.out.println("Delete me"); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> throw new RuntimeException("It's a string!"); + case Number unused -> { + return; + } + case Object unused -> throw new RuntimeException("It's an object!"); + } + // LINT.Something(...) + + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_booleanSwitch_noError() { + // Boolean switch is not supported by this tool + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Boolean b = s == null; + if (b == true) { + System.out.println("It's true"); + } else if (b == false) { + System.out.println("It's false"); + } else if (b instanceof Boolean boo) { + throw new RuntimeException("It's a boolean that's neither true nor false"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_longSwitch_noError() { + // Switch on long is not supported by this tool + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + long l = s == null ? 1 : 2; + if (l == 23l) { + System.out.println("It's 23"); + } else if (l == 45l) { + System.out.println("It's 45"); + } else if (l == 67l) { + System.out.println("It's neither 23 nor 45"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_intSwitch_error() { + // Switch on int is supported + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + int i = s == null ? 1 : 2; + if (i == 23) { + System.out.println("It's 23"); + } else if (i == 45) { + System.out.println("It's 45"); + } else if (i == 67) { + System.out.println("It's 67"); + } else { + System.out.println("None of the above"); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + int i = s == null ? 1 : 2; + switch (i) { + case 23 -> System.out.println("It's 23"); + case 45 -> System.out.println("It's 45"); + case 67 -> System.out.println("It's 67"); + default -> System.out.println("None of the above"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_dontAlwaysPullUp_error() { + // Pull up should not occur if it would result in a conflict + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = null; + System.out.println("yo"); + if (this.suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit instanceof Number) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else if (this.suit instanceof Object o) { + System.out.println("It's an object!"); + } + throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = null; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Number unused -> System.out.println("It's a number!"); + case Suit unused -> System.out.println("It's a Suit!"); + case Object o -> System.out.println("It's an object!"); + } + throw new AssertionError(); + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_nonexhausivePattern_noError() { + // Pulling up the throw would change semantics + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = null; + System.out.println("yo"); + if (this.suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit instanceof Number) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } + throw new AssertionError(); + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_twoLinesAfterDefault_error() { + // Convert but don't delete following lines if they remain reachable + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = s; + System.out.println("yo"); + if (this.suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit instanceof Number) { + System.out.println("It's a number!"); + } else if (suit instanceof Object) { + System.out.println("It's an object!"); + } + System.out.println("Don't delete me!"); + System.out.println("Don't delete me either!"); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Number unused -> System.out.println("It's a number!"); + case Object unused -> System.out.println("It's an object!"); + } + System.out.println("Don't delete me!"); + System.out.println("Don't delete me either!"); + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_bindingPatternTree_error() { + // Note `instanceof Number n`, otherwise same as ifChain_twoLinesAfterDefault_error + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = s; + System.out.println("yo"); + if (this.suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit instanceof Number n) { + System.out.println("It's a number!"); + } else if (suit instanceof Object) { + System.out.println("It's an object!"); + } + System.out.println("Don't delete me!"); + System.out.println("Don't delete me either!"); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Number n -> System.out.println("It's a number!"); + case Object unused -> System.out.println("It's an object!"); + } + System.out.println("Don't delete me!"); + System.out.println("Don't delete me either!"); + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_removesParens_error() { + // Removes redundant parens around if condition + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if ((suit instanceof Number)) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Number unused -> System.out.println("It's a number!"); + case Suit unused -> System.out.println("It's a Suit!"); + default -> throw new AssertionError(); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_equality_error() { + // Enum equality + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit == Suit.DIAMOND) { + System.out.println("It's a diamond!"); + } else if ((suit instanceof Number)) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Suit.DIAMOND -> System.out.println("It's a diamond!"); + case Number unused -> System.out.println("It's a number!"); + case Suit unused -> System.out.println("It's a Suit!"); + default -> throw new AssertionError(); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_exhaustiveEnum_error() { + // Enum equality + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + public void foo(Suit suit) { + if (suit == Suit.CLUB) { + System.out.println("It's a club!"); + } else if (suit == Suit.DIAMOND) { + System.out.println("It's a diamond!"); + } else if ((suit == Suit.HEART)) { + System.out.println("It's a heart!"); + } else { // c1 + { + System.out.println("It's a diamond!"); + } + } + } + } + """) + .addOutputLines( + "Test.java", +""" +class Test { + public void foo(Suit suit) { + switch (suit) { + case Suit.CLUB -> System.out.println("It's a club!"); + case Suit.DIAMOND -> System.out.println("It's a diamond!"); + case Suit.HEART -> System.out.println("It's a heart!"); + default -> + // c1 + System.out.println("It's a diamond!"); + } + } +} +""") + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_equalityNull_error() { + // Equality with null + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit == null) { + System.out.println("It's a diamond!"); + } else if ((suit instanceof Number)) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Number unused -> System.out.println("It's a number!"); + case Suit unused -> System.out.println("It's a Suit!"); + case null -> System.out.println("It's a diamond!"); + default -> throw new AssertionError(); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_equalityNullAndNoExplicitDefault_error() { + // Equality with null, and default must be synthesized + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Integer i = s == null ? 1 : 2; + if (i == 1) { + System.out.println("s is null"); + } else if (i == null) { + System.out.println("a mystery"); + } else if (i == 2) { + System.out.println("s is non-null"); + } else if (i == 3) { + System.out.println("another mystery"); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Integer i = s == null ? 1 : 2; + switch (i) { + case 1 -> System.out.println("s is null"); + case null -> System.out.println("a mystery"); + case 2 -> System.out.println("s is non-null"); + case 3 -> System.out.println("another mystery"); + default -> {} + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_hashCode_noError() { + // Switch on the result of a method; here `hashCode`. + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + Object suit2 = s; + System.out.println("yo"); + if (suit.hashCode() == 213) { + System.out.println("It's a string!"); + } else if (suit.hashCode() == 456) { + System.out.println("It's a diamond!"); + } else if (suit.hashCode() == 789) { + System.out.println("It's a diamond!"); + } + System.out.println("Don't delete me!"); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + Object suit2 = s; + System.out.println("yo"); + switch (suit.hashCode()) { + case 213 -> System.out.println("It's a string!"); + case 456 -> System.out.println("It's a diamond!"); + case 789 -> System.out.println("It's a diamond!"); + default -> {} + } + System.out.println("Don't delete me!"); + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_returnPullUp_error() { + // The last case can complete normally, so the return cannot be pulled up. + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + public int foo(Suit s) { + int i = s == null ? 1 : 2; + if (i == 1) { + return 1; + } else if (i == 2) { + return 2; + } else if (i == 3) { + if (i > 33) { + return 33; + } + } + return -1; + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + public int foo(Suit s) { + int i = s == null ? 1 : 2; + switch (i) { + case 1 -> { + return 1; + } + case 2 -> { + return 2; + } + case 3 -> { + if (i > 33) { + return 33; + } + } + default -> {} + } + return -1; + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_proto_error() { + // Switch on the result of a method; here a proto getter. + helper + .addSourceLines( + "com/google/protobuf/GeneratedMessage.java", + """ + package com.google.protobuf; + + public class GeneratedMessage {} + """) + .addSourceLines( + "Proto.java", + """ + public abstract class Proto extends com.google.protobuf.GeneratedMessage { + public abstract Long getMessage(); + } + """) + .addSourceLines( + "Test.java", + """ + class Test { + boolean g(Proto proto, Suit s) { + Object suit = s; + Object suit2 = s; + System.out.println("yo"); + // BUG: Diagnostic contains: + if (proto.getMessage() == 1l) { + System.out.println("It's red"); + } else if (proto.getMessage() == 2l) { + System.out.println("It's yellow"); + } else if (proto.getMessage() == 3l) { + System.out.println("It's red"); + } else throw new AssertionError(); + return true; + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_string_noError() { + // Comparing strings with == is probably not what is intended + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit suit) { + String s = suit == null ? "null" : "nonnull"; + if (s == "null") { + System.out.println("iffy way to compare strings"); + } else if (s == "nonnull") { + System.out.println("also this"); + } else if (s == "something else") { + System.out.println("unlikely"); + } else { + System.out.println("Probably landing here"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_legalDuplicate_error() { + // Although the guard effectively duplicates the diamond constant case, this construction is + // legal + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + if (s == Suit.DIAMOND) { + System.out.println("Diamond"); + } else if (s instanceof Suit r) { + System.out.println("It's some black suit"); + } else if (s == Suit.HEART) { + System.out.println("Heart"); + } else if (s instanceof Suit ss && ss == Suit.DIAMOND) { + System.out.println("Technically allowed"); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + switch (s) { + case Suit.DIAMOND -> System.out.println("Diamond"); + case Suit.HEART -> System.out.println("Heart"); + case Suit ss when ss == Suit.DIAMOND -> System.out.println("Technically allowed"); + case Suit r -> System.out.println("It's some black suit"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_dupeEnum_noError() { + // Duplicate enum + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + if (s == Suit.DIAMOND) { + System.out.println("Diamond"); + } else if (s instanceof Suit r) { + System.out.println("It's some black suit"); + } else if (Suit.HEART == s) { + System.out.println("Hearts"); + } else if (Suit.DIAMOND == s) { + System.out.println("Uh oh, diamond again"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_loopContext_noError() { + // Wrapping in a switch will change the meaning of the break + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + while (true) { + if (s == Suit.DIAMOND) { + System.out.println("Diamond"); + } else if (s instanceof Suit r) { + System.out.println("It's some black suit"); + break; + } else if (Suit.HEART == s) { + System.out.println("Hearts"); + } else if (Suit.SPADE == s) { + System.out.println("Spade"); + } + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_loopContextYield_noError() { + // Putting a switch in another switch would change the meaning of the `yield` + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + int q = + switch (s) { + default -> { + if (s == Suit.DIAMOND) { + throw new AssertionError("Diamond"); + } else if (s instanceof Suit r) { + throw new AssertionError("Club"); + } else if (Suit.HEART == s) { + throw new AssertionError("Heart"); + } else if (Suit.SPADE == s) { + System.out.println("Spade"); + yield 4; + } + throw new AssertionError(); + } + }; + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_loopContextPullUp_noError() { + // Can't pull up break into switch context + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + while (true) { + if (s == Suit.DIAMOND) { + System.out.println("Diamond"); + } else if (s instanceof Suit r) { + System.out.println("It's some black suit"); + break; + } else if (Suit.HEART == s) { + System.out.println("Hearts"); + } else if (Suit.SPADE == s) { + System.out.println("Spade"); + } + break; + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_dupeConstant_noError() { + // Duplicate int constant + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Integer i = s == null ? 1 : 2; + if (i == 1) { + System.out.println("1"); + } else if (i == 2) { + System.out.println("2"); + } else if (i instanceof Integer) { + System.out.println("Integer"); + } else if (1 == i) { + System.out.println("Uh oh, 1 again"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_record_noError() { + // Don't attempt to convert record types (unsupported) + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + + record Person(String name, int age) {} + + public void foo(Object o) { + if (o instanceof Person(String name, int age)) { + System.out.println("generic person"); + } else if (o instanceof Person(String name, int age) && name.equals("alice") && age == 20) { + System.out.println("alice20"); + } else if (o instanceof Person(String name, int age) && name.equals("bob") && age == 21) { + System.out.println("bob21"); + } else if (o instanceof Object obj) { + System.out.println("make exhaustive"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_nonVarSubject_noError() { + // Should not merely compare method name (e.g. `hashCode`) + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + Object suit2 = s; + System.out.println("yo"); + if (suit.hashCode() == 13242) { + System.out.println("It's a string!"); + } else if (suit2.hashCode() == 23) { + System.out.println("Weird"); + } else throw new AssertionError(); + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_equalityAlone_noError() { + // Can't use guard with equality + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit.hashCode() == 13242 && suit.hashCode() > 0) { + System.out.println("It's a string!"); + } else if (suit.hashCode() == 23) { + System.out.println("Weird"); + } else throw new AssertionError(); + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_nested_error() { + // Only report on the outermost if-chain + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + import java.util.Optional; + import java.lang.Integer; + import java.lang.Long; + import java.math.BigDecimal; + import java.time.Duration; + import java.time.Instant; + import java.util.Date; + + class Test { + public void foo(Suit s) { + Object[] parts = {s, s}; + for (Object o : parts) { + Optional c = Optional.empty(); + if (o instanceof Number) { + System.out.println("It's a number!"); + } + if (o instanceof Number) { + if (o instanceof Integer) { + System.out.println("It's an integer!"); + } else if (o instanceof Long) { + System.out.println("It's a long!"); + } else if (o instanceof Double) { + System.out.println("It's a double!"); + } else if (o instanceof Float) { + System.out.println("It's a float!"); + } else if (o instanceof BigDecimal) { + System.out.println("It's a BigDecimal!"); + } else { + throw new IllegalArgumentException("Weird number type"); + } + } else if (o instanceof String) { + System.out.println("It's a string!"); + } else if (o instanceof byte[]) { + System.out.println("It's a byte array!"); + } else if (o instanceof int[][] ii) { + System.out.println("It's an int array array"); + } else if (o instanceof Boolean) { + System.out.println("It's a Boolean!"); + } else if (o == null) { + System.out.println("It's null!"); + } else if (o instanceof Instant[][]) { + System.out.println("It's an Instant array array"); + } else if (o instanceof Duration[] d) { + System.out.println("It's a Duration array"); + } else if (o instanceof Date) { + System.out.println("It's a Date!"); + } else { + throw new IllegalArgumentException("Some unexpected type"); + } + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + import java.util.Optional; + import java.lang.Integer; + import java.lang.Long; + import java.math.BigDecimal; + import java.time.Duration; + import java.time.Instant; + import java.util.Date; + + class Test { + public void foo(Suit s) { + Object[] parts = {s, s}; + for (Object o : parts) { + Optional c = Optional.empty(); + if (o instanceof Number) { + System.out.println("It's a number!"); + } + switch (o) { + case Number unused -> { + if (o instanceof Integer) { + System.out.println("It's an integer!"); + } else if (o instanceof Long) { + System.out.println("It's a long!"); + } else if (o instanceof Double) { + System.out.println("It's a double!"); + } else if (o instanceof Float) { + System.out.println("It's a float!"); + } else if (o instanceof BigDecimal) { + System.out.println("It's a BigDecimal!"); + } else { + throw new IllegalArgumentException("Weird number type"); + } + } + case String unused -> System.out.println("It's a string!"); + case byte[] unused -> System.out.println("It's a byte array!"); + case int[][] ii -> System.out.println("It's an int array array"); + case Boolean unused -> System.out.println("It's a Boolean!"); + case Instant[][] unused -> System.out.println("It's an Instant array array"); + case Duration[] d -> System.out.println("It's a Duration array"); + case Date unused -> System.out.println("It's a Date!"); + case null -> System.out.println("It's null!"); + default -> throw new IllegalArgumentException("Some unexpected type"); + } + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_elseBecomesDefault_error() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit == Suit.DIAMOND) { + System.out.println("It's a diamond!"); + } else if ((suit instanceof Number)) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else { + throw new AssertionError(); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Suit.DIAMOND -> System.out.println("It's a diamond!"); + case Number unused -> System.out.println("It's a number!"); + case Suit unused -> System.out.println("It's a Suit!"); + default -> throw new AssertionError(); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_commentHandling_error() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + // alpha + /* beta */ if /* gamma */ (suit /* delta */ instanceof String /* epsilon */) { + // zeta + return; + /* eta */ + } /* nu */ else /* theta */ if (suit == /* iota */ Suit.DIAMOND) { + /* kappa */ { + return; // lambda + } + } else if (((suit instanceof Number) /* tao */)) { + // Square + throw new NullPointerException(/* chi */ ); + } else if (suit /* omicron */ instanceof Suit /* pi */) { + /* mu */ + return; + /* nu */ + } + return; + /* xi */ + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + // alpha + /* beta */ switch (suit) { + case String unused -> + /* gamma */ + /* delta */ + /* epsilon */ + /* nu */ + /* theta */ + { + // zeta + return; + /* eta */ + } + case Suit.DIAMOND -> + /* iota */ + /* kappa */ + { + return; // lambda + } + case Number unused -> + /* tao */ + // Square + throw new NullPointerException(/* chi */ ); + case Suit unused -> + /* omicron */ + /* pi */ + { + /* mu */ + return; + /* nu */ + } + default -> { + return; + } + } + + /* xi */ + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_pullUp_error() { + + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + return; + } else if (suit == Suit.DIAMOND) { + return; + } else if ((suit instanceof Number)) { + return; + } else if (suit instanceof Suit) { + return; + } + return; + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> { + return; + } + case Suit.DIAMOND -> { + return; + } + case Number unused -> { + return; + } + case Suit unused -> { + return; + } + default -> { + return; + } + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_pullUpReachability_error() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + { + Suit suit = s; + if (suit == Suit.SPADE) { + return; + } else if (suit == Suit.DIAMOND) { + return; + } else if (suit == Suit.HEART) { + return; + } else if (suit == Suit.CLUB) { + return; + } + System.out.println("this will become unreachable"); + System.out.println("this will too"); + } + System.out.println("this will vanish"); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + { + Suit suit = s; + switch (suit) { + case Suit.SPADE -> { + return; + } + case Suit.DIAMOND -> { + return; + } + case Suit.HEART -> { + return; + } + case Suit.CLUB -> { + return; + } + } + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_instanceOfWithGuard_error() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit instanceof Suit && suit == Suit.SPADE) { + System.out.println("It's a diamond!"); + } else if ((suit instanceof Number)) { + { + { + /* stay silent about numbers */ + } + } + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Suit unused when suit == Suit.SPADE -> System.out.println("It's a diamond!"); + case Number unused -> { + /* stay silent about numbers */ + } + case Suit unused -> System.out.println("It's a Suit!"); + default -> throw new AssertionError(); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_tooShallow_noError() { + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + System.out.println("It's a string!"); + } else throw new AssertionError(); + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_mismatchSubject_noError() { + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + Object suit2 = s; + System.out.println("yo"); + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit2 instanceof Number) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else throw new AssertionError(); + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_parameterizedType_error() { + // Raw types are converted to the wildcard type. + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + import java.util.List; + import java.util.ArrayList; + + class Test { + public void foo(Suit s) { + List list = new ArrayList<>(); + if (list instanceof ArrayList && list.size() == 0) { + System.out.println("int empty array list"); + } else if (list instanceof ArrayList && list.size() == 0) { + System.out.println("raw empty array list"); + } else if (list instanceof ArrayList && list.size() == 1) { + System.out.println("wildcard element array list"); + } else if (list instanceof ArrayList && list.size() == 1) { + System.out.println("number element array list"); + } else if (list instanceof List l && l.hashCode() == 17) { + System.out.println("hash 17 list"); + } else if (list instanceof List l) { + System.out.println("list"); + } + } + } + """) + .addOutputLines( + "Test.java", +""" +import java.lang.Number; +import java.util.List; +import java.util.ArrayList; + +class Test { + public void foo(Suit s) { + List list = new ArrayList<>(); + switch (list) { + case ArrayList unused when list.size() == 0 -> + System.out.println("int empty array list"); + case ArrayList unused when list.size() == 0 -> System.out.println("raw empty array list"); + case ArrayList unused when list.size() == 1 -> + System.out.println("wildcard element array list"); + case ArrayList unused when list.size() == 1 -> + System.out.println("number element array list"); + case List l when l.hashCode() == 17 -> System.out.println("hash 17 list"); + case List l -> System.out.println("list"); + } + } +} +""") + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_multiParameterizedType_error() { + // Raw types are converted to the wildcard type. + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + import java.util.Map; + import java.util.HashMap; + + class Test { + public void foo(Suit s) { + Map map = new HashMap<>(); + if (map instanceof HashMap && map.size() == 0) { + System.out.println("empty hash map"); + } else if (map instanceof HashMap && map.size() == 0) { + System.out.println("empty hash map with type parameters"); + } else if (map instanceof HashMap && map.size() == 0) { + System.out.println("empty hash map with wildcard"); + } else if (map instanceof HashMap && map.size() == 1) { + System.out.println("one element hash map"); + } else if (map instanceof Map m && m.hashCode() == 17) { + System.out.println("hash 17"); + } else if (map instanceof Map m) { + System.out.println("map"); + } + } + } + """) + .addOutputLines( + "Test.java", +""" +import java.lang.Number; +import java.util.Map; +import java.util.HashMap; + +class Test { + public void foo(Suit s) { + Map map = new HashMap<>(); + switch (map) { + case HashMap unused when map.size() == 0 -> System.out.println("empty hash map"); + case HashMap unused when map.size() == 0 -> + System.out.println("empty hash map with type parameters"); + case HashMap unused when map.size() == 0 -> + System.out.println("empty hash map with wildcard"); + case HashMap unused when map.size() == 1 -> System.out.println("one element hash map"); + case Map m when m.hashCode() == 17 -> + System.out.println("hash 17"); + case Map m -> System.out.println("map"); + } + } +} +""") + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_duplicateConstant_noError() { + // Duplicate constant + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Integer i = s == null ? 0 : 1; + if (i == 88) { + System.out.println("i is 88"); + } else if (i instanceof Integer) { + System.out.println("It's a integer!"); + } else if (i == 88) { + System.out.println("i is one again 88"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_domination1_error() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit instanceof Suit) { + System.out.println("It's a diamond!"); + } else if ((suit instanceof Number)) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit && suit == Suit.DIAMOND) { + System.out.println("It's a Suit!"); + } else if (suit instanceof Suit suity && suit == Suit.SPADE) { + System.out.println("It's a Suity!"); + } else throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Suit unused when suit == Suit.DIAMOND -> System.out.println("It's a Suit!"); + case Suit suity when suit == Suit.SPADE -> System.out.println("It's a Suity!"); + case Suit unused -> System.out.println("It's a diamond!"); + case Number unused -> System.out.println("It's a number!"); + default -> throw new AssertionError(); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_domination2_error() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Integer i = s == null ? 0 : 1; + if (i instanceof Integer) { + System.out.println("It's a integer!"); + } else if (i == 0) { + System.out.println("It's 0!"); + } else if (i instanceof Integer o && o.hashCode() == 17) { + System.out.println("Its hashcode is 17"); + } else if (i == 23) { + System.out.println("It's a 23"); + } else if (i instanceof Integer in && i > 348) { + System.out.println("It's a big integer!"); + } + } + } + """) + .addOutputLines( + "Test.java", +""" +import java.lang.Number; + +class Test { + public void foo(Suit s) { + Integer i = s == null ? 0 : 1; + switch (i) { + case 0 -> System.out.println("It's 0!"); + case Integer o when o.hashCode() == 17 -> System.out.println("Its hashcode is 17"); + case 23 -> System.out.println("It's a 23"); + case Integer in when i > 348 -> System.out.println("It's a big integer!"); + case Integer unused -> System.out.println("It's a integer!"); + } + } +} +""") + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_domination3_error() { + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Integer i = s == null ? 0 : 1; + if (i instanceof Number n) { + System.out.println("It's a number!"); + } else if (i == null) { + System.out.println("It's 0!"); + } else if (i instanceof Object o && o.hashCode() == 17) { + System.out.println("Its hashcode is 17"); + } else if (i == 23) { + System.out.println("It's a 23"); + } else if (i instanceof Integer) { + System.out.println("It's a integer!"); + } else if (i instanceof Integer in && i > 348) { + System.out.println("It's a big integer!"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_domination4_noError() { + // Number and Integer are conflicting (both unconditional for Integer) + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Integer i = s == null ? 0 : 1; + if (i instanceof Number n) { + System.out.println("It's a number!"); + } else if (i == 88) { + System.out.println("i is 88"); + } else if (i instanceof Integer) { + System.out.println("It's a integer!"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + @Test + public void ifChain_domination5_noError() { + // Both a default and unconditional conflict + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Integer i = s == null ? 0 : 1; + if (i == 123) { + System.out.println("i is 123"); + } else if (i == 456) { + System.out.println("i is 456"); + } else if (i instanceof Integer) { + System.out.println("Some other int!"); + } else { + System.out.println("not possible to have default and unconditional"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } + + /** + * Asserts that there is exactly one suggested fix and returns it. + * + *

Similar to {@code FixChoosers.FIRST}, but also asserts that there is exactly one fix. + */ + public static Fix assertOneFixAndChoose(List fixes) { + assertThat(fixes).hasSize(1); + return fixes.get(0); + } +} diff --git a/docs/bugpattern/IfChainToSwitch.md b/docs/bugpattern/IfChainToSwitch.md new file mode 100644 index 00000000000..1720408de07 --- /dev/null +++ b/docs/bugpattern/IfChainToSwitch.md @@ -0,0 +1,120 @@ +We're trying to make long chains of `if` statements clearer (and potentially +faster) by converting them into `switch`es. + +### Long chains of `if` statements + +* When a chain of `if ... else if ... else if ...` statements has `K` total + branches, at runtime one needs to check `O(K)` conditions on average + (assuming equal likelihood of each branch) +* Condition-checking expressions are often repeated multiple times, once in + each `if (...)`. Besides redundancy, this introduces a potential bug vector: + an `if` in the chain could unintentionally have a slightly different + condition than others, an ordering bug (see below), *etc.* + +### `switch`es: + +* Support constants (`1`, `2`, ...), `enum` values, `null`, and pattern + matching, including mixtures of these +* Reduces redundancy +* Runtime performance may benefit + +### Examples + +#### 1. Enum conversion + +``` {.bad} +enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; + +private void foo(Suit suit) { + if (suit == Suit.SPADE) { + System.out.println("spade"); + } else if (suit == Suit.DIAMOND) { + System.out.println("diamond"); + } else if (suit == Suit.HEART) { + System.out.println("heart); + } else if (suit == Suit.CLUB) { + System.out.println("club"); + } +} +``` + +Which can be converted into: + +``` {.good} +enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; + +private void foo(Suit suit) { + switch (suit) { + case Suit.SPADE -> System.out.println("spade"); + case Suit.DIAMOND -> System.out.println("diamond"); + case Suit.HEART -> System.out.println("heart"); + case Suit.CLUB -> System.out.println("club"); + } +} +``` + +Note that with the `switch` style, one gets exhaustiveness checking "for free". +That is, if a new `Suit` value were to be added to the `enum`, then the `switch` +would raise a compile-time error, whereas the original chain of `if` statements +would need to be manually detected and edited. + +#### 2. Patterns + +This conversion works for `instanceof`s too: + +``` {.bad} +enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; + +private void describeObject(Object obj) { + + if (obj instanceof String) { + System.out.println("It's a string!"); + } else if (obj instanceof Number n) { + System.out.println("It's a number!"); + } else if (obj instanceof Object) { + System.out.println("It's an object!"); + } +} +``` + +This can be converted as follows (if the `instanceof` does not originally have a +pattern variable, then `unused` will be inserted): + +``` {.good} +enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; + +private void describeObject(Object obj) { + + switch(obj) { + case String unused -> System.out.println("It's a string!"); + case Number n -> System.out.println("It's a number!"); + case Object unused -> System.out.println("It's an object!"); + } +} +``` + +In later Java versions, an unnamed variable (`_`) can be used in place of +`unused`. + +#### 3. Ordering Bugs + +With `if` chains, it's possible to write code such as: + +``` {.bad} +private void describeObject(Object obj) { + + if (obj instanceof Object) { + System.out.println("It's an object!"); + } else if (obj instanceof Number n) { + System.out.println("It's a number!"); + } else if (obj instanceof String) { + System.out.println("It's a string!"); + } +} +``` + +When calling `describeObject("hello")`, one might expect to have `It's a +string!` printed, but this is not what happens. Because the `Object` check +happens first in code, it matches, resulting in `It's an object!`. This behavior +is most likely a bug, and can sometimes be hard to spot. (This check can be +suppressed if the behavior is intentional.)