verbatimModuleSyntax + import of ambient const enum missing error

[jablko]

Bug Report

preserveValueImports doesn't error when you import an ambient const enum with isolatedModules on. Instead you get an error at runtime.

🩹 Proposed Solution

If the named import were a type vs. an ambient const enum, you'd get a compile-time error instead.

#47817 raises that error on ambient const enums, by changing its predicate to include ambient const enums.

🔍 Search Terms

preserveValueImports ambient const enum

🕗 Version & Regression Information

4.5.0-dev.20210909 - 4.7.0-dev.20220225

⏯️ Playground Link

Workbench Repro

🧑‍💻 Code

// @preserveValueImports
// @isolatedModules
import { RoundingMode } from "big.js";

// @filename: node_modules/big.js/index.d.ts
export const enum RoundingMode {}

🙁 Actual Behavior

Runtime error:

import { RoundingMode } from "big.js";
         ^^^^^^^^^^^^
SyntaxError: Named export 'RoundingMode' not found. The requested module 'big.js' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'big.js';
const { RoundingMode } = pkg;

🙂 Expected Behavior

Compile-time error (with #47817):

index.ts:1:10 - error TS1446: 'RoundingMode' resolves to a type-only declaration and must be imported using a type-only import when 'preserveValueImports' and 'isolatedModules' are both enabled.

1 import { RoundingMode } from "big.js";
           ~~~~~~~~~~~~

[RyanCavanaugh]

@andrewbranch what's your take? The linked PR has some additional context

[andrewbranch]

I think differentiating on ambient is highly problematic, though I understand why it’s tempting to do so. If we weren’t talking about the enigma of const enums specifically, it would be super wrong to consider ambience. Ambient declarations are assumed to exist in the same way non-ambient declarations exist, just in a location that the compiler doesn’t have direct knowledge of. Type-checking behavior should generally not be impacted by ambience; otherwise you’ll get different behavior between consuming .ts files and consuming the .d.ts outputs of those same files. Even more confusingly, in project references, you’ll get different behavior between the CLI and the editor, because the former uses .d.ts files for referenced projects while the latter defaults to using .ts files.

The story changes slightly when talking about const enums, because when you tsc -d a const enum declaration, we emit an (ambient) const enum declaration in the .d.ts file, and whether there is an existing runtime value can be toggled based on other compiler options. So given an ambient const enum declaration, we really have no idea whether or not a runtime equivalent exists. (This was the basis for my proposal to introduce some kind of syntactic marker in .d.ts files for const enums that indicates whether there is a real runtime equivalent, which was rejected because it appeared to make enums more complex. I think this issue demonstrates that such a marker doesn’t expand the bits of information we care about with enums, but rather codifies a fairly fundamental one—whether or not there is any runtime representation—that we already try to consider but fail to do so accurately.) On the other hand, if the compiler sees a non-ambient const enum declaration, it can know whether or not there will be a runtime equivalent, because the current compilation will be responsible for producing it. So I think in many ways the proposal here makes sense:

• ambient const enum → don’t know if reified → error always
• non-ambient const enum → detect if reified → error if not

It’s really the project references scenario that makes me uncomfortable with this. Without other changes, you will be unable to consume your own preserved const enums from a referenced project with --isolatedModules --preserveValueImports, but this error will not show up in the editor. We could check if the declaration came from a project reference redirect, and check the compiler options of the referenced project (which would require an additional API added to TypeCheckerHost) to see if it would have been preserved. But this too may not be quite right, as the const enum declaration could have been hand-written with declare in the input file, where by this complicated logic we arguably should error (???) since we again are unsure whether a runtime equivalent exists.

Summary:

• I agree the current state errs on being too permissive; it does not catch a class of problematic imports that I would like to catch.
• This proposal is perhaps the best we can do in terms of classifying which imports will work or fail at runtime without adding more information to declaration emit of const enums going forward, but the fact that project references will behave differently in the CLI and in the editor seems like a blocker to me.
• The inconsistency between the CLI and editor for project references can perhaps be mitigated with some additional checker logic, but this logic will be imperfect and does not already exist anywhere in the checker.

[jablko]

@andrewbranch Thanks!

• Don't we already differentiate on ambient to raise an analogous error?

  error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided.

  https://github.com/microsoft/TypeScript/blob/e4fe50cca477a3e46f9b629a6c5be0b0ed8b010f/src/compiler/checker.ts#L34251

  So already, you can't in most cases consume your own preserved const enums from a referenced project with --isolatedModules, and that error won't show up in the editor?

• I guess the issue with "deconstifying" preserved const enums is if the CLI uses .d.ts files for referenced projects, it won't inline those enums ... Unless you use a "sufficiently smart bundler":tm:?

  Discussion of dropping the const modifier from vs. adding a marker to the declaration emit of preserved const enums: Design Meeting Notes, 12/1/2021 #46983 (comment)

In other words this here proposal doesn't introduce any new issues? For the existing issue you mention, there's a solution without adding more information to declaration emit of const enums ... but it has problems with inlining referenced projects' enums? ... but maybe that can be worked around? It's a separate issue?

[andrewbranch]

This is still an issue for verbatimModuleSyntax, but the project references discrepancy I was concerned about in earlier messages was handled recently in #57914. I think we should go ahead and do this for 5.6. It doesn’t make much sense to error on the use sites but not on the import that will be preserved. Under verbatimModuleSyntax, we should probably make the error only appear on the import, not on the use sites. That way, a single ts-ignore can be used to escape situations where we don’t know that an ambient const enum was reified with preserveConstEnums.

[typescript-bot]

👋 Hi, I'm the Repro bot. I can help narrow down and track compiler bugs across releases! This comment reflects the current state of the repro in the issue body running against the nightly TypeScript.

---

Issue body code block by @jablko

‼️ Exception: Error - error TS5102: Option 'preserveValueImports' has been removed. Please remove it from your configuration. Use 'verbatimModuleSyntax' instead.

Error: error TS5102: Option 'preserveValueImports' has been removed. Please remove it from your configuration.
  Use 'verbatimModuleSyntax' instead.

    at Object.createVirtualTypeScriptEnvironment (/home/runner/work/_actions/microsoft/TypeScript-Twoslash-Repro-Action/8680b5b290d48a7badbc7ba65971d526c61b86b8/dist/index.js:8128:11)
    at twoslasher (/home/runner/work/_actions/microsoft/TypeScript-Twoslash-Repro-Action/8680b5b290d48a7badbc7ba65971d526c61b86b8/dist/index.js:7618:17)
    at /home/runner/work/_actions/microsoft/TypeScript-Twoslash-Repro-Action/8680b5b290d48a7badbc7ba65971d526c61b86b8/dist/index.js:439:44
    at runTwoslashRequests (/home/runner/work/_actions/microsoft/TypeScript-Twoslash-Repro-Action/8680b5b290d48a7badbc7ba65971d526c61b86b8/dist/index.js:406:56)
    at run (/home/runner/work/_actions/microsoft/TypeScript-Twoslash-Repro-Action/8680b5b290d48a7badbc7ba65971d526c61b86b8/dist/index.js:20096:75)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Historical Information

Version	Reproduction Outputs	Time
5.0.2, 5.1.3, 5.2.2, 5.3.2, 5.4.2	‼️ Exception: Error - error TS5101: Option 'preserveValueImports' is deprecated and will stop functioning in TypeScript 5.5. Specify compilerOption '"ignoreDeprecations": "5.0"' to silence this error. Use 'verbatimModuleSyntax' instead. Error: error TS5101: Option 'preserveValueImports' is deprecated and will stop functioning in TypeScript 5.5. Specify compilerOption '"ignoreDeprecations": "5.0"' to silence this error. Use 'verbatimModuleSyntax' instead. at Object.createVirtualTypeScriptEnvironment (/home/runner/work/_actions/microsoft/TypeScript-Twoslash-Repro-Action/8680b5b290d48a7badbc7ba65971d526c61b86b8/dist/index.js:8128:11) at twoslasher (/home/runner/work/_actions/microsoft/TypeScript-Twoslash-Repro-Action/8680b5b290d48a7badbc7ba65971d526c61b86b8/dist/index.js:7618:17) at /home/runner/work/_actions/microsoft/TypeScript-Twoslash-Repro-Action/8680b5b290d48a7badbc7ba65971d526c61b86b8/dist/index.js:439:44 at /home/runner/work/_actions/microsoft/TypeScript-Twoslash-Repro-Action/8680b5b290d48a7badbc7ba65971d526c61b86b8/dist/index.js:427:51 at Array.map (<anonymous>) at runTwoSlashOnOlderVersions (/home/runner/work/_actions/microsoft/TypeScript-Twoslash-Repro-Action/8680b5b290d48a7badbc7ba65971d526c61b86b8/dist/index.js:425:23) at runTwoslashRequests (/home/runner/work/_actions/microsoft/TypeScript-Twoslash-Repro-Action/8680b5b290d48a7badbc7ba65971d526c61b86b8/dist/index.js:408:66) at run (/home/runner/work/_actions/microsoft/TypeScript-Twoslash-Repro-Action/8680b5b290d48a7badbc7ba65971d526c61b86b8/dist/index.js:20096:75) at runMicrotasks (<anonymous>) at processTicksAndRejections (node:internal/process/task_queues:96:5)	⚠️ Way slower