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 <andrewbranch@users.noreply.github.com>

* 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 <andrewbranch@users.noreply.github.com>
Co-authored-by: Andrew Branch <andrew@wheream.io>
This commit is contained in:
Chris West
2021-01-13 15:51:08 -08:00
committed by GitHub
co-authored by Andrew Branch Andrew Branch
parent 9171aa5a1b
commit 368cdfd29a
28 changed files with 514 additions and 11 deletions
+1 -1
View File
@@ -3435,7 +3435,7 @@ namespace ts {
function shouldReportErrorOnModuleDeclaration(node: ModuleDeclaration): boolean {
const instanceState = getModuleInstanceState(node);
return instanceState === ModuleInstanceState.Instantiated || (instanceState === ModuleInstanceState.ConstEnumOnly && !!options.preserveConstEnums);
return instanceState === ModuleInstanceState.Instantiated || (instanceState === ModuleInstanceState.ConstEnumOnly && shouldPreserveConstEnums(options));
}
function checkUnreachable(node: Node): boolean {
+18 -7
View File
@@ -2395,7 +2395,7 @@ namespace ts {
}
else {
Debug.assert(!!(result.flags & SymbolFlags.ConstEnum));
if (compilerOptions.preserveConstEnums) {
if (shouldPreserveConstEnums(compilerOptions)) {
diagnosticMessage = error(errorLocation, Diagnostics.Enum_0_used_before_its_declaration, declarationName);
}
}
@@ -23215,7 +23215,13 @@ namespace ts {
if (isNonLocalAlias(symbol, /*excludes*/ SymbolFlags.Value) && !isInTypeQuery(location) && !getTypeOnlyAliasDeclaration(symbol)) {
const target = resolveAlias(symbol);
if (target.flags & SymbolFlags.Value) {
if (compilerOptions.preserveConstEnums && isExportOrExportExpression(location) || !isConstEnumOrConstEnumOnlyModule(target)) {
// An alias resolving to a const enum cannot be elided if (1) 'isolatedModules' is enabled
// (because the const enum value will not be inlined), or if (2) the alias is an export
// of a const enum declaration that will be preserved.
if (compilerOptions.isolatedModules ||
shouldPreserveConstEnums(compilerOptions) && isExportOrExportExpression(location) ||
!isConstEnumOrConstEnumOnlyModule(target)
) {
markAliasSymbolAsReferenced(symbol);
}
else {
@@ -26271,7 +26277,11 @@ namespace ts {
}
prop = getPropertyOfType(apparentType, right.escapedText);
}
if (isIdentifier(left) && parentSymbol && !(prop && isConstEnumOrConstEnumOnlyModule(prop))) {
// In `Foo.Bar.Baz`, 'Foo' is not referenced if 'Bar' is a const enum or a module containing only const enums.
// The exceptions are:
// 1. if 'isolatedModules' is enabled, because the const enum value will not be inlined, and
// 2. if 'preserveConstEnums' is enabled and the expression is itself an export, e.g. `export = Foo.Bar.Baz`.
if (isIdentifier(left) && parentSymbol && (compilerOptions.isolatedModules || !(prop && isConstEnumOrConstEnumOnlyModule(prop)) || shouldPreserveConstEnums(compilerOptions) && isExportOrExportExpression(node))) {
markAliasReferenced(parentSymbol, node);
}
@@ -36648,7 +36658,7 @@ namespace ts {
if (symbol.flags & SymbolFlags.ValueModule
&& !inAmbientContext
&& symbol.declarations.length > 1
&& isInstantiatedModule(node, !!compilerOptions.preserveConstEnums || !!compilerOptions.isolatedModules)) {
&& isInstantiatedModule(node, shouldPreserveConstEnums(compilerOptions))) {
const firstNonAmbientClassOrFunc = getFirstNonAmbientClassOrFunctionDeclaration(symbol);
if (firstNonAmbientClassOrFunc) {
if (getSourceFileOfNode(node) !== getSourceFileOfNode(firstNonAmbientClassOrFunc)) {
@@ -38571,7 +38581,7 @@ namespace ts {
// const enums and modules that contain only const enums are not considered values from the emit perspective
// unless 'preserveConstEnums' option is set to true
return !!(target.flags & SymbolFlags.Value) &&
(compilerOptions.preserveConstEnums || !isConstEnumOrConstEnumOnlyModule(target));
(shouldPreserveConstEnums(compilerOptions) || !isConstEnumOrConstEnumOnlyModule(target));
}
function isConstEnumOrConstEnumOnlyModule(s: Symbol): boolean {
@@ -38581,13 +38591,14 @@ namespace ts {
function isReferencedAliasDeclaration(node: Node, checkChildren?: boolean): boolean {
if (isAliasSymbolDeclaration(node)) {
const symbol = getSymbolOfNode(node);
if (symbol && getSymbolLinks(symbol).referenced) {
const links = symbol && getSymbolLinks(symbol);
if (links?.referenced) {
return true;
}
const target = getSymbolLinks(symbol!).target; // TODO: GH#18217
if (target && getEffectiveModifierFlags(node) & ModifierFlags.Export &&
target.flags & SymbolFlags.Value &&
(compilerOptions.preserveConstEnums || !isConstEnumOrConstEnumOnlyModule(target))) {
(shouldPreserveConstEnums(compilerOptions) || !isConstEnumOrConstEnumOnlyModule(target))) {
// An `export import ... =` of a value symbol is always considered referenced
return true;
}
+4
View File
@@ -3813,6 +3813,10 @@
"category": "Error",
"code": 5090
},
"Option 'preserveConstEnums' cannot be disabled when 'isolatedModules' is enabled.": {
"category": "Error",
"code": 5091
},
"Generates a sourcemap for each corresponding '.d.ts' file.": {
"category": "Message",
+4
View File
@@ -3157,6 +3157,10 @@ namespace ts {
createDiagnosticForOptionName(Diagnostics.Option_isolatedModules_can_only_be_used_when_either_option_module_is_provided_or_option_target_is_ES2015_or_higher, "isolatedModules", "target");
}
if (options.preserveConstEnums === false) {
createDiagnosticForOptionName(Diagnostics.Option_preserveConstEnums_cannot_be_disabled_when_isolatedModules_is_enabled, "preserveConstEnums", "isolatedModules");
}
const firstNonExternalModuleSourceFile = find(files, f => !isExternalModule(f) && !isSourceFileJS(f) && !f.isDeclarationFile && f.scriptKind !== ScriptKind.JSON);
if (firstNonExternalModuleSourceFile) {
const span = getErrorSpanForNode(firstNonExternalModuleSourceFile, firstNonExternalModuleSourceFile);
+2 -3
View File
@@ -2316,8 +2316,7 @@ namespace ts {
*/
function shouldEmitEnumDeclaration(node: EnumDeclaration) {
return !isEnumConst(node)
|| compilerOptions.preserveConstEnums
|| compilerOptions.isolatedModules;
|| shouldPreserveConstEnums(compilerOptions);
}
/**
@@ -2507,7 +2506,7 @@ namespace ts {
// If we can't find a parse tree node, assume the node is instantiated.
return true;
}
return isInstantiatedModule(node, !!compilerOptions.preserveConstEnums || !!compilerOptions.isolatedModules);
return isInstantiatedModule(node, shouldPreserveConstEnums(compilerOptions));
}
/**
+4
View File
@@ -6001,6 +6001,10 @@ namespace ts {
return !!(compilerOptions.declaration || compilerOptions.composite);
}
export function shouldPreserveConstEnums(compilerOptions: CompilerOptions): boolean {
return !!(compilerOptions.preserveConstEnums || compilerOptions.isolatedModules);
}
export function isIncrementalCompilation(options: CompilerOptions) {
return !!(options.incremental || options.composite);
}
@@ -0,0 +1,35 @@
//// [tests/cases/compiler/constEnumNamespaceReferenceCausesNoImport.ts] ////
//// [foo.ts]
export const enum ConstFooEnum {
Some,
Values,
Here
};
export function fooFunc(): void { /* removed */ }
//// [index.ts]
import * as Foo from "./foo";
function check(x: Foo.ConstFooEnum): void {
switch (x) {
case Foo.ConstFooEnum.Some:
break;
}
}
//// [foo.js]
"use strict";
exports.__esModule = true;
exports.fooFunc = void 0;
;
function fooFunc() { }
exports.fooFunc = fooFunc;
//// [index.js]
"use strict";
exports.__esModule = true;
function check(x) {
switch (x) {
case 0 /* Some */:
break;
}
}
@@ -0,0 +1,40 @@
=== tests/cases/compiler/foo.ts ===
export const enum ConstFooEnum {
>ConstFooEnum : Symbol(ConstFooEnum, Decl(foo.ts, 0, 0))
Some,
>Some : Symbol(ConstFooEnum.Some, Decl(foo.ts, 0, 32))
Values,
>Values : Symbol(ConstFooEnum.Values, Decl(foo.ts, 1, 9))
Here
>Here : Symbol(ConstFooEnum.Here, Decl(foo.ts, 2, 11))
};
export function fooFunc(): void { /* removed */ }
>fooFunc : Symbol(fooFunc, Decl(foo.ts, 4, 2))
=== tests/cases/compiler/index.ts ===
import * as Foo from "./foo";
>Foo : Symbol(Foo, Decl(index.ts, 0, 6))
function check(x: Foo.ConstFooEnum): void {
>check : Symbol(check, Decl(index.ts, 0, 29))
>x : Symbol(x, Decl(index.ts, 2, 15))
>Foo : Symbol(Foo, Decl(index.ts, 0, 6))
>ConstFooEnum : Symbol(Foo.ConstFooEnum, Decl(foo.ts, 0, 0))
switch (x) {
>x : Symbol(x, Decl(index.ts, 2, 15))
case Foo.ConstFooEnum.Some:
>Foo.ConstFooEnum.Some : Symbol(Foo.ConstFooEnum.Some, Decl(foo.ts, 0, 32))
>Foo.ConstFooEnum : Symbol(Foo.ConstFooEnum, Decl(foo.ts, 0, 0))
>Foo : Symbol(Foo, Decl(index.ts, 0, 6))
>ConstFooEnum : Symbol(Foo.ConstFooEnum, Decl(foo.ts, 0, 0))
>Some : Symbol(Foo.ConstFooEnum.Some, Decl(foo.ts, 0, 32))
break;
}
}
@@ -0,0 +1,39 @@
=== tests/cases/compiler/foo.ts ===
export const enum ConstFooEnum {
>ConstFooEnum : ConstFooEnum
Some,
>Some : ConstFooEnum.Some
Values,
>Values : ConstFooEnum.Values
Here
>Here : ConstFooEnum.Here
};
export function fooFunc(): void { /* removed */ }
>fooFunc : () => void
=== tests/cases/compiler/index.ts ===
import * as Foo from "./foo";
>Foo : typeof Foo
function check(x: Foo.ConstFooEnum): void {
>check : (x: Foo.ConstFooEnum) => void
>x : Foo.ConstFooEnum
>Foo : any
switch (x) {
>x : Foo.ConstFooEnum
case Foo.ConstFooEnum.Some:
>Foo.ConstFooEnum.Some : Foo.ConstFooEnum.Some
>Foo.ConstFooEnum : typeof Foo.ConstFooEnum
>Foo : typeof Foo
>ConstFooEnum : typeof Foo.ConstFooEnum
>Some : Foo.ConstFooEnum.Some
break;
}
}
@@ -0,0 +1,42 @@
//// [tests/cases/compiler/constEnumNamespaceReferenceCausesNoImport.ts] ////
//// [foo.ts]
export const enum ConstFooEnum {
Some,
Values,
Here
};
export function fooFunc(): void { /* removed */ }
//// [index.ts]
import * as Foo from "./foo";
function check(x: Foo.ConstFooEnum): void {
switch (x) {
case Foo.ConstFooEnum.Some:
break;
}
}
//// [foo.js]
"use strict";
exports.__esModule = true;
exports.fooFunc = exports.ConstFooEnum = void 0;
var ConstFooEnum;
(function (ConstFooEnum) {
ConstFooEnum[ConstFooEnum["Some"] = 0] = "Some";
ConstFooEnum[ConstFooEnum["Values"] = 1] = "Values";
ConstFooEnum[ConstFooEnum["Here"] = 2] = "Here";
})(ConstFooEnum = exports.ConstFooEnum || (exports.ConstFooEnum = {}));
;
function fooFunc() { }
exports.fooFunc = fooFunc;
//// [index.js]
"use strict";
exports.__esModule = true;
var Foo = require("./foo");
function check(x) {
switch (x) {
case Foo.ConstFooEnum.Some:
break;
}
}
@@ -0,0 +1,40 @@
=== tests/cases/compiler/foo.ts ===
export const enum ConstFooEnum {
>ConstFooEnum : Symbol(ConstFooEnum, Decl(foo.ts, 0, 0))
Some,
>Some : Symbol(ConstFooEnum.Some, Decl(foo.ts, 0, 32))
Values,
>Values : Symbol(ConstFooEnum.Values, Decl(foo.ts, 1, 9))
Here
>Here : Symbol(ConstFooEnum.Here, Decl(foo.ts, 2, 11))
};
export function fooFunc(): void { /* removed */ }
>fooFunc : Symbol(fooFunc, Decl(foo.ts, 4, 2))
=== tests/cases/compiler/index.ts ===
import * as Foo from "./foo";
>Foo : Symbol(Foo, Decl(index.ts, 0, 6))
function check(x: Foo.ConstFooEnum): void {
>check : Symbol(check, Decl(index.ts, 0, 29))
>x : Symbol(x, Decl(index.ts, 2, 15))
>Foo : Symbol(Foo, Decl(index.ts, 0, 6))
>ConstFooEnum : Symbol(Foo.ConstFooEnum, Decl(foo.ts, 0, 0))
switch (x) {
>x : Symbol(x, Decl(index.ts, 2, 15))
case Foo.ConstFooEnum.Some:
>Foo.ConstFooEnum.Some : Symbol(Foo.ConstFooEnum.Some, Decl(foo.ts, 0, 32))
>Foo.ConstFooEnum : Symbol(Foo.ConstFooEnum, Decl(foo.ts, 0, 0))
>Foo : Symbol(Foo, Decl(index.ts, 0, 6))
>ConstFooEnum : Symbol(Foo.ConstFooEnum, Decl(foo.ts, 0, 0))
>Some : Symbol(Foo.ConstFooEnum.Some, Decl(foo.ts, 0, 32))
break;
}
}
@@ -0,0 +1,39 @@
=== tests/cases/compiler/foo.ts ===
export const enum ConstFooEnum {
>ConstFooEnum : ConstFooEnum
Some,
>Some : ConstFooEnum.Some
Values,
>Values : ConstFooEnum.Values
Here
>Here : ConstFooEnum.Here
};
export function fooFunc(): void { /* removed */ }
>fooFunc : () => void
=== tests/cases/compiler/index.ts ===
import * as Foo from "./foo";
>Foo : typeof Foo
function check(x: Foo.ConstFooEnum): void {
>check : (x: Foo.ConstFooEnum) => void
>x : Foo.ConstFooEnum
>Foo : any
switch (x) {
>x : Foo.ConstFooEnum
case Foo.ConstFooEnum.Some:
>Foo.ConstFooEnum.Some : Foo.ConstFooEnum.Some
>Foo.ConstFooEnum : typeof Foo.ConstFooEnum
>Foo : typeof Foo
>ConstFooEnum : typeof Foo.ConstFooEnum
>Some : Foo.ConstFooEnum.Some
break;
}
}
@@ -0,0 +1,50 @@
//// [tests/cases/compiler/constEnumNamespaceReferenceCausesNoImport2.ts] ////
//// [foo.ts]
export module ConstEnumOnlyModule {
export const enum ConstFooEnum {
Some,
Values,
Here
}
}
//// [reexport.ts]
import * as Foo from "./foo";
export = Foo.ConstEnumOnlyModule;
//// [index.ts]
import Foo = require("./reexport");
function check(x: Foo.ConstFooEnum): void {
switch (x) {
case Foo.ConstFooEnum.Some:
break;
}
}
//// [foo.js]
"use strict";
exports.__esModule = true;
exports.ConstEnumOnlyModule = void 0;
var ConstEnumOnlyModule;
(function (ConstEnumOnlyModule) {
var ConstFooEnum;
(function (ConstFooEnum) {
ConstFooEnum[ConstFooEnum["Some"] = 0] = "Some";
ConstFooEnum[ConstFooEnum["Values"] = 1] = "Values";
ConstFooEnum[ConstFooEnum["Here"] = 2] = "Here";
})(ConstFooEnum = ConstEnumOnlyModule.ConstFooEnum || (ConstEnumOnlyModule.ConstFooEnum = {}));
})(ConstEnumOnlyModule = exports.ConstEnumOnlyModule || (exports.ConstEnumOnlyModule = {}));
//// [reexport.js]
"use strict";
var Foo = require("./foo");
module.exports = Foo.ConstEnumOnlyModule;
//// [index.js]
"use strict";
exports.__esModule = true;
function check(x) {
switch (x) {
case 0 /* Some */:
break;
}
}
@@ -0,0 +1,25 @@
//// [tests/cases/compiler/isolatedModulesImportConstEnum.ts] ////
//// [file1.ts]
import { Foo } from './file2';
console.log(Foo.BAR);
//// [file2.ts]
export const enum Foo {
BAR,
}
//// [file2.js]
"use strict";
exports.__esModule = true;
exports.Foo = void 0;
var Foo;
(function (Foo) {
Foo[Foo["BAR"] = 0] = "BAR";
})(Foo = exports.Foo || (exports.Foo = {}));
//// [file1.js]
"use strict";
exports.__esModule = true;
var file2_1 = require("./file2");
console.log(file2_1.Foo.BAR);
@@ -0,0 +1,20 @@
=== tests/cases/compiler/file1.ts ===
import { Foo } from './file2';
>Foo : Symbol(Foo, Decl(file1.ts, 0, 8))
console.log(Foo.BAR);
>console.log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>console : Symbol(console, Decl(lib.dom.d.ts, --, --))
>log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>Foo.BAR : Symbol(Foo.BAR, Decl(file2.ts, 0, 23))
>Foo : Symbol(Foo, Decl(file1.ts, 0, 8))
>BAR : Symbol(Foo.BAR, Decl(file2.ts, 0, 23))
=== tests/cases/compiler/file2.ts ===
export const enum Foo {
>Foo : Symbol(Foo, Decl(file2.ts, 0, 0))
BAR,
>BAR : Symbol(Foo.BAR, Decl(file2.ts, 0, 23))
}
@@ -0,0 +1,21 @@
=== tests/cases/compiler/file1.ts ===
import { Foo } from './file2';
>Foo : typeof Foo
console.log(Foo.BAR);
>console.log(Foo.BAR) : void
>console.log : (...data: any[]) => void
>console : Console
>log : (...data: any[]) => void
>Foo.BAR : Foo
>Foo : typeof Foo
>BAR : Foo
=== tests/cases/compiler/file2.ts ===
export const enum Foo {
>Foo : Foo
BAR,
>BAR : Foo.BAR
}
@@ -0,0 +1,22 @@
//// [tests/cases/compiler/isolatedModulesImportConstEnumTypeOnly.ts] ////
//// [enum.ts]
export const enum Foo { Bar }
//// [index.ts]
import { Foo } from "./enum";
function f(foo: Foo) { return; }
//// [enum.js]
"use strict";
exports.__esModule = true;
exports.Foo = void 0;
var Foo;
(function (Foo) {
Foo[Foo["Bar"] = 0] = "Bar";
})(Foo = exports.Foo || (exports.Foo = {}));
//// [index.js]
"use strict";
exports.__esModule = true;
function f(foo) { return; }
@@ -0,0 +1,14 @@
=== tests/cases/compiler/enum.ts ===
export const enum Foo { Bar }
>Foo : Symbol(Foo, Decl(enum.ts, 0, 0))
>Bar : Symbol(Foo.Bar, Decl(enum.ts, 0, 23))
=== tests/cases/compiler/index.ts ===
import { Foo } from "./enum";
>Foo : Symbol(Foo, Decl(index.ts, 0, 8))
function f(foo: Foo) { return; }
>f : Symbol(f, Decl(index.ts, 0, 29))
>foo : Symbol(foo, Decl(index.ts, 1, 11))
>Foo : Symbol(Foo, Decl(index.ts, 0, 8))
@@ -0,0 +1,13 @@
=== tests/cases/compiler/enum.ts ===
export const enum Foo { Bar }
>Foo : Foo
>Bar : Foo.Bar
=== tests/cases/compiler/index.ts ===
import { Foo } from "./enum";
>Foo : typeof Foo
function f(foo: Foo) { return; }
>f : (foo: Foo) => void
>foo : Foo
@@ -0,0 +1,8 @@
error TS5091: Option 'preserveConstEnums' cannot be disabled when 'isolatedModules' is enabled.
!!! error TS5091: Option 'preserveConstEnums' cannot be disabled when 'isolatedModules' is enabled.
==== tests/cases/compiler/file1.ts (0 errors) ====
export {};
@@ -0,0 +1,8 @@
//// [file1.ts]
export {};
//// [file1.js]
"use strict";
exports.__esModule = true;
@@ -0,0 +1,5 @@
=== tests/cases/compiler/file1.ts ===
export {};
No type information for this code.
No type information for this code.
No type information for this code.
@@ -0,0 +1,5 @@
=== tests/cases/compiler/file1.ts ===
export {};
No type information for this code.
No type information for this code.
No type information for this code.
@@ -1,3 +1,5 @@
// @isolatedModules: true, false
// @filename: foo.ts
export const enum ConstFooEnum {
Some,
@@ -0,0 +1,24 @@
// @preserveConstEnums: true
// @noTypesAndSymbols: true
// @filename: foo.ts
export module ConstEnumOnlyModule {
export const enum ConstFooEnum {
Some,
Values,
Here
}
}
// @filename: reexport.ts
import * as Foo from "./foo";
export = Foo.ConstEnumOnlyModule;
// @filename: index.ts
import Foo = require("./reexport");
function check(x: Foo.ConstFooEnum): void {
switch (x) {
case Foo.ConstFooEnum.Some:
break;
}
}
@@ -0,0 +1,13 @@
// @isolatedModules: true
// @module: commonjs
// @filename: file1.ts
import { Foo } from './file2';
console.log(Foo.BAR);
// @filename: file2.ts
export const enum Foo {
BAR,
}
@@ -0,0 +1,8 @@
// @isolatedModules: true
// @filename: enum.ts
export const enum Foo { Bar }
// @filename: index.ts
import { Foo } from "./enum";
function f(foo: Foo) { return; }
@@ -0,0 +1,8 @@
// @isolatedModules: true
// @preserveConstEnums: false
// @module: commonjs
// @filename: file1.ts
export {};