From a277aef7352069c1251d30041f7989f5bcfcd0ac Mon Sep 17 00:00:00 2001 From: cushon Date: Tue, 22 Mar 2016 11:36:10 -0700 Subject: [PATCH] Add suggested fixes to MissingCasesInEnumSwitch RELNOTES: N/A ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=117845252 --- .../bugpatterns/MissingCasesInEnumSwitch.java | 77 ++++++++++++------- .../google/errorprone/fixes/Replacement.java | 2 +- .../scanner/BuiltInCheckerSuppliers.java | 2 +- .../MissingCasesInEnumSwitchTest.java | 24 +++++- 4 files changed, 73 insertions(+), 32 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MissingCasesInEnumSwitch.java b/core/src/main/java/com/google/errorprone/bugpatterns/MissingCasesInEnumSwitch.java index 852c2adea24..6e2f1354680 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MissingCasesInEnumSwitch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MissingCasesInEnumSwitch.java @@ -17,13 +17,16 @@ package com.google.errorprone.bugpatterns; import static com.google.errorprone.BugPattern.Category.JDK; -import static com.google.errorprone.BugPattern.MaturityLevel.EXPERIMENTAL; -import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.BugPattern.MaturityLevel.MATURE; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import com.google.common.collect.Sets; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.SwitchTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Description.Builder; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.CaseTree; @@ -31,23 +34,30 @@ import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.SwitchTree; import com.sun.tools.javac.code.Symbol.TypeSymbol; +import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.JCTree.JCSwitch; +import com.sun.tools.javac.tree.TreeInfo; import java.util.Iterator; import java.util.LinkedHashSet; +import java.util.Set; import javax.lang.model.element.ElementKind; /** * @author cushon@google.com (Liam Miller-Cushon) */ -@BugPattern(name = "MissingCasesInEnumSwitch", - summary = "Enum switch statement is missing cases", - explanation = "Enums on switches should either handle all possible values of the enum, or" - + " have an explicit 'default' case.", - category = JDK, severity = ERROR, maturity = EXPERIMENTAL) -public class MissingCasesInEnumSwitch extends BugChecker - implements SwitchTreeMatcher { +@BugPattern( + name = "MissingCasesInEnumSwitch", + summary = "Enum switch statement is missing cases", + explanation = + "Enums on switches should either handle all possible values of the enum, or" + + " have an explicit default case", + category = JDK, + severity = WARNING, + maturity = MATURE +) +public class MissingCasesInEnumSwitch extends BugChecker implements SwitchTreeMatcher { @Override public Description matchSwitch(SwitchTree tree, VisitorState state) { @@ -61,13 +71,39 @@ public Description matchSwitch(SwitchTree tree, VisitorState state) { return Description.NO_MATCH; } - LinkedHashSet unhandled = - setDifference(ASTHelpers.enumValues(switchType), collectEnumSwitchCases(tree)); + Set unhandled = + Sets.difference(ASTHelpers.enumValues(switchType), collectEnumSwitchCases(tree)); if (unhandled.isEmpty()) { return Description.NO_MATCH; } - return buildDescription(tree).setMessage(buildMessage(unhandled)).build(); + Description.Builder description = buildDescription(tree).setMessage(buildMessage(unhandled)); + buildFixes(tree, state, unhandled, description); + return description.build(); + } + + /** Adds suggested fixes. */ + private void buildFixes( + SwitchTree tree, VisitorState state, Set unhandled, Builder description) { + + int idx = state.getEndPosition((JCTree) tree) - 1; // preserve closing '}' + + description.addFix( + SuggestedFix.replace( + idx, + idx, + String.format( + "default: throw new AssertionError(\"unexpected case: \" + %s);\n", + state.getSourceForNode(TreeInfo.skipParens((JCTree) tree.getExpression()))))); + + StringBuilder sb = new StringBuilder(); + for (String label : unhandled) { + sb.append(String.format("case %s: ", label)); + } + sb.append("break;\n"); + description.addFix(SuggestedFix.replace(idx, idx, sb.toString())); + + description.addFix(SuggestedFix.replace(idx, idx, "default: break;\n")); } /** @@ -76,11 +112,10 @@ public Description matchSwitch(SwitchTree tree, VisitorState state) { *

Examples: *

    *
  • Non-exhaustive switch, expected cases for: FOO - *
  • Non-exhaustive switch, expected cases for: FOO, BAR, BAZ, and 42 others. Did you mean to - * include a 'default' case? + *
  • Non-exhaustive switch, expected cases for: FOO, BAR, BAZ, and 42 others. *
*/ - private String buildMessage(LinkedHashSet unhandled) { + private String buildMessage(Set unhandled) { final int maxCasesToPrint = 5; StringBuilder message = new StringBuilder("Non-exhaustive switch, expected cases for: "); @@ -99,10 +134,7 @@ private String buildMessage(LinkedHashSet unhandled) { } if (tooManyCasesToPrint) { - message.append(String.format(", and %d others. Did you mean to include a 'default' case?", - unhandled.size() - numberToShow)); - } else { - message.append("."); + message.append(String.format(", and %d others", unhandled.size() - numberToShow)); } return message.toString(); } @@ -119,13 +151,6 @@ private static LinkedHashSet collectEnumSwitchCases(SwitchTree tree) { return cases; } - /** Return the difference of sets ax and bx. */ - private static LinkedHashSet setDifference(LinkedHashSet ax, LinkedHashSet bx) { - LinkedHashSet result = new LinkedHashSet<>(ax); - result.removeAll(bx); - return result; - } - /** Return true if the switch has a 'default' case. */ private static boolean hasDefaultCase(SwitchTree tree) { for (CaseTree caseTree : tree.getCases()) { diff --git a/core/src/main/java/com/google/errorprone/fixes/Replacement.java b/core/src/main/java/com/google/errorprone/fixes/Replacement.java index d08a7633538..4779295a03f 100644 --- a/core/src/main/java/com/google/errorprone/fixes/Replacement.java +++ b/core/src/main/java/com/google/errorprone/fixes/Replacement.java @@ -30,7 +30,7 @@ public abstract class Replacement { public static Replacement create(int startPosition, int endPosition, String replaceWith) { checkArgument( 0 <= startPosition && startPosition <= endPosition, - "Illegal range [%s, %s]", + "Illegal range [%s, %s)", startPosition, endPosition); return new AutoValue_Replacement(startPosition, endPosition, replaceWith); 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 c953da90aef..7014b00d569 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -257,6 +257,7 @@ public static ScannerSupplier errorChecks() { EqualsHashCode.class, Finally.class, IncompatibleModifiersChecker.class, + MissingCasesInEnumSwitch.class, MultipleTopLevelClasses.class, NonAtomicVolatileUpdate.class, NonOverridingEquals.class, @@ -313,7 +314,6 @@ public static ScannerSupplier errorChecks() { JUnitAmbiguousTestClass.class, LockMethodChecker.class, MalformedFormatString.class, - MissingCasesInEnumSwitch.class, MissingFail.class, MissingOverride.class, ModifyingCollectionWithItself.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/MissingCasesInEnumSwitchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/MissingCasesInEnumSwitchTest.java index dbe62ec63be..3da50fb16b7 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/MissingCasesInEnumSwitchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/MissingCasesInEnumSwitchTest.java @@ -106,16 +106,14 @@ public void testNonExhaustive_manyCases() throws Exception { " enum Case { ONE, TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT }", " void m(Case c) {", " // BUG: Diagnostic contains:", - " // Non-exhaustive switch, expected cases for: TWO, THREE, FOUR, and 4 others." - + " Did you mean to include a 'default' case?", + " // Non-exhaustive switch, expected cases for: TWO, THREE, FOUR, and 4 others", " switch (c) {", " case ONE:", " System.err.println(\"found it!\");", " break;", " }", " }", - "}" - ) + "}") .doTest(); } @@ -136,4 +134,22 @@ public void testNonExhaustive_nonEnum() throws Exception { "}") .doTest(); } + + @Test + public void empty() throws Exception { + compilationHelper + .addSourceLines( + "Test.java", + "class Test {", + " enum Case { ONE, TWO }", + " void m(Case e) {", + " // BUG: Diagnostic contains:", + " // mean 'default: throw new AssertionError(\"unexpected case: \" + e);'" + + " or 'case ONE: case TWO: break;' or 'default: break;'?", + " switch (e) {", + " }", + " }", + "}") + .doTest(); + } }