Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 22, 2025

Port codeactions import fixes to new autoimport stack

Summary

This PR continues the migration of auto-import codefixes from the old SymbolExportInfo-based system to the new autoimport.Registry/autoimport.View infrastructure.

Changes

JSDoc import type fixes

  • Added UsagePosition field to Fix struct
  • Added usagePosition parameter to GetFixes() function
  • Implemented AutoImportFixKindJsdocTypeImport case in Edits() to insert import("./a").T prefix
  • For JS files with pure type exports, uses inline import() syntax instead of adding import statements

Type-only import handling

  • Added isValidTypeOnlyUseSite parameter to GetFixes() and GetCompletions()
  • Added getAddAsTypeOnly() function to compute the AddAsTypeOnly value
  • Fixed AddToExisting to properly use addAsTypeOnly when creating import specifiers
  • Fixed topLevelTypeOnly condition to handle named-imports-only case

Bug fixes

  • Fixed panic in tryAddToExistingImport for side-effect imports (check for nil ImportClause)

Tests now passing

  • TestImportNameCodeFix_importType (JSDoc import type in JS files)
  • TestImportNameCodeFix_importType3 (type-only import specifiers)
  • TestImportNameCodeFixExistingImport5 (side-effect imports)
  • TestImportNameCodeFix_typeOnly

Tests remaining: 19 failing

Most remaining failures are related to:

  • Default export handling
  • CommonJS/synthetic imports
  • JSX-related imports
  • Namespace qualification fixes
Original prompt

I'm in the middle of a rewrite of auto-imports. Auto-imports is a feature exposed by two language service features: completions and codefixes. #file:completions.go has already been rewritten to use the new stack, located in the #file:autoimport directory. #file:codeactions_importfixes.go is codefixes, and I just barely started to swap it over to the new stack. I want you to work on continuing the port. The current code calls into many functions in #file:autoimport.go, #file:autoimportsexportinfo.go, and #file:autoimportfixes.go, which will eventually be completely deleted. The export info map in particular is fully supplanted by the new data structure (autoimport.Registry, exposed through autoimport.View). A lot of things on the export-finding part of the pipeline will have to be rewritten or adapted because the new data structure doesn't store Symbols or Nodes directly, instead cheaply pre-computing relevant info and storing it in the registry. Much of the fix-generation code, though, can be reused with modest adaptations. Many functions have already been copied and adapted into #file:fix.go. More functions will likely need to be copied from the current auto-import files in ls into the ls/autoimport directory. Do not delete any of the old stack yet, just copy and adapt as necessary.

Important: I do not expect you to cleanly finish this job, I just want you to make some progress on it. I will finish later, so don't make any big design decisions. In particular, do whatever you can do without modifying the autoimport.Registry or autoimport.exportExtractor. There may be some features that cannot be completed without making changes to the export extractor, but just do what you can without touching that.

The user has attached the following file paths as relevant context:

  • internal/ls/autoimport/fix.go
  • internal/ls/autoimportfixes.go
  • internal/ls/autoimportsexportinfo.go
  • internal/project/autoimport.go
  • internal/ls/codeactions_importfixes.go
  • internal/ls/autoimport
  • internal/ls/completions.go
  • .github/copilot-instructions.md

Created from VS Code.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Continue porting codefixes to new auto-import stack Port codeactions import fixes to new autoimport stack Nov 22, 2025
Copilot finished work on behalf of andrewbranch November 22, 2025 01:37
Copilot AI requested a review from andrewbranch November 22, 2025 01:37
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot this looks like a good start. Instead of mixing the new fix and old fix types, can you keep the existing logic for UMD and type-only fixes, but modify it to produce autoimport.Fix types? You can add fields to that struct if necessary.

Copy link
Contributor Author

Copilot AI commented Nov 24, 2025

@copilot this looks like a good start. Instead of mixing the new fix and old fix types, can you keep the existing logic for UMD and type-only fixes, but modify it to produce autoimport.Fix types? Yo...

Done in 675a3f4. I've converted both UMD and type-only fixes to produce autoimport.Fix types:

  • Added TypeOnlyAliasDeclaration field to autoimport.Fix struct
  • Implemented PromoteTypeOnly case in Fix.Edits() method
  • Moved promoteFromTypeOnly and helper functions to the autoimport package
  • Created convertImportFixToAutoImportFix() adapter for UMD fixes (which still use the old getImportFixes internally but convert to the new type)
  • Removed the oldFix field from fixInfo - all fixes now consistently use autoimport.Fix

Copilot AI requested a review from andrewbranch November 24, 2025 16:35
Copilot finished work on behalf of andrewbranch November 24, 2025 16:35
@andrewbranch
Copy link
Member

@copilot I updated the base branch to finish eliminating usage of the old types from codeactions_importfixes.go. There are now 25 tests failing with go test ./internal/fourslash/tests/gen/..., mostly from functionality that hasn't been ported from the old stack's fix logic to the new fix.go. It should be straightforward to make some progress on those tests now.

Copy link
Contributor Author

Copilot AI commented Nov 25, 2025

@copilot I updated the base branch to finish eliminating usage of the old types from codeactions_importfixes.go. There are now 25 tests failing with go test ./internal/fourslash/tests/gen/..., mostl...

I've made progress on porting the fix logic. Key changes:

  1. PromoteTypeOnly case - Added to Fix.Edits() with promoteFromTypeOnly, promoteImportClause, deleteTypeKeyword helpers
  2. Type-only handling - Added isValidTypeOnlyUseSite param to GetFixes/GetCompletions, HasTypeOnlyAlias field to Export, and getAddAsTypeOnly function
  3. Fixed topLevelTypeOnly logic - Changed defaultImport != nil to (defaultImport == nil || ...) to handle named-imports-only case

