From 293eba6203e16e7f26c3d1457f32f4447925ff15 Mon Sep 17 00:00:00 2001 From: Alan Pierce Date: Sat, 10 Nov 2018 14:45:26 -0800 Subject: [PATCH 1/2] Change isolatedModules to allow const enum declaration and disallow access Fixes #20703 with solution suggested in https://github.com/Microsoft/TypeScript/issues/20703#issuecomment-361434795 Previously, `--isolatedModules` gave an error for any ambient const enum, which meant that some third-party libraries would always give errors even if the ambient const enums they declare were never used. Now, we only give an error when an ambient const enum is referenced, which allows such libraries to still be used as long as the const enums are never accessed. Some nuances: * As before, the error is only surfaced for *ambient* const enums. With non-ambient const enums, we know that an `isolatedModules` build will emit the enum and produce a plain reference rather than inlining the constant, so everything will still work. * I originally planned to do this check in the code path that inlines the constant, but that code is only exercised at emit time, so, for example, the TS language service wasn't giving an error in my editor. Instead, I do the check at typecheck time next to another const-enum-related check. * This can be a breaking change when using `skipLibCheck` because the error is typically moved from a .d.ts file to a .ts file. Testing done: I ran this TS build on a large project of mine that previously had disabled `isolatedModules` so I could use the `chalk` library. With `isolatedModules` enabled, there was no longer an error in the chalk typedefs, and a reference to the `Level` const enum produced an error in my editor. --- src/compiler/checker.ts | 40 +++++++++++-------- src/compiler/diagnosticMessages.json | 8 ++-- ...isolatedModulesAmbientConstEnum.errors.txt | 9 +++-- .../isolatedModulesAmbientConstEnum.js | 5 ++- .../isolatedModulesAmbientConstEnum.symbols | 5 ++- .../isolatedModulesAmbientConstEnum.types | 7 +++- .../isolatedModulesAmbientConstEnum.ts | 2 +- 7 files changed, 46 insertions(+), 30 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index e24c707d2d78e..1e54b5d7848ff 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -22567,21 +22567,33 @@ namespace ts { } if (isConstEnumObjectType(type)) { - // enum object type for const enums are only permitted in: - // - 'left' in property access - // - 'object' in indexed access - // - target in rhs of import statement - const ok = - (node.parent.kind === SyntaxKind.PropertyAccessExpression && (node.parent).expression === node) || - (node.parent.kind === SyntaxKind.ElementAccessExpression && (node.parent).expression === node) || - ((node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.QualifiedName) && isInRightSideOfImportOrExportAssignment(node) || + checkConstEnumAccess(node, type); + } + return type; + } + + function checkConstEnumAccess(node: Expression | QualifiedName, type: Type) { + // enum object type for const enums are only permitted in: + // - 'left' in property access + // - 'object' in indexed access + // - target in rhs of import statement + const ok = + (node.parent.kind === SyntaxKind.PropertyAccessExpression && (node.parent).expression === node) || + (node.parent.kind === SyntaxKind.ElementAccessExpression && (node.parent).expression === node) || + ((node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.QualifiedName) && isInRightSideOfImportOrExportAssignment(node) || (node.parent.kind === SyntaxKind.TypeQuery && (node.parent).exprName === node)); - if (!ok) { - error(node, Diagnostics.const_enums_can_only_be_used_in_property_or_index_access_expressions_or_the_right_hand_side_of_an_import_declaration_or_export_assignment_or_type_query); + if (!ok) { + error(node, Diagnostics.const_enums_can_only_be_used_in_property_or_index_access_expressions_or_the_right_hand_side_of_an_import_declaration_or_export_assignment_or_type_query); + } + + if (compilerOptions.isolatedModules) { + Debug.assert(!!(type.symbol.flags & SymbolFlags.ConstEnum)); + const constEnumDeclaration = type.symbol.valueDeclaration as EnumDeclaration; + if (constEnumDeclaration.flags & NodeFlags.Ambient) { + error(node, Diagnostics.Cannot_access_ambient_const_enums_when_the_isolatedModules_flag_is_provided); } } - return type; } function checkParenthesizedExpression(node: ParenthesizedExpression, checkMode?: CheckMode): Type { @@ -26721,11 +26733,6 @@ namespace ts { computeEnumMemberValues(node); - const enumIsConst = isEnumConst(node); - if (compilerOptions.isolatedModules && enumIsConst && node.flags & NodeFlags.Ambient) { - error(node.name, Diagnostics.Ambient_const_enums_are_not_allowed_when_the_isolatedModules_flag_is_provided); - } - // Spec 2014 - Section 9.3: // It isn't possible for one enum declaration to continue the automatic numbering sequence of another, // and when an enum type has multiple declarations, only one declaration is permitted to omit a value @@ -26736,6 +26743,7 @@ namespace ts { const firstDeclaration = getDeclarationOfKind(enumSymbol, node.kind); if (node === firstDeclaration) { if (enumSymbol.declarations.length > 1) { + const enumIsConst = isEnumConst(node); // check that const is placed\omitted on all enum declarations forEach(enumSymbol.declarations, decl => { if (isEnumDeclaration(decl) && isEnumConst(decl) !== enumIsConst) { diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index 3ba74f0307317..f593a49398d37 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -659,10 +659,6 @@ "category": "Error", "code": 1208 }, - "Ambient const enums are not allowed when the '--isolatedModules' flag is provided.": { - "category": "Error", - "code": 1209 - }, "Invalid use of '{0}'. Class definitions are automatically in strict mode.": { "category": "Error", "code": 1210 @@ -2513,6 +2509,10 @@ "category": "Message", "code": 2738 }, + "Cannot access ambient const enums when the '--isolatedModules' flag is provided.": { + "category": "Error", + "code": 2739 + }, "Import declaration '{0}' is using private name '{1}'.": { "category": "Error", diff --git a/tests/baselines/reference/isolatedModulesAmbientConstEnum.errors.txt b/tests/baselines/reference/isolatedModulesAmbientConstEnum.errors.txt index 642e38cc6e0d7..c4569f2ac3ed0 100644 --- a/tests/baselines/reference/isolatedModulesAmbientConstEnum.errors.txt +++ b/tests/baselines/reference/isolatedModulesAmbientConstEnum.errors.txt @@ -1,8 +1,9 @@ -tests/cases/compiler/file1.ts(1,20): error TS1209: Ambient const enums are not allowed when the '--isolatedModules' flag is provided. +tests/cases/compiler/file1.ts(2,16): error TS2739: Cannot access ambient const enums when the '--isolatedModules' flag is provided. ==== tests/cases/compiler/file1.ts (1 errors) ==== declare const enum E { X = 1} - ~ -!!! error TS1209: Ambient const enums are not allowed when the '--isolatedModules' flag is provided. - export var y; \ No newline at end of file + export var y = E.X; + ~ +!!! error TS2739: Cannot access ambient const enums when the '--isolatedModules' flag is provided. + \ No newline at end of file diff --git a/tests/baselines/reference/isolatedModulesAmbientConstEnum.js b/tests/baselines/reference/isolatedModulesAmbientConstEnum.js index 44b6e1c1d76f3..2977c8ab82749 100644 --- a/tests/baselines/reference/isolatedModulesAmbientConstEnum.js +++ b/tests/baselines/reference/isolatedModulesAmbientConstEnum.js @@ -1,6 +1,7 @@ //// [file1.ts] declare const enum E { X = 1} -export var y; +export var y = E.X; + //// [file1.js] -export var y; +export var y = E.X; diff --git a/tests/baselines/reference/isolatedModulesAmbientConstEnum.symbols b/tests/baselines/reference/isolatedModulesAmbientConstEnum.symbols index 2da04dbdf02f1..828f79379b1c2 100644 --- a/tests/baselines/reference/isolatedModulesAmbientConstEnum.symbols +++ b/tests/baselines/reference/isolatedModulesAmbientConstEnum.symbols @@ -3,6 +3,9 @@ declare const enum E { X = 1} >E : Symbol(E, Decl(file1.ts, 0, 0)) >X : Symbol(E.X, Decl(file1.ts, 0, 22)) -export var y; +export var y = E.X; >y : Symbol(y, Decl(file1.ts, 1, 10)) +>E.X : Symbol(E.X, Decl(file1.ts, 0, 22)) +>E : Symbol(E, Decl(file1.ts, 0, 0)) +>X : Symbol(E.X, Decl(file1.ts, 0, 22)) diff --git a/tests/baselines/reference/isolatedModulesAmbientConstEnum.types b/tests/baselines/reference/isolatedModulesAmbientConstEnum.types index 8b714d98f6eca..bb4212895f98d 100644 --- a/tests/baselines/reference/isolatedModulesAmbientConstEnum.types +++ b/tests/baselines/reference/isolatedModulesAmbientConstEnum.types @@ -4,6 +4,9 @@ declare const enum E { X = 1} >X : E.X >1 : 1 -export var y; ->y : any +export var y = E.X; +>y : E +>E.X : E +>E : typeof E +>X : E diff --git a/tests/cases/compiler/isolatedModulesAmbientConstEnum.ts b/tests/cases/compiler/isolatedModulesAmbientConstEnum.ts index 59fcdcd1fe422..993235c2c0c57 100644 --- a/tests/cases/compiler/isolatedModulesAmbientConstEnum.ts +++ b/tests/cases/compiler/isolatedModulesAmbientConstEnum.ts @@ -4,4 +4,4 @@ // @filename: file1.ts declare const enum E { X = 1} -export var y; \ No newline at end of file +export var y = E.X; From beae9d7be94337482bf4539b9547bfd512f53364 Mon Sep 17 00:00:00 2001 From: Alan Pierce Date: Fri, 25 Jan 2019 09:31:36 -0800 Subject: [PATCH 2/2] Accept whitespace change in baseline after merge --- .../reference/isolatedModulesAmbientConstEnum.errors.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/baselines/reference/isolatedModulesAmbientConstEnum.errors.txt b/tests/baselines/reference/isolatedModulesAmbientConstEnum.errors.txt index 417436486de48..6deae7b10e138 100644 --- a/tests/baselines/reference/isolatedModulesAmbientConstEnum.errors.txt +++ b/tests/baselines/reference/isolatedModulesAmbientConstEnum.errors.txt @@ -6,3 +6,4 @@ tests/cases/compiler/file1.ts(2,16): error TS2748: Cannot access ambient const e export var y = E.X; ~ !!! error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided. + \ No newline at end of file