Import statement completions #43149

andrewbranch · 2021-03-08T21:28:46Z

Screen capture demo

Enables auto-import-style completions on import statements themselves, e.g.

import useS|

can offer a completion to

import { useState| } from "react";

(The | indicates the position of the cursor both before and after the completion.)

This PR is large because, although the feature appears very similar to existing auto imports, it requires us to resolve module specifiers immediately instead of in a subsequent completion details request. Being able to do that with any reasonable amount of code reuse required quite a bit of untangling. The implementation is explained in some detail in code review comments, but note that a large amount of the PR diff is just functions being shuffled around.

Note also this feature is opt-in via a user preference (includeCompletionsForImportStatements) because it requires corresponding editor changes. So, you won’t be able to test this yourself without building my VS Code PR as well.

Design

When these completions are offered, they are generally the same as the auto-imports you’d get elsewhere in the file:
- Same list of modules and exports are considered, but we require the first character of what you’ve typed so far to match the first character of the export name, instead of just the “contains characters in order” check. This significantly reduces the number of matches we see after you type a single character, which lets us return results way faster.
- Quote preference is respected and semicolon usage is inferred
- Inference of preferred import style is same as normal auto-import rules (i.e., when esModuleInterop introduces ambiguity we try to give you what you want)
Does not work on const foo = require("mod"); only works on imports (though import foo = require("mod") is supported in TS)
Does not work on imports where there’s already a module specifier, for I think obvious reasons
Does not work on imports where there’s already another identifier in the declaration, like import foo, { bar| or import { foo, bar|. This could be relaxed a bit later, but it was complicated and seemed fairly low value.
Inserts a snippet, so your cursor snaps to the end of the identifier you were completing, then you can tab to the end of the statement.
Due to client limitations (replacementRange must be a single line), does not work on imports that already span multiple lines, e.g.
```
import {
  foo|
}
```

Performance

Following measurements assume these dependencies

"dependencies": {
    "@angular/core": "^10.0.7",
    "@types/eslint": "^7.2.5",
    "@types/lodash": "^4.14.165"
    "@angular/material": "^11.2.5",
    "@reduxjs/toolkit": "^1.5.0",
    "@types/node": "^14.14.17",
    "@types/puppeteer": "^5.4.2",
    "@types/react": "^17.0.3",
    "@types/serverless": "^1.78.17",
    "aggregate-error": "^3.1.0",
    "antd": "^4.5.1",
    "aws-sdk": "*",
    "eslint": "^7.14.0",
    "lodash": "4.17.15",
    "mobx": "^5.15.4",
    "moment": "2.24.0",
    "puppeteer": "^6.0.0",
    "react": "16.12.0",
    "typescript": "^4.3.0-dev.20210322",
  }

Import statement completions

Stress-tested with aws-sdk installed, which has just a ridiculous number of exports. First draft took several seconds without caches, which was too much, so I decided to require the first character of the identifier to match the first character of the export name before continuing the fuzzy match. So whereas regular auto imports will offer useState when you have just typed state, import statement completions require you to start with u.

import a|: 865 ms (3604 entries)
import b|: 456 ms(1279 entries) (some cache benefit from previous request)
import a|: 625 ms (max cache benefit)

After npm rm aws-sdk:

import a|: 116 ms (197 entries)
import b|: 44 ms (105 entries)
import a|: 70 ms

Normal auto imports

The first draft of this PR incurred about a 15% performance penalty on all auto-imports; that has now been reduced to zero or has improved performance in some scenarios. The auto import cache has been split into two pieces that are independently more durable and more reusable than they were combined.

Previously, the process went something like this:

For each module in the program:
Can we come up with a module specifier from the importing file to that module without crossing someone else’s node_modules?
If that module specifier is a bare specifier (e.g. "lodash"), do the package.json files visible to the importing file list that package?
If the answer to both of those are yes, proceed with adding info about that module's exports to a big array, deduplicating re-exports along the way.
Save all that info in a cache.
Iterate the big map of info and pull out the exports whose names match the partial identifier typed so far.

The main problem with this process is that steps (2) and (3) are pretty expensive, and that work might go to waste after the filter in step (6) is applied. The other problem is that they are dependent on the location of the importing file and the contents of any package.jsons visible to that file, which makes us have to invalidate the cache a lot. Too many inputs combined into a single cache means that when we invalidate the cache, a lot of work that isn’t actually invalid has to be thrown away. Now, we cache “what are the exports and re-exports of every module” and “is file A importable from file B, and by what module specifier” separately. So the process becomes more like:

For each module in the program:
Add info about that module’s exports to a big map (not throwing away info about re-exports)
Cache that big map
Iterate the big map of info and pull out the exports whose names match the partial identifier typed so far.
For each name match, see if the module is importable by the importing file (both by module specifier validity and package.json filtering)
Add that module specifier / importability info to a cache.

This means for subsequent auto imports, we have better chances of getting cache hits, and we do less expensive work up front, even with the caches are totally empty.

Triggering auto import completions in a project with

	dev.20210322 (ms)	This PR (ms)
`completionInfo` (cold)	259	264
`completionEntryDetails`	102	74
`completionInfo` (cached)	68	63
`completionEntryDetails`	44	35

To-do

Do we need a separate user preference for enabling snippet-formatted completions? Yes, done.
Do some profiling to make sure normal completions / auto imports are not slower
Check performance when there are huge numbers of possible modules to import from (e.g. with aws-sdk installed) and probably implement some limits/mitigations there. Increased strictness of filter to make list smaller.
Get approval on corresponding VS Code PR
Finish and merge corresponding TypeScript-VS PR (need help from @uniqueiniquity on this one 🙏)

Fixes #31658

typescript-bot · 2021-03-08T21:28:49Z

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

andrewbranch · 2021-03-15T17:28:32Z

src/server/protocol.ts

@@ -2127,7 +2127,7 @@ namespace ts.server.protocol {
        arguments: FormatOnKeyRequestArgs;
    }

-    export type CompletionsTriggerCharacter = "." | '"' | "'" | "`" | "/" | "@" | "<" | "#";
+    export type CompletionsTriggerCharacter = "." | '"' | "'" | "`" | "/" | "@" | "<" | "#" | " ";


The space, in combination with the isIncomplete property added, is a targeted fix for the problem described at #31658 (comment).

andrewbranch · 2021-03-15T17:45:01Z

src/services/codefixes/importFixes.ts

+        function key(name: string, alias: Symbol, moduleSymbol: Symbol, checker: TypeChecker) {
+            const moduleName = stripQuotes(moduleSymbol.name);
+            const moduleKey = isExternalModuleNameRelative(moduleName) ? "/" : moduleName;
+            return `${name}|${getSymbolId(skipAlias(alias, checker))}|${moduleKey}`;


At the moment, the contents of the map are not meant to be looked up by keying into it; rather, the key serves to manage grouping. Each key maps to a group of exports/re-exports of the same symbol by the same name from a set of modules that are effectively interchangeable until we need to calculate what the best module specifier to use is. In other words, if we’re generating a completions list of auto imports, we should get exactly one completion entry per key, even if that key maps to many re-exports.

andrewbranch · 2021-03-15T17:50:40Z

src/services/codefixes/importFixes.ts

-    /** Information about how a symbol is exported from a module. (We don't need to store the exported symbol, just its module.) */
-    interface SymbolExportInfo {
-        readonly moduleSymbol: Symbol;
-        readonly importKind: ImportKind;


Some of the refactors here weren’t strictly necessary, but are a step forward in understandability and hopefully cacheability in the future—SymbolExportInfo previously had info about how a specific file would want to import the exported symbol instead of just plain info about how the symbol is exported, which made it a misnomer and uncacheable.

andrewbranch · 2021-03-15T17:55:28Z

src/services/completions.ts

@@ -82,11 +100,12 @@ namespace ts.Completions {
    }

    /**
-     * Map from symbol id -> SymbolOriginInfo.
+     * Map from symbol index in `symbols` -> SymbolOriginInfo.


A semi-related bug that became apparent in the process here was that the same symbol can have multiple origins (e.g., one symbol exported from two different ambient module declarations is allowed to show up as two separate auto import completions), and keying by symbol id didn’t allow that.

andrewbranch · 2021-03-15T18:00:16Z

src/services/completions.ts

@@ -598,8 +619,55 @@ namespace ts.Completions {
            has: name => uniques.has(name),
            add: name => uniques.set(name, true),
        };
+
+        function shouldIncludeSymbol(symbol: Symbol, symbolToSortTextMap: SymbolSortTextMap): boolean {


This was just moved from a separate filter statement elsewhere into this spot where we already iterate through the whole list. Should be a small performance improvement, but I moved it because of some order-of-operations change that I honestly don’t remember anymore.

andrewbranch · 2021-03-15T18:03:47Z

src/services/completions.ts

@@ -1153,7 +1249,7 @@ namespace ts.Completions {
            Debug.assertEachIsDefined(tagSymbols, "getJsxIntrinsicTagNames() should all be defined");
            tryGetGlobalSymbols();
            symbols = tagSymbols.concat(symbols);
-            completionKind = CompletionKind.MemberLike;
+            completionKind = CompletionKind.Global;


AFAICT this completionKind had no effect prior to me moving the aforementioned filtering code, but was conceptually wrong, and changing it to be conceptually right allowed me to move the filter.

andrewbranch · 2021-03-15T18:07:02Z

src/services/completions.ts

@@ -1349,13 +1446,24 @@ namespace ts.Completions {
                const nameSymbol = leftMostName && typeChecker.getSymbolAtLocation(leftMostName);
                // If this is nested like for `namespace N { export const sym = Symbol(); }`, we'll add the completion for `N`.
                const firstAccessibleSymbol = nameSymbol && getFirstSymbolInChain(nameSymbol, contextToken, typeChecker);
-                if (firstAccessibleSymbol && !symbolToOriginInfoMap[getSymbolId(firstAccessibleSymbol)]) {
+                if (firstAccessibleSymbol && addToSeen(seenPropertySymbols, getSymbolId(firstAccessibleSymbol))) {


Small tweaks here due to changing the origin info map to be keyed by index instead of by symbol id—we now need a Map for deduplication here, since symbol duplication is explicitly allowed in other cases.

Co-authored-by: Daniel Rosenwasser <[email protected]>

src/server/project.ts

…ed file

jasonwilliams · 2021-04-22T19:08:14Z

Hey @andrewbranch this doesn't appear to be working for me am I doing something wrong?

andrewbranch · 2021-04-22T19:20:26Z

It looks like something might have broken on the VS Code side. It doesn't work for me either at the moment, but I can see TS Server returning the right things in the log. I'll take a look. Thanks!

rchl · 2021-06-02T14:08:55Z

Have it been considered to add support for that in the LSP spec? Or is it already supported?

That question maybe belongs better in microsoft/vscode#119009 but that one is locked.

andrewbranch · 2021-06-02T16:14:44Z

I made sure that everything we’re doing with VS Code can also be done over LSP so that this will work in VS once their TS completions support swaps over to LSP. I actually have a draft PR up for that in the TypeScript-VS codebase (which is not open source). I’m not sure if that answers your question—let me know if you have more specific questions, and I can try to answer or pull in @uniqueiniquity who is our resident LSP expert.

jasonwilliams · 2021-06-02T16:32:12Z

It looks like something might have broken on the VS Code side. It doesn't work for me either at the moment, but I can see TS Server returning the right things in the log. I'll take a look. Thanks!

Hey @andrewbranch was that issue ever fixed? Or is that what you need to change in the VSCode repo?

andrewbranch · 2021-06-02T16:38:01Z

@jasonwilliams it was fixed at microsoft/vscode#121975

rchl · 2021-06-02T16:57:44Z

I’m not sure if that answers your question—let me know if you have more specific questions, and I can try to answer or pull in @uniqueiniquity who is our resident LSP expert.

I guess the more specific question I have is whether that functionality requires changes to the LSP spec (https://github.com/microsoft/language-server-protocol) and if so whether those were proposed or even already implemented?

andrewbranch · 2021-06-02T20:51:26Z

No, this feature can be implemented with LSP 3.16 (and likely earlier versions) without changes. From the protocol standpoint, this is actually a good bit simpler than normal auto imports (though those are possible with LSP too). The only somewhat interesting thing about this feature is the tab stop at the end, which is supported via snippet text. The only caveat is that I made the space character a trigger character only when preceded by /^import/ via some special logic in VS Code’s TypeScript support layer, which I don’t think is possible in LSP. However, that was only to work around a conflict with a snippet completion whose name is import statement preventing a new completions request triggering on import s. It’s a little kludgy and if it came down to it, we could find other workarounds (the easiest one being ensuring that snippet completions don’t have multi-word names, since they can unexpectedly block new requests anywhere).

andrewbranch added 12 commits March 2, 2021 09:25

WIP

60422cb

WIP

081f564

Get completion details working

48bfb22

Start unifying eager and lazy auto imports

9e456b5

Fix export=

89dba08

Fix completion details for totally misspelled names

3a4e275

Almost fixed duplication...

12bb1fe

Fix remaining completion tests

0ae905c

Refactor to support multiple origins for same symbol

e9f3ec1

Make import fixes make slightly more sense

b5b58f8

Add cache back in

c7e9bc7

Set insertText based on import kind

393fce5

typescript-bot assigned andrewbranch Mar 8, 2021

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 8, 2021

andrewbranch added 8 commits March 8, 2021 13:29

Update API baselines

8999db9

Add semicolons, snippet support, and sourceDisplay

6c8457b

Add some tests

db75d56

Update baselines

6e6c8ff

Fix pattern ambient modules appearing in auto imports

6382ece

Fix tests

409fa90

Remove commented code

2ae85af

Switch to valueDeclaration for getting module source file

77b6e3d

andrewbranch commented Mar 15, 2021

View reviewed changes

microsoft deleted a comment from typescript-bot Mar 15, 2021

andrewbranch mentioned this pull request Mar 15, 2021

[typescript-language-features] Support import statement completions for TypeScript 4.3 microsoft/vscode#119009

Merged

andrewbranch marked this pull request as ready for review March 15, 2021 18:42

andrewbranch requested a review from sheetalkamat March 15, 2021 18:42

andrewbranch and others added 3 commits March 26, 2021 12:17

Use logical or assignment

66caaf2

Co-authored-by: Daniel Rosenwasser <[email protected]>

Simply conditional by reversing

d10aa0c

Co-authored-by: Daniel Rosenwasser <[email protected]>

Merge branch 'master' into feature/31658

5e868e3

sheetalkamat requested changes Mar 26, 2021

View reviewed changes

src/server/project.ts Outdated Show resolved Hide resolved

sheetalkamat and others added 2 commits March 26, 2021 13:22

When file is deleted need to marked correctly in the project as remov…

9ea1bd4

…ed file

Simplify hasAddedOrRemovedSymlinks with cherry-picked fix

eb52faa

sheetalkamat approved these changes Mar 26, 2021

View reviewed changes

andrewbranch added 3 commits March 26, 2021 13:59

Merge branch 'master' into feature/31658

f7028f6

Ensure replacement range is on one line

f5df94b

Update baselines

badcd83

andrewbranch merged commit 2d6a490 into microsoft:master Mar 26, 2021

andrewbranch deleted the feature/31658 branch March 26, 2021 21:47

andrewbranch mentioned this pull request Jun 24, 2021

Resolve module specifiers for auto imports in completion list (in incomplete chunks) #44713

Merged

andrewbranch mentioned this pull request Aug 26, 2021

TSServer: snippet completions #25207

Closed

furrycatherder mentioned this pull request Sep 13, 2021

typescript-language-server: LSP snippet completions aren't working after upgrading to 4.4.3 neovim/nvim-lspconfig#1259

Closed

dmitmel mentioned this pull request Dec 13, 2021

Tsserver now provides snippet completion, does this unblock arg ghost-test in completions? hrsh7th/nvim-cmp#646

Closed

jasonwilliams mentioned this pull request Feb 18, 2022

Missing subfolder path completions with wildcard typesVersions #47952

Closed

tanishiking mentioned this pull request Mar 3, 2022

Import Statement Completion scalameta/metals-feature-requests#259

Closed

jasonwilliams mentioned this pull request Mar 23, 2022

TypeScript (and emmet) suggestions show up in the wrong order microsoft/vscode#145877

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import statement completions #43149

Import statement completions #43149

andrewbranch commented Mar 8, 2021 •

edited

Loading

typescript-bot commented Mar 8, 2021

andrewbranch Mar 15, 2021

andrewbranch Mar 15, 2021

andrewbranch Mar 15, 2021

andrewbranch Mar 15, 2021

andrewbranch Mar 15, 2021

andrewbranch Mar 15, 2021

andrewbranch Mar 15, 2021

jasonwilliams commented Apr 22, 2021

andrewbranch commented Apr 22, 2021

rchl commented Jun 2, 2021

andrewbranch commented Jun 2, 2021

jasonwilliams commented Jun 2, 2021

andrewbranch commented Jun 2, 2021

rchl commented Jun 2, 2021

andrewbranch commented Jun 2, 2021

Import statement completions #43149

Import statement completions #43149

Conversation

andrewbranch commented Mar 8, 2021 • edited Loading

Design

Performance

Import statement completions

Normal auto imports

To-do

typescript-bot commented Mar 8, 2021

andrewbranch Mar 15, 2021

Choose a reason for hiding this comment

andrewbranch Mar 15, 2021

Choose a reason for hiding this comment

andrewbranch Mar 15, 2021

Choose a reason for hiding this comment

andrewbranch Mar 15, 2021

Choose a reason for hiding this comment

andrewbranch Mar 15, 2021

Choose a reason for hiding this comment

andrewbranch Mar 15, 2021

Choose a reason for hiding this comment

andrewbranch Mar 15, 2021

Choose a reason for hiding this comment

jasonwilliams commented Apr 22, 2021

andrewbranch commented Apr 22, 2021

rchl commented Jun 2, 2021

andrewbranch commented Jun 2, 2021

jasonwilliams commented Jun 2, 2021

andrewbranch commented Jun 2, 2021

rchl commented Jun 2, 2021

andrewbranch commented Jun 2, 2021

andrewbranch commented Mar 8, 2021 •

edited

Loading