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(transformer/arrow-functions): _this = this should be inserted after super call expression #8024

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 60 additions & 9 deletions crates/oxc_transformer/src/common/arrow_function_converter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,9 @@

use compact_str::CompactString;
use indexmap::IndexMap;
use rustc_hash::{FxBuildHasher, FxHashSet};
use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet};

use oxc_allocator::{Box as ArenaBox, Vec as ArenaVec};
use oxc_allocator::{Address, Box as ArenaBox, GetAddress, Vec as ArenaVec};
use oxc_ast::{ast::*, NONE};
use oxc_data_structures::stack::{NonEmptyStack, SparseStack};
use oxc_semantic::{ReferenceFlags, SymbolId};
Expand All @@ -102,7 +102,7 @@ use oxc_syntax::{
};
use oxc_traverse::{Ancestor, BoundIdentifier, Traverse, TraverseCtx};

use crate::EnvOptions;
use crate::{context::TransformCtx, EnvOptions};

type FxIndexMap<K, V> = IndexMap<K, V, FxBuildHasher>;

Expand Down Expand Up @@ -135,9 +135,12 @@ struct SuperMethodInfo<'a> {
is_computed: bool,
}

pub struct ArrowFunctionConverter<'a> {
pub struct ArrowFunctionConverter<'a, 'ctx> {
ctx: &'ctx TransformCtx<'a>,
mode: ArrowFunctionConverterMode,
this_var_stack: SparseStack<BoundIdentifier<'a>>,
/// Stores the address of statement of containing `super()` expression
super_call_addresses: FxHashMap<ScopeId, Address>,
arguments_var_stack: SparseStack<BoundIdentifier<'a>>,
arguments_needs_transform_stack: NonEmptyStack<bool>,
renamed_arguments_symbol_ids: FxHashSet<SymbolId>,
Expand All @@ -146,8 +149,8 @@ pub struct ArrowFunctionConverter<'a> {
super_methods: Option<FxIndexMap<SuperMethodKey<'a>, SuperMethodInfo<'a>>>,
}

