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

feat(traverse): introduce MaybeBoundIdentifier #7265

Merged

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Nov 13, 2024

MaybeBoundIdentifier is similar to BoundIdentifier, but can be used where the identifier may or may not have have a SymbolId (may or may not be bound).

Typical usage:

// Create `MaybeBoundIdentifier` from an existing `IdentifierReference`
let binding = MaybeBoundIdentifier::from_identifier_reference(ident, ctx);

// Generate `IdentifierReference`s and insert them into AST
assign_expr.left = binding.create_write_target(ctx);
assign_expr.right = binding.create_read_expression(ctx);

Copy link

graphite-app bot commented Nov 13, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

Copy link
Contributor Author

overlookmotel commented Nov 13, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @overlookmotel and the rest of your teammates on Graphite Graphite

@github-actions github-actions bot added the C-enhancement Category - New feature or request label Nov 13, 2024
@overlookmotel overlookmotel marked this pull request as ready for review November 13, 2024 11:24
Copy link

codspeed-hq bot commented Nov 13, 2024

CodSpeed Performance Report

Merging #7265 will not alter performance

Comparing 11-13-feat_traverse_introduce_maybeboundidentifier_ (8c754b1) with main (de472ca)

Summary

✅ 30 untouched benchmarks

@overlookmotel
Copy link
Contributor Author

@Boshen This API is intended for your spread transform use case.

@overlookmotel overlookmotel force-pushed the 11-13-feat_traverse_introduce_maybeboundidentifier_ branch from 02dcd21 to c2d693a Compare November 13, 2024 12:44
@Boshen
Copy link
Member

Boshen commented Nov 13, 2024

Confirmed working as intended.

        let maybe_bound_identifier = if let Expression::Identifier(ident) = init {
            MaybeBoundIdentifier::from_identifier_reference(ident, ctx)
        } else {
            let bound_identifier =
                ctx.generate_uid_in_current_scope_based_on_node(init, builder.symbol_flags);
            let id = bound_identifier.create_binding_pattern(ctx);
            let init = ctx.ast.move_expression(init);
            builder.decls.push(ctx.ast.variable_declarator(SPAN, decl.kind, id, Some(init), false));
            bound_identifier.to_maybe_bound_identifier()
        };

@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Nov 13, 2024
Copy link

graphite-app bot commented Nov 13, 2024

Merge activity

`MaybeBoundIdentifier` is similar to `BoundIdentifier`, but can be used where the identifier may or may not have have a `SymbolId` (may or may not be bound).

Typical usage:

```rs
// Create `MaybeBoundIdentifier` from an existing `IdentifierReference`
let binding = MaybeBoundIdentifier::from_identifier_reference(ident, ctx);

// Generate `IdentifierReference`s and insert them into AST
assign_expr.left = binding.create_write_target(ctx);
assign_expr.right = binding.create_read_expression(ctx);
```
@Boshen Boshen force-pushed the 11-13-feat_traverse_introduce_maybeboundidentifier_ branch from c2d693a to 8c754b1 Compare November 13, 2024 15:12
@graphite-app graphite-app bot merged commit 8c754b1 into main Nov 13, 2024
27 checks passed
@graphite-app graphite-app bot deleted the 11-13-feat_traverse_introduce_maybeboundidentifier_ branch November 13, 2024 15:17
@overlookmotel
Copy link
Contributor Author

What's odd is that in all the other transforms, Babel generates a temp var for unbound identifiers, because accessing a global x can have side effects if global.x is a getter.

e.g. Babel REPL

Not sure why that's not the case in the rest/spread transform. Strictly speaking, it's a bug.

Dunqing pushed a commit that referenced this pull request Nov 17, 2024
`MaybeBoundIdentifier` is similar to `BoundIdentifier`, but can be used where the identifier may or may not have have a `SymbolId` (may or may not be bound).

Typical usage:

```rs
// Create `MaybeBoundIdentifier` from an existing `IdentifierReference`
let binding = MaybeBoundIdentifier::from_identifier_reference(ident, ctx);

// Generate `IdentifierReference`s and insert them into AST
assign_expr.left = binding.create_write_target(ctx);
assign_expr.right = binding.create_read_expression(ctx);
```
Dunqing pushed a commit that referenced this pull request Nov 18, 2024
`MaybeBoundIdentifier` is similar to `BoundIdentifier`, but can be used where the identifier may or may not have have a `SymbolId` (may or may not be bound).

Typical usage:

```rs
// Create `MaybeBoundIdentifier` from an existing `IdentifierReference`
let binding = MaybeBoundIdentifier::from_identifier_reference(ident, ctx);

// Generate `IdentifierReference`s and insert them into AST
assign_expr.left = binding.create_write_target(ctx);
assign_expr.right = binding.create_read_expression(ctx);
```
Dunqing pushed a commit that referenced this pull request Nov 18, 2024
`MaybeBoundIdentifier` is similar to `BoundIdentifier`, but can be used where the identifier may or may not have have a `SymbolId` (may or may not be bound).

Typical usage:

```rs
// Create `MaybeBoundIdentifier` from an existing `IdentifierReference`
let binding = MaybeBoundIdentifier::from_identifier_reference(ident, ctx);

// Generate `IdentifierReference`s and insert them into AST
assign_expr.left = binding.create_write_target(ctx);
assign_expr.right = binding.create_read_expression(ctx);
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-merge Merge with Graphite Merge Queue C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants