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

Add --importsNotUsedAsValues=preserve-exact #44137

Closed

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented May 18, 2021

Fixes #43393

This adds a new value for --importsNotUsedAsValues called preserve-exact (name is bikesheddable), which is an emit mode where (non-type-only) imports and exports are never elided or transformed in any way. Every check and diagnostic added simply ensures that your imports and exports are plausibly valid as written at runtime:

  • module must be set to es2015 or higher, because asking for them to be converted to another module format and asking for them not to be messed with are obviously incompatible requests
  • Types, or values that resolve to type-only imports or exports, must be imported in type-only imports (since preserving an import specifier that doesn't resolve to a runtime export would be a runtime error)
  • Types, or values that resolve to type-only imports or exports, that are exported in an export declaration must use a type-only export declaration (an export modifier on a type alias or interface declaration is still allowed)

If this PR is merged, a follow-up PR will propose:

  • Allowing import { A, type B } from "./mod" to make it possible to combine types and values in single import statements under preserve-exact
  • Adding/editing appropriate code fixes for fixing import/export portability errors
  • Updating auto imports to respect the rules of preserve-exact

The work is not really complete without these tasks, but I thought splitting it at this point would make it would make it easier to review, as the code changes at this point are pretty small, but there are some points worth discussing:

  • Name of the flag value
  • Whether exports should be included in the “exact preservation”—the primary motivation of the emit mode is allowing apparently unused imports to be referenced by code that TypeScript can’t analyze, like Vue/Svelte templates or stringified execution. These concerns don’t apply to exports. However, a secondary effect of the emit mode is that it makes transpilers’ jobs really easy when it comes to determining what needs to be elided—isolatedModules ensures that only one file at a time needs to be analyzed, but it still requires some semantic analysis within that file to determine if something is used in an emitting position. With this mode, transpilers could skip that work, for imports, and it seems like it would be convenient to extend that same benefit to exports. That said, it’s not clear how much transpilers really benefit from this convenience anyway, since they’re already set up to elide imports and exports that are not used in emitting positions today, which is presumably not a ton of overhead. (It would be interesting to hear if any Babel, esbuild, or swc folks have opinions on this.)

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 18, 2021
"category": "Error",
"code": 1437
},
"'{0}' resolves to a type-only declaration and must be re-exported with a type-only re-export when 'isolatedModules' is enabled.": {
Copy link
Member Author

Choose a reason for hiding this comment

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

The addition of this diagnostic for isolatedModules is actually a bug fix which should have been included in type-only import work from the beginning.

@@ -1364,6 +1364,26 @@
"category": "Error",
"code": 1433
},
"'{0}' is a type and must be imported with a type-only import when 'importsNotUsedAsValues' is set to 'preserve-exact'.": {
Copy link
Member Author

Choose a reason for hiding this comment

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

The predecessors to these messages try to say 'import type' and 'export type' instead of type-only import and type-only export, but these were worded with an eye toward allowing type-only import and export specifiers, as mentioned in the PR description. If we decide not to do that, I would reword these to say 'import type' and 'export type' since there would only be one syntax that can fix the error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, the imported thing with this error could technically be an uninstantiated namespace, so “is a type” might be technically incorrect, but I think I prefer calling an uninstantiated namespaces a “type” to calling it an “uninstantiated namespace” in user-facing messages.

Copy link
Member

Choose a reason for hiding this comment

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

  • is a type
  • is a type-only entity
  • is only used for type constructs
  • is only used for type-checking
  • is a typey boi

Copy link
Member

Choose a reason for hiding this comment

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

"has no runtime representation"

Copy link
Member

Choose a reason for hiding this comment

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

Or, more specifically:

"'{0}' is a type or namespace that does not have an associated runtime value. It must be declared as a type-only import when 'importsNotUsedAsValues' is set to 'preserve-exact'."

@andrewbranch
Copy link
Member Author

andrewbranch commented May 21, 2021

For TL;DR, skip to 🦓

The design meeting raised questions about how the behaviors added here are organized into compiler options, specifically:

  1. Is importsNotUsedAsValues the right flag for this?
  2. Should --importsNotUsedAsValues=preserve be changed to mean that the identifiers are preserved?
  3. Should there be an independent flag that just enforces that types are imported with type-only imports?
  4. Should there be an independent flag that just disables the elision of apparently elidable values (not types, which must always be elided)?

I’ll first discuss each of these questions, and then try to draw some conclusions from their answers.

  1. Semantically, importsNotUsedAsValues is not the right flag for this. The correct understanding of importsNotUsedAsValues is that it expands into the more precise English, “import declarations that contain only aliases that are never referenced in an emitting position,” or in other words, import declarations that would be totally elided from JS emit under default settings. The preserve value means that these are emitted as a side-effect import instead of elided, and the error value means that it is illegal to have such import declarations (you must explicitly mark them as elidable with import type). The fact that the behavior enabled by preserve-exact in this PR affects import declaration errors based not on how all its aliases are referenced but rather on how some of its aliases resolve is totally inconsistent with the stated purpose of the flag.

    The reason it feels somewhat natural to make this a new value of importsNotUsedAsValues is that it subsumes preserve and is in direct conflict with error and remove, and seems to match the behavior people often guess that preserve has, likely based on an interpretive mode where you read --imports, and then your eyes glaze over during NotUsedAsValues, and then you snap back to attention with preserve. I know what imports are, and I know what it would mean to preserve them; that stuff in the middle... 🤷

  2. I don’t think we can make --importsNotUsedAsValues=preserve mean that additional imported identifiers are preserved (whether they are types, or just never referenced as values), because this would require --module=es2015+ for the same reason this PR does. That would be a breaking change, and one without a viable workaround. The people who we added --importsNotUsedAsValues=preserve for, who relied on side effect evaluation in imported modules, would be back to their original predicament unless they were already using --module=es2015+.

  3. Some people have asked for a way to enforce always using import type for types, and our stance has been that they should use a linter—basically, that usage of import type should be motivated by emit and not by code style. I still think that’s a good stance, but there is nothing else preventing us from creating this as a standalone option—it could be adopted independently of any other options; it would just be a type checking feature. That said, I think we should be cautious of adding flags that could lead to stylistic dogma without much practical reason—does labeling all your imported types help you write better, safer JavaScript, or write it more productively?

  4. The option to preserve imported identifiers with a value side that we believe are elidable is the part of this which requires es2015+ for --module. Also, as discussed earlier, this option either subsumes or contradicts every existing value of --importsNotUsedAsValues. It should also be noted that combining this option with --isolatedModules should necessitate importing types with import type, because otherwise a transpiler cannot tell the difference between an unused value import and an unused type import. As long as we handle the interactions between these other compiler options, this is possible as a separate flag, and fairly easily explainable.

🦓

I think this leads to a conclusion: instead of introducing an option that says “leave my imports exactly as I’ve written them,” this PR should be reimagined as the compiler option described above in (4), one which, on its own, only prevents the elision of values that we believe can be elided due to lack of references in emitting positions. With all other settings at their defaults, types would still be elided so that emitted imports would function without runtime errors, and we wouldn’t have to add any additional checking errors in this scenario. But with the addition of --isolatedModules, which says “make my code transpiler-safe,” you would be forced to import types with type-only imports.

This flag could still be a value of --importsNotUsedAsValues since it would be mutually exclusive with all existing values of that flag, but I’ve convinced myself that that’s semantically incorrect. I think if we like this mode enough to make it the default in tsc --init (as suggested in the design meeting), it’s worth naming it something understandable by a human. Something like --preserveImports or --preserveImportedNames?

@robpalme
Copy link

Thanks for providing all details here @andrewbranch

I agree with the conclusion of introducing a new compiler option (e.g. preserveImports), with perhaps different weightings on the justification.

The semantics of the full domain space discussed above are very subtle. Only TypeScriptistas will care or understand them. Most users just want a good safe default. So introducing a new option that has an easy adoption-path and sufficient consensus to become the ecosystem default is awesome. That's the winning argument for me. The opportunity to make the new option human-readable is a welcome bonus.


If I understand correctly, you are saying that preserveImports behavior will change based on isolatedModules

  • With isolatedModules, preserveImports will cause cause all names to be emitted, forcing the user to use type-only import syntax when they want to import names that do not exist as values (i.e. type-only exports) in the target module
  • Without isolatedModules, preserveImports will still elide names that the checker/resolver automatically determines do not exist as values in the target module

Bike-shedding the name is the least important thing to me. I will be happy with anything you select. Here's some thoughts anyway 😉 I like to first think of how we might describe the option in the docs in a way that leads non-experts to helpful conclusions. The docs might say:

In the default JavaScript emit, imported names are removed if they are not directly used as a runtime values. This can be undesirable if your application uses those names in ways TypeScript cannot detect, e.g. in templates or eval. This option stops those names being removed.

This option is turning off existing behavior. We are doing less. So I suggest something like noRemovingImportNames or noErasingImportNames

@andrewbranch andrewbranch added the Experiment A fork with an experimental idea which might not make it into master label Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Experiment A fork with an experimental idea which might not make it into master For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

importsNotUsedAsValues: preserve is not preserving
5 participants