-
Notifications
You must be signed in to change notification settings - Fork 804
New auto-import infrastructure #2390
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
Conversation
gabritto
left a comment
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.
Mostly done reviewing, looks good, just had some questions
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.
The tests that differ in a good way should be updated then and not show up here, ideally.
| ```ts | ||
| // @FileName: /project-b/index.ts | ||
| pkg_/**/ | ||
| ``````ts |
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 think auto-imports baseline is missing a newline here? Should be ```\n``` ts
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.
Yes, but it was always like this, so I don't want to change every baseline in this PR.
| // GetLanguageServiceWithAutoImports clones the current snapshot with a request to | ||
| // prepare auto-imports for the given URI, then returns a LanguageService for the | ||
| // default project of that URI. It should only be called after GetLanguageService. | ||
| // !!! take snapshot that GetLanguageService initially returned |
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.
Will we actually need to take the original LS snapshot? At least from the current usage of GetLanguageServiceWithAutoImports, the old snapshot is only used to determine whether we need autoimports, so I don't see why we'd need to worry about using a possibly newer snapshot...
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 forgot to revisit this TODO. The completions request was made against a certain document version, and when we come back to here, we are continuing that request. If the document changed in the meantime, the document position might not make sense anymore. I think in theory this is important, but in practice, there are only a couple ms during which things could have changed, and if a change did sneak into that window, it's probably a single character insertion, which wouldn't change things much. It would add a little complexity to handle the theoretical edge cases correctly, so I'm on the fence about whether to address it now. I'll try to take a look and see how bad it is.
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.
Ah, I see. Yeah, I remember I had to do some handling of this in #1871, so it may be worth fixing.
| TestAutoImportNodeNextJSRequire | ||
| TestAutoImportPackageJsonImportsCaseSensitivity | ||
| TestAutoImportProvider_exportMap1 | ||
| TestAutoImportPackageRootPath |
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 one is failing because of the note I made about not searching node_modules for untyped JS. We might improve that later so I'll leave it marked failing here.
| TestAutoImportPackageJsonImportsCaseSensitivity | ||
| TestAutoImportProvider_exportMap1 | ||
| TestAutoImportPackageRootPath | ||
| TestAutoImportPathsNodeModules |
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 failure represents a new edge case limitation that would incur a performance penalty to fix, but can be worked around with user preferences. The test asserts that auto-imports respects tsconfig.json paths as an override for a node_modules package’s module specifier. Now that node_modules specifiers are computed outside the context of a program and compiler options, we short circuit long before the code that comes up with the paths-based specifier. I don't want to support this and make everyone who uses paths pay the cost of us double checking every node_modules specifier against their paths, but a user could make it work by excluding the node_modules specifier in the autoImportSpecifierExcludeRegexes user preference, which would make us fall back to trying the older slower specifier generation that takes paths into account.
I opted to leave "wontfix" issues like this as failing rather than move them to manual and reverse the assertion.
| TestAutoImportProvider_wildcardExports3 | ||
| TestAutoImportProvider4 | ||
| TestAutoImportProvider9 | ||
| TestAutoImportRelativePathToMonorepoPackage |
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 a wontfix. The test contains a file that imports from a monorepo package via relative path instead of via node_modules package specifier, and asserts that an import fix of a different file from the same monorepo package also gets written as a relative path, overriding default specifier generation ranking by detecting an unusual pattern that already exists in the file.
| TestCompletionListInNamedFunctionExpression1 | ||
| TestCompletionListInNamedFunctionExpressionWithShadowing | ||
| TestCompletionListInScope | ||
| TestCompletionListInScope_doesNotIncludeAugmentations |
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 think I noted this difference in the description. We now offer auto-imports from module augmentations that don't resolve to anything. I think this has pretty neutral desirability. It will result in a checker error if you accept the import, but maybe it would be fixed after an npm install. It doesn't seem very important. I guess I could move this one to manual and reverse the assertion.
| TestCompletionsImport_details_withMisspelledName | ||
| TestCompletionsImport_exportEquals_global | ||
| TestCompletionsImport_filteredByInvalidPackageJson_direct | ||
| TestCompletionsImport_filteredByPackageJson_ambient |
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.
The behavior of this one improved. I'll move it.
| TestCompletionsImport_filteredByPackageJson_typesOnly | ||
| TestCompletionsImport_jsxOpeningTagImportDefault | ||
| TestCompletionsImport_mergedReExport | ||
| TestCompletionsImport_importType |
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 I meant to skip this; it was unintentionally broken. Fixing.
| TestCompletionsImport_mergedReExport | ||
| TestCompletionsImport_importType | ||
| TestCompletionsImport_named_didNotExistBefore | ||
| TestCompletionsImport_named_namespaceImportExists |
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.
Same, for the same root cause as _importType. Fixing.
| TestImportNameCodeFix_uriStyleNodeCoreModules2 | ||
| TestImportNameCodeFix_uriStyleNodeCoreModules3 | ||
| TestImportNameCodeFix_withJson | ||
| TestImportNameCodeFixDefaultExport4 |
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.
Wontfix. Asserts that an export default a shows up as a fix for the identifier foo if the exporting file is named foo.ts. We will still use filename as a fallback, but not if the export has a local name. We used to jump through a lot of hoops to find every possible name. Now we stop collecting lower priority names once we have something.
| TestImportNameCodeFix_uriStyleNodeCoreModules3 | ||
| TestImportNameCodeFix_withJson | ||
| TestImportNameCodeFixDefaultExport4 | ||
| TestImportNameCodeFixDefaultExport7 |
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.
Same. The original test made a similar assertion to the one mentioned above, but this one was only intended to repro a Strada crash. Actually, I'll move this to manual and assert no fixes to show it doesn't crash.
| TestImportNameCodeFixNewImportFileQuoteStyleMixed0 | ||
| TestImportNameCodeFixNewImportFileQuoteStyleMixed1 | ||
| TestImportNameCodeFixNewImportTypeRoots1 | ||
| TestImportNameCodeFixOptionalImport0 |
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 one now offers an extra valid fix. I'll update and move to manual.
gabritto
left a comment
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'll leave it up to you what to do about #2390 (comment).
jakebailey
left a comment
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.
LGTM though I think you are still making test updates
| return v.compareModuleSpecifiersForRanking(a, b) | ||
| } | ||
|
|
||
| func compareFixKinds(a, b lsproto.AutoImportFixKind) 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.
This could just be cmp.Compare at the one call site, though I suppose there are other comparisons here anyhow
|
@andrewbranch Sorry to bother you, I see that it has been released. I was eager to try that out but I don't see a difference in the autoimport speed. I didn't want to ping you here in case it hasn't been release but i can see in the vscode logs Which has been introduced in your PR so I guess I have it. I just tried to import a single identifier in my (private) project. I tested both TS version and Go version. This is what i found in the logs with the Go version So it takes about 5.6s to complete completion. Now the TS version |
|
@jansedlon I had some issues with auto-import on the previous versions, and I was having the same issues as you yesterday when I first upgraded. But as of today, it's working properly for me and way faster. I don't have a conclusion as to why that happened, but I will update you if I find something relevant. @andrewbranch Just out of curiosity, when I press ctrl+cmd (even 2/3 consecutive times), it's taking around 3 to 4 seconds for the dropdown to show up. I wonder if it's expected behavior (with this new cache infrastructure) or if I have something happening on my side. Autocomplete and auto-import are super fast now, but it's just this initial slowdown. 0.20260111.1 edit: it varies a lot depending on the file. |
|
@rubnogueira Interestering, i completely restarted vscode, updated from 0.20260111.1 to 0.20260112 and i'm now encountering #2467 so something has definitely changed. Will report when it's resolved. |
|
@rubnogueira @jansedlon in tonight's nightly, #2467 will be fixed and #2474 will allow you to run "Developer: TypeScript Native Preview: Start CPU Profile" from the VS Code command palette. I would love to see a profile of some of these multi-second completion times. You can email me at [firstname].[lastname]@microsoft.com. Thanks for trying this out! |
|
@andrewbranch Thank you for fixing this 🙏 I have contacted you on email |
This PR introduces a completely redesigned auto-import collection infrastructure. As we ported Strada's auto-import code to Corsa, two things became clear:
We wanted to address these issues before (or while) adding package.json-based dependency discovery, the main feature missing from the initial port of Strada's auto-imports code.
While the data collection backend has been completely redesigned, the fix generation code is mostly unchanged, although the files have been moved from
internal/lsintointernal/ls/autoimport.I’m closing existing all bugs/crashes filed against this repo’s auto-imports implementation with this PR. Please comment if you have a bug that still repros the exact same way after this PR.
Strada's collection and cache infrastructure
The earliest version of auto-imports in TypeScript had no caching or separate collection phase. During every global completions request, we used the type checker to gather every export from every source file in the program. This was actually fairly fast, in part because we deferred computing the actual import module specifier until a completion item was highlighted. Eventually, we wanted to address two common complaints: (1) users wanted to see the import module specifier for every item in the completion list without having to highlight it first, and (2) we would often suggest imports from transitive dependencies, bloating the list with entries that should really never be imported. Computing module specifiers for every item in the list slowed things down enough that we implemented a caching layer to compensate. Since the cache included module specifiers and filtering based on what dependencies were accessible to the file requesting completions, much of the cache was specific to the requesting file, and needed to be rebuilt whenever the user started editing a different file.
Later, we added a very commonly requested feature: auto-imports from package.json dependencies. Since the data source for auto-imports was source files already in the program, a common complaint was that immediately after
npm installing a dependency, auto-imports from that package wouldn't show up. Users would have to manually write an import from the new package once, then auto-imports would start appearing, even in other files. To address this without completely overhauling all the existing infrastructure, which was designed to use a type checker to extract exports from source files in a program, we added a separate program and type checker “in the background” (scare quotes because we couldn’t actually do anything in a separate process; we just added it onto project loading) containing entrypoints from package.json dependencies that were absent from the main program. Then, the language service could basically just run its existing auto-import collection code twice: once for the main program, and once for the package.json dependencies program. Since this had the potential to add significant amounts of work both to project loading and completions requests, we added a conservative bail-out if more than ten entrypoints were going to be loaded into the background program. This bail-out strategy was a big and imprecise hammer, since those ten entrypoints could trigger an arbitrarily large or small amount of work through transitive imports that would be eventually be filtered out.In short, auto-imports was due for a rewrite even before typescript-go.
New infrastructure
The primary innovations of the new system are:
Granular buckets
The new infrastructure stores exports in separate buckets that are independent of each other and independent of the requesting file. The
autoimport.Registrycontains one bucket for each project owning the editor's open files, plus one bucket for eachnode_modulesdirectory visible to the open files. Project buckets contain exports from the project's local, non-node_modulesfiles. This means that when switching between open files, any buckets that are already populated can be reused. When a project updates, allnode_modulesbuckets are unaffected; when annpm installhappens, only thenode_modulesdirectories that were touched need to be updated.Highly parallelized collection
Previously, collection of exports for package.json dependencies worked by creating a program containing all the entrypoints we care about, then using the type checker to extract exports from every source file in that program. The disadvantage of creating a program for this purpose is that we would resolve and parse transitive dependencies that have absolutely no impact on the exports we actually care about. In the new infrastructure, we instead create a mock program (
autoimport.aliasResolver) for each dependency package, seeded with just the entrypoint files for that package. We then walk the exports of those entrypoints, and only use a checker when one of those exports is a re-export from another module. The result is that we only process exactly what we need, we limit our reliance on type checkers, and when we do need a type checker, the data backing it is much smaller, so we can create many checkers very cheaply.Pre-computed module specifiers for
node_modulesSince the old infrastructure originally started from a program's source files, module specifier generation always went through the full process of “importing file name + destination file name => module specifier.” That process could be complicated for node_modules files, since the destination file could be the realpath of a symlinked package, or could have been a transitive import that’s not actually reachable from the importing file, or could be blocked by the package’s package.json
"exports". Without knowing how that file got into the program, there were a dozen edge cases that needed to be considered to reverse engineer the correct module specifier, or even to determine whether a valid one exists at all. With the new directory-based buckets and extraction only from dependency entrypoints, we actually know the module specifier for every entrypoint as we find it, so we just store it alongside the export information.Limitations / observable differences compared to Strada
sourceFile.Symbol.Exportsinstead ofchecker.GetExportsAndPropertiesOfModule, properties ofexport =assignments are not collected. Many of these properties were filtered out in the old logic because they were highly suspicious anyway (e.g. class instance methods, properties of arrays). There is a special syntactic carve-out for JavaScriptmodule.exports = { foo, bar, ... }patterns, which are still collected. This limitation did not cause any new test failures.export declare const foo: SomeTypewould appear as a function ifSomeTypedeclared call signatures, but now appears as a variable.Performance
It's extremely hard to compare this to
mainto Strada, because each has its own limitations that results in very different behavior, not just different timing characteristics:That said, here are some numbers on two example projects.
Signal-Desktop
VS Code
Follow-up work
completionItem/resolveis surprisingly slow (not just for auto-imports), and may be missing info like JSDoc that was previously(?) there for auto-imports