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
Fix TypeScript Enum self-references #14093
Conversation
magic-akari
commented
Dec 31, 2021
•
edited by gitpod-io
bot
edited by gitpod-io
bot
Q | A |
---|---|
Fixed Issues? | Fixes #14092 |
Patch: Bug Fix? | Yes |
Major: Breaking Change? | |
Minor: New Feature? | |
Tests Added + Pass? | Yes |
Documentation PR Link | |
Any Dependency Changes? | |
License | MIT |
The replacement should skip nested identifiers. Otherwise this breaks: enum Bar {
a = 1,
}
enum Foo {
a = 2,
b = Bar.a,
} |
It works. I updated the tests. |
I am surprised that the enum reference can be resolved to either an enum member or the whole enum struct. For example TS nightly resolves enum A {
a = 0,
b = 1,
x = b.a,
} |
This seems very weird, and I'm going to report the issue to TypeScript. |
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.
This seems very weird, and I'm going to report the issue to TypeScript.
I agree, it looks like a TS bug.
@@ -124,7 +125,29 @@ export function translateEnumValues( | |||
value = t.stringLiteral(constValue); | |||
} | |||
} else { | |||
value = initializer; | |||
const IdentifierVisitor: Visitor = { |
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.
Please move this visitor to the top-level of the file. We do some processing+validation to visitors, and if it's always the same object this work can be cached. You can get the seen
map by using the "state" parameter:
Identifier(expr, { seen }) {
// ...
(style nit: we always call non-classes with a name that starts with a lowercase letter. Maybe enumSelfReferenceVisitor
.
const exprStmt = t.expressionStatement(initializer); | ||
|
||
traverse(t.program([exprStmt]), IdentifierVisitor); | ||
|
||
value = exprStmt.expression; |
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.
Rather than creating a "fake" program in order to traverse this, you can:
- Replace
return path.node.members.map(member => {
a few lines above withso that you have access to thereturn path.members.map(memberPath => {
NodePath
s rather than to the raw nodes - Get the
initializerPath
withmemberPath.get("initializer")
- Traverse it with
if (initializerPath.isIdentifier()) { enumSelfReferenceVisitor.Identifier(initializerPath, { seen }); } else { initializerPath.traverse(enumSelfReferenceVisitor, { seen }); }
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.
Thanks, it helps a lot.
I've run into another problem.
After Visitor is used once, the internal ReferencedIdentifier
function is not accessible again, which causes me to bring it up in the top scope.
MemberExpression(expr) { | ||
expr.skip(); | ||
}, |
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 think that skipping member expressions breaks transpilation of this code:
enum A {
a = 0,
b = 1,
// @ts-ignore
x = a.toString(),
}
You can try visiting ReferencedIdentifier
instead of Identifier
, so that it only visits the "correct" identifiers and you don't need to manually skip member expressions.
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 think we should handle enum reference in scope tracking, because enum initializer can have nested scopes, e.g.
enum A {
a = 0,
b = (() => { let a = 1; return a + 1 })(), // a is shadowed
c = (() => { return a + 2 })(), // a refers to A.a
}
This is exactly what scope tracking handles. We can mark TSEnumDeclaration
as Scopeable
in babel-types, so that babel-traverse will setup new scope for it. Then we can register const binding for every enum members in
const collectorVisitor: Visitor<CollectVisitorState> = { |
Now in the TS transform we can retrieve all the referenced Paths from the scope info: enumDelcarationPath.scope.getOwnBinding("a").referencePaths
, then replace them by member expressions.
As a bonus, the scope tracking should automatically throws on the following invalid cases, which we are now incorrectly passing:
enum A {
b = a + 1, // can not reference a before initialization
a = 0
}
This comment has been minimized.
This comment has been minimized.
Hi, @JLHwung , enum A {
C = 2,
}
namespace A {
export const B = 1;
} output changes: var A;
(function (A) {
A[A["C"] = 2] = "C";
})(A || (A = {}));
+ let A;
(function (A) {
A.B = 1;
})(A || (A = {}));
|
Here are some of the interesting things I found while exploring TS Scope. enum Foo {
a = 1,
}
enum Foo {
b = 2,
} var Foo;
(function (Foo) {
Foo[Foo["a"] = 1] = "a";
})(Foo || (Foo = {}));
(function (Foo) {
Foo[Foo["b"] = 2] = "b";
})(Foo || (Foo = {})); But namespace can also be merged with enum. enum Foo {
a = 1,
}
namespace Foo {
export const b = 2;
} var Foo;
(function (Foo) {
Foo[Foo["a"] = 1] = "a";
})(Foo || (Foo = {}));
(function (Foo) {
Foo.b = 2;
})(Foo || (Foo = {})); In fact namespace can be merged with many things like function and class. class Foo {
a = 1;
}
namespace Foo {
export const b = 2;
}
function Bar() {}
namespace Bar {
export const __esModule = true;
} class Foo {
constructor() {
this.a = 1;
}
}
(function (Foo) {
Foo.b = 2;
})(Foo || (Foo = {}));
function Bar() { }
(function (Bar) {
Bar.__esModule = true;
})(Bar || (Bar = {})); What if you define both function, enum and namespace with the same name? |
I think the current PR is good enough to go. To fully support enum reference and the merging behaviour, we can introduce accumulation scope and create An accumulation scope is a scope 1) shared within an array of nodes and 2) incrementally created for nodes spanned in different AST. For example, in an enum declaration interface EnumDeclaration {
id: Identifier,
members: EnumMember[]
} The enum id should be registered in the upper scope, so the enum scope is only shared within its members. Currently for every Scopeable AST node, we create an isolated scope for the node. To support accumulation scope, let's say we first register a new enum-kind binding for the enum id, with some extra binding info: Binding {
kind: "enum",
enumMembers: Scope
} now we can extend
when As the accumulation scope is now determined only by the enum id binding, we can handle the mergable enum because The storage of accumulation scope can be improved: as long as the scope can be accessed and uniquely determined. For example, it can be placed in parent scope (where enum is defined) as This approach can potentially handle another issue: #14023 (comment), we can create an accumulation scope for all non-simple formal parameters. |
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.
Thanks!