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.

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

Allow to use a const in the initializer of a const enum #18887

Closed
wants to merge 7 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Oct 2, 2017

Was there any reason we didn't do this already? It doesn't seem to cause errors in the presence of circularity.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Oct 2, 2017

file1.ts

import { b } from './file2';

export const enum X {
  a = 1,
  c = b
}

file2.ts

export const b = 10;

In your branch this compiles cleanly under --isolatedModules but file1.ts will generate different (and possibly non-working) code under transpileModule

@ghost
Copy link
Author

ghost commented Oct 3, 2017

From discussion with @RyanCavanaugh: It looks like --isolatedModules just ignores the const in const enum anyway and produces accesses like X.c everywhere, so this won't affect that.
Ping @ahejlsberg to see if there are any other reasons we don't support this.

@@ -36,9 +38,13 @@ tests/cases/compiler/constEnumErrors.ts(42,9): error TS2478: 'const' enum member
Y = E1.Z,
~~~~
!!! error TS2474: In 'const' enum declarations member initializer must be constant expression.
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice to have this error go away and see only the new one.

Copy link
Author

Choose a reason for hiding this comment

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

Is it good enough to have the new error message?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I guess so.

@@ -21898,6 +21908,13 @@ namespace ts {
}
return undefined;
}

function getFromLiteralType(expr: Identifier | ElementAccessExpression | PropertyAccessExpression): string | number | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

getTypeFromLiteral[Type]Node? The current name doesn't mean anything to me at all.

case SyntaxKind.ElementAccessExpression:
case SyntaxKind.PropertyAccessExpression:
const ex = <PropertyAccessExpression | ElementAccessExpression>expr;
if (isConstantMemberAccess(ex)) {
const type = getTypeOfExpression(ex.expression);
const type = checkExpression(ex.expression);
Copy link
Member

Choose a reason for hiding this comment

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

why can't we use getTypeOfExpression anymore?

Copy link
Author

Choose a reason for hiding this comment

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

We can, but it just has special handling for CallExpression that we don't need, then calls checkExpression.

Copy link
Member

Choose a reason for hiding this comment

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

oh, that makes sense. I thought getTypeOfExpression was the fast path that was called by checkExpression, but I guess it actually just has some initial fast-path checks before delegating to checkExpression.

const enum E {
y = x,
~
!!! error TS2474: In 'const' enum declarations member initializer must be constant expression.
Copy link
Member

Choose a reason for hiding this comment

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

could do with a better error message here too; x is const, but circular.

@ajafff
Copy link
Contributor

ajafff commented Oct 6, 2017

This only works for constants with literal types? Otherwise there's no way the value can be inlined.
There should probably be a test for non-literal types, e.g. : string or : 1 | 2

@ghost
Copy link
Author

ghost commented Oct 6, 2017

@ahejlsberg Good to go?

@ghost
Copy link
Author

ghost commented Oct 18, 2017

@ahejlsberg bump

@ghost
Copy link
Author

ghost commented Nov 4, 2017

@ahejlsberg

@mhegazy
Copy link
Contributor

mhegazy commented Jan 8, 2018

@andy-ms can you talk to @ahejlsberg about this change

@typescript-bot
Copy link
Collaborator

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

@ghost ghost deleted the enumFoo branch June 13, 2018 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants