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

Fix TypeScript Enum self-references #14093

Merged
merged 6 commits into from Jan 6, 2022
Merged

Fix TypeScript Enum self-references #14093

merged 6 commits into from Jan 6, 2022

Conversation

magic-akari
Copy link
Contributor

@magic-akari magic-akari commented Dec 31, 2021

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

@lightmare
Copy link
Contributor

lightmare commented Dec 31, 2021

The replacement should skip nested identifiers. Otherwise this breaks:

enum Bar {
  a = 1,
}
enum Foo {
  a = 2,
  b = Bar.a,
}

@magic-akari
Copy link
Contributor Author

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.

@magic-akari magic-akari marked this pull request as draft January 1, 2022 01:40
@JLHwung
Copy link
Contributor

JLHwung commented Jan 1, 2022

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 b.a to 0 as if it were A.a (playground).

enum A {
    a = 0,
    b = 1,
    x = b.a,
}

@JLHwung JLHwung added area: typescript PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jan 1, 2022
@magic-akari magic-akari marked this pull request as ready for review January 1, 2022 06:00
@magic-akari
Copy link
Contributor Author

magic-akari commented Jan 1, 2022

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 b.a to 0 as if it were A.a (playground).

enum A {
    a = 0,
    b = 1,
    x = b.a,
}

This seems very weird, and I'm going to report the issue to TypeScript.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a 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 = {
Copy link
Member

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.

Comment on lines 145 to 149
const exprStmt = t.expressionStatement(initializer);

traverse(t.program([exprStmt]), IdentifierVisitor);

value = exprStmt.expression;
Copy link
Member

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:

  1. Replace return path.node.members.map(member => { a few lines above with
    return path.members.map(memberPath => {
    so that you have access to the NodePaths rather than to the raw nodes
  2. Get the initializerPath with memberPath.get("initializer")
  3. Traverse it with
     if (initializerPath.isIdentifier()) {
       enumSelfReferenceVisitor.Identifier(initializerPath, { seen });
     } else {
       initializerPath.traverse(enumSelfReferenceVisitor, { seen });
     }

Copy link
Contributor Author

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.

Comment on lines 140 to 142
MemberExpression(expr) {
expr.skip();
},
Copy link
Member

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.

Copy link
Contributor

@JLHwung JLHwung left a 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
}

@magic-akari

This comment has been minimized.

@magic-akari
Copy link
Contributor Author

magic-akari commented Jan 3, 2022

Hi, @JLHwung ,
I tried to mark the TSEnumDeclaration as scopable.
It breaks a test case.

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 = {}));

enum and namespace are mergeable.
Adding a scope to enum will cause them not to be merged.
Refactoring enum and namespace at the same time is a big work.

@magic-akari
Copy link
Contributor Author

Here are some of the interesting things I found while exploring TS Scope.
The enum in the definition can be merged with each other.

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?
You get an error 'Enum declarations can only merge with namespace or other enum declarations.'

@JLHwung
Copy link
Contributor

JLHwung commented Jan 3, 2022

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 enumMembers binding info.

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

export function setScope(this: NodePath) {

when this is an EnumMember, we return parentPath.scope.getOwnBinding(parentPath.id.name).enumMembers as path.scope. Because it is an accumulation scope, we always run scope.crawl() instead of scope.init() (which is basically run scope.crawl() only once), so every enum member is effectively registered within this scope.

As the accumulation scope is now determined only by the enum id binding, we can handle the mergable enum because parentPath.scope.getOwnBinding(parentPath.id.name).enumMembers returns the same scope for every enum defined within the same scope. We can also handle namespace with extra codes.

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 Map<string, Scope>.

This approach can potentially handle another issue: #14023 (comment), we can create an accumulation scope for all non-simple formal parameters.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thanks!

@nicolo-ribaudo nicolo-ribaudo changed the title Fix TypeScript Enum Reference Fix TypeScript Enum self-references Jan 6, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit bee3e35 into babel:main Jan 6, 2022
@magic-akari magic-akari deleted the fix/ts-enum branch February 4, 2022 03:26
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 6, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Enum member init expressions should qualify member name references in certain cases
4 participants