Type-only import specifiers

[andrewbranch]

Follow-up feature to #44619, allows

import { SomeComponent, type SomeProps } from "./component"

Emit behavior

• Adding the type keyword before an import or export specifier causes it to be elided from JS emit.
• The type keyword on an import or export specifier plays no direct role in whether the parent import/export declaration is elided.

This means that under default settings,

import { type SomeProps } from "./component";

gets erased from the JS entirely, because default settings remove any import declarations that can be removed. But under --importsNotUsedAsValues=preserve (or error), the same code is emitted as

import {} from "./component";

because the type keyword on the imports specifier does not indicate that there is no runtime module dependency on "./component".

Auto-imports behavior

As discussed in #44619, the combination of --isolatedModules and --preserveValueImports requires that exported symbols that have no value meaning (i.e., they’re purely types or uninstantiated namespaces) or are imported/exported with a type-only import/export somewhere earlier in the chain be imported with a type-only import so transpilers can know to drop those import specifiers. Obviously, auto-imports had to be updated to account for this when importing types or already-type-only symbols. This PR includes the changes to make working under those settings manageable, though there are further experience enhancements I need to investigate. Here’s a non-exhaustive summary of what happens now:

• Barring bugs in the implementation, auto-imports should never trigger an error provided that the location you’re using the identifier to import was a valid place to use it (i.e., you can possibly mess it up by trying to import a type-only-exported class in an emitting position, but this is a rare edge case), so importing a type under these settings will always place the type in a type-only import.
• Auto-imports still generally prefers not to use type-only imports, so if you’re not using these settings or --importsNotUsedAsValues=error, you won’t see any type-only auto-imports. You’ll only get a type-only auto-import if it is needed to prevent an error or if it can use an existing type-only import declaration as opposed to adding a new import declaration.
• When adding a type-only import in a new import declaration (as opposed to adding a specifier to an existing declaration), it prefers the top-level type-only declaration form import type { Foo } over import { type Foo }.
• A top-level type-only import declaration will be converted to a regular import upon importing a value from the same module (import type { SomeType } → import { SomeValue, type SomeType })
• There are exceptions to that rule due to the fact that default imports can only be type-only via a top-level type-only import declaration and a type-only import declaration can't have both a default and named bindings (import type Foo, { Bar } is not allowed).

Future improvements to look into:

• If you have enough specifiers in import type { A, B, C, D, ... }, it is probably undesirable to convert that to a regular import upon adding a value import to it, since it would result in an explosion of type prefixes. At some point, you probably just want to add a new import statement. On the other hand, if each specifier is on its own line, it might not be bad to add all the prefixes and keep them combined, so this scenario needs some thought in order for us to guess the best thing to do.

• There’s still no support for promoting a type-only import to a regular import by attempting to use a value that you’ve already imported as type-only in an emitting position, e.g.

  import type { SomeClass } from "./whatever";
  const x = new SomeClass| // <- what to do here?

  I used to think this was kind of sketchy, but now that auto-imports has to be willing to make a lot more guesses and be willing to put things in type-only imports more often, I’m coming around to the thought that the best thing we can do is just try to be as flexible as possible and fluidly rewrite your import format to match your usage as you go. (Existing import type prevents autocompletion of value import for TypeScript #44804 is related.)

Other follow-up work

• Update the TmGrammar for syntax highlighting

[gabritto]

Everything looked fine to me and I could follow the code. The comments really helped understand what was going on. Just left some minor comments on possible typos.
Also didn't have time to review importFixes.ts and some of the tests, but like I said, the rest looks good to me.

[DanielRosenwasser]

Ooof, this is awful.

[DanielRosenwasser]

Do you have completion list tests for these?

[andrewbranch]

I have tests for type and then completions working as usual after a type. After that, I don’t think you should get any completions, since the remaining things you type are / could be new identifier definitions... which I guess should still be tested ideally

[DanielRosenwasser]

Overall looks good; would be nice to get some of the suggestions and tests in.

We will have to update other tooling teams on this (e.g. eslint, Babel, esbuild, swc, others). We also might want to think about these for dts-downlevel (@sandersn).

[sandersn]

Some ideas on the parser

[sandersn]

aside: since these are only used for the error span, it would be nice for their names to contain "error".

[sandersn]

Looks good (as far as I understand auto imports at least).

[sandersn]

🦬 🦬 🦬 🦬

[sandersn]

🦬 🦬 🐃🐃

[andrewbranch]

Update: getting the keyword type to show up in completions where it’s supposed to was a way bigger pain than I imagined. Completions code has really gotten tangled and could use a refreshed mental model. Syntactic and semantic analysis are mixed willy-nilly and don’t play nicely with each other, and we end up re-checking what syntactic constructs we’re in more often than we need to. It would be nice to do all the syntactic analysis up front, which would imply what keywords are valid, and then let the results of the same analysis guide the semantic analysis. As it is now, I had to add in a lot of little fixes where failed semantic analysis was short-circuiting locations where we could have returned some keywords. It’s a mess.

[gabritto]

Update: getting the keyword type to show up in completions where it’s supposed to was a way bigger pain than I imagined. Completions code has really gotten tangled and could use a refreshed mental model. Syntactic and semantic analysis are mixed willy-nilly and don’t play nicely with each other, and we end up re-checking what syntactic constructs we’re in more often than we need to. It would be nice to do all the syntactic analysis up front, which would imply what keywords are valid, and then let the results of the same analysis guide the semantic analysis. As it is now, I had to add in a lot of little fixes where failed semantic analysis was short-circuiting locations where we could have returned some keywords. It’s a mess.

+1 we should consider refactoring the completions code before working on more completion features/not-so-small changes.

[glasser]

Will this change mean that importsNotUsedAsValues: "error" will now consider import { symbolsUsedAsValue, SymbolNotUsedAsValue } from 'module' as an error, where it was previously OK? (Seems reasonable, just curious what the impact on a codebase with that setting will be at upgrade time.)

[andrewbranch]

Nope. That would be an error only under the combination of --preserveValueImports and --isolatedModules. The behavior of importsNotUsedAsValues: "error" alone is unchanged, except there were a few minor bug fixes for it between this PR and #44619.