Throw together an auto-import fix implementation #2053
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements auto-import fix functionality (code actions) for the TypeScript language server port. It adds support for generating quick-fix code actions that automatically add missing import statements when identifiers cannot be found.
Key changes:
- New code actions handler that detects import-related diagnostics and suggests fixes
- LSP server integration for the
textDocument/codeActionrequest - Enhanced test infrastructure to support fourslash tests with implicit first files
- Utility function to check symbol meaning compatibility
Reviewed Changes
Copilot reviewed 170 out of 170 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/ls/codeactions.go |
New file implementing code action provider with auto-import fix logic |
internal/lsp/server.go |
Registers code action handler and advertises capability to clients |
internal/ls/utilities.go |
Adds symbolFlagsHaveMeaning helper for filtering exports by semantic meaning |
internal/testrunner/test_case_parser.go |
Adds support for implicit first file in fourslash tests |
internal/fourslash/fourslash.go |
Implements VerifyImportFixAtPosition test method |
internal/fourslash/test_parser.go |
Updates parser to use new implicit first file option |
internal/fourslash/_scripts/convertFourslash.mts |
Adds conversion support for importFixAtPosition verification |
internal/fourslash/_scripts/failingTests.txt |
Tracks test status (many still failing as noted in PR description) |
| Multiple test files | Adds 160+ fourslash tests for import fix scenarios |
| if e != nil { | ||
| parseError = e | ||
| break | ||
| if currentFileContent.Len() != 0 { |
There was a problem hiding this comment.
[nitpick] The logic for handling implicit first file content (lines 182-213) has complex nested conditions that could be simplified. Consider extracting the implicit first file handling into a separate helper function to improve readability and reduce nesting depth.
| ch, | ||
| l.GetProgram(), | ||
| func(moduleSymbol *ast.Symbol, moduleFile *ast.SourceFile, ch *checker.Checker, isFromPackageJson bool) { | ||
| if moduleCount = moduleCount + 1; moduleCount%100 == 0 && ctx.Err() != nil { |
There was a problem hiding this comment.
[nitpick] The moduleCount increment and context check is condensed into a single line with multiple side effects. This would be clearer as separate statements for better readability and debugging.
| for _, textChange := range edits { | ||
| start := int(f.converters.LineAndCharacterToPosition(script, textChange.Range.Start)) | ||
| end := int(f.converters.LineAndCharacterToPosition(script, textChange.Range.End)) | ||
| deletedText := originalContent[start:end] | ||
| insertedText := textChange.NewText | ||
| f.editScriptAndUpdateMarkers(t, f.activeFilename, start, start+len(insertedText), deletedText) | ||
| } |
There was a problem hiding this comment.
The undo logic for reverting text edits is not straightforward. Adding a comment explaining that this reverses each edit by replacing the inserted text with the original deleted text would help future maintainers understand this non-obvious code.
Fixes #1658
This is not perfect. It passes maybe 60 out of 160 ported fourslash tests so far. Many issues exist with import sorting or formatting, and there are other diagnostics fixed by import fixes that are not being addressed here (like swapping a non-type to a type), and there are other auto-imports that seem weird that I need to double check.
But for the typical cases of just missing an import, this seems to work.