This fixes TestImportNameCodeFix_importType5, importType6, typeOnly, and AutoImportTypeOnlyPreferred1. ~21 tests remain (namespace imports, JSX, CommonJS features not yet ported).

Note: There's a rebase conflict with the remote branch that prevented pushing.

Copilot finished work on behalf of andrewbranch November 25, 2025 00:12
Copilot AI and others added 3 commits November 24, 2025 16:14
- Replace getExportInfos() with view.Search() to find exports
- Replace getImportFixes() with view.GetFixes() for each export
- Use autoimport.Fix.Edits() instead of codeActionForFixWorker for new fixes
- Export View.CompareFixes for use in sorting
- Keep UMD and type-only promotion using old stack for now (via oldFix field)
- Update sortFixInfo to handle both new and old-style fixes

Co-authored-by: andrewbranch <[email protected]>
Co-authored-by: andrewbranch <[email protected]>
- Add TypeOnlyAliasDeclaration field to autoimport.Fix
- Implement PromoteTypeOnly case in Fix.Edits()
- Move promoteFromTypeOnly and helper functions to autoimport package
- Convert UMD fixes from old ImportFix to new autoimport.Fix via converter
- Remove oldFix field from fixInfo struct - now all fixes use autoimport.Fix
- Simplify getImportCodeActions to only handle autoimport.Fix type

Co-authored-by: andrewbranch <[email protected]>
@andrewbranch andrewbranch force-pushed the copilot/continue-port-codeactions-importfixes branch from 675a3f4 to 1bcf069 Compare November 25, 2025 00:17
@andrewbranch
Copy link
Member

@copilot I resolved merge conflicts, but I think you had some work that never got pushed, related to this:

Added isValidTypeOnlyUseSite parameter to GetFixes() and GetCompletions(), HasTypeOnlyAlias field to Export struct, and getAddAsTypeOnly() function to compute proper AddAsTypeOnly value

I don't know if you're able to retrieve it. At any rate, you should be able to continue without problems now. Please keep working on the failing tests. Prioritize ones that currently panic, followed by ones that return no codefixes but should.

Pro tip: for tests that report a panic like

fourslash.go:1211: At position (Ln 0, Col 0): Unexpected textDocument/codeAction response type: <nil>, error: [-32603]: InternalError: panic handling request textDocument/codeAction: runtime error: invalid memory address or nil pointer dereference

you can better see the stack trace by temporarily commenting out the body of (*Server).recover in internal/lsp/server.go.

- Fix nil pointer dereference when accessing ImportClause on side-effect imports
- Add isValidTypeOnlyUseSite parameter to GetFixes and GetCompletions
- Add getAddAsTypeOnly function to compute AddAsTypeOnly value
- Fix topLevelTypeOnly logic to handle named-imports-only case correctly
- Propagate AddAsTypeOnly through fix generation pipeline

Co-authored-by: andrewbranch <[email protected]>
Copy link
Contributor Author

Copilot AI commented Nov 25, 2025

@copilot I resolved merge conflicts, but I think you had some work that never got pushed, related to this:

Added isValidTypeOnlyUseSite parameter to GetFixes() and GetCompletions(), HasTypeOnlyAl...

I've recovered and pushed the changes. Fixed in commit 1e0ca2b:

  1. Fixed panics - Nil check for ImportClause before accessing it (fixes side-effect import handling)
  2. Added type-only import support - isValidTypeOnlyUseSite parameter, getAddAsTypeOnly function, and fixed topLevelTypeOnly logic

Tests fixed: TestImportNameCodeFixExistingImport5, TestImportNameCodeFix_typeOnly

Remaining 21 tests need:

  • Namespace qualification fixes (tryUseExistingNamespaceImport) - e.g., ns.foo() instead of adding new import
  • Default export search (searching by usage name should find renameable default exports)
  • JSDoc import type syntax for JS files

Copilot finished work on behalf of andrewbranch November 25, 2025 00:52
@andrewbranch
Copy link
Member

@copilot can you focus on the JSDoc import type fixes next please

- Add UsagePosition field to Fix struct for JSDoc import type fixes
- Add usagePosition parameter to GetFixes function
- Implement AutoImportFixKindJsdocTypeImport case in Edits() to insert import(...) prefix
- Check if file is JS and export is pure type to use JSDoc import type syntax
- Fix AddToExisting to properly use addAsTypeOnly for import specifiers

Fixes:
- TestImportNameCodeFix_importType (JSDoc import type in JS files)
- TestImportNameCodeFix_importType3 (type-only import specifiers)

Co-authored-by: andrewbranch <[email protected]>
Copy link
Contributor Author

Copilot AI commented Nov 25, 2025

@copilot can you focus on the JSDoc import type fixes next please

Implemented in 46f3e40. Added support for JSDoc import type fixes:

  1. Added UsagePosition field to Fix struct and usagePosition parameter to GetFixes()
  2. For JS files with pure type exports, generates import("./a").T syntax instead of adding import statements
  3. Also fixed type-only import specifiers - AddToExisting now properly uses addAsTypeOnly when creating import { type SomeClass } style imports

Tests fixed: TestImportNameCodeFix_importType, TestImportNameCodeFix_importType3

Change silent failure to panic with descriptive message when UsagePosition is nil

Co-authored-by: andrewbranch <[email protected]>
Copilot finished work on behalf of andrewbranch November 25, 2025 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants