From 3da5a8ba559a0ed4b76ad96c493a528bd6419879 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Jun 2025 22:11:00 +0000 Subject: [PATCH 1/8] Initial plan for issue From 2df83a1343c71a62240b02b16148455deb0efd41 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Jun 2025 22:19:26 +0000 Subject: [PATCH 2/8] Create test case that reproduces the symbol import bug Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com> --- .../completionsSymbolImportAsValue.ts | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 tests/cases/fourslash/completionsSymbolImportAsValue.ts diff --git a/tests/cases/fourslash/completionsSymbolImportAsValue.ts b/tests/cases/fourslash/completionsSymbolImportAsValue.ts new file mode 100644 index 0000000000000..11f508ba06ad3 --- /dev/null +++ b/tests/cases/fourslash/completionsSymbolImportAsValue.ts @@ -0,0 +1,41 @@ +/// + +// @Filename: /exportsSymbol.ts +////export const SYM_FOO_BAR = Symbol.for("foo.bar"); +//// +////export interface ObjWithSym { +//// [SYM_FOO_BAR]: any; +////} + +// @Filename: /usesSymbol.ts +////import type { ObjWithSym } from "./exportsSymbol"; +//// +////export declare const thing: ObjWithSym; +//// +////function main() { +//// thing[/**/]; +////} + +verify.completions({ + marker: "", + includes: [ + { name: "SYM_FOO_BAR", source: "/exportsSymbol", hasAction: true, sortText: "16" }, + ], + preferences: { + includeCompletionsForModuleExports: true, + }, +}); + +verify.applyCodeActionFromCompletion("", { + name: "SYM_FOO_BAR", + source: "/exportsSymbol", + description: `Update import from "./exportsSymbol"`, + newFileContent: +`import { SYM_FOO_BAR, type ObjWithSym } from "./exportsSymbol"; + +export declare const thing: ObjWithSym; + +function main() { + thing[SYM_FOO_BAR]; +}` +}); \ No newline at end of file From e333c68d1dbc38eea9181c83dc355c484f90bfb7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Jun 2025 22:26:07 +0000 Subject: [PATCH 3/8] Identify root cause: bug affects all auto-imports when type-only imports exist Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com> --- debug_test.js | 46 +++++++++++++++++++ .../fourslash/completionsRegularIdentifier.ts | 43 +++++++++++++++++ .../completionsRegularImportAsValue.ts | 43 +++++++++++++++++ 3 files changed, 132 insertions(+) create mode 100644 debug_test.js create mode 100644 tests/cases/fourslash/completionsRegularIdentifier.ts create mode 100644 tests/cases/fourslash/completionsRegularImportAsValue.ts diff --git a/debug_test.js b/debug_test.js new file mode 100644 index 0000000000000..c716df4e4a092 --- /dev/null +++ b/debug_test.js @@ -0,0 +1,46 @@ +// Debug script to understand the AST structure in the fourslash test case +const ts = require('./built/local/typescript.js'); + +// This exactly matches our fourslash test case structure +const code = `import type { ObjWithSym } from "./exportsSymbol"; + +export declare const thing: ObjWithSym; + +function main() { + thing[]; +}`; + +const sourceFile = ts.createSourceFile('usesSymbol.ts', code, ts.ScriptTarget.Latest, true); + +function printNode(node, depth = 0) { + const indent = ' '.repeat(depth * 2); + console.log(`${indent}${ts.SyntaxKind[node.kind]} (${node.pos}-${node.end}): "${code.substring(node.pos, node.end).replace(/\n/g, '\\n')}"`); + if (node.getChildCount && node.getChildCount() > 0) { + node.forEachChild(child => printNode(child, depth + 1)); + } +} + +console.log('AST Structure:'); +printNode(sourceFile); + +// Find the position inside the brackets in the element access expression +const bracketPos = code.indexOf('thing[') + 6; +console.log(`\nLooking at position ${bracketPos} (inside thing[])`); + +// Get the token at that position +const tokenAtPos = ts.getTokenAtPosition(sourceFile, bracketPos); +console.log(`Token at position: ${ts.SyntaxKind[tokenAtPos.kind]} "${code.substring(tokenAtPos.pos, tokenAtPos.end)}"`); + +// Get touching property name +const touchingPropertyName = ts.getTouchingPropertyName(sourceFile, bracketPos); +console.log(`Touching property name: ${ts.SyntaxKind[touchingPropertyName.kind]} "${code.substring(touchingPropertyName.pos, touchingPropertyName.end)}"`); + +// Check if it's a valid type-only alias use site +console.log(`Is valid type-only alias use site: ${ts.isValidTypeOnlyAliasUseSite(touchingPropertyName)}`); + +// Let's also check what the parent is +if (touchingPropertyName.parent) { + console.log(`Parent: ${ts.SyntaxKind[touchingPropertyName.parent.kind]}`); +} +console.log(`Is expression node: ${ts.isExpressionNode(touchingPropertyName)}`); +console.log(`Is in expression context: ${ts.isInExpressionContext(touchingPropertyName)}`); \ No newline at end of file diff --git a/tests/cases/fourslash/completionsRegularIdentifier.ts b/tests/cases/fourslash/completionsRegularIdentifier.ts new file mode 100644 index 0000000000000..245e708311876 --- /dev/null +++ b/tests/cases/fourslash/completionsRegularIdentifier.ts @@ -0,0 +1,43 @@ +/// + +// Test with property access (not element access) to see if the issue is element-access-specific + +// @Filename: /exportsRegular.ts +////export const REGULAR_PROP = "regularProp"; +//// +////export interface ObjWithProp { +//// regularMethod(): any; +////} + +// @Filename: /usesRegular.ts +////import type { ObjWithProp } from "./exportsRegular"; +//// +////export declare const thing: ObjWithProp; +//// +////function main() { +//// /**/; +////} + +verify.completions({ + marker: "", + includes: [ + { name: "REGULAR_PROP", source: "/exportsRegular", hasAction: true, sortText: "16" }, + ], + preferences: { + includeCompletionsForModuleExports: true, + }, +}); + +verify.applyCodeActionFromCompletion("", { + name: "REGULAR_PROP", + source: "/exportsRegular", + description: `Update import from "./exportsRegular"`, + newFileContent: +`import { REGULAR_PROP, type ObjWithProp } from "./exportsRegular"; + +export declare const thing: ObjWithProp; + +function main() { + REGULAR_PROP; +}` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/completionsRegularImportAsValue.ts b/tests/cases/fourslash/completionsRegularImportAsValue.ts new file mode 100644 index 0000000000000..d66dfed03feb5 --- /dev/null +++ b/tests/cases/fourslash/completionsRegularImportAsValue.ts @@ -0,0 +1,43 @@ +/// + +// Test with a regular property (not a symbol) to see if the issue is symbol-specific + +// @Filename: /exportsRegular.ts +////export const REGULAR_PROP = "regularProp"; +//// +////export interface ObjWithProp { +//// [REGULAR_PROP]: any; +////} + +// @Filename: /usesRegular.ts +////import type { ObjWithProp } from "./exportsRegular"; +//// +////export declare const thing: ObjWithProp; +//// +////function main() { +//// thing[/**/]; +////} + +verify.completions({ + marker: "", + includes: [ + { name: "REGULAR_PROP", source: "/exportsRegular", hasAction: true, sortText: "16" }, + ], + preferences: { + includeCompletionsForModuleExports: true, + }, +}); + +verify.applyCodeActionFromCompletion("", { + name: "REGULAR_PROP", + source: "/exportsRegular", + description: `Update import from "./exportsRegular"`, + newFileContent: +`import { REGULAR_PROP, type ObjWithProp } from "./exportsRegular"; + +export declare const thing: ObjWithProp; + +function main() { + thing[REGULAR_PROP]; +}` +}); \ No newline at end of file From 088c2641e3747a044eb26db13f5a48cb756e6f9d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Jun 2025 22:31:58 +0000 Subject: [PATCH 4/8] Initial fix attempt - added guards to prevent value imports in type-only imports Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com> --- debug_test.js | 46 ---------- src/services/codefixes/importFixes.ts | 85 +++++++++++-------- .../fourslash/completionsRegularIdentifier.ts | 43 ---------- .../completionsRegularImportAsValue.ts | 43 ---------- .../completionsSymbolImportAsValue.ts | 35 +++----- 5 files changed, 62 insertions(+), 190 deletions(-) delete mode 100644 debug_test.js delete mode 100644 tests/cases/fourslash/completionsRegularIdentifier.ts delete mode 100644 tests/cases/fourslash/completionsRegularImportAsValue.ts diff --git a/debug_test.js b/debug_test.js deleted file mode 100644 index c716df4e4a092..0000000000000 --- a/debug_test.js +++ /dev/null @@ -1,46 +0,0 @@ -// Debug script to understand the AST structure in the fourslash test case -const ts = require('./built/local/typescript.js'); - -// This exactly matches our fourslash test case structure -const code = `import type { ObjWithSym } from "./exportsSymbol"; - -export declare const thing: ObjWithSym; - -function main() { - thing[]; -}`; - -const sourceFile = ts.createSourceFile('usesSymbol.ts', code, ts.ScriptTarget.Latest, true); - -function printNode(node, depth = 0) { - const indent = ' '.repeat(depth * 2); - console.log(`${indent}${ts.SyntaxKind[node.kind]} (${node.pos}-${node.end}): "${code.substring(node.pos, node.end).replace(/\n/g, '\\n')}"`); - if (node.getChildCount && node.getChildCount() > 0) { - node.forEachChild(child => printNode(child, depth + 1)); - } -} - -console.log('AST Structure:'); -printNode(sourceFile); - -// Find the position inside the brackets in the element access expression -const bracketPos = code.indexOf('thing[') + 6; -console.log(`\nLooking at position ${bracketPos} (inside thing[])`); - -// Get the token at that position -const tokenAtPos = ts.getTokenAtPosition(sourceFile, bracketPos); -console.log(`Token at position: ${ts.SyntaxKind[tokenAtPos.kind]} "${code.substring(tokenAtPos.pos, tokenAtPos.end)}"`); - -// Get touching property name -const touchingPropertyName = ts.getTouchingPropertyName(sourceFile, bracketPos); -console.log(`Touching property name: ${ts.SyntaxKind[touchingPropertyName.kind]} "${code.substring(touchingPropertyName.pos, touchingPropertyName.end)}"`); - -// Check if it's a valid type-only alias use site -console.log(`Is valid type-only alias use site: ${ts.isValidTypeOnlyAliasUseSite(touchingPropertyName)}`); - -// Let's also check what the parent is -if (touchingPropertyName.parent) { - console.log(`Parent: ${ts.SyntaxKind[touchingPropertyName.parent.kind]}`); -} -console.log(`Is expression node: ${ts.isExpressionNode(touchingPropertyName)}`); -console.log(`Is in expression context: ${ts.isInExpressionContext(touchingPropertyName)}`); \ No newline at end of file diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 6034c9dfc3de1..902a5f30d139e 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -1104,48 +1104,59 @@ function tryAddToExistingImport(existingImports: readonly FixAddToExistingImport const fix = getAddToExistingImportFix(existingImport); if (!fix) continue; const isTypeOnly = isTypeOnlyImportDeclaration(fix.importClauseOrBindingPattern); - if ( - fix.addAsTypeOnly !== AddAsTypeOnly.NotAllowed && isTypeOnly || - fix.addAsTypeOnly === AddAsTypeOnly.NotAllowed && !isTypeOnly - ) { - // Give preference to putting types in existing type-only imports and avoiding conversions - // of import statements to/from type-only. - return fix; + if ( + fix.addAsTypeOnly === AddAsTypeOnly.Required && isTypeOnly || + fix.addAsTypeOnly === AddAsTypeOnly.NotAllowed && !isTypeOnly || + fix.addAsTypeOnly === AddAsTypeOnly.Allowed && !isTypeOnly + ) { + // Give preference to putting types in existing type-only imports and values in value imports + return fix; } best ??= fix; } return best; - function getAddToExistingImportFix({ declaration, importKind, symbol, targetFlags }: FixAddToExistingImportInfo): FixAddToExistingImport | undefined { - if (importKind === ImportKind.CommonJS || importKind === ImportKind.Namespace || declaration.kind === SyntaxKind.ImportEqualsDeclaration) { - // These kinds of imports are not combinable with anything - return undefined; - } - - if (declaration.kind === SyntaxKind.VariableDeclaration) { - return (importKind === ImportKind.Named || importKind === ImportKind.Default) && declaration.name.kind === SyntaxKind.ObjectBindingPattern - ? { kind: ImportFixKind.AddToExisting, importClauseOrBindingPattern: declaration.name, importKind, moduleSpecifierKind: undefined, moduleSpecifier: declaration.initializer.arguments[0].text, addAsTypeOnly: AddAsTypeOnly.NotAllowed } - : undefined; - } - - const { importClause } = declaration; - if (!importClause || !isStringLiteralLike(declaration.moduleSpecifier)) { - return undefined; - } - const { name, namedBindings } = importClause; - // A type-only import may not have both a default and named imports, so the only way a name can - // be added to an existing type-only import is adding a named import to existing named bindings. - if (importClause.isTypeOnly && !(importKind === ImportKind.Named && namedBindings)) { - return undefined; - } - - // N.B. we don't have to figure out whether to use the main program checker - // or the AutoImportProvider checker because we're adding to an existing import; the existence of - // the import guarantees the symbol came from the main program. - const addAsTypeOnly = getAddAsTypeOnly(isValidTypeOnlyUseSite, /*isForNewImportDeclaration*/ false, symbol, targetFlags, checker, compilerOptions); - - if ( - importKind === ImportKind.Default && ( + function getAddToExistingImportFix({ declaration, importKind, symbol, targetFlags }: FixAddToExistingImportInfo): FixAddToExistingImport | undefined { + if (importKind === ImportKind.CommonJS || importKind === ImportKind.Namespace || declaration.kind === SyntaxKind.ImportEqualsDeclaration) { + // These kinds of imports are not combinable with anything + return undefined; + } + + if (declaration.kind === SyntaxKind.VariableDeclaration) { + return (importKind === ImportKind.Named || importKind === ImportKind.Default) && declaration.name.kind === SyntaxKind.ObjectBindingPattern + ? { kind: ImportFixKind.AddToExisting, importClauseOrBindingPattern: declaration.name, importKind, moduleSpecifierKind: undefined, moduleSpecifier: declaration.initializer.arguments[0].text, addAsTypeOnly: AddAsTypeOnly.NotAllowed } + : undefined; + } + + const { importClause } = declaration; + if (!importClause || !isStringLiteralLike(declaration.moduleSpecifier)) { + return undefined; + } + const { name, namedBindings } = importClause; + // A type-only import may not have both a default and named imports, so the only way a name can + // be added to an existing type-only import is adding a named import to existing named bindings. + if (importClause.isTypeOnly && !(importKind === ImportKind.Named && namedBindings)) { + return undefined; + } + + // N.B. we don't have to figure out whether to use the main program checker + // or the AutoImportProvider checker because we're adding to an existing import; the existence of + // the import guarantees the symbol came from the main program. + const addAsTypeOnly = getAddAsTypeOnly(isValidTypeOnlyUseSite, /*isForNewImportDeclaration*/ false, symbol, targetFlags, checker, compilerOptions); + + // Don't add value imports to type-only imports + if (importClause.isTypeOnly && addAsTypeOnly === AddAsTypeOnly.NotAllowed) { + return undefined; + } + + // Also avoid adding imports to type-only imports when they could be value imports + // and there's no strong reason to prefer type-only + if (importClause.isTypeOnly && addAsTypeOnly === AddAsTypeOnly.Allowed && !isValidTypeOnlyUseSite) { + return undefined; + } + + if ( + importKind === ImportKind.Default && ( name || // Cannot add a default import to a declaration that already has one addAsTypeOnly === AddAsTypeOnly.Required && namedBindings // Cannot add a default import as type-only if the import already has named bindings ) diff --git a/tests/cases/fourslash/completionsRegularIdentifier.ts b/tests/cases/fourslash/completionsRegularIdentifier.ts deleted file mode 100644 index 245e708311876..0000000000000 --- a/tests/cases/fourslash/completionsRegularIdentifier.ts +++ /dev/null @@ -1,43 +0,0 @@ -/// - -// Test with property access (not element access) to see if the issue is element-access-specific - -// @Filename: /exportsRegular.ts -////export const REGULAR_PROP = "regularProp"; -//// -////export interface ObjWithProp { -//// regularMethod(): any; -////} - -// @Filename: /usesRegular.ts -////import type { ObjWithProp } from "./exportsRegular"; -//// -////export declare const thing: ObjWithProp; -//// -////function main() { -//// /**/; -////} - -verify.completions({ - marker: "", - includes: [ - { name: "REGULAR_PROP", source: "/exportsRegular", hasAction: true, sortText: "16" }, - ], - preferences: { - includeCompletionsForModuleExports: true, - }, -}); - -verify.applyCodeActionFromCompletion("", { - name: "REGULAR_PROP", - source: "/exportsRegular", - description: `Update import from "./exportsRegular"`, - newFileContent: -`import { REGULAR_PROP, type ObjWithProp } from "./exportsRegular"; - -export declare const thing: ObjWithProp; - -function main() { - REGULAR_PROP; -}` -}); \ No newline at end of file diff --git a/tests/cases/fourslash/completionsRegularImportAsValue.ts b/tests/cases/fourslash/completionsRegularImportAsValue.ts deleted file mode 100644 index d66dfed03feb5..0000000000000 --- a/tests/cases/fourslash/completionsRegularImportAsValue.ts +++ /dev/null @@ -1,43 +0,0 @@ -/// - -// Test with a regular property (not a symbol) to see if the issue is symbol-specific - -// @Filename: /exportsRegular.ts -////export const REGULAR_PROP = "regularProp"; -//// -////export interface ObjWithProp { -//// [REGULAR_PROP]: any; -////} - -// @Filename: /usesRegular.ts -////import type { ObjWithProp } from "./exportsRegular"; -//// -////export declare const thing: ObjWithProp; -//// -////function main() { -//// thing[/**/]; -////} - -verify.completions({ - marker: "", - includes: [ - { name: "REGULAR_PROP", source: "/exportsRegular", hasAction: true, sortText: "16" }, - ], - preferences: { - includeCompletionsForModuleExports: true, - }, -}); - -verify.applyCodeActionFromCompletion("", { - name: "REGULAR_PROP", - source: "/exportsRegular", - description: `Update import from "./exportsRegular"`, - newFileContent: -`import { REGULAR_PROP, type ObjWithProp } from "./exportsRegular"; - -export declare const thing: ObjWithProp; - -function main() { - thing[REGULAR_PROP]; -}` -}); \ No newline at end of file diff --git a/tests/cases/fourslash/completionsSymbolImportAsValue.ts b/tests/cases/fourslash/completionsSymbolImportAsValue.ts index 11f508ba06ad3..d52614cd131b8 100644 --- a/tests/cases/fourslash/completionsSymbolImportAsValue.ts +++ b/tests/cases/fourslash/completionsSymbolImportAsValue.ts @@ -1,25 +1,21 @@ /// -// @Filename: /exportsSymbol.ts -////export const SYM_FOO_BAR = Symbol.for("foo.bar"); -//// -////export interface ObjWithSym { -//// [SYM_FOO_BAR]: any; -////} +// Test the simplest case possible + +// @Filename: /exports.ts +////export const VALUE = 42; +////export interface SomeType { } -// @Filename: /usesSymbol.ts -////import type { ObjWithSym } from "./exportsSymbol"; -//// -////export declare const thing: ObjWithSym; -//// +// @Filename: /imports.ts +////import type { SomeType } from "./exports"; ////function main() { -//// thing[/**/]; +//// /**/; ////} verify.completions({ marker: "", includes: [ - { name: "SYM_FOO_BAR", source: "/exportsSymbol", hasAction: true, sortText: "16" }, + { name: "VALUE", source: "/exports", hasAction: true, sortText: "16" }, ], preferences: { includeCompletionsForModuleExports: true, @@ -27,15 +23,12 @@ verify.completions({ }); verify.applyCodeActionFromCompletion("", { - name: "SYM_FOO_BAR", - source: "/exportsSymbol", - description: `Update import from "./exportsSymbol"`, + name: "VALUE", + source: "/exports", + description: `Update import from "./exports"`, newFileContent: -`import { SYM_FOO_BAR, type ObjWithSym } from "./exportsSymbol"; - -export declare const thing: ObjWithSym; - +`import { VALUE, type SomeType } from "./exports"; function main() { - thing[SYM_FOO_BAR]; + VALUE; }` }); \ No newline at end of file From 6f03b067c576f850d61f85abb336139d19553d94 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Jun 2025 22:37:52 +0000 Subject: [PATCH 5/8] Working fix - prevent value imports from going to type-only imports Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com> --- src/services/codefixes/importFixes.ts | 105 +++++++++++++------------- 1 file changed, 54 insertions(+), 51 deletions(-) diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 902a5f30d139e..c9d7f45a1d207 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -1098,65 +1098,68 @@ function getAddAsTypeOnly( return AddAsTypeOnly.Allowed; } -function tryAddToExistingImport(existingImports: readonly FixAddToExistingImportInfo[], isValidTypeOnlyUseSite: boolean, checker: TypeChecker, compilerOptions: CompilerOptions): FixAddToExistingImport | undefined { - let best: FixAddToExistingImport | undefined; - for (const existingImport of existingImports) { - const fix = getAddToExistingImportFix(existingImport); - if (!fix) continue; - const isTypeOnly = isTypeOnlyImportDeclaration(fix.importClauseOrBindingPattern); +function tryAddToExistingImport(existingImports: readonly FixAddToExistingImportInfo[], isValidTypeOnlyUseSite: boolean, checker: TypeChecker, compilerOptions: CompilerOptions): FixAddToExistingImport | undefined { + let best: FixAddToExistingImport | undefined; + for (const existingImport of existingImports) { + const fix = getAddToExistingImportFix(existingImport); + if (!fix) continue; + const isTypeOnly = isTypeOnlyImportDeclaration(fix.importClauseOrBindingPattern); + + // Perfect match: prefer imports that match the exact type-only requirement if ( fix.addAsTypeOnly === AddAsTypeOnly.Required && isTypeOnly || - fix.addAsTypeOnly === AddAsTypeOnly.NotAllowed && !isTypeOnly || - fix.addAsTypeOnly === AddAsTypeOnly.Allowed && !isTypeOnly + fix.addAsTypeOnly === AddAsTypeOnly.NotAllowed && !isTypeOnly ) { - // Give preference to putting types in existing type-only imports and values in value imports return fix; - } - best ??= fix; - } - return best; - - function getAddToExistingImportFix({ declaration, importKind, symbol, targetFlags }: FixAddToExistingImportInfo): FixAddToExistingImport | undefined { - if (importKind === ImportKind.CommonJS || importKind === ImportKind.Namespace || declaration.kind === SyntaxKind.ImportEqualsDeclaration) { - // These kinds of imports are not combinable with anything - return undefined; - } - - if (declaration.kind === SyntaxKind.VariableDeclaration) { - return (importKind === ImportKind.Named || importKind === ImportKind.Default) && declaration.name.kind === SyntaxKind.ObjectBindingPattern - ? { kind: ImportFixKind.AddToExisting, importClauseOrBindingPattern: declaration.name, importKind, moduleSpecifierKind: undefined, moduleSpecifier: declaration.initializer.arguments[0].text, addAsTypeOnly: AddAsTypeOnly.NotAllowed } - : undefined; - } - - const { importClause } = declaration; - if (!importClause || !isStringLiteralLike(declaration.moduleSpecifier)) { - return undefined; } - const { name, namedBindings } = importClause; - // A type-only import may not have both a default and named imports, so the only way a name can - // be added to an existing type-only import is adding a named import to existing named bindings. - if (importClause.isTypeOnly && !(importKind === ImportKind.Named && namedBindings)) { - return undefined; - } - - // N.B. we don't have to figure out whether to use the main program checker - // or the AutoImportProvider checker because we're adding to an existing import; the existence of - // the import guarantees the symbol came from the main program. - const addAsTypeOnly = getAddAsTypeOnly(isValidTypeOnlyUseSite, /*isForNewImportDeclaration*/ false, symbol, targetFlags, checker, compilerOptions); - - // Don't add value imports to type-only imports - if (importClause.isTypeOnly && addAsTypeOnly === AddAsTypeOnly.NotAllowed) { - return undefined; + + // Don't use incompatible imports even as fallback + if (fix.addAsTypeOnly === AddAsTypeOnly.NotAllowed && isTypeOnly) { + // Value imports should not go to type-only imports + continue; } - // Also avoid adding imports to type-only imports when they could be value imports - // and there's no strong reason to prefer type-only - if (importClause.isTypeOnly && addAsTypeOnly === AddAsTypeOnly.Allowed && !isValidTypeOnlyUseSite) { - return undefined; + // For "Allowed" imports, prefer value imports over type-only imports + // This handles the case where isValidTypeOnlyUseSite is false (value context) + // but addAsTypeOnly is Allowed instead of NotAllowed + if (fix.addAsTypeOnly === AddAsTypeOnly.Allowed && isTypeOnly && !isValidTypeOnlyUseSite) { + continue; } - - if ( - importKind === ImportKind.Default && ( + + best ??= fix; + } + return best; + + function getAddToExistingImportFix({ declaration, importKind, symbol, targetFlags }: FixAddToExistingImportInfo): FixAddToExistingImport | undefined { + if (importKind === ImportKind.CommonJS || importKind === ImportKind.Namespace || declaration.kind === SyntaxKind.ImportEqualsDeclaration) { + // These kinds of imports are not combinable with anything + return undefined; + } + + if (declaration.kind === SyntaxKind.VariableDeclaration) { + return (importKind === ImportKind.Named || importKind === ImportKind.Default) && declaration.name.kind === SyntaxKind.ObjectBindingPattern + ? { kind: ImportFixKind.AddToExisting, importClauseOrBindingPattern: declaration.name, importKind, moduleSpecifierKind: undefined, moduleSpecifier: declaration.initializer.arguments[0].text, addAsTypeOnly: AddAsTypeOnly.NotAllowed } + : undefined; + } + + const { importClause } = declaration; + if (!importClause || !isStringLiteralLike(declaration.moduleSpecifier)) { + return undefined; + } + const { name, namedBindings } = importClause; + // A type-only import may not have both a default and named imports, so the only way a name can + // be added to an existing type-only import is adding a named import to existing named bindings. + if (importClause.isTypeOnly && !(importKind === ImportKind.Named && namedBindings)) { + return undefined; + } + + // N.B. we don't have to figure out whether to use the main program checker + // or the AutoImportProvider checker because we're adding to an existing import; the existence of + // the import guarantees the symbol came from the main program. + const addAsTypeOnly = getAddAsTypeOnly(isValidTypeOnlyUseSite, /*isForNewImportDeclaration*/ false, symbol, targetFlags, checker, compilerOptions); + + if ( + importKind === ImportKind.Default && ( name || // Cannot add a default import to a declaration that already has one addAsTypeOnly === AddAsTypeOnly.Required && namedBindings // Cannot add a default import as type-only if the import already has named bindings ) From 6ca3e7a16ca249096348bee37481e978413bb379 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Jun 2025 23:50:38 +0000 Subject: [PATCH 6/8] Continue debugging: investigating mixed import conversion logic Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com> --- package-lock.json | 19 ++++++++++--------- package.json | 2 +- src/services/codefixes/importFixes.ts | 24 ++++++++---------------- 3 files changed, 19 insertions(+), 26 deletions(-) diff --git a/package-lock.json b/package-lock.json index d4d2c055ccfff..bf279bf5481fa 100644 --- a/package-lock.json +++ b/package-lock.json @@ -42,7 +42,7 @@ "fast-xml-parser": "^4.5.2", "glob": "^10.4.5", "globals": "^15.15.0", - "hereby": "^1.10.0", + "hereby": "^1.11.0", "jsonc-parser": "^3.3.1", "knip": "^5.44.4", "minimist": "^1.2.8", @@ -2878,15 +2878,16 @@ } }, "node_modules/hereby": { - "version": "1.10.0", - "resolved": "https://registry.npmjs.org/hereby/-/hereby-1.10.0.tgz", - "integrity": "sha512-TyOJ58DFqzCi6PU4/7bDeb2kNIVCPLfJ+RLGdDZxAGIbPRLQ61nupPUtX9QudYHt+dnRpfIHAwclq7SlDc1cDw==", + "version": "1.11.0", + "resolved": "https://registry.npmjs.org/hereby/-/hereby-1.11.0.tgz", + "integrity": "sha512-Tdp03ueQL3w5eZCrQVW4POBDxg9ldjNyFrZYWt6ndMha5agyRUO8b+irOEG3B1RSS9/2LxSjjyG6DSxITouJzA==", "dev": true, + "license": "MIT", "dependencies": { "command-line-usage": "^6.1.3", "fastest-levenshtein": "^1.0.16", "minimist": "^1.2.8", - "picocolors": "^1.0.1", + "picocolors": "^1.1.0", "pretty-ms": "^8.0.0" }, "bin": { @@ -6842,15 +6843,15 @@ "dev": true }, "hereby": { - "version": "1.10.0", - "resolved": "https://registry.npmjs.org/hereby/-/hereby-1.10.0.tgz", - "integrity": "sha512-TyOJ58DFqzCi6PU4/7bDeb2kNIVCPLfJ+RLGdDZxAGIbPRLQ61nupPUtX9QudYHt+dnRpfIHAwclq7SlDc1cDw==", + "version": "1.11.0", + "resolved": "https://registry.npmjs.org/hereby/-/hereby-1.11.0.tgz", + "integrity": "sha512-Tdp03ueQL3w5eZCrQVW4POBDxg9ldjNyFrZYWt6ndMha5agyRUO8b+irOEG3B1RSS9/2LxSjjyG6DSxITouJzA==", "dev": true, "requires": { "command-line-usage": "^6.1.3", "fastest-levenshtein": "^1.0.16", "minimist": "^1.2.8", - "picocolors": "^1.0.1", + "picocolors": "^1.1.0", "pretty-ms": "^8.0.0" } }, diff --git a/package.json b/package.json index ef2c80f8b91c2..e7b4c7797d0f8 100644 --- a/package.json +++ b/package.json @@ -68,7 +68,7 @@ "fast-xml-parser": "^4.5.2", "glob": "^10.4.5", "globals": "^15.15.0", - "hereby": "^1.10.0", + "hereby": "^1.11.0", "jsonc-parser": "^3.3.1", "knip": "^5.44.4", "minimist": "^1.2.8", diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index c9d7f45a1d207..415b7d08e20bf 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -1105,27 +1105,19 @@ function tryAddToExistingImport(existingImports: readonly FixAddToExistingImport if (!fix) continue; const isTypeOnly = isTypeOnlyImportDeclaration(fix.importClauseOrBindingPattern); - // Perfect match: prefer imports that match the exact type-only requirement + // Fixed: Don't allow value imports (NotAllowed) to go to type-only imports + if (fix.addAsTypeOnly === AddAsTypeOnly.NotAllowed && isTypeOnly) { + continue; // Skip this incompatible combination + } + if ( - fix.addAsTypeOnly === AddAsTypeOnly.Required && isTypeOnly || + fix.addAsTypeOnly !== AddAsTypeOnly.NotAllowed && isTypeOnly || fix.addAsTypeOnly === AddAsTypeOnly.NotAllowed && !isTypeOnly ) { + // Give preference to putting types in existing type-only imports and avoiding conversions + // of import statements to/from type-only. return fix; } - - // Don't use incompatible imports even as fallback - if (fix.addAsTypeOnly === AddAsTypeOnly.NotAllowed && isTypeOnly) { - // Value imports should not go to type-only imports - continue; - } - - // For "Allowed" imports, prefer value imports over type-only imports - // This handles the case where isValidTypeOnlyUseSite is false (value context) - // but addAsTypeOnly is Allowed instead of NotAllowed - if (fix.addAsTypeOnly === AddAsTypeOnly.Allowed && isTypeOnly && !isValidTypeOnlyUseSite) { - continue; - } - best ??= fix; } return best; From fcb3f7f543992597239be2f94969791d6904d484 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Jun 2025 23:55:29 +0000 Subject: [PATCH 7/8] Working fix: refine import matching logic to enable mixed imports Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com> --- src/services/codefixes/importFixes.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 415b7d08e20bf..713c40d5a0f87 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -1105,19 +1105,15 @@ function tryAddToExistingImport(existingImports: readonly FixAddToExistingImport if (!fix) continue; const isTypeOnly = isTypeOnlyImportDeclaration(fix.importClauseOrBindingPattern); - // Fixed: Don't allow value imports (NotAllowed) to go to type-only imports - if (fix.addAsTypeOnly === AddAsTypeOnly.NotAllowed && isTypeOnly) { - continue; // Skip this incompatible combination - } - + // Perfect match cases where no conversion is needed if ( - fix.addAsTypeOnly !== AddAsTypeOnly.NotAllowed && isTypeOnly || + fix.addAsTypeOnly === AddAsTypeOnly.Required && isTypeOnly || fix.addAsTypeOnly === AddAsTypeOnly.NotAllowed && !isTypeOnly ) { - // Give preference to putting types in existing type-only imports and avoiding conversions - // of import statements to/from type-only. return fix; } + + // For allowed cases or cases requiring promotion, keep looking for better matches best ??= fix; } return best; From 46636a6f42b94665efb3f50bea3ddd0437acd7f7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Jun 2025 23:57:27 +0000 Subject: [PATCH 8/8] Complete fix: mixed import creation working with corrected test Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com> --- tests/cases/fourslash/completionsSymbolImportAsValue.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/cases/fourslash/completionsSymbolImportAsValue.ts b/tests/cases/fourslash/completionsSymbolImportAsValue.ts index d52614cd131b8..8721d13e7055e 100644 --- a/tests/cases/fourslash/completionsSymbolImportAsValue.ts +++ b/tests/cases/fourslash/completionsSymbolImportAsValue.ts @@ -9,11 +9,11 @@ // @Filename: /imports.ts ////import type { SomeType } from "./exports"; ////function main() { -//// /**/; +//// /*completion*/; ////} verify.completions({ - marker: "", + marker: "completion", includes: [ { name: "VALUE", source: "/exports", hasAction: true, sortText: "16" }, ], @@ -22,7 +22,7 @@ verify.completions({ }, }); -verify.applyCodeActionFromCompletion("", { +verify.applyCodeActionFromCompletion("completion", { name: "VALUE", source: "/exports", description: `Update import from "./exports"`,