Fixed switch statement edge cases

src/language-js/reprinter/test_assets/typescript.tslint_nested_switches.output.ts.txt → src/language-js/reprinter/test_assets/typescript.nested_switches.output.ts.txt

@@ -8,9 +8,6 @@ function canHaveTrailingTrivia(tokenKind: ts.SyntaxKind, parent: ts.Node): boole
                 parent.kind !== ts.SyntaxKind.JsxExpression ||
                 parent.parent.kind !== ts.SyntaxKind.JsxElement
             );
-        case ts.SyntaxKind.JsxText:
-            // there is no trivia after JsxText
-            return false;
         case ts.SyntaxKind.GreaterThanToken:
             switch (parent.kind) {
                 case ts.SyntaxKind.JsxClosingElement:
@@ -23,6 +20,9 @@ function canHaveTrailingTrivia(tokenKind: ts.SyntaxKind, parent: ts.Node): boole
                 default:
                     return true;
             }
+        case ts.SyntaxKind.JsxText:
+            // there is no trivia after JsxText
+            return false;
         default:
             return true;
     }

src/language-js/sortSwitchCases/index.ts

@@ -11,6 +11,12 @@ import {
   reorderValues
 } from "../../utilities/sort-utils";
 
+enum HasImmediateExitOption {
+  Indeterminate,
+  True,
+  False
+}
+
 export interface SortSwitchCaseOptions {}
 
 export function sortSwitchCases(
@@ -37,6 +43,7 @@ export function sortSwitchCases(
     return fileContents;
   }
 
+  let doesBreakOut = HasImmediateExitOption.Indeterminate;
   let newFileContents = fileContents.slice();
   let contextGroups = getContextGroups(cases, comments, fileContents);
 
@@ -47,14 +54,23 @@ export function sortSwitchCases(
       continue;
     }
 
+    doesBreakOut = doesCaseBreakOutOfSwitch(cases[cases.length - 1]);
+    if (doesBreakOut !== HasImmediateExitOption.True) {
+      cases.pop();
+    }
+
     // Determine where the "break" statements are so we dont' sort through them
     // which would change the logic of the code
     let switchGroupsWithBreaks: Array<SwitchCase[]> = [];
     let switchCaseStart = 0;
     let switchCaseEnd = 0;
     for (switchCaseEnd = 0; switchCaseEnd < cases.length; switchCaseEnd++) {
       // Do not rearrange items that are in a non-break statement
-      if (!doesCaseBreakOutOfSwitch(cases[switchCaseEnd])) {
+      doesBreakOut = doesCaseBreakOutOfSwitch(cases[switchCaseEnd]);
+      if (doesBreakOut === HasImmediateExitOption.Indeterminate) {
+        return fileContents;
+      }
+      if (doesBreakOut === HasImmediateExitOption.False) {
         continue;
       }
 
@@ -109,11 +125,9 @@ export function sortSwitchCases(
     }
 
     // If the last switch group is a fall through, don't include it in the swap
-    if (!doesCaseBreakOutOfSwitch(cases[cases.length - 1])) {
-      switchGroupsWithBreaks = switchGroupsWithBreaks.slice(
-        0,
-        switchGroupsWithBreaks.length - 1
-      );
+    doesBreakOut = doesCaseBreakOutOfSwitch(cases[cases.length - 1]);
+    if (doesBreakOut !== HasImmediateExitOption.True) {
+      switchGroupsWithBreaks.pop();
     }
 
     // Now sort the actual switch groups
@@ -165,20 +179,48 @@ export function sortSwitchCases(
   return newFileContents;
 }
 
-function doesCaseBreakOutOfSwitch(caseStatement: any) {
-  let breakStatement = caseStatement.consequent.filter((value: any) => {
-    if (value.type === "BlockStatement") {
-      return (
-        value.body.filter((value: any) => {
-          return (
-            value.type === "BreakStatement" || value.type === "ReturnStatement"
-          );
-        }).length !== 0
-      );
+function doesCaseBreakOutOfSwitch(caseStatement: any): HasImmediateExitOption {
+  return doesHaveImmediateExit(caseStatement.consequent);
+}
+
+function doesHaveImmediateExit(values: any): HasImmediateExitOption {
+  for (let value of values) {
+    switch (value.type) {
+      case "BlockStatement": {
+        return doesHaveImmediateExit(value.body);
+      }
+      case "BreakStatement":
+      case "ReturnStatement":
+      case "ThrowStatement":
+        return HasImmediateExitOption.True;
+      case "SwitchStatement": {
+        // If the last option in the switch statement has an exit, then either
+        // all previosu consequents have an exit (e.g. not getting to the last one)
+        // or they all fall through to the last one which means we exit
+        if (
+          doesHaveImmediateExit(
+            value.cases[value.cases.length - 1].consequent
+          ) === HasImmediateExitOption.True
+        ) {
+          return HasImmediateExitOption.True;
+        }
+      }
+      default:
+        // There are several types which are a bit more complicated which
+        // leaves us in an undeterminate state if we will exit or not
+        if (
+          // Value is some sort of loop
+          value.body != null ||
+          // Value is some sort of conditional
+          value.consequent != null ||
+          // Value is a try catch
+          value.block != null
+        ) {
+          return HasImmediateExitOption.Indeterminate;
+        }
     }
-    return value.type === "BreakStatement" || value.type === "ReturnStatement";
-  });
-  return breakStatement.length !== 0;
+  }
+  return HasImmediateExitOption.False;
 }
 
 function getSortableText(a: any, fileContents: string) {

src/language-js/sortSwitchCases/test_assets/typescript.fallthrough_as_last_doesnt_sort.input.txt

@@ -1,8 +1,9 @@
 // Since the last case statement falls through, if we sort without ignoring the last break, we will log "a" and "b", not just "a"
-switch ("a") {
-    case "b":
-        log("b");
+switch (unify.kind) {
+    case "single-parameter-difference": {
         break;
-    case "a":
-        log("a");
+    }
+    case "extra-parameter": {
+        const { extraParameter, otherSignature } = unify;
+    }
 }

src/language-js/sortSwitchCases/test_assets/typescript.fallthrough_as_last_doesnt_sort.output.txt

@@ -1,8 +1,9 @@
 // Since the last case statement falls through, if we sort without ignoring the last break, we will log "a" and "b", not just "a"
-switch ("a") {
-    case "b":
-        log("b");
+switch (unify.kind) {
+    case "single-parameter-difference": {
         break;
-    case "a":
-        log("a");
+    }
+    case "extra-parameter": {
+        const { extraParameter, otherSignature } = unify;
+    }
 }

src/language-js/sortSwitchCases/test_assets/typescript.indeterminate_break_in_case.input.txt

@@ -0,0 +1,29 @@
+// Currently we declare this "indeterminate" because "ts.SyntaxKind.PropertyAccessExpression2" has a conditional that sometimes breaks and sometimes doesn't.
+// In these situations we should return the original switch statement as to not break the execution code
+switch (node.kind) {
+    case ts.SyntaxKind.PropertyAccessExpression2:
+        if (false) {
+            break;
+        } // Fallthrough into next line
+    case ts.SyntaxKind.PropertyAccessExpression:
+        return isSimple((node as ts.PropertyAccessExpression).expression);
+    case ts.SyntaxKind.PrefixUnaryExpression:
+        switch ((node as ts.PrefixUnaryExpression).operator) {
+            case ts.SyntaxKind.PlusPlusToken:
+            case ts.SyntaxKind.MinusMinusToken:
+                return false;
+            default:
+                return isSimple((node as ts.PrefixUnaryExpression).operand);
+        }
+    case ts.SyntaxKind.Identifier:
+    case ts.SyntaxKind.NumericLiteral:
+    case ts.SyntaxKind.StringLiteral:
+    case ts.SyntaxKind.ThisKeyword:
+    case ts.SyntaxKind.NoSubstitutionTemplateLiteral:
+    case ts.SyntaxKind.TrueKeyword:
+    case ts.SyntaxKind.FalseKeyword:
+    case ts.SyntaxKind.NullKeyword:
+        return true;
+    default:
+        return false;
+}

src/language-js/sortSwitchCases/test_assets/typescript.indeterminate_break_in_case.output.txt

@@ -0,0 +1,29 @@
+// Currently we declare this "indeterminate" because "ts.SyntaxKind.PropertyAccessExpression2" has a conditional that sometimes breaks and sometimes doesn't.
+// In these situations we should return the original switch statement as to not break the execution code
+switch (node.kind) {
+    case ts.SyntaxKind.PropertyAccessExpression2:
+        if (false) {
+            break;
+        } // Fallthrough into next line
+    case ts.SyntaxKind.PropertyAccessExpression:
+        return isSimple((node as ts.PropertyAccessExpression).expression);
+    case ts.SyntaxKind.PrefixUnaryExpression:
+        switch ((node as ts.PrefixUnaryExpression).operator) {
+            case ts.SyntaxKind.PlusPlusToken:
+            case ts.SyntaxKind.MinusMinusToken:
+                return false;
+            default:
+                return isSimple((node as ts.PrefixUnaryExpression).operand);
+        }
+    case ts.SyntaxKind.Identifier:
+    case ts.SyntaxKind.NumericLiteral:
+    case ts.SyntaxKind.StringLiteral:
+    case ts.SyntaxKind.ThisKeyword:
+    case ts.SyntaxKind.NoSubstitutionTemplateLiteral:
+    case ts.SyntaxKind.TrueKeyword:
+    case ts.SyntaxKind.FalseKeyword:
+    case ts.SyntaxKind.NullKeyword:
+        return true;
+    default:
+        return false;
+}