impl<'a> ArrowFunctionConverter<'a> {
pub fn new(env: &EnvOptions) -> Self {
impl<'a, 'ctx> ArrowFunctionConverter<'a, 'ctx> {
pub fn new(env: &EnvOptions, ctx: &'ctx TransformCtx<'a>) -> Self {
let mode = if env.es2015.arrow_function.is_some() {
ArrowFunctionConverterMode::Enabled
} else if env.es2017.async_to_generator || env.es2018.async_generator_functions {
Expand All @@ -157,8 +160,10 @@ impl<'a> ArrowFunctionConverter<'a> {
};
// `SparseStack`s are created with 1 empty entry, for `Program`
Self {
ctx,
mode,
this_var_stack: SparseStack::new(),
super_call_addresses: FxHashMap::default(),
arguments_var_stack: SparseStack::new(),
arguments_needs_transform_stack: NonEmptyStack::new(false),
renamed_arguments_symbol_ids: FxHashSet::default(),
Expand All @@ -167,7 +172,7 @@ impl<'a> ArrowFunctionConverter<'a> {
}
}

impl<'a> Traverse<'a> for ArrowFunctionConverter<'a> {
impl<'a, 'ctx> Traverse<'a> for ArrowFunctionConverter<'a, 'ctx> {
// Note: No visitors for `TSModuleBlock` because `this` is not legal in TS module blocks.
// <https://www.typescriptlang.org/play/?#code/HYQwtgpgzgDiDGEAEAxA9mpBvAsAKCSXjWCgBckANJAXiQAoBKWgPiTIAsBLKAbnwC++fGDQATAK4AbZACEQAJ2z5CxUhWp0mrdtz6D8QA>

Expand Down Expand Up @@ -397,7 +402,7 @@ impl<'a> Traverse<'a> for ArrowFunctionConverter<'a> {
}
}

impl<'a> ArrowFunctionConverter<'a> {
impl<'a, 'ctx> ArrowFunctionConverter<'a, 'ctx> {
/// Check if arrow function conversion is disabled
fn is_disabled(&self) -> bool {
self.mode == ArrowFunctionConverterMode::Disabled
Expand Down Expand Up @@ -436,6 +441,26 @@ impl<'a> ArrowFunctionConverter<'a> {
.unwrap();
ctx.generate_uid("this", target_scope_id, SymbolFlags::FunctionScopedVariable)
});

if !self.super_call_addresses.is_empty() {
let address = ctx
.scopes()
.get_parent_id(arrow_scope_id)
.and_then(|scope_id| self.super_call_addresses.remove(&scope_id));
if let Some(address) = address {
// Insert a dummy address to indicate that should inserting `var _this;`
// without `init` at the top of the statements.
self.super_call_addresses.insert(arrow_scope_id, Address::DUMMY);
let assignment = ctx.ast.expression_assignment(
SPAN,
AssignmentOperator::Assign,
this_var.create_write_target(ctx),
ctx.ast.expression_this(SPAN),
);
let statement = ctx.ast.statement_expression(SPAN, assignment);
self.ctx.statement_injector.insert_after(&address, statement);
}
}
Some(ctx.ast.alloc(this_var.create_spanned_read_reference(span, ctx)))
}

Expand Down Expand Up @@ -703,6 +728,25 @@ impl<'a> ArrowFunctionConverter<'a> {
ctx: &mut TraverseCtx<'a>,
) -> Option<Expression<'a>> {
if self.super_methods.is_none() || !call.callee.is_member_expression() {
// `super()`
// Store the address in case we need to insert `var _this;` after it.
if call.callee.is_super() {
let scope_id = ctx.current_scope_id();
self.super_call_addresses.entry(scope_id).or_insert_with(|| {
ctx.ancestors()
.find(|ancestor| {
matches!(
ancestor,
// const A = super():
Ancestor::VariableDeclarationDeclarations(_)
// super();
| Ancestor::ExpressionStatementExpression(_)
)
})
.unwrap()
.address()
Comment on lines +736 to +747
Copy link
Member Author

Choose a reason for hiding this comment

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

Unwrap failed here, we need to find all possible statements, and this may not be a good implementation

Copy link
Contributor

@overlookmotel overlookmotel Dec 19, 2024

Choose a reason for hiding this comment

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

You can use Ancestor::is_via_statement for this.

Note it doesn't tell you if that ancestor is a statement, but whether its child is a statement.

So if you are visiting a Function, it could be either an expression or a statement. ctx.parent().is_via_statement() will tell you.

To find the closest statement, loop through ancestors, and when ancestor.is_via_statement() returns true, then the previous ancestor you passed is the statement.

Put another way, you have to walk 1 step further up the tree than the node you're looking for.

The naming is_via_* is weird, and the fact that you have to look at the ancestor above is confusing as hell. So it's a terrible API! But it does do what you need.

Copy link
Contributor

Choose a reason for hiding this comment

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

#8031 renames this method to Ancestor::is_parent_of_statement. I think that's clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just found that we have these methods!

});
}
return None;
}

Expand Down Expand Up @@ -1070,12 +1114,19 @@ impl<'a> ArrowFunctionConverter<'a> {

// `_this = this;`
if let Some(this_var) = this_var {
let init = if self.super_call_addresses.is_empty() {
Some(ctx.ast.expression_this(SPAN))
} else {
// Clear the dummy address.
self.super_call_addresses.clear();
None
};
Self::adjust_binding_scope(target_scope_id, &this_var, ctx);
let variable_declarator = ctx.ast.variable_declarator(
SPAN,
VariableDeclarationKind::Var,
this_var.create_binding_pattern(ctx),
Some(ctx.ast.expression_this(SPAN)),
init,
false,
);
declarations.push(variable_declarator);
Expand Down
4 changes: 2 additions & 2 deletions crates/oxc_transformer/src/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub struct Common<'a, 'ctx> {
var_declarations: VarDeclarations<'a, 'ctx>,
statement_injector: StatementInjector<'a, 'ctx>,
top_level_statements: TopLevelStatements<'a, 'ctx>,
arrow_function_converter: ArrowFunctionConverter<'a>,
arrow_function_converter: ArrowFunctionConverter<'a, 'ctx>,
}

impl<'a, 'ctx> Common<'a, 'ctx> {
Expand All @@ -35,7 +35,7 @@ impl<'a, 'ctx> Common<'a, 'ctx> {
var_declarations: VarDeclarations::new(ctx),
statement_injector: StatementInjector::new(ctx),
top_level_statements: TopLevelStatements::new(ctx),
arrow_function_converter: ArrowFunctionConverter::new(options),
arrow_function_converter: ArrowFunctionConverter::new(options, ctx),
}
}
}
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

4 changes: 2 additions & 2 deletions tasks/transform_conformance/snapshots/oxc.snap.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
commit: 54a8389f

Passed: 114/130
Passed: 116/132

# All Passed:
* babel-plugin-transform-class-static-block
Expand Down Expand Up @@ -34,7 +34,7 @@ after transform: SymbolId(0): [ReferenceId(0), ReferenceId(2), ReferenceId(6), R
rebuilt : SymbolId(0): [ReferenceId(0), ReferenceId(2), ReferenceId(6), ReferenceId(10)]


# babel-plugin-transform-async-to-generator (14/15)
# babel-plugin-transform-async-to-generator (16/17)
* super/nested/input.js
x Output mismatch

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
class Outer {
constructor() {
async () => { return [this, 2]; };

class Inner extends Outer{
constructor() {
if (condition) {
const _super = super()
this.fn = async () => { return [this, 1]; };
}

super()
async () => { return [this, 2]; };
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
class Outer {
constructor() {
var _this = this;
babelHelpers.asyncToGenerator(function* () {
return [_this, 2];
});

class Inner extends Outer {
constructor() {
var _this2;

if (condition) {
const _super = super();
_this2 = this;
this.fn = babelHelpers.asyncToGenerator(function* () {
return [_this2, 1];
});
}

super();
_this2 = this;
babelHelpers.asyncToGenerator(function* () {
return [_this2, 2];
});
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class C extends S {
constructor() {
if (condition) {
const _super = super()
this.fn = async () => { return [this, 1]; };
}

super()
async () => { return [this, 2]; };
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
class C extends S {
constructor() {
var _this;
if (condition) {
const _super = super();
_this = this;
this.fn = babelHelpers.asyncToGenerator(function* () {
return [_this, 1];
});
}

super();
_this = this;
babelHelpers.asyncToGenerator(function* () {
return [_this, 2];
});
}
}
Loading