Skip to content
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

Type-only import specifiers #45998

Merged
merged 26 commits into from
Sep 27, 2021

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Sep 22, 2021

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

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Sep 22, 2021
src/compiler/parser.ts Outdated Show resolved Hide resolved
Copy link
Member

@gabritto gabritto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/compiler/parser.ts Outdated Show resolved Hide resolved
src/compiler/parser.ts Outdated Show resolved Hide resolved
src/compiler/parser.ts Outdated Show resolved Hide resolved
Comment on lines +7367 to +7370
// import { type } from "mod"; - isTypeOnly: false, name: type
// import { type as } from "mod"; - isTypeOnly: true, name: as
// import { type as as } from "mod"; - isTypeOnly: false, name: as, propertyName: type
// import { type as as as } from "mod"; - isTypeOnly: true, name: as, propertyName: as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooof, this is awful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have completion list tests for these?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

src/services/codefixes/importFixes.ts Outdated Show resolved Hide resolved
src/services/codefixes/importFixes.ts Show resolved Hide resolved
src/services/codefixes/importFixes.ts Outdated Show resolved Hide resolved
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some ideas on the parser

Comment on lines 7357 to 7358
let checkIdentifierStart = scanner.getTokenPos();
let checkIdentifierEnd = scanner.getTextPos();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/compiler/parser.ts Outdated Show resolved Hide resolved
src/compiler/parser.ts Show resolved Hide resolved
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

!!! related TS1376 /c.ts:1:10: 'as' was imported here.

==== /d.ts (3 errors) ====
import { type as as as as } from "./mod.js"; // Error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦬 🦬 🦬 🦬

!!! error TS2300: Duplicate identifier 'as'.

==== /e.ts (1 errors) ====
import { type type as as } from "./mod.js";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦬 🦬 🐃🐃

@andrewbranch
Copy link
Member Author

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
Copy link
Member

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
Copy link

glasser commented Oct 7, 2021

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants