From 0c40c18e98baca899b1b70c8c138a5320ccacda0 Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Mon, 10 Jul 2017 12:49:29 -0700 Subject: [PATCH 01/10] Applying edits in Fourslash can cause caret to move off-range --- src/harness/fourslash.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 434a3981461..04fc21e60e0 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -1797,6 +1797,10 @@ namespace FourSlash { // this.languageService.getScriptLexicalStructure(fileName); } + if (this.currentCaretPosition < 0) { + this.currentCaretPosition = 0; + } + if (isFormattingEdit) { const newContent = this.getFileContent(fileName); From b6b6d0516eb3971bf55f869bb99dc919da5b97b6 Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Mon, 10 Jul 2017 12:49:47 -0700 Subject: [PATCH 02/10] More detailed error logging in Fourslash --- src/harness/fourslash.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 04fc21e60e0..fc8299f57e9 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2352,7 +2352,7 @@ namespace FourSlash { private applyCodeActions(actions: ts.CodeAction[], index?: number): void { if (index === undefined) { if (!(actions && actions.length === 1)) { - this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found.`); + this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `"${a.description}"`).join(", ") : "" }`); } index = 0; } From 39e4b1f9e304351f6e9e081a20fb7a7f637589b1 Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Mon, 10 Jul 2017 14:16:31 -0700 Subject: [PATCH 03/10] Code fix to remove unused import should preserve default import --- src/services/codefixes/fixUnusedIdentifier.ts | 41 +++++++++++-------- tests/cases/fourslash/unusedImports13FS.ts | 12 ++++++ 2 files changed, 36 insertions(+), 17 deletions(-) create mode 100644 tests/cases/fourslash/unusedImports13FS.ts diff --git a/src/services/codefixes/fixUnusedIdentifier.ts b/src/services/codefixes/fixUnusedIdentifier.ts index cbe2ba5b1c5..51c8a59627d 100644 --- a/src/services/codefixes/fixUnusedIdentifier.ts +++ b/src/services/codefixes/fixUnusedIdentifier.ts @@ -87,9 +87,7 @@ namespace ts.codefix { case SyntaxKind.ImportSpecifier: const namedImports = parent.parent; if (namedImports.elements.length === 1) { - // Only 1 import and it is unused. So the entire declaration should be removed. - const importSpec = getAncestor(identifier, SyntaxKind.ImportDeclaration); - return [deleteNode(importSpec)]; + return deleteNamedImportBinding(namedImports); } else { // delete import specifier @@ -100,7 +98,7 @@ namespace ts.codefix { // or "'import {a, b as ns} from './file'" case SyntaxKind.ImportClause: // this covers both 'import |d|' and 'import |d,| *' const importClause = parent; - if (!importClause.namedBindings) { // |import d from './file'| or |import * as ns from './file'| + if (!importClause.namedBindings) { // |import d from './file'| const importDecl = getAncestor(importClause, SyntaxKind.ImportDeclaration); return [deleteNode(importDecl)]; } @@ -118,25 +116,34 @@ namespace ts.codefix { } case SyntaxKind.NamespaceImport: - const namespaceImport = parent; - if (namespaceImport.name === identifier && !(namespaceImport.parent).name) { - const importDecl = getAncestor(namespaceImport, SyntaxKind.ImportDeclaration); - return [deleteNode(importDecl)]; - } - else { - const previousToken = getTokenAtPosition(sourceFile, namespaceImport.pos - 1, /*includeJsDocComment*/ false); - if (previousToken && previousToken.kind === SyntaxKind.CommaToken) { - const startPosition = textChanges.getAdjustedStartPosition(sourceFile, previousToken, {}, textChanges.Position.FullStart); - return [deleteRange({ pos: startPosition, end: namespaceImport.end })]; - } - return [deleteRange(namespaceImport)]; - } + return deleteNamedImportBinding(parent); default: return [deleteDefault()]; } } + function deleteNamedImportBinding(namedBindings: NamedImportBindings): CodeAction[] | undefined { + if ((namedBindings.parent).name) { + // Delete named imports while preserving the default import + // import d|, * as ns| from './file' + // import d|, { a }| from './file' + const previousToken = getTokenAtPosition(sourceFile, namedBindings.pos - 1, /*includeJsDocComment*/ false); + if (previousToken && previousToken.kind === SyntaxKind.CommaToken) { + const startPosition = textChanges.getAdjustedStartPosition(sourceFile, previousToken, {}, textChanges.Position.FullStart); + return [deleteRange({ pos: startPosition, end: namedBindings.end })]; + } + return undefined; + } + else { + // Delete the entire import declaration + // |import * as ns from './file'| + // |import { a } from './file'| + const importDecl = getAncestor(namedBindings, SyntaxKind.ImportDeclaration); + return [deleteNode(importDecl)]; + } + } + // token.parent is a variableDeclaration function deleteVariableDeclarationOrPrefixWithUnderscore(identifier: Identifier, varDecl: ts.VariableDeclaration): CodeAction[] | undefined { switch (varDecl.parent.parent.kind) { diff --git a/tests/cases/fourslash/unusedImports13FS.ts b/tests/cases/fourslash/unusedImports13FS.ts new file mode 100644 index 00000000000..4e19f4d7f1a --- /dev/null +++ b/tests/cases/fourslash/unusedImports13FS.ts @@ -0,0 +1,12 @@ +/// + +// @noUnusedLocals: true +// @Filename: file2.ts +//// [| import A, { x } from './a'; |] +//// console.log(A); + +// @Filename: file1.ts +//// export default 10; +//// export var x = 10; + +verify.rangeAfterCodeFix("import A from './a';"); \ No newline at end of file From 003c28f1ef8ef093d2927901af8fafd3b388dc88 Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Tue, 11 Jul 2017 13:32:56 -0700 Subject: [PATCH 04/10] Fix caret update logic in fourslash tests --- src/harness/fourslash.ts | 13 ++++++++----- tests/cases/fourslash/formattingOnEnter.ts | 6 +++++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index fc8299f57e9..9688c565f57 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -1790,17 +1790,20 @@ namespace FourSlash { this.editScriptAndUpdateMarkers(fileName, offsetStart, offsetEnd, edit.newText); const editDelta = edit.newText.length - edit.span.length; if (offsetStart <= this.currentCaretPosition) { - this.currentCaretPosition += editDelta; + if (offsetEnd <= this.currentCaretPosition) { + // The entirety of the edit span falls before the caret position, shift the caret accordingly + this.currentCaretPosition += editDelta; + } + else { + // The span being replaced includes the caret position, place the caret at the beginning of the span + this.currentCaretPosition = offsetStart; + } } runningOffset += editDelta; // TODO: Consider doing this at least some of the time for higher fidelity. Currently causes a failure (bug 707150) // this.languageService.getScriptLexicalStructure(fileName); } - if (this.currentCaretPosition < 0) { - this.currentCaretPosition = 0; - } - if (isFormattingEdit) { const newContent = this.getFileContent(fileName); diff --git a/tests/cases/fourslash/formattingOnEnter.ts b/tests/cases/fourslash/formattingOnEnter.ts index 1c0915e839e..0e7d5af1a6d 100644 --- a/tests/cases/fourslash/formattingOnEnter.ts +++ b/tests/cases/fourslash/formattingOnEnter.ts @@ -6,4 +6,8 @@ goTo.marker(); edit.insertLine(""); -verify.currentLineContentIs('class bar {'); +verify.currentFileContentIs( +`class foo { } +class bar { +} +// new line here`); From 5fd16cae18a653d3f4e6a82d0bb5d1f216d00b4f Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Tue, 11 Jul 2017 13:55:34 -0700 Subject: [PATCH 05/10] format error message with newlines --- src/harness/fourslash.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 9688c565f57..7b5f53298ca 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2355,7 +2355,7 @@ namespace FourSlash { private applyCodeActions(actions: ts.CodeAction[], index?: number): void { if (index === undefined) { if (!(actions && actions.length === 1)) { - this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `"${a.description}"`).join(", ") : "" }`); + this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `\r\n "${a.description}"`) : "" }`); } index = 0; } From bb063f1b5c13e830ee8fbcd5c3899b769804cad1 Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Tue, 11 Jul 2017 14:15:43 -0700 Subject: [PATCH 06/10] Remove incorrect comment --- src/services/codefixes/fixUnusedIdentifier.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/services/codefixes/fixUnusedIdentifier.ts b/src/services/codefixes/fixUnusedIdentifier.ts index 51c8a59627d..6335ebb226b 100644 --- a/src/services/codefixes/fixUnusedIdentifier.ts +++ b/src/services/codefixes/fixUnusedIdentifier.ts @@ -94,8 +94,6 @@ namespace ts.codefix { return [deleteNodeInList(parent)]; } - // handle case where "import d, * as ns from './file'" - // or "'import {a, b as ns} from './file'" case SyntaxKind.ImportClause: // this covers both 'import |d|' and 'import |d,| *' const importClause = parent; if (!importClause.namedBindings) { // |import d from './file'| From 80b64de1e45bc37df699b48d2a7ca452f711545b Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Tue, 11 Jul 2017 14:50:55 -0700 Subject: [PATCH 07/10] Fix comment behavior in remove unused named bindings --- src/services/codefixes/fixUnusedIdentifier.ts | 3 +-- tests/cases/fourslash/unusedImports14FS.ts | 12 ++++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 tests/cases/fourslash/unusedImports14FS.ts diff --git a/src/services/codefixes/fixUnusedIdentifier.ts b/src/services/codefixes/fixUnusedIdentifier.ts index 6335ebb226b..dd4c030b69d 100644 --- a/src/services/codefixes/fixUnusedIdentifier.ts +++ b/src/services/codefixes/fixUnusedIdentifier.ts @@ -128,8 +128,7 @@ namespace ts.codefix { // import d|, { a }| from './file' const previousToken = getTokenAtPosition(sourceFile, namedBindings.pos - 1, /*includeJsDocComment*/ false); if (previousToken && previousToken.kind === SyntaxKind.CommaToken) { - const startPosition = textChanges.getAdjustedStartPosition(sourceFile, previousToken, {}, textChanges.Position.FullStart); - return [deleteRange({ pos: startPosition, end: namedBindings.end })]; + return [deleteRange({ pos: previousToken.getStart(), end: namedBindings.end })]; } return undefined; } diff --git a/tests/cases/fourslash/unusedImports14FS.ts b/tests/cases/fourslash/unusedImports14FS.ts new file mode 100644 index 00000000000..90bd2c50587 --- /dev/null +++ b/tests/cases/fourslash/unusedImports14FS.ts @@ -0,0 +1,12 @@ +/// + +// @noUnusedLocals: true +// @Filename: file2.ts +//// [| import /* 1 */ A /* 2 */, /* 3 */ { x } from './a'; |] +//// console.log(A); + +// @Filename: file1.ts +//// export default 10; +//// export var x = 10; + +verify.rangeAfterCodeFix("import /* 1 */ A /* 2 */ from './a';"); \ No newline at end of file From 3915d46913f753911ea1b86a996b25c7befcac53 Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Tue, 11 Jul 2017 15:10:04 -0700 Subject: [PATCH 08/10] Fix case where we can return [undefined] --- src/services/codefixes/fixUnusedIdentifier.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/services/codefixes/fixUnusedIdentifier.ts b/src/services/codefixes/fixUnusedIdentifier.ts index dd4c030b69d..18491aaba26 100644 --- a/src/services/codefixes/fixUnusedIdentifier.ts +++ b/src/services/codefixes/fixUnusedIdentifier.ts @@ -25,15 +25,15 @@ namespace ts.codefix { return [deleteNode(token.parent)]; default: - return [deleteDefault()]; + return deleteDefault(); } - function deleteDefault() { + function deleteDefault(): CodeAction[] | undefined { if (isDeclarationName(token)) { - return deleteNode(token.parent); + return [deleteNode(token.parent)]; } else if (isLiteralComputedPropertyDeclarationName(token)) { - return deleteNode(token.parent.parent); + return [deleteNode(token.parent.parent)]; } else { return undefined; @@ -117,7 +117,7 @@ namespace ts.codefix { return deleteNamedImportBinding(parent); default: - return [deleteDefault()]; + return deleteDefault(); } } From 0694a38728c81d45c344c2e88538ada3f8a0c0a2 Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Tue, 11 Jul 2017 16:05:10 -0700 Subject: [PATCH 09/10] Use platform agnostic newline --- src/harness/fourslash.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 7b5f53298ca..e5dc3b0023a 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2355,7 +2355,7 @@ namespace FourSlash { private applyCodeActions(actions: ts.CodeAction[], index?: number): void { if (index === undefined) { if (!(actions && actions.length === 1)) { - this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `\r\n "${a.description}"`) : "" }`); + this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `${Harness.IO.newLine()} "${a.description}"`) : "" }`); } index = 0; } From abb229e91b83c06799d2f51200ff364afdc989b3 Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Wed, 12 Jul 2017 11:14:48 -0700 Subject: [PATCH 10/10] Add a bit more validation around comments --- tests/cases/fourslash/unusedImports14FS.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/cases/fourslash/unusedImports14FS.ts b/tests/cases/fourslash/unusedImports14FS.ts index 90bd2c50587..75124802d35 100644 --- a/tests/cases/fourslash/unusedImports14FS.ts +++ b/tests/cases/fourslash/unusedImports14FS.ts @@ -2,11 +2,14 @@ // @noUnusedLocals: true // @Filename: file2.ts -//// [| import /* 1 */ A /* 2 */, /* 3 */ { x } from './a'; |] +//// [| import /* 1 */ A /* 2 */, /* 3 */ { /* 4 */ x /* 5 */ } /* 6 */ from './a'; |] //// console.log(A); // @Filename: file1.ts //// export default 10; //// export var x = 10; -verify.rangeAfterCodeFix("import /* 1 */ A /* 2 */ from './a';"); \ No newline at end of file + +// It's ambiguous which token comment /* 6 */ applies to or whether it should be removed. +// In the current implementation the comment is left behind, but this behavior isn't a requirement. +verify.rangeAfterCodeFix("import /* 1 */ A /* 2 */ /* 6 */ from './a';"); \ No newline at end of file