From 7063ce88ba0b15f5f407323640c56a0037968b56 Mon Sep 17 00:00:00 2001 From: Ricky Patel Date: Tue, 1 Jan 2019 11:42:27 -0800 Subject: [PATCH 1/3] Made case statements a bit safer --- ...> typescript.nested_switches.input.ts.txt} | 0 ... typescript.nested_switches.output.ts.txt} | 6 +-- src/language-js/sortSwitchCases/index.ts | 37 ++++++++++++++----- ...typescript.unknown_break_in_case.input.txt | 23 ++++++++++++ ...ypescript.unknown_break_in_case.output.txt | 23 ++++++++++++ 5 files changed, 77 insertions(+), 12 deletions(-) rename src/language-js/reprinter/test_assets/{typescript.tslint_nested_switches.input.ts.txt => typescript.nested_switches.input.ts.txt} (100%) rename src/language-js/reprinter/test_assets/{typescript.tslint_nested_switches.output.ts.txt => typescript.nested_switches.output.ts.txt} (100%) create mode 100644 src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.input.txt create mode 100644 src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.output.txt diff --git a/src/language-js/reprinter/test_assets/typescript.tslint_nested_switches.input.ts.txt b/src/language-js/reprinter/test_assets/typescript.nested_switches.input.ts.txt similarity index 100% rename from src/language-js/reprinter/test_assets/typescript.tslint_nested_switches.input.ts.txt rename to src/language-js/reprinter/test_assets/typescript.nested_switches.input.ts.txt diff --git a/src/language-js/reprinter/test_assets/typescript.tslint_nested_switches.output.ts.txt b/src/language-js/reprinter/test_assets/typescript.nested_switches.output.ts.txt similarity index 100% rename from src/language-js/reprinter/test_assets/typescript.tslint_nested_switches.output.ts.txt rename to src/language-js/reprinter/test_assets/typescript.nested_switches.output.ts.txt index 7f90c1f5d..ca3253290 100644 --- a/src/language-js/reprinter/test_assets/typescript.tslint_nested_switches.output.ts.txt +++ b/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; } diff --git a/src/language-js/sortSwitchCases/index.ts b/src/language-js/sortSwitchCases/index.ts index ce6053ea0..6aefe83b6 100644 --- a/src/language-js/sortSwitchCases/index.ts +++ b/src/language-js/sortSwitchCases/index.ts @@ -167,16 +167,35 @@ export function sortSwitchCases( 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 - ); + switch (value.type) { + case "BlockStatement": { + return ( + value.body.filter((value: any) => { + return ( + value.type === "BreakStatement" || + value.type === "ReturnStatement" + ); + }).length !== 0 + ); + } + case "BreakStatement": + case "ReturnStatement": + case "ThrowStatement": + return 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 + return ( + // Value is some sort of loop + value.body != null || + // Value is a switch statement + value.cases != null || + // Value is some sort of conditional + value.consequent != null || + // Value is a try catch + value.block != null + ); } - return value.type === "BreakStatement" || value.type === "ReturnStatement"; }); return breakStatement.length !== 0; } diff --git a/src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.input.txt b/src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.input.txt new file mode 100644 index 000000000..f8cb8e6e8 --- /dev/null +++ b/src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.input.txt @@ -0,0 +1,23 @@ +switch (node.kind) { + 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; +} \ No newline at end of file diff --git a/src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.output.txt b/src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.output.txt new file mode 100644 index 000000000..9f185b76a --- /dev/null +++ b/src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.output.txt @@ -0,0 +1,23 @@ +switch (node.kind) { + case ts.SyntaxKind.FalseKeyword: + case ts.SyntaxKind.Identifier: + case ts.SyntaxKind.NoSubstitutionTemplateLiteral: + case ts.SyntaxKind.NullKeyword: + case ts.SyntaxKind.NumericLiteral: + case ts.SyntaxKind.StringLiteral: + case ts.SyntaxKind.ThisKeyword: + case ts.SyntaxKind.TrueKeyword: + return true; + 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.PropertyAccessExpression: + return isSimple((node as ts.PropertyAccessExpression).expression); + default: + return false; +} \ No newline at end of file From 19d31fbe61d7185ecdc8af46c8a8e984fb3ca2e3 Mon Sep 17 00:00:00 2001 From: Ricky Patel Date: Tue, 1 Jan 2019 12:57:28 -0800 Subject: [PATCH 2/3] Fixed tslint --- src/language-js/sortSwitchCases/index.ts | 65 ++++++++++++------- ....fallthrough_as_last_doesnt_sort.input.txt | 11 ++-- ...fallthrough_as_last_doesnt_sort.output.txt | 11 ++-- ...ipt.indeterminate_break_in_case.input.txt} | 6 ++ ...pt.indeterminate_break_in_case.output.txt} | 28 ++++---- 5 files changed, 77 insertions(+), 44 deletions(-) rename src/language-js/sortSwitchCases/test_assets/{typescript.unknown_break_in_case.input.txt => typescript.indeterminate_break_in_case.input.txt} (68%) rename src/language-js/sortSwitchCases/test_assets/{typescript.unknown_break_in_case.output.txt => typescript.indeterminate_break_in_case.output.txt} (68%) diff --git a/src/language-js/sortSwitchCases/index.ts b/src/language-js/sortSwitchCases/index.ts index 6aefe83b6..31edaabb4 100644 --- a/src/language-js/sortSwitchCases/index.ts +++ b/src/language-js/sortSwitchCases/index.ts @@ -37,6 +37,7 @@ export function sortSwitchCases( return fileContents; } + let doesBreakOut = "indeterminate"; let newFileContents = fileContents.slice(); let contextGroups = getContextGroups(cases, comments, fileContents); @@ -47,6 +48,11 @@ export function sortSwitchCases( continue; } + doesBreakOut = doesCaseBreakOutOfSwitch(cases[cases.length - 1]); + if (doesBreakOut !== "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 = []; @@ -54,7 +60,11 @@ export function sortSwitchCases( 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 === "indeterminate") { + return fileContents; + } + if (doesBreakOut === "false") { continue; } @@ -109,11 +119,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 !== "true") { + switchGroupsWithBreaks.pop(); } // Now sort the actual switch groups @@ -165,39 +173,50 @@ export function sortSwitchCases( return newFileContents; } -function doesCaseBreakOutOfSwitch(caseStatement: any) { - let breakStatement = caseStatement.consequent.filter((value: any) => { +function doesCaseBreakOutOfSwitch( + caseStatement: any +): "false" | "indeterminate" | "true" { + return doesHaveImmediateExit(caseStatement.consequent); +} + +function doesHaveImmediateExit(values: any) { + for (let value of values) { switch (value.type) { case "BlockStatement": { - return ( - value.body.filter((value: any) => { - return ( - value.type === "BreakStatement" || - value.type === "ReturnStatement" - ); - }).length !== 0 - ); + return doesHaveImmediateExit(value.body); } case "BreakStatement": case "ReturnStatement": case "ThrowStatement": - return true; + return "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 + ) === "true" + ) { + return "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 - return ( + if ( // Value is some sort of loop value.body != null || - // Value is a switch statement - value.cases != null || // Value is some sort of conditional value.consequent != null || // Value is a try catch value.block != null - ); + ) { + return "indeterminate"; + } } - }); - return breakStatement.length !== 0; + } + return "false"; } function getSortableText(a: any, fileContents: string) { diff --git a/src/language-js/sortSwitchCases/test_assets/typescript.fallthrough_as_last_doesnt_sort.input.txt b/src/language-js/sortSwitchCases/test_assets/typescript.fallthrough_as_last_doesnt_sort.input.txt index 7c422e25e..f366949fe 100644 --- a/src/language-js/sortSwitchCases/test_assets/typescript.fallthrough_as_last_doesnt_sort.input.txt +++ b/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; + } } \ No newline at end of file diff --git a/src/language-js/sortSwitchCases/test_assets/typescript.fallthrough_as_last_doesnt_sort.output.txt b/src/language-js/sortSwitchCases/test_assets/typescript.fallthrough_as_last_doesnt_sort.output.txt index 7c422e25e..f366949fe 100644 --- a/src/language-js/sortSwitchCases/test_assets/typescript.fallthrough_as_last_doesnt_sort.output.txt +++ b/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; + } } \ No newline at end of file diff --git a/src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.input.txt b/src/language-js/sortSwitchCases/test_assets/typescript.indeterminate_break_in_case.input.txt similarity index 68% rename from src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.input.txt rename to src/language-js/sortSwitchCases/test_assets/typescript.indeterminate_break_in_case.input.txt index f8cb8e6e8..a164f7b42 100644 --- a/src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.input.txt +++ b/src/language-js/sortSwitchCases/test_assets/typescript.indeterminate_break_in_case.input.txt @@ -1,4 +1,10 @@ +// 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: diff --git a/src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.output.txt b/src/language-js/sortSwitchCases/test_assets/typescript.indeterminate_break_in_case.output.txt similarity index 68% rename from src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.output.txt rename to src/language-js/sortSwitchCases/test_assets/typescript.indeterminate_break_in_case.output.txt index 9f185b76a..a164f7b42 100644 --- a/src/language-js/sortSwitchCases/test_assets/typescript.unknown_break_in_case.output.txt +++ b/src/language-js/sortSwitchCases/test_assets/typescript.indeterminate_break_in_case.output.txt @@ -1,13 +1,12 @@ +// 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.FalseKeyword: - case ts.SyntaxKind.Identifier: - case ts.SyntaxKind.NoSubstitutionTemplateLiteral: - case ts.SyntaxKind.NullKeyword: - case ts.SyntaxKind.NumericLiteral: - case ts.SyntaxKind.StringLiteral: - case ts.SyntaxKind.ThisKeyword: - case ts.SyntaxKind.TrueKeyword: - return true; + 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: @@ -16,8 +15,15 @@ switch (node.kind) { default: return isSimple((node as ts.PrefixUnaryExpression).operand); } - case ts.SyntaxKind.PropertyAccessExpression: - return isSimple((node as ts.PropertyAccessExpression).expression); + 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; } \ No newline at end of file From 9fd88231423c7b1fedd8ca28ac9ddbea83661590 Mon Sep 17 00:00:00 2001 From: Ricky Patel Date: Tue, 1 Jan 2019 13:06:47 -0800 Subject: [PATCH 3/3] Moved strings to an enum --- src/language-js/sortSwitchCases/index.ts | 32 +++++++++++++----------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/language-js/sortSwitchCases/index.ts b/src/language-js/sortSwitchCases/index.ts index 31edaabb4..0824ce562 100644 --- a/src/language-js/sortSwitchCases/index.ts +++ b/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,7 +43,7 @@ export function sortSwitchCases( return fileContents; } - let doesBreakOut = "indeterminate"; + let doesBreakOut = HasImmediateExitOption.Indeterminate; let newFileContents = fileContents.slice(); let contextGroups = getContextGroups(cases, comments, fileContents); @@ -49,7 +55,7 @@ export function sortSwitchCases( } doesBreakOut = doesCaseBreakOutOfSwitch(cases[cases.length - 1]); - if (doesBreakOut !== "true") { + if (doesBreakOut !== HasImmediateExitOption.True) { cases.pop(); } @@ -61,10 +67,10 @@ export function sortSwitchCases( for (switchCaseEnd = 0; switchCaseEnd < cases.length; switchCaseEnd++) { // Do not rearrange items that are in a non-break statement doesBreakOut = doesCaseBreakOutOfSwitch(cases[switchCaseEnd]); - if (doesBreakOut === "indeterminate") { + if (doesBreakOut === HasImmediateExitOption.Indeterminate) { return fileContents; } - if (doesBreakOut === "false") { + if (doesBreakOut === HasImmediateExitOption.False) { continue; } @@ -120,7 +126,7 @@ export function sortSwitchCases( // If the last switch group is a fall through, don't include it in the swap doesBreakOut = doesCaseBreakOutOfSwitch(cases[cases.length - 1]); - if (doesBreakOut !== "true") { + if (doesBreakOut !== HasImmediateExitOption.True) { switchGroupsWithBreaks.pop(); } @@ -173,13 +179,11 @@ export function sortSwitchCases( return newFileContents; } -function doesCaseBreakOutOfSwitch( - caseStatement: any -): "false" | "indeterminate" | "true" { +function doesCaseBreakOutOfSwitch(caseStatement: any): HasImmediateExitOption { return doesHaveImmediateExit(caseStatement.consequent); } -function doesHaveImmediateExit(values: any) { +function doesHaveImmediateExit(values: any): HasImmediateExitOption { for (let value of values) { switch (value.type) { case "BlockStatement": { @@ -188,7 +192,7 @@ function doesHaveImmediateExit(values: any) { case "BreakStatement": case "ReturnStatement": case "ThrowStatement": - return "true"; + 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) @@ -196,9 +200,9 @@ function doesHaveImmediateExit(values: any) { if ( doesHaveImmediateExit( value.cases[value.cases.length - 1].consequent - ) === "true" + ) === HasImmediateExitOption.True ) { - return "true"; + return HasImmediateExitOption.True; } } default: @@ -212,11 +216,11 @@ function doesHaveImmediateExit(values: any) { // Value is a try catch value.block != null ) { - return "indeterminate"; + return HasImmediateExitOption.Indeterminate; } } } - return "false"; + return HasImmediateExitOption.False; } function getSortableText(a: any, fileContents: string) {