-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Implement erasable Enum Annotations #61414
base: main
Are you sure you want to change the base?
Conversation
… to Enum Literals)
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
@@ -822,6 +826,9 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void { | |||
if (isNamedDeclaration(node)) { | |||
setParent(node.name, node); | |||
} | |||
if (isEnumLiteralExpression(node) && symbol.valueDeclaration === node.parent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXME: delete
@@ -16706,6 +16711,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
links.resolvedSymbol = unknownSymbol; | |||
return links.resolvedType = checkExpressionCached(node.parent.expression); | |||
} | |||
// `var MyEnum: enum = { FirstValue: 1, SecondValue: 2 }` should resolve to a union of the enum values. | |||
if (isEnumTypeReference(node) && isVariableDeclaration(node.parent) && node.parent.initializer && isEnumLiteralExpression(node.parent.initializer)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXME: isEnumLiteralDeclaration(node)
@@ -44128,7 +44140,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
checkClassNameCollisionWithObject(name); | |||
} | |||
} | |||
else if (isEnumDeclaration(node)) { | |||
else if (isEnumDeclaration(node) || (isVariableDeclaration(node) && node.initializer && isEnumLiteralExpression(node.initializer))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too
const nonexist: E1 = E1.NonexistingShorthand; // ok | ||
const exist: E1 = E1.ExistingShorthand; // ok | ||
const ival: E1 = E1.Int; // ok | ||
const sval: E1 = E1.String; // ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this implementation support types for individual variants? In other words, would the following work?
const exist: E1.ExistingShorthand = E1.ExistingShorthand; // ok
const ival: E1.Int = E1.Int; // ok
const sval: E1.String = E1.String;
I looked through the PR, but did not see code handling it or tests for this. It seems however to delegate to the existing enum handling in some places so I assume that it should work. I would appreciate a confirmation, thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made an effort to add a few of these tests (and they seem to be working, although it is a bit loose with accepting a wider number type for a unique numeric enum value. I believe that's normal for TS enums, though), but mostly I was focused on getting the invariant checks working. Thanks for the suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 🙂
….ts for individual members as types
In order to meet the goal of allowing "erasable" syntax only, which can be trivially converted to JS by removing type annotations, as described in #60790, including input from the TypeScript team to prefer the annotation to the "as enum" syntax.
This permits emitting code by simply erasing the Type Annotation. The TypeChecker sees references to the variable as a reference to an Enum-like type.
There are currently a few remaining obvious issues, for instance comments surrounding EnumMembers in the source are omitted from the emitted JS.
CC @nicolo-ribaudo @romulocintra