Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change isolatedModules to allow const enum declaration and disallow access #28465

Merged
merged 4 commits into from
Feb 6, 2019
Merged

Change isolatedModules to allow const enum declaration and disallow access #28465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
293eba6
Change isolatedModules to allow const enum declaration and disallow a…
alangpierce Nov 10, 2018
a60795f
Merge remote-tracking branch 'origin/master' into enforce-const-enum-…
alangpierce Jan 25, 2019
beae9d7
Accept whitespace change in baseline after merge
alangpierce Jan 25, 2019
942b020
Merge remote-tracking branch 'origin/master' into enforce-const-enum-…
alangpierce Feb 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 25 additions & 17 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23261,22 +23261,34 @@ namespace ts {
const uninstantiatedType = checkExpressionWorker(node, checkMode, forceTuple);
const type = instantiateTypeWithSingleGenericCallSignature(node, uninstantiatedType, checkMode);
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 && (<PropertyAccessExpression>node.parent).expression === node) ||
(node.parent.kind === SyntaxKind.ElementAccessExpression && (<ElementAccessExpression>node.parent).expression === node) ||
((node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.QualifiedName) && isInRightSideOfImportOrExportAssignment(<Identifier>node) ||
checkConstEnumAccess(node, type);
}
currentNode = saveCurrentNode;
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 && (<PropertyAccessExpression>node.parent).expression === node) ||
(node.parent.kind === SyntaxKind.ElementAccessExpression && (<ElementAccessExpression>node.parent).expression === node) ||
((node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.QualifiedName) && isInRightSideOfImportOrExportAssignment(<Identifier>node) ||
(node.parent.kind === SyntaxKind.TypeQuery && (<TypeQueryNode>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);
}
}
currentNode = saveCurrentNode;
return type;
}

function checkParenthesizedExpression(node: ParenthesizedExpression, checkMode?: CheckMode): Type {
Expand Down Expand Up @@ -27456,11 +27468,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
Expand All @@ -27471,6 +27478,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) {
Expand Down
8 changes: 4 additions & 4 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -2581,6 +2577,10 @@
"category": "Error",
"code": 2747
},
"Cannot access ambient const enums when the '--isolatedModules' flag is provided.": {
"category": "Error",
"code": 2748
},

"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
Expand Down
9 changes: 5 additions & 4 deletions tests/baselines/reference/isolatedModulesAmbientConstEnum.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 TS2748: 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;
export var y = E.X;
Copy link
Member

@sheetalkamat sheetalkamat Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this error be any other file than the declaration of const enum file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention with this change is for the error to be on the access, not on the declaration, so in practice it could be in a completely different file than the const enum declaration. In a typical case, I might import the chalk library, which shouldn't error, but any attempt to use the Level enum should error.

I could see an argument for putting an additional error on the const enum (like before), but the point of this change is that simply declaring a const enum is not an error anymore; the error is if you try to use it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the error is on access but declaring const enum a {} and using a in module1 shouldn't cause error. It should cause when accessing in module2...n instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I just tested it a bit more, and I believe the error still makes sense even within the same file. To be clear, this only happens with isolatedModules enabled and only happens for ambient const enums (the declare is necessary in the code above example).

From my testing, it looks like with isolatedModules enabled, const enums are always compiled as plain enums and const enum accesses are always compiled as plain enum accesses, even when the const enum is in the same file. That means that in this code snippet, the first line is erased (since it's a declare) and the second line is compiled as-is as export var y = E.X. With isolatedModules, TypeScript will not inline the constant even within the same file, so this code would crash at runtime (because E doesn't exist) even though it would work at runtime without isolatedModules. So I think it's fair for TS to error here.

It certainly seems like a reasonable future improvement to implement inlining within the same file even when isolatedModules is on. In that case, this error could be limited to cross-file accesses, but for now, even same-file ambient const enum accesses need to be errors. This would also require the Babel implementation to implement same-file constant inlining.

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

5 changes: 3 additions & 2 deletions tests/baselines/reference/isolatedModulesAmbientConstEnum.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
5 changes: 4 additions & 1 deletion tests/baselines/reference/isolatedModulesAmbientConstEnum.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))

7 changes: 5 additions & 2 deletions tests/baselines/reference/isolatedModulesAmbientConstEnum.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

2 changes: 1 addition & 1 deletion tests/cases/compiler/isolatedModulesAmbientConstEnum.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@
// @filename: file1.ts

declare const enum E { X = 1}
export var y;
export var y = E.X;