TypeScript icon indicating copy to clipboard operation
TypeScript copied to clipboard

Certain resolution-dependent enum emit isn't correctly flagged as an error under `isolatedModules`

Open magic-akari opened this issue 2 years ago • 17 comments

🔎 Search Terms

  • isolatedModules
  • enum
  • cross module
  • export

🕗 Version & Regression Information

  • This is a crash
  • This changed between versions ______ and _______
  • This changed in commit or PR _______
  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about _________
  • I was unable to test this on prior versions because _______

Test on Playground with version 3.9.7 and version 5.2.2, got the same incorrect result.

⏯ Playground Link

Workbench Repro

💻 Code

// @showEmit
// @isolatedModules: true
// @filename: foo.ts
export enum Foo {
    A = 10
}

// @filename: index.ts
import { Foo } from "./foo";
export enum Bar {
    B = Foo.A,
    C
}

Workbench Repro

🙁 Actual behavior

Please notice the value of Bar.C

import { Foo } from "./foo";
export var Bar;
(function (Bar) {
    Bar[Bar["B"] = 10] = "B";
    Bar[Bar["C"] = 11] = "C";
})(Bar || (Bar = {}));

🙂 Expected behavior

Emit a warning or an error, and generate code that does not rely on other imports.

Example:

import { Foo } from "./foo";
export var Bar;
(function(Bar) {
    Bar[Bar["B"] = Foo.A] = "B";
    Bar[Bar["C"] = void 0] = "C";
})(Bar || (Bar = {}));

The Bar.C is void 0, since it cannot be refered.

Additional information about the issue

  • https://github.com/swc-project/swc/issues/8147

magic-akari avatar Oct 19 '23 06:10 magic-akari

Emit a warning or an error, and generate code that does not rely on other imports.

Just to be clear, isolatedModules doesn't mean "never emit code that relies on an import", it means "correct emit doesn't require resolving imports". This should probably still be an error under that reasoning though, since the emit requires the actual value of Foo.A.

fatcerberus avatar Oct 19 '23 13:10 fatcerberus

isolatedModules means that a non-resolving transpiler can correctly emit any file in the project. Arguably, a non-resolving transpiler can correctly emit index.ts:

import { Foo } from "./foo";
export var Bar;
(function(Bar) {
    Bar[Bar["B"] = Foo.A] = "B";
    Bar[Bar["C"] = Foo.A + 1] = "C";
})(Bar || (Bar = {}));

So the bug here is either that:

  • ts.transpileModule isn't emitting the addition expression
  • or we say that transpilers shouldn't do the addition, and C should be an error

I'm leaning toward the first option for the sake of compat and convenience.

RyanCavanaugh avatar Oct 19 '23 18:10 RyanCavanaugh

I don't believe Foo.A + 1 is correct. You can do this only if you know Foo.A is a number.

// @filename: foo.ts
export enum Foo{
    A = "10"
}

// @filename: index.ts
import { Foo } from "./foo";
export enum Bar {
    B = Foo.A,
    C
}

In the above example, tsc will return void 0 for Bar.C, while Foo.A + 1 will produce the string "101"

// @filename: foo.ts
export var Foo = {
    A: 10
};

// @filename: index.ts
import { Foo } from "./foo";
export enum Bar {
    B = Foo.A,
    C
}

In this example, Foo.A + 1 gets 11, while tsc returns the value of void 0.

magic-akari avatar Oct 19 '23 19:10 magic-akari

C is an error, so its correct emit is not guaranteed. Correct transpilation (in the presence of no errors) a motivating scenario behind the breaking changes we made in #50528

RyanCavanaugh avatar Oct 19 '23 19:10 RyanCavanaugh

C is an error, so its correct emit is not guaranteed. Correct transpilation (in the presence of no errors) a motivating scenario behind the breaking changes we made in #50528

You are right. If people always handle errors and do not expect consistent results from erroneous inputs, then things would be much simpler.

However, as far as I know, there are still people who try to use any to bypass error checking, but expect to get the same results as the TypeScript compiler.

One possible input:

// @filename: foo.ts
export enum Foo {
    A = 10,
}
(Foo.A as any) = "10"

// @filename: index.ts
import { Foo } from "./foo";
export enum Bar {
    B = Foo.A,
    C
}
  • https://github.com/swc-project/swc/issues/8114

magic-akari avatar Oct 19 '23 19:10 magic-akari

CC @nicolo-ribaudo @evanw

Maybe same issue in ~Babel~ and esbuild (transform mode)

magic-akari avatar Oct 19 '23 20:10 magic-akari

I would think that the moment you're doing something unsafe like casting an enum or ts-ignoreing, all bets are off about actual runtime behavior. The only "safe" thing I think exists is to assume one can use a const enum as a value unsafely if preserveConstEnums is enabled (esbuild will keep them around), but that idea only works for local enums.

jakebailey avatar Oct 19 '23 20:10 jakebailey

@RyanCavanaugh Sorry to bother you again. I am trying to implement this, but it does not work.

// @showEmit
// @isolatedModules: true
// @filename: foo.ts
export enum Foo {
    "Infinity" = 1,
    A = 1 / 0,
}

// @filename: index.ts
import { Foo } from "./foo";
export enum Bar {
    B = Foo.Infinity,
    C
}

Workbench Repro

The tsc says that the Bar.C is 2. But I got A1 through Bar.B + 1, or 1A through 1 + Bar.B.

Babel playground cc @nicolo-ribaudo

magic-akari avatar Oct 20 '23 08:10 magic-akari

That's a separate bug -- TSC should probably disallow "Infinity" and "NaN" as keys the same way as it disallows "3".

nicolo-ribaudo avatar Oct 20 '23 09:10 nicolo-ribaudo

That's a separate bug -- TSC should probably disallow "Infinity" and "NaN" as keys the same way as it disallows "3".

I am trying to fix this. But I found that "Infinity" "-Infinity" and "NaN" were intentionally excluded.

https://github.com/microsoft/TypeScript/blob/6424e181d4d7460913e730249a078b176fc8aaf0/src/compiler/checker.ts#L44730-L44735

magic-akari avatar Oct 20 '23 10:10 magic-akari

It was accepted as a bug at https://github.com/microsoft/TypeScript/issues/48956.

nicolo-ribaudo avatar Oct 20 '23 10:10 nicolo-ribaudo

TL;DR from #56164.

Under isolatedModules only, the following should be errors:

  • When an enum initializer expression that isn't a string literal has a string type
  • When an enum member without an initialization expression follows an enum member with an initialization expression that isn't a numeric literal

RyanCavanaugh avatar Oct 20 '23 20:10 RyanCavanaugh

(from the meeting notes)

On the other hand, there is nothing preventing compilers from making these enums "work".

Note that Babel currently transforms

enum Bar {
    B = Foo.A,
    C
}

to

var Bar = function (Bar) {
  Bar[Bar["B"] = Foo.A] = "B";
  Bar[Bar["C"] = 1 + Bar["B"]] = "C";
  return Bar;
}(Bar || {});

Is this what was meant by "work"? Or is this correct only under the new proposed behavior?

nicolo-ribaudo avatar Oct 20 '23 21:10 nicolo-ribaudo

Is this what was meant by "work"

Yes, though under the agreed-on rules, C would be an error

RyanCavanaugh avatar Oct 20 '23 22:10 RyanCavanaugh

:wave: 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 @magic-akari

:+1: Compiled
Emit:

import { Foo } from "./foo";
export var Bar;
(function (Bar) {
    Bar[Bar["B"] = 10] = "B";
    Bar[Bar["C"] = 11] = "C";
})(Bar || (Bar = {}));

Historical Information
Version Reproduction Outputs
4.8.2, 4.9.3, 5.0.2, 5.1.3, 5.2.2

:+1: Compiled
Emit:

import { Foo } from "./foo";
export var Bar;
(function (Bar) {
    Bar[Bar["B"] = 10] = "B";
    Bar[Bar["C"] = 11] = "C";
})(Bar || (Bar = {}));

typescript-bot avatar Oct 21 '23 08:10 typescript-bot

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


Comment by @magic-akari

:+1: Compiled
Emit:

import { Foo } from "./foo";
export var Bar;
(function (Bar) {
    Bar[Bar["B"] = 1] = "B";
    Bar[Bar["C"] = 2] = "C";
})(Bar || (Bar = {}));

Historical Information
Version Reproduction Outputs
4.8.2, 4.9.3, 5.0.2, 5.1.3, 5.2.2

:+1: Compiled
Emit:

import { Foo } from "./foo";
export var Bar;
(function (Bar) {
    Bar[Bar["B"] = 1] = "B";
    Bar[Bar["C"] = 2] = "C";
})(Bar || (Bar = {}));

typescript-bot avatar Oct 21 '23 08:10 typescript-bot

  • When an enum initializer expression that isn't a string literal has a string type

Could we allow template expressions? E.g. the following is guaranteed to have a string value. How do you feel about allowing that and not generating a reverse mapping for it?

enum Foo { A = `${Bar.A}` }

Edit: Never mind. Turns out in our code base this pattern happens so rarely, this doesn't feel worth the effort.

frigus02 avatar Dec 11 '23 10:12 frigus02