From 88b7948830413a04e0d71466296de9bf463231d6 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 10 May 2018 10:20:56 -0700 Subject: [PATCH 1/3] Add code fix to remove unreachable code --- src/compiler/diagnosticMessages.json | 8 ++++ src/harness/tsconfig.json | 1 + src/server/tsconfig.json | 1 + src/server/tsconfig.library.json | 1 + src/services/codefixes/fixUnreachableCode.ts | 34 +++++++++++++++++ src/services/tsconfig.json | 1 + .../cases/fourslash/codeFixUnreachableCode.ts | 14 +++++++ .../fourslash/codeFixUnreachableCode_if.ts | 37 +++++++++++++++++++ 8 files changed, 97 insertions(+) create mode 100644 src/services/codefixes/fixUnreachableCode.ts create mode 100644 tests/cases/fourslash/codeFixUnreachableCode.ts create mode 100644 tests/cases/fourslash/codeFixUnreachableCode_if.ts diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index e5db42605316d..2f7a07e69d5ee 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -4245,5 +4245,13 @@ "Convert all 'require' to 'import'": { "category": "Message", "code": 95048 + }, + "Remove unreachable code": { + "category": "Message", + "code": 95049 + }, + "Remove all unreachable code": { + "category": "Message", + "code": 95050 } } diff --git a/src/harness/tsconfig.json b/src/harness/tsconfig.json index fd1d66f394dcf..32691b6a6b977 100644 --- a/src/harness/tsconfig.json +++ b/src/harness/tsconfig.json @@ -105,6 +105,7 @@ "../services/codefixes/fixExtendsInterfaceBecomesImplements.ts", "../services/codefixes/fixForgottenThisPropertyAccess.ts", "../services/codefixes/fixUnusedIdentifier.ts", + "../services/codefixes/fixUnreachableCode.ts", "../services/codefixes/fixJSDocTypes.ts", "../services/codefixes/fixAwaitInSyncFunction.ts", "../services/codefixes/disableJsDiagnostics.ts", diff --git a/src/server/tsconfig.json b/src/server/tsconfig.json index 71ff00e6ac1ab..ea0ece3aa3885 100644 --- a/src/server/tsconfig.json +++ b/src/server/tsconfig.json @@ -101,6 +101,7 @@ "../services/codefixes/fixExtendsInterfaceBecomesImplements.ts", "../services/codefixes/fixForgottenThisPropertyAccess.ts", "../services/codefixes/fixUnusedIdentifier.ts", + "../services/codefixes/fixUnreachableCode.ts", "../services/codefixes/fixJSDocTypes.ts", "../services/codefixes/fixAwaitInSyncFunction.ts", "../services/codefixes/disableJsDiagnostics.ts", diff --git a/src/server/tsconfig.library.json b/src/server/tsconfig.library.json index 95489ff0ea357..5f475883e7328 100644 --- a/src/server/tsconfig.library.json +++ b/src/server/tsconfig.library.json @@ -107,6 +107,7 @@ "../services/codefixes/fixExtendsInterfaceBecomesImplements.ts", "../services/codefixes/fixForgottenThisPropertyAccess.ts", "../services/codefixes/fixUnusedIdentifier.ts", + "../services/codefixes/fixUnreachableCode.ts", "../services/codefixes/fixJSDocTypes.ts", "../services/codefixes/fixAwaitInSyncFunction.ts", "../services/codefixes/disableJsDiagnostics.ts", diff --git a/src/services/codefixes/fixUnreachableCode.ts b/src/services/codefixes/fixUnreachableCode.ts new file mode 100644 index 0000000000000..66349193a576d --- /dev/null +++ b/src/services/codefixes/fixUnreachableCode.ts @@ -0,0 +1,34 @@ +/* @internal */ +namespace ts.codefix { + const fixId = "fixUnreachableCode"; + const errorCodes = [Diagnostics.Unreachable_code_detected.code]; + registerCodeFix({ + errorCodes, + getCodeActions: context => { + const { sourceFile, span } = context; + const changes = textChanges.ChangeTracker.with(context, t => t.deleteNode(sourceFile, getStatement(sourceFile, span.start))); + return [createCodeFixAction(fixId, changes, Diagnostics.Remove_unreachable_code, fixId, Diagnostics.Remove_all_unreachable_code)]; + }, + fixIds: [fixId], + getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => { + changes.deleteNode(diag.file, getStatement(diag.file, diag.start)); + }), + }); + + function getStatement(sourceFile: SourceFile, start: number): Statement { + const token = getTokenAtPosition(sourceFile, start, /*includeJsDocComment*/ false); + const statement = findAncestor(token, isStatement); + Debug.assert(statement.getStart(sourceFile) === token.getStart(sourceFile)); + + const container = (isBlock(statement.parent) ? statement.parent : statement).parent; + switch (container.kind) { + case SyntaxKind.IfStatement: + return (container as IfStatement).elseStatement ? statement : container as IfStatement; + case SyntaxKind.WhileStatement: + case SyntaxKind.ForStatement: + return container as WhileStatement | ForStatement; + default: + return statement; + } + } +} diff --git a/src/services/tsconfig.json b/src/services/tsconfig.json index be46c21e06b12..dbbe6c0832dee 100644 --- a/src/services/tsconfig.json +++ b/src/services/tsconfig.json @@ -98,6 +98,7 @@ "codefixes/fixExtendsInterfaceBecomesImplements.ts", "codefixes/fixForgottenThisPropertyAccess.ts", "codefixes/fixUnusedIdentifier.ts", + "codefixes/fixUnreachableCode.ts", "codefixes/fixJSDocTypes.ts", "codefixes/fixAwaitInSyncFunction.ts", "codefixes/disableJsDiagnostics.ts", diff --git a/tests/cases/fourslash/codeFixUnreachableCode.ts b/tests/cases/fourslash/codeFixUnreachableCode.ts new file mode 100644 index 0000000000000..8dc38bbc475b0 --- /dev/null +++ b/tests/cases/fourslash/codeFixUnreachableCode.ts @@ -0,0 +1,14 @@ +/// + +////function f() { +//// return 0; +//// return 1; +////} + +verify.codeFix({ + description: "Remove unreachable code", + newFileContent: +`function f() { + return 0; +}`, +}); diff --git a/tests/cases/fourslash/codeFixUnreachableCode_if.ts b/tests/cases/fourslash/codeFixUnreachableCode_if.ts new file mode 100644 index 0000000000000..740d23e1515c2 --- /dev/null +++ b/tests/cases/fourslash/codeFixUnreachableCode_if.ts @@ -0,0 +1,37 @@ +/// + +////if (false) a; +////if (false) { +//// a; +////} +//// +////// No good way to delete just the 'if' part +////if (false) { +//// a; +////} else { +//// b; +////} +//// +////while (false) a; +////while (false) { +//// a; +////} +//// +////for (let x = 0; false; ++x) a; +////for (let x = 0; false; ++x) { +//// a; +////} + +verify.codeFixAll({ + fixId: "fixUnreachableCode", + fixAllDescription: "Remove all unreachable code", + newFileContent: +` +// No good way to delete just the 'if' part +if (false) else { + b; +} + + +`, +}); From ce03cf602523f29172e7a38d20c29997c7e4d158 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 10 May 2018 13:03:24 -0700 Subject: [PATCH 2/3] Code review --- src/services/codefixes/fixUnreachableCode.ts | 56 +++++++++++++++---- .../cases/fourslash/codeFixUnreachableCode.ts | 7 ++- .../fourslash/codeFixUnreachableCode_if.ts | 5 +- 3 files changed, 55 insertions(+), 13 deletions(-) diff --git a/src/services/codefixes/fixUnreachableCode.ts b/src/services/codefixes/fixUnreachableCode.ts index 66349193a576d..21975ab87534b 100644 --- a/src/services/codefixes/fixUnreachableCode.ts +++ b/src/services/codefixes/fixUnreachableCode.ts @@ -4,18 +4,15 @@ namespace ts.codefix { const errorCodes = [Diagnostics.Unreachable_code_detected.code]; registerCodeFix({ errorCodes, - getCodeActions: context => { - const { sourceFile, span } = context; - const changes = textChanges.ChangeTracker.with(context, t => t.deleteNode(sourceFile, getStatement(sourceFile, span.start))); + getCodeActions(context) { + const changes = textChanges.ChangeTracker.with(context, t => doChange(t, context.sourceFile, context.span.start)); return [createCodeFixAction(fixId, changes, Diagnostics.Remove_unreachable_code, fixId, Diagnostics.Remove_all_unreachable_code)]; }, fixIds: [fixId], - getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => { - changes.deleteNode(diag.file, getStatement(diag.file, diag.start)); - }), + getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => doChange(changes, diag.file, diag.start)), }); - function getStatement(sourceFile: SourceFile, start: number): Statement { + function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, start: number): void { const token = getTokenAtPosition(sourceFile, start, /*includeJsDocComment*/ false); const statement = findAncestor(token, isStatement); Debug.assert(statement.getStart(sourceFile) === token.getStart(sourceFile)); @@ -23,12 +20,51 @@ namespace ts.codefix { const container = (isBlock(statement.parent) ? statement.parent : statement).parent; switch (container.kind) { case SyntaxKind.IfStatement: - return (container as IfStatement).elseStatement ? statement : container as IfStatement; + if ((container as IfStatement).elseStatement) { + if (isBlock(statement.parent)) { + changes.deleteNodeRange(sourceFile, first(statement.parent.statements), last(statement.parent.statements)); + } + else { + changes.replaceNode(sourceFile, statement, createBlock(emptyArray)); + } + break; + } + // falls through case SyntaxKind.WhileStatement: case SyntaxKind.ForStatement: - return container as WhileStatement | ForStatement; + changes.deleteNode(sourceFile, container); + break; default: - return statement; + if (isBlock(statement.parent)) { + split(sliceAfter(statement.parent.statements, statement), s => !isFunctionDeclaration(s), (start, end) => changes.deleteNodeRange(sourceFile, start, end)); + } + else { + changes.deleteNode(sourceFile, statement); + } } } + + function sliceAfter(arr: ReadonlyArray, value: T): ReadonlyArray { + const index = arr.indexOf(value); + Debug.assert(index !== -1); + return arr.slice(index); + } + + // Calls 'cb' with the start and end of each range where 'pred' is true. + function split(arr: ReadonlyArray, pred: (t: T) => boolean, cb: (start: T, end: T) => void): void { + let start: T | undefined; + for (let i = 0; i < arr.length; i++) { + const value = arr[i]; + if (pred(value)) { + start = start || value; + } + else { + if (start) { + cb(start, arr[i - 1]); + start = undefined; + } + } + } + if (start) cb(start, arr[arr.length - 1]); + } } diff --git a/tests/cases/fourslash/codeFixUnreachableCode.ts b/tests/cases/fourslash/codeFixUnreachableCode.ts index 8dc38bbc475b0..d2cb9a89e489a 100644 --- a/tests/cases/fourslash/codeFixUnreachableCode.ts +++ b/tests/cases/fourslash/codeFixUnreachableCode.ts @@ -1,14 +1,17 @@ /// ////function f() { -//// return 0; +//// return f(); //// return 1; +//// function f() {} +//// return 2; ////} verify.codeFix({ description: "Remove unreachable code", newFileContent: `function f() { - return 0; + return f(); + function f() {} }`, }); diff --git a/tests/cases/fourslash/codeFixUnreachableCode_if.ts b/tests/cases/fourslash/codeFixUnreachableCode_if.ts index 740d23e1515c2..ac94cfd4eb2aa 100644 --- a/tests/cases/fourslash/codeFixUnreachableCode_if.ts +++ b/tests/cases/fourslash/codeFixUnreachableCode_if.ts @@ -6,6 +6,7 @@ ////} //// ////// No good way to delete just the 'if' part +////if (false) a; else b; ////if (false) { //// a; ////} else { @@ -28,7 +29,9 @@ verify.codeFixAll({ newFileContent: ` // No good way to delete just the 'if' part -if (false) else { +if (false) { } else b; +if (false) { +} else { b; } From ccfcadfebcef48121bfa784a82ad8e5039bfdab4 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 10 May 2018 13:53:39 -0700 Subject: [PATCH 3/3] Preserve more kinds of statements --- src/services/codefixes/fixUnreachableCode.ts | 21 ++++++++++++++++++- .../cases/fourslash/codeFixUnreachableCode.ts | 14 +++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/services/codefixes/fixUnreachableCode.ts b/src/services/codefixes/fixUnreachableCode.ts index 21975ab87534b..5b075d6af1fdf 100644 --- a/src/services/codefixes/fixUnreachableCode.ts +++ b/src/services/codefixes/fixUnreachableCode.ts @@ -36,7 +36,7 @@ namespace ts.codefix { break; default: if (isBlock(statement.parent)) { - split(sliceAfter(statement.parent.statements, statement), s => !isFunctionDeclaration(s), (start, end) => changes.deleteNodeRange(sourceFile, start, end)); + split(sliceAfter(statement.parent.statements, statement), shouldRemove, (start, end) => changes.deleteNodeRange(sourceFile, start, end)); } else { changes.deleteNode(sourceFile, statement); @@ -44,6 +44,25 @@ namespace ts.codefix { } } + function shouldRemove(s: Statement): boolean { + // Don't remove statements that can validly be used before they appear. + return !isFunctionDeclaration(s) && !isPurelyTypeDeclaration(s) && + // `var x;` may declare a variable used above + !(isVariableStatement(s) && !(getCombinedNodeFlags(s) & (NodeFlags.Let | NodeFlags.Const)) && s.declarationList.declarations.some(d => !d.initializer)); + } + + function isPurelyTypeDeclaration(s: Statement): boolean { + switch (s.kind) { + case SyntaxKind.InterfaceDeclaration: + case SyntaxKind.TypeAliasDeclaration: + return true; + case SyntaxKind.ModuleDeclaration: + return getModuleInstanceState(s as ModuleDeclaration) !== ModuleInstanceState.Instantiated; + case SyntaxKind.EnumDeclaration: + return hasModifier(s, ModifierFlags.Const); + } + } + function sliceAfter(arr: ReadonlyArray, value: T): ReadonlyArray { const index = arr.indexOf(value); Debug.assert(index !== -1); diff --git a/tests/cases/fourslash/codeFixUnreachableCode.ts b/tests/cases/fourslash/codeFixUnreachableCode.ts index d2cb9a89e489a..bbe716e792e35 100644 --- a/tests/cases/fourslash/codeFixUnreachableCode.ts +++ b/tests/cases/fourslash/codeFixUnreachableCode.ts @@ -5,13 +5,27 @@ //// return 1; //// function f() {} //// return 2; +//// type T = number; +//// interface I {} +//// const enum E {} +//// enum E {} +//// namespace N { export type T = number; } +//// namespace N { export const x = 0; } +//// var x; +//// var y = 0; ////} verify.codeFix({ description: "Remove unreachable code", + index: 0, newFileContent: `function f() { return f(); function f() {} + type T = number; + interface I {} + const enum E {} + namespace N { export type T = number; } + var x; }`, });