Merge pull request #17078 from minestarks/removeimportfix

Code fix to remove unused named import should preserve default import

Author: minestarks

src/harness/fourslash.ts

@@ -1806,7 +1806,14 @@ 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)
@@ -2354,7 +2361,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 => `${Harness.IO.newLine()} "${a.description}"`) : "" }`);
                 }
                 index = 0;
             }

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;
@@ -87,20 +87,16 @@ namespace ts.codefix {
                     case SyntaxKind.ImportSpecifier:
                         const namedImports = <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
                             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 = <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,22 +114,30 @@ namespace ts.codefix {
                         }
 
                     case SyntaxKind.NamespaceImport:
-                        const namespaceImport = <NamespaceImport>parent;
-                        if (namespaceImport.name === identifier && !(<ImportClause>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(<NamespaceImport>parent);
 
                     default:
-                        return [deleteDefault()];
+                        return deleteDefault();
+                }
+            }
+
+            function deleteNamedImportBinding(namedBindings: NamedImportBindings): CodeAction[] | undefined {
+                if ((<ImportClause>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) {
+                        return [deleteRange({ pos: previousToken.getStart(), 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)];
                 }
             }
 

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`);

tests/cases/fourslash/unusedImports13FS.ts

@@ -0,0 +1,12 @@
+/// <reference path='fourslash.ts' />
+
+// @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';");

tests/cases/fourslash/unusedImports14FS.ts

@@ -0,0 +1,15 @@
+/// <reference path='fourslash.ts' />
+
+// @noUnusedLocals: true
+// @Filename: file2.ts
+//// [| import /* 1 */ A /* 2 */, /* 3 */ { /* 4 */ x /* 5 */ } /* 6 */ from './a'; |]
+//// console.log(A);
+
+// @Filename: file1.ts
+//// export default 10;
+//// export var x = 10;
+
+
+// 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';");