Existing import type prevents autocompletion of value import for TypeScript

[lgarron]

Issue Type: Bug

Consider the following code:

// a.ts
import type { B } from "./b";
import { makeB } from "./b";
const variable: B = makeB();

// b.ts
export class B {}
export function makeB(): B { return new B(); }

Now type:

// new line in `a.ts`
new B

on a new line inside a.ts and try to autocomplete the import of B to use it as a constructor.

Expected behaviour

VSCode changes the import of B from a type import to a value import.

(It would be a syntax error to add the value import while keeping the type import, so the type import of B has to be removed at the same time. In this particular example, I would expect the entire import type line to be removed and the import of B added to the same line as makeB.)

Observed behaviour:

VSCode offers an autocompletion dropdown, but the default choice does nothing. There is no choice that will result in a value import of B unless the existing type import is deleted.

Screencap:

Screen.Recording.2021-06-29.at.01.47.40.mov

---

This is a frustrating experience: If I "happen" to autocomplete something as a value in a given file before I complete it as a type the first time, then autocomplete works naturally and the code is fine. But if I autocomplete something as a type the first time, then VSCode is unable to autocomplete the value import. Even though I know the issue and can work around it every time, autocomplete feels subjectively unreliable.

This kind of situation can be common in a large codebase with importsNotUsedAsValues. In particular, I'm working on a project where it is common to use a particular class (Alg from cubing/alg) as a the type of a variable in a given file, only to need the constructor later on.

VS Code version: Code 1.57.1 (507ce72a4466fbb27b715c3722558bb15afa9f48, 2021-06-17T13:28:32.912Z)
OS version: Darwin x64 20.5.0
Restricted Mode: No

System Info

Item	Value
CPUs	Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz (12 x 2900)
GPU Status	2d_canvas: enabled gpu_compositing: enabled metal: disabled_off multiple_raster_threads: enabled_on oop_rasterization: enabled opengl: enabled_on rasterization: enabled skia_renderer: disabled_off_ok video_decode: enabled webgl: enabled webgl2: enabled
Load (avg)	3, 5, 4
Memory (System)	32.00GB (1.37GB free)
Process Argv	--disable-extensions --crash-reporter-id 35bab6a6-41b6-4e5f-9d55-05b4be8834fd
Screen Reader	no
VM	0%

Extensions disabled

A/B Experiments

vsliv368cf:30146710
vsreu685:30147344
python383cf:30185419
pythonvspyt602:30300191
vspor879:30202332
vspor708:30202333
vspor363:30204092
pythonvspyt639:30300192
pythontb:30283811
pythonvspyt551:30311712
vspre833:30321513
pythonptprofiler:30281270
vshan820:30294714
pythondataviewer:30285071
vscus158:30321503
pythonvsuse255:30323308
vscorehovct:30309550
vscod805cf:30301675
binariesv615:30325510
vsccppwtct:30329789

[andrewbranch]

I think the completion entry showing up at all might be a mistake, but the decision to not “promote” imports from type-only to regular imports was very intentional. If you bother to write an import as type-only, we would like to take that as a very strong cue that you don’t want to accidentally add a runtime dependency on the module you’re importing, and so we don’t want to quietly undermine your intention by undoing that type-only annotation as soon as you try to reference those identifiers in a value position. Unfortunately, we don’t actually know what imports were auto-imported and what were written by hand. There seem to be an assumption that we don’t auto-import anything as type-only, but that’s not true under --importsNotUsedAsValues=error, and I can see how that would be a problem over and over again with that setting. Maybe we just need to enable the “promotion” behavior under any settings where auto-imports could be written as type-only imports.

[lgarron]

Unfortunately, we don’t actually know what imports were auto-imported and what were written by hand. There seem to be an assumption that we don’t auto-import anything as type-only, but that’s not true under --importsNotUsedAsValues=error, and I can see how that would be a problem over and over again with that setting. Maybe we just need to enable the “promotion” behavior under any settings where auto-imports could be written as type-only imports.

Yeah. The assumption makes some sense by default, but what you wrote definitely applies in our case. We use importsNotUsedAsValues set to error to help address some tooling issues, and the presence of import type does not (and cannot) reflect any hand-crafted author intent.

I'd be glad for the "promotion" to happen automatically when that config is enabled. I'd also be happy with some sort of "warning, you're promoting this import from type to value, this might affect your build more than you expect" if regular autocompletion muscle memory still works.

In cases where the setting is not enabled, I think the current behaviour is still kind of confusing — autocomplete muscle memory does select something, but that something has no effect. I'm not sure what a good UX in that case would be.

[andrewbranch]

I think the current behaviour is still kind of confusing — autocomplete muscle memory does select something, but that something has no effect

Well, it has the same effect as any other non-auto-import completion: it completes the identifier so you don’t have to type it yourself 😄 I’m pretty sure what I expected was that type-only-imported identifiers wouldn’t show up in completions in value positions at all. I also have a memory of a codefix being available to do the promotion, so the sequence would be (1) accept completion, (2) see error with lightbulb, (3) perform codefix. But apparently I made that up, as no such codefix exists. That seems like it would be an ok baseline experience, and we could extend the promotion behavior to everyone regardless of settings if there is sufficient demand.

[lgarron]

Anything I could do to help this along?

This is still tripping me up, sometimes (literally) hundreds of times a day.

[andrewbranch]

I plan to get to this in for 4.6 (possibly this week). My thinking on it changed after we added import { type T } and --preserveValueImports: I now lean toward automatically promoting type-only imports to non-type-only under any settings.