-
Notifications
You must be signed in to change notification settings - Fork 686
autoimport completions #1553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
autoimport completions #1553
Conversation
I'm gonna closely follow this one... |
bug squashing atm :). Hopefully it'll be ready soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements autoimports functionality, enabling automatic import suggestions through completions in the TypeScript language server. The implementation adds the ability to suggest and automatically import symbols from other modules when typing identifiers.
Key changes include:
- Added comprehensive autoimport completion system with export info mapping and module specifier resolution
- Implemented import statement completion support for various import syntaxes
- Added text change tracking infrastructure for code modifications
Reviewed Changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
internal/ls/completions.go |
Main autoimport logic including export collection, module specifier resolution, and completion item generation |
internal/ls/autoimportstypes.go |
Type definitions for import/export kinds, import fixes, and related data structures |
internal/ls/changetrackerimpl.go |
Change tracking implementation for applying text edits and code modifications |
internal/printer/changetrackerwriter.go |
Writer implementation for tracking position changes during code generation |
internal/modulespecifiers/specifiers.go |
Enhanced module specifier generation with autoimport support |
internal/ls/organizeimports.go |
Import organization utilities and comparison functions |
Various utility files | Helper functions, type conversions, and API enhancements to support autoimport functionality |
@@ -139,7 +139,7 @@ func getActualIndentationForListItem(node *ast.Node, sourceFile *ast.SourceFile, | |||
// VariableDeclarationList has no wrapping tokens | |||
return -1 | |||
} | |||
containingList := getContainingList(node, sourceFile) | |||
containingList := GetContainingList(node, sourceFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know this func existed; I wonder if it's general enough to use for sig help. (No action needed, just opining)
} | ||
|
||
func findFirstNonWhitespaceColumn(startPos int, endPos int, sourceFile *ast.SourceFile, options *FormatCodeSettings) int { | ||
func FindFirstNonWhitespaceColumn(startPos int, endPos int, sourceFile *ast.SourceFile, options *FormatCodeSettings) int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am suspicious of this function, given it appears to return the column in terms of runes? I'm not sure it walks the chars correctly either, hmm...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I was writing when I first went "wait, do we work in runes or in byte offsets for columns? Looks inconsistent right now". Both ways look wrong to some part of the stack around here because of that inconsistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we need everything to be UTF-8 byte offsets, until they hit the LS and get converted based on the client preferences.
The only exception might be symbol baselines (UTF-16 compat?), source maps, and maybe the API, and maybe printed diagnostics? (That's a lot of exceptions...)
Related, #1578
The import in the demo is inserted in a non-pretty way; is that something being worked on? |
Yes, this is a formatting bug/limitation. Strada formats with a Edit: Fixed with help from @weswigham New statements come out pretty now (haven't updated the demo vid yet tho) |
internal/fourslash/tests/gen/completionsObjectLiteralModuleExports_test.go
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,145 @@ | |||
package fourslash_test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically the tests in internal/fourslash/tests/manual/
are supposed to be generated tests that had to be manually modified, and new tests go into internal/fourslash/tests/
, though that doesn't have practical differences I think other than our own organization of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I would suggest moving this file for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, just had a couple comments and some (many? 😅) questions
…to generated nodes
var initialIndentation, delta int | ||
if options.indentation == nil { | ||
// !!! indentation for position | ||
// initialIndentation = format.GetIndentationForPos(pos, sourceFile, formatOptions, options.prefix == ct.newLine || scanner.GetLineStartPositionForPosition(pos, targetFileLineMap) == pos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakebailey That part of the formatter isn't ported yet (getSmartIndent
) and involves the FindFirstNonWhitespaceColumn
functions, so I didn't include it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't assume anything else is gonna necessitate porting more parts of getSmartIndent
- they're only nominally "in the formatter" - none of the FormatX
entrypoints use anything as-yet unported. Anything else is basically just part of the quickfix pipeline that lives elsewhere, for some reason.
isFromPackageJson bool | ||
} | ||
|
||
type exportInfoMap struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have liked to see this data structure rethought, for a few reasons:
- Storing symbols is problematic; that's why there's a lot of complexity around serializing and "rehydrating" symbols. Some symbols are checker-generated, which we have to be extra careful of now that we have multiple checkers.
- A lot of up-front checker work goes into building this data structure, which is super tailored to providing global completions with no prefix text, which was justified in Strada by caching it across most edits to the same file. Without that caching in place, or even in a future where we rethink the caching strategy, building this wastes a lot of work a lot of the time.
- The two points above make it unclear how to integrate package.json auto imports - it would be nice to be able to process those without a program/checker, but this data structure relies on having those.
- "Rehydrating" symbol info across requests has been a source of crashes in the past, so it might pay off to revisit from a clean slate.
- A checker-free search of files could be massively parallel; building this is going to be single-threaded over potentially thousands of files.
All that said, it seems like the overriding motivation is to get some form of auto imports implemented fast, so a direct port makes sense. But I see a strong potential that this goes the way of the initial project service port, with a relatively short lifetime before getting a proper rewrite.
var exportInfos []*SymbolExportInfo | ||
// The new way: `exportMapKey` should be in the `data` of each auto-import completion entry and | ||
// sent back when asking for details. | ||
exportInfos = l.getExportInfoMap(ch, sourceFile, preferences).get(sourceFile.Path(), ch, exportMapKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need to pass ctx
and check for cancellation where Strada does.
cb func(*resolvingModuleSpecifiersForCompletions) *ImportFix, | ||
) *ImportFix { | ||
// !!! timestamp | ||
// Under `--moduleResolution nodenext` or `bundler`, we have to resolve module specifiers up front, because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of code needs to be deleted, because these are the only module resolution modes existing in Corsa / 6.0. All auto import completions should be "fully resolved," which should dramatically simplify the code in this file.
@@ -4654,6 +5094,11 @@ func hasCompletionItem(clientOptions *lsproto.CompletionClientCapabilities) bool | |||
return clientOptions != nil && clientOptions.CompletionItem != nil | |||
} | |||
|
|||
// strada TODO: this function is, at best, poorly named. Use sites are pretty suspicious. | |||
func compilerOptionsIndicateEsModules(options *core.CompilerOptions) bool { | |||
return options.Module == core.ModuleKindNone || options.GetEmitScriptTarget() >= core.ScriptTargetES2015 || options.NoEmit.IsTrue() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always true in Corsa / 6.0
@@ -23,6 +23,11 @@ func (l *LanguageService) GetProgram() *compiler.Program { | |||
return l.host.GetProgram() | |||
} | |||
|
|||
func (l *LanguageService) GetPackageJsonAutoImportProvider() *compiler.Program { | |||
// !!! packageJsonAutoImportProvider | |||
return l.GetProgram() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be stubbed this way; couldn't this cause duplicate work?
This PR implements autoimports, and allows symbols to be autoimported through completions. Also ports the basic functionalities of the changetracker (adds changes and calculates/formats the output string).
demo (note: the formatting has been fixed, see test cases)
corsa.completion.autoimports.1.mp4
Notes on implementation:
ExportInfoMap
,ImportMap
, etc) have been ported for functionality, but are not saved between server requests). After the ls is more fully implemented and perf tested, we will revisit to see what caching we need to doIncludeCompletionsForModuleExports
andIncludeCompletionsForImportStatements
in theUserPreferences
passed into any completion get/resolve request has been set totrue
to allow autoimports to work autoimport completions #1553 (comment)Related features to be implemented after this PR: