Skip to content

Commit 368cdfd

Browse files
FauxFauxandrewbranch
FauxFaux
and
andrewbranch
authored
fix: const enums + isolatedModules emit invalid code (#41933)
* chore: failing test for const enums and isolatedModules * fix: const enums + isolatedModules emit invalid code In `isolatedModules` mode, the compiler does not inline const enums, but also decides not to `import` them, leaving invalid code that throws a `ReferenceError` at runtime. This code: ``` import { SomeEnum } from './bar'; sink(SomeEnum.VALUE); ``` ..should compile to either: ``` var { SomeEnum } = require('./bar'); sink(SomeEnum.VALUE); ``` ..or (with const enum inlining): ``` sink(1 /* VALUE */); ``` ..but actually compiles to: ``` sink(SomeEnum.VALUE); ``` ..with no imports, which throws a ReferenceError at runtime. --- The compiler has already realised that the symbol is a referenced const enum, it just doesn't use this information when it comes to deciding whether to emit an import. This commit additionally checks that information, if we are compiling in isolatedModules mode. --- In my opinion, this is not the correct fix, but it is the simplest. In `isolatedModules` mode, `const enum` should probably be a compile error (because there are no benefits and the complexity is high, and, apparently, buggy). If not, the compiler should not be checking whether something is a const enum, and should just be treating it as a regular enum, and checking as if it was? Fixes #40499. * chore: extra test for type-only * feat: explicitly ban --isolatedModules --preserveConstEnums false * feat: isolatedModules implies preserveConstEnum * Update src/compiler/diagnosticMessages.json Co-authored-by: Andrew Branch <[email protected]> * chore: compiler test for argument incompatibility * Add and fix test for namespace import of const enum * Fix additional bug with reexport of const-enum-only module Co-authored-by: Andrew Branch <[email protected]> Co-authored-by: Andrew Branch <[email protected]>
1 parent 9171aa5 commit 368cdfd

28 files changed

+514
-11
lines changed

‎src/compiler/binder.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3435,7 +3435,7 @@ namespace ts {
34353435

34363436
function shouldReportErrorOnModuleDeclaration(node: ModuleDeclaration): boolean {
34373437
const instanceState = getModuleInstanceState(node);
3438-
return instanceState === ModuleInstanceState.Instantiated || (instanceState === ModuleInstanceState.ConstEnumOnly && !!options.preserveConstEnums);
3438+
return instanceState === ModuleInstanceState.Instantiated || (instanceState === ModuleInstanceState.ConstEnumOnly && shouldPreserveConstEnums(options));
34393439
}
34403440

34413441
function checkUnreachable(node: Node): boolean {

‎src/compiler/checker.ts

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2395,7 +2395,7 @@ namespace ts {
23952395
}
23962396
else {
23972397
Debug.assert(!!(result.flags & SymbolFlags.ConstEnum));
2398-
if (compilerOptions.preserveConstEnums) {
2398+
if (shouldPreserveConstEnums(compilerOptions)) {
23992399
diagnosticMessage = error(errorLocation, Diagnostics.Enum_0_used_before_its_declaration, declarationName);
24002400
}
24012401
}
@@ -23215,7 +23215,13 @@ namespace ts {
2321523215
if (isNonLocalAlias(symbol, /*excludes*/ SymbolFlags.Value) && !isInTypeQuery(location) && !getTypeOnlyAliasDeclaration(symbol)) {
2321623216
const target = resolveAlias(symbol);
2321723217
if (target.flags & SymbolFlags.Value) {
23218-
if (compilerOptions.preserveConstEnums && isExportOrExportExpression(location) || !isConstEnumOrConstEnumOnlyModule(target)) {
23218+
// An alias resolving to a const enum cannot be elided if (1) 'isolatedModules' is enabled
23219+
// (because the const enum value will not be inlined), or if (2) the alias is an export
23220+
// of a const enum declaration that will be preserved.
23221+
if (compilerOptions.isolatedModules ||
23222+
shouldPreserveConstEnums(compilerOptions) && isExportOrExportExpression(location) ||
23223+
!isConstEnumOrConstEnumOnlyModule(target)
23224+
) {
2321923225
markAliasSymbolAsReferenced(symbol);
2322023226
}
2322123227
else {
@@ -26271,7 +26277,11 @@ namespace ts {
2627126277
}
2627226278
prop = getPropertyOfType(apparentType, right.escapedText);
2627326279
}
26274-
if (isIdentifier(left) && parentSymbol && !(prop && isConstEnumOrConstEnumOnlyModule(prop))) {
26280+
// In `Foo.Bar.Baz`, 'Foo' is not referenced if 'Bar' is a const enum or a module containing only const enums.
26281+
// The exceptions are:
26282+
// 1. if 'isolatedModules' is enabled, because the const enum value will not be inlined, and
26283+
// 2. if 'preserveConstEnums' is enabled and the expression is itself an export, e.g. `export = Foo.Bar.Baz`.
26284+
if (isIdentifier(left) && parentSymbol && (compilerOptions.isolatedModules || !(prop && isConstEnumOrConstEnumOnlyModule(prop)) || shouldPreserveConstEnums(compilerOptions) && isExportOrExportExpression(node))) {
2627526285
markAliasReferenced(parentSymbol, node);
2627626286
}
2627726287

@@ -36648,7 +36658,7 @@ namespace ts {
3664836658
if (symbol.flags & SymbolFlags.ValueModule
3664936659
&& !inAmbientContext
3665036660
&& symbol.declarations.length > 1
36651-
&& isInstantiatedModule(node, !!compilerOptions.preserveConstEnums || !!compilerOptions.isolatedModules)) {
36661+
&& isInstantiatedModule(node, shouldPreserveConstEnums(compilerOptions))) {
3665236662
const firstNonAmbientClassOrFunc = getFirstNonAmbientClassOrFunctionDeclaration(symbol);
3665336663
if (firstNonAmbientClassOrFunc) {
3665436664
if (getSourceFileOfNode(node) !== getSourceFileOfNode(firstNonAmbientClassOrFunc)) {
@@ -38571,7 +38581,7 @@ namespace ts {
3857138581
// const enums and modules that contain only const enums are not considered values from the emit perspective
3857238582
// unless 'preserveConstEnums' option is set to true
3857338583
return !!(target.flags & SymbolFlags.Value) &&
38574-
(compilerOptions.preserveConstEnums || !isConstEnumOrConstEnumOnlyModule(target));
38584+
(shouldPreserveConstEnums(compilerOptions) || !isConstEnumOrConstEnumOnlyModule(target));
3857538585
}
3857638586

3857738587
function isConstEnumOrConstEnumOnlyModule(s: Symbol): boolean {
@@ -38581,13 +38591,14 @@ namespace ts {
3858138591
function isReferencedAliasDeclaration(node: Node, checkChildren?: boolean): boolean {
3858238592
if (isAliasSymbolDeclaration(node)) {
3858338593
const symbol = getSymbolOfNode(node);
38584-
if (symbol && getSymbolLinks(symbol).referenced) {
38594+
const links = symbol && getSymbolLinks(symbol);
38595+
if (links?.referenced) {
3858538596
return true;
3858638597
}
3858738598
const target = getSymbolLinks(symbol!).target; // TODO: GH#18217
3858838599
if (target && getEffectiveModifierFlags(node) & ModifierFlags.Export &&
3858938600
target.flags & SymbolFlags.Value &&
38590-
(compilerOptions.preserveConstEnums || !isConstEnumOrConstEnumOnlyModule(target))) {
38601+
(shouldPreserveConstEnums(compilerOptions) || !isConstEnumOrConstEnumOnlyModule(target))) {
3859138602
// An `export import ... =` of a value symbol is always considered referenced
3859238603
return true;
3859338604
}

‎src/compiler/diagnosticMessages.json

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3813,6 +3813,10 @@
38133813
"category": "Error",
38143814
"code": 5090
38153815
},
3816+
"Option 'preserveConstEnums' cannot be disabled when 'isolatedModules' is enabled.": {
3817+
"category": "Error",
3818+
"code": 5091
3819+
},
38163820

38173821
"Generates a sourcemap for each corresponding '.d.ts' file.": {
38183822
"category": "Message",

‎src/compiler/program.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3157,6 +3157,10 @@ namespace ts {
31573157
createDiagnosticForOptionName(Diagnostics.Option_isolatedModules_can_only_be_used_when_either_option_module_is_provided_or_option_target_is_ES2015_or_higher, "isolatedModules", "target");
31583158
}
31593159

3160+
if (options.preserveConstEnums === false) {
3161+
createDiagnosticForOptionName(Diagnostics.Option_preserveConstEnums_cannot_be_disabled_when_isolatedModules_is_enabled, "preserveConstEnums", "isolatedModules");
3162+
}
3163+
31603164
const firstNonExternalModuleSourceFile = find(files, f => !isExternalModule(f) && !isSourceFileJS(f) && !f.isDeclarationFile && f.scriptKind !== ScriptKind.JSON);
31613165
if (firstNonExternalModuleSourceFile) {
31623166
const span = getErrorSpanForNode(firstNonExternalModuleSourceFile, firstNonExternalModuleSourceFile);

‎src/compiler/transformers/ts.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2316,8 +2316,7 @@ namespace ts {
23162316
*/
23172317
function shouldEmitEnumDeclaration(node: EnumDeclaration) {
23182318
return !isEnumConst(node)
2319-
|| compilerOptions.preserveConstEnums
2320-
|| compilerOptions.isolatedModules;
2319+
|| shouldPreserveConstEnums(compilerOptions);
23212320
}
23222321

23232322
/**
@@ -2507,7 +2506,7 @@ namespace ts {
25072506
// If we can't find a parse tree node, assume the node is instantiated.
25082507
return true;
25092508
}
2510-
return isInstantiatedModule(node, !!compilerOptions.preserveConstEnums || !!compilerOptions.isolatedModules);
2509+
return isInstantiatedModule(node, shouldPreserveConstEnums(compilerOptions));
25112510
}
25122511

25132512
/**

‎src/compiler/utilities.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6001,6 +6001,10 @@ namespace ts {
60016001
return !!(compilerOptions.declaration || compilerOptions.composite);
60026002
}
60036003

6004+
export function shouldPreserveConstEnums(compilerOptions: CompilerOptions): boolean {
6005+
return !!(compilerOptions.preserveConstEnums || compilerOptions.isolatedModules);
6006+
}
6007+
60046008
export function isIncrementalCompilation(options: CompilerOptions) {
60056009
return !!(options.incremental || options.composite);
60066010
}

‎tests/baselines/reference/constEnumNamespaceReferenceCausesNoImport(isolatedmodules=false).js

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
//// [tests/cases/compiler/constEnumNamespaceReferenceCausesNoImport.ts] ////
2+
3+
//// [foo.ts]
4+
export const enum ConstFooEnum {
5+
Some,
6+
Values,
7+
Here
8+
};
9+
export function fooFunc(): void { /* removed */ }
10+
//// [index.ts]
11+
import * as Foo from "./foo";
12+
13+
function check(x: Foo.ConstFooEnum): void {
14+
switch (x) {
15+
case Foo.ConstFooEnum.Some:
16+
break;
17+
}
18+
}
19+
20+
//// [foo.js]
21+
"use strict";
22+
exports.__esModule = true;
23+
exports.fooFunc = void 0;
24+
;
25+
function fooFunc() { }
26+
exports.fooFunc = fooFunc;
27+
//// [index.js]
28+
"use strict";
29+
exports.__esModule = true;
30+
function check(x) {
31+
switch (x) {
32+
case 0 /* Some */:
33+
break;
34+
}
35+
}

‎tests/baselines/reference/constEnumNamespaceReferenceCausesNoImport(isolatedmodules=false).symbols

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
=== tests/cases/compiler/foo.ts ===
2+
export const enum ConstFooEnum {
3+
>ConstFooEnum : Symbol(ConstFooEnum, Decl(foo.ts, 0, 0))
4+
5+
Some,
6+
>Some : Symbol(ConstFooEnum.Some, Decl(foo.ts, 0, 32))
7+
8+
Values,
9+
>Values : Symbol(ConstFooEnum.Values, Decl(foo.ts, 1, 9))
10+
11+
Here
12+
>Here : Symbol(ConstFooEnum.Here, Decl(foo.ts, 2, 11))
13+
14+
};
15+
export function fooFunc(): void { /* removed */ }
16+
>fooFunc : Symbol(fooFunc, Decl(foo.ts, 4, 2))
17+
18+
=== tests/cases/compiler/index.ts ===
19+
import * as Foo from "./foo";
20+
>Foo : Symbol(Foo, Decl(index.ts, 0, 6))
21+
22+
function check(x: Foo.ConstFooEnum): void {
23+
>check : Symbol(check, Decl(index.ts, 0, 29))
24+
>x : Symbol(x, Decl(index.ts, 2, 15))
25+
>Foo : Symbol(Foo, Decl(index.ts, 0, 6))
26+
>ConstFooEnum : Symbol(Foo.ConstFooEnum, Decl(foo.ts, 0, 0))
27+
28+
switch (x) {
29+
>x : Symbol(x, Decl(index.ts, 2, 15))
30+
31+
case Foo.ConstFooEnum.Some:
32+
>Foo.ConstFooEnum.Some : Symbol(Foo.ConstFooEnum.Some, Decl(foo.ts, 0, 32))
33+
>Foo.ConstFooEnum : Symbol(Foo.ConstFooEnum, Decl(foo.ts, 0, 0))
34+
>Foo : Symbol(Foo, Decl(index.ts, 0, 6))
35+
>ConstFooEnum : Symbol(Foo.ConstFooEnum, Decl(foo.ts, 0, 0))
36+
>Some : Symbol(Foo.ConstFooEnum.Some, Decl(foo.ts, 0, 32))
37+
38+
break;
39+
}
40+
}

‎tests/baselines/reference/constEnumNamespaceReferenceCausesNoImport(isolatedmodules=false).types

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
=== tests/cases/compiler/foo.ts ===
2+
export const enum ConstFooEnum {
3+
>ConstFooEnum : ConstFooEnum
4+
5+
Some,
6+
>Some : ConstFooEnum.Some
7+
8+
Values,
9+
>Values : ConstFooEnum.Values
10+
11+
Here
12+
>Here : ConstFooEnum.Here
13+
14+
};
15+
export function fooFunc(): void { /* removed */ }
16+
>fooFunc : () => void
17+
18+
=== tests/cases/compiler/index.ts ===
19+
import * as Foo from "./foo";
20+
>Foo : typeof Foo
21+
22+
function check(x: Foo.ConstFooEnum): void {
23+
>check : (x: Foo.ConstFooEnum) => void
24+
>x : Foo.ConstFooEnum
25+
>Foo : any
26+
27+
switch (x) {
28+
>x : Foo.ConstFooEnum
29+
30+
case Foo.ConstFooEnum.Some:
31+
>Foo.ConstFooEnum.Some : Foo.ConstFooEnum.Some
32+
>Foo.ConstFooEnum : typeof Foo.ConstFooEnum
33+
>Foo : typeof Foo
34+
>ConstFooEnum : typeof Foo.ConstFooEnum
35+
>Some : Foo.ConstFooEnum.Some
36+
37+
break;
38+
}
39+
}

‎tests/baselines/reference/constEnumNamespaceReferenceCausesNoImport(isolatedmodules=true).js

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
//// [tests/cases/compiler/constEnumNamespaceReferenceCausesNoImport.ts] ////
2+
3+
//// [foo.ts]
4+
export const enum ConstFooEnum {
5+
Some,
6+
Values,
7+
Here
8+
};
9+
export function fooFunc(): void { /* removed */ }
10+
//// [index.ts]
11+
import * as Foo from "./foo";
12+
13+
function check(x: Foo.ConstFooEnum): void {
14+
switch (x) {
15+
case Foo.ConstFooEnum.Some:
16+
break;
17+
}
18+
}
19+
20+
//// [foo.js]
21+
"use strict";
22+
exports.__esModule = true;
23+
exports.fooFunc = exports.ConstFooEnum = void 0;
24+
var ConstFooEnum;
25+
(function (ConstFooEnum) {
26+
ConstFooEnum[ConstFooEnum["Some"] = 0] = "Some";
27+
ConstFooEnum[ConstFooEnum["Values"] = 1] = "Values";
28+
ConstFooEnum[ConstFooEnum["Here"] = 2] = "Here";
29+
})(ConstFooEnum = exports.ConstFooEnum || (exports.ConstFooEnum = {}));
30+
;
31+
function fooFunc() { }
32+
exports.fooFunc = fooFunc;
33+
//// [index.js]
34+
"use strict";
35+
exports.__esModule = true;
36+
var Foo = require("./foo");
37+
function check(x) {
38+
switch (x) {
39+
case Foo.ConstFooEnum.Some:
40+
break;
41+
}
42+
}

‎tests/baselines/reference/constEnumNamespaceReferenceCausesNoImport(isolatedmodules=true).symbols

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
=== tests/cases/compiler/foo.ts ===
2+
export const enum ConstFooEnum {
3+
>ConstFooEnum : Symbol(ConstFooEnum, Decl(foo.ts, 0, 0))
4+
5+
Some,
6+
>Some : Symbol(ConstFooEnum.Some, Decl(foo.ts, 0, 32))
7+
8+
Values,
9+
>Values : Symbol(ConstFooEnum.Values, Decl(foo.ts, 1, 9))
10+
11+
Here
12+
>Here : Symbol(ConstFooEnum.Here, Decl(foo.ts, 2, 11))
13+
14+
};
15+
export function fooFunc(): void { /* removed */ }
16+
>fooFunc : Symbol(fooFunc, Decl(foo.ts, 4, 2))
17+
18+
=== tests/cases/compiler/index.ts ===
19+
import * as Foo from "./foo";
20+
>Foo : Symbol(Foo, Decl(index.ts, 0, 6))
21+
22+
function check(x: Foo.ConstFooEnum): void {
23+
>check : Symbol(check, Decl(index.ts, 0, 29))
24+
>x : Symbol(x, Decl(index.ts, 2, 15))
25+
>Foo : Symbol(Foo, Decl(index.ts, 0, 6))
26+
>ConstFooEnum : Symbol(Foo.ConstFooEnum, Decl(foo.ts, 0, 0))
27+
28+
switch (x) {
29+
>x : Symbol(x, Decl(index.ts, 2, 15))
30+
31+
case Foo.ConstFooEnum.Some:
32+
>Foo.ConstFooEnum.Some : Symbol(Foo.ConstFooEnum.Some, Decl(foo.ts, 0, 32))
33+
>Foo.ConstFooEnum : Symbol(Foo.ConstFooEnum, Decl(foo.ts, 0, 0))
34+
>Foo : Symbol(Foo, Decl(index.ts, 0, 6))
35+
>ConstFooEnum : Symbol(Foo.ConstFooEnum, Decl(foo.ts, 0, 0))
36+
>Some : Symbol(Foo.ConstFooEnum.Some, Decl(foo.ts, 0, 32))
37+
38+
break;
39+
}
40+
}

‎tests/baselines/reference/constEnumNamespaceReferenceCausesNoImport(isolatedmodules=true).types

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
=== tests/cases/compiler/foo.ts ===
2+
export const enum ConstFooEnum {
3+
>ConstFooEnum : ConstFooEnum
4+
5+
Some,
6+
>Some : ConstFooEnum.Some
7+
8+
Values,
9+
>Values : ConstFooEnum.Values
10+
11+
Here
12+
>Here : ConstFooEnum.Here
13+
14+
};
15+
export function fooFunc(): void { /* removed */ }
16+
>fooFunc : () => void
17+
18+
=== tests/cases/compiler/index.ts ===
19+
import * as Foo from "./foo";
20+
>Foo : typeof Foo
21+
22+
function check(x: Foo.ConstFooEnum): void {
23+
>check : (x: Foo.ConstFooEnum) => void
24+
>x : Foo.ConstFooEnum
25+
>Foo : any
26+
27+
switch (x) {
28+
>x : Foo.ConstFooEnum
29+
30+
case Foo.ConstFooEnum.Some:
31+
>Foo.ConstFooEnum.Some : Foo.ConstFooEnum.Some
32+
>Foo.ConstFooEnum : typeof Foo.ConstFooEnum
33+
>Foo : typeof Foo
34+
>ConstFooEnum : typeof Foo.ConstFooEnum
35+
>Some : Foo.ConstFooEnum.Some
36+
37+
break;
38+
}
39+
}

0 commit comments

Comments
 (0)