Skip to content

Commit

Permalink
refactor(transformer/class-properties): use stack of ClassDetails (#…
Browse files Browse the repository at this point in the history
…7947)

Replace stack of `PrivateProps` with a stack of `ClassDetails`.

Primary purpose is to prepare for further changes to come, but this also allows removing the messy hack of storing `ClassBindings` in 2 places, and having to decide which is the source of truth at any given moment. Now there is only 1 copy of `ClassBindings`.
  • Loading branch information
overlookmotel committed Dec 17, 2024
1 parent fc53804 commit 98340bb
Show file tree
Hide file tree
Showing 9 changed files with 217 additions and 238 deletions.
104 changes: 40 additions & 64 deletions crates/oxc_transformer/src/es2022/class_properties/class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,12 @@ use oxc_syntax::{
};
use oxc_traverse::{BoundIdentifier, TraverseCtx};

use crate::common::helper_loader::Helper;
use crate::{common::helper_loader::Helper, TransformCtx};

use super::{
constructor::InstanceInitsInsertLocation,
private_props::{PrivateProp, PrivateProps},
utils::{create_assignment, create_variable_declaration, exprs_into_stmts},
ClassBindings, ClassProperties, FxIndexMap,
ClassBindings, ClassDetails, ClassProperties, FxIndexMap, PrivateProp,
};

impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
Expand Down Expand Up @@ -63,10 +62,8 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {

// Return number of expressions to be inserted before/after the class
let mut expr_count = self.insert_before.len() + self.insert_after_exprs.len();

let private_props = self.private_props_stack.last();
if let Some(private_props) = private_props {
expr_count += private_props.props.len();
if let Some(private_props) = &self.current_class().private_props {
expr_count += private_props.len();
}

if expr_count > 0 {
Expand Down Expand Up @@ -99,32 +96,32 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
// TODO: Deduct static private props from `expr_count`.
// Or maybe should store count and increment it when create private static props?
// They're probably pretty rare, so it'll be rarely used.
expr_count += 1 + usize::from(self.class_bindings.temp.is_some());
let class_details = self.classes_stack.last();
expr_count += 1 + usize::from(class_details.bindings.temp.is_some());

let mut exprs = ctx.ast.vec_with_capacity(expr_count);

// Insert `_prop = new WeakMap()` expressions for private instance props
// (or `_prop = _classPrivateFieldLooseKey("prop")` if loose mode).
// Babel has these always go first, regardless of order of class elements.
// Also insert `var _prop;` temp var declarations for private static props.
let private_props = self.private_props_stack.last();
if let Some(private_props) = private_props {
if let Some(private_props) = &class_details.private_props {
// Insert `var _prop;` declarations here rather than when binding was created to maintain
// same order of `var` declarations as Babel.
// `c = class C { #x = 1; static y = 2; }` -> `var _C, _x;`
// TODO(improve-on-babel): Simplify this.
if self.private_fields_as_properties {
exprs.extend(private_props.props.iter().map(|(name, prop)| {
exprs.extend(private_props.iter().map(|(name, prop)| {
// Insert `var _prop;` declaration
self.ctx.var_declarations.insert_var(&prop.binding, ctx);

// `_prop = _classPrivateFieldLooseKey("prop")`
let value = self.create_private_prop_key_loose(name, ctx);
let value = Self::create_private_prop_key_loose(name, self.ctx, ctx);
create_assignment(&prop.binding, value, ctx)
}));
} else {
let mut weakmap_symbol_id = None;
exprs.extend(private_props.props.values().filter_map(|prop| {
exprs.extend(private_props.values().filter_map(|prop| {
// Insert `var _prop;` declaration
self.ctx.var_declarations.insert_var(&prop.binding, ctx);

Expand All @@ -144,7 +141,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {

// Insert class + static property assignments + static blocks
let class_expr = ctx.ast.move_expression(expr);
if let Some(binding) = &self.class_bindings.temp {
if let Some(binding) = &class_details.bindings.temp {
// Insert `var _Class` statement, if it wasn't already in `transform_class`
if !self.temp_var_is_created {
self.ctx.var_declarations.insert_var(binding, ctx);
Expand Down Expand Up @@ -193,7 +190,8 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {

// TODO: Run other transforms on inserted statements. How?

if let Some(temp_binding) = &self.class_bindings.temp {
let class_details = self.classes_stack.last_mut();
if let Some(temp_binding) = &class_details.bindings.temp {
// Binding for class name is required
if let Some(ident) = &class.id {
// Insert `var _Class` statement, if it wasn't already in `transform_class`
Expand Down Expand Up @@ -225,13 +223,13 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
);
}

if let Some(private_props) = self.private_props_stack.last() {
if let Some(private_props) = &class_details.private_props {
if self.private_fields_as_properties {
self.ctx.statement_injector.insert_many_before(
&stmt_address,
private_props.props.iter().map(|(name, prop)| {
private_props.iter().map(|(name, prop)| {
// `var _prop = _classPrivateFieldLooseKey("prop");`
let value = self.create_private_prop_key_loose(name, ctx);
let value = Self::create_private_prop_key_loose(name, self.ctx, ctx);
create_variable_declaration(&prop.binding, value, ctx)
}),
);
Expand All @@ -240,7 +238,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
let mut weakmap_symbol_id = None;
self.ctx.statement_injector.insert_many_before(
&stmt_address,
private_props.props.values().filter_map(|prop| {
private_props.values().filter_map(|prop| {
if prop.is_static {
return None;
}
Expand All @@ -258,15 +256,27 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
.statement_injector
.insert_many_after(&stmt_address, self.insert_after_stmts.drain(..));
}

// Update class bindings prior to traversing class body and insertion of statements/expressions
// before/after the class. See comments on `ClassBindings`.
// Static private fields reference class name (not temp var) in class declarations.
// `class Class { static #prop; method() { return obj.#prop; } }`
// -> `method() { return _assertClassBrand(Class, obj, _prop)._; }`
// (note `Class` in `_assertClassBrand(Class, ...)`, not `_Class`)
// So set "temp" binding to actual class name while visiting class body.
// Note: If declaration is `export default class {}` with no name, and class has static props,
// then class has had name binding created already in `transform_class`.
// So name binding is always `Some`.
class_details.bindings.temp = class_details.bindings.name.clone();
}

/// `_classPrivateFieldLooseKey("prop")`
fn create_private_prop_key_loose(
&self,
name: &Atom<'a>,
transform_ctx: &TransformCtx<'a>,
ctx: &mut TraverseCtx<'a>,
) -> Expression<'a> {
self.ctx.helper_call_expr(
transform_ctx.helper_call_expr(
Helper::ClassPrivateFieldLooseKey,
SPAN,
ctx.ast.vec1(Argument::from(ctx.ast.expression_string_literal(
Expand Down Expand Up @@ -349,7 +359,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {

// Exit if nothing to transform
if instance_prop_count == 0 && !has_static_prop && !has_static_block {
self.private_props_stack.push(None);
self.classes_stack.push(ClassDetails::default());
return;
}

Expand Down Expand Up @@ -379,25 +389,14 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
None
};

self.class_bindings = ClassBindings::new(class_name_binding, class_temp_binding);
let class_bindings = ClassBindings::new(class_name_binding, class_temp_binding);

// Add entry to `private_props_stack`
if private_props.is_empty() {
self.private_props_stack.push(None);
} else {
// `class_bindings.temp` in the `PrivateProps` entry is the temp var (if one has been created).
// Private fields in static prop initializers use the temp var in the transpiled output
// e.g. `_assertClassBrand(_Class, obj, _prop)`.
// At end of this function, if it's a class declaration, we set `class_bindings.temp` to be
// the binding for the class name, for when the class body is visited, because in the class
// body, private fields use the class name
// e.g. `_assertClassBrand(Class, obj, _prop)` (note `Class` not `_Class`).
self.private_props_stack.push(Some(PrivateProps {
props: private_props,
class_bindings: self.class_bindings.clone(),
is_declaration: self.is_declaration,
}));
}
// Add entry to `classes_stack`
self.classes_stack.push(ClassDetails {
is_declaration: self.is_declaration,
private_props: if private_props.is_empty() { None } else { Some(private_props) },
bindings: class_bindings,
});

// Determine where to insert instance property initializers in constructor
let instance_inits_insert_location = if instance_prop_count == 0 {
Expand Down Expand Up @@ -488,29 +487,6 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
ctx,
);
}

// Update class bindings prior to traversing class body and insertion of statements/expressions
// before/after the class. See comments on `ClassBindings`.
if let Some(private_props) = self.private_props_stack.last_mut() {
// Transfer state of `temp` binding from `private_props_stack` to `self`.
// A temp binding may have been created while transpiling private fields in
// static prop initializers.
// TODO: Do this where `class_bindings.temp` is consumed instead?
if let Some(temp_binding) = &private_props.class_bindings.temp {
self.class_bindings.temp = Some(temp_binding.clone());
}

// Static private fields reference class name (not temp var) in class declarations.
// `class Class { static #prop; method() { return obj.#prop; } }`
// -> `method() { return _assertClassBrand(Class, obj, _prop)._; }`
// (note `Class` in `_assertClassBrand(Class, ...)`, not `_Class`)
// So set "temp" binding to actual class name while visiting class body.
// Note: If declaration is `export default class {}` with no name, and class has static props,
// then class has had name binding created already above. So name binding is always `Some`.
if self.is_declaration {
private_props.class_bindings.temp = private_props.class_bindings.name.clone();
}
}
}

/// Pop from private props stack.
Expand All @@ -523,7 +499,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
return;
}

self.private_props_stack.pop();
self.classes_stack.pop();
}

/// Insert an expression after the class.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use oxc_traverse::{BoundIdentifier, TraverseCtx};
/// 2. Temp var `_Class`, which may or may not be required.
///
/// Temp var is required in the following circumstances:
///
/// * Class expression has static properties.
/// e.g. `C = class { x = 1; }`
/// * Class declaration has static properties and one of the static prop's initializers contains:
Expand All @@ -17,11 +18,6 @@ use oxc_traverse::{BoundIdentifier, TraverseCtx};
/// c. A private field referring to one of the class's static private props.
/// e.g. `class C { static #x; static y = obj.#x; }`
///
/// An instance of `ClassBindings` is stored in main `ClassProperties` transform, and a 2nd is stored
/// in `PrivateProps` for the class, if the class has any private properties.
/// If the class has private props, the instance of `ClassBindings` in `PrivateProps` is the source
/// of truth.
///
/// The logic for when transpiled private fields use a reference to class name or class temp var
/// is unfortunately rather complicated.
///
Expand All @@ -34,12 +30,12 @@ use oxc_traverse::{BoundIdentifier, TraverseCtx};
/// e.g. `class C { static #x; y = obj.#x; }`
///
/// To cover all these cases, the meaning of `temp` binding here changes while traversing the class body.
/// [`ClassProperties::transform_class`] sets `temp` binding to be a copy of the `name` binding before
/// that traversal begins. So the name `temp` is misleading at that point.
/// [`ClassProperties::transform_class_declaration`] sets `temp` binding to be a copy of the
/// `name` binding before that traversal begins. So the name `temp` is misleading at that point.
///
/// Debug assertions are used to make sure this complex logic is correct.
///
/// [`ClassProperties::transform_class`]: super::ClassProperties::transform_class
/// [`ClassProperties::transform_class_declaration`]: super::ClassProperties::transform_class_declaration
#[derive(Default, Clone)]
pub(super) struct ClassBindings<'a> {
/// Binding for class name, if class has name
Expand Down
120 changes: 120 additions & 0 deletions crates/oxc_transformer/src/es2022/class_properties/class_details.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
use oxc_ast::ast::*;
use oxc_data_structures::stack::NonEmptyStack;
use oxc_span::Atom;
use oxc_traverse::BoundIdentifier;

use super::{ClassBindings, ClassProperties, FxIndexMap};

/// Details of a class.
///
/// These are stored in `ClassesStack`.
#[derive(Default)]
pub(super) struct ClassDetails<'a> {
/// `true` for class declaration, `false` for class expression
pub is_declaration: bool,
/// Private properties.
/// Mapping private prop name to binding for temp var.
/// This is then used as lookup when transforming e.g. `this.#x`.
/// `None` if class has no private properties.
pub private_props: Option<FxIndexMap<Atom<'a>, PrivateProp<'a>>>,
/// Bindings for class name and temp var for class
pub bindings: ClassBindings<'a>,
}

/// Details of a private property.
pub(super) struct PrivateProp<'a> {
pub binding: BoundIdentifier<'a>,
pub is_static: bool,
}

/// Stack of `ClassDetails`.
/// Pushed to when entering a class, popped when exiting.
///
/// We use a `NonEmptyStack` to make `last` and `last_mut` cheap (these are used a lot).
/// The first entry is a dummy.
///
/// This is a separate structure, rather than just storing stack as a property of `ClassProperties`
/// to work around borrow-checker. You can call `find_private_prop` and retain the return value
/// without holding a mut borrow of the whole of `&mut ClassProperties`. This allows accessing other
/// properties of `ClassProperties` while that borrow is held.
#[derive(Default)]
pub(super) struct ClassesStack<'a> {
stack: NonEmptyStack<ClassDetails<'a>>,
}

impl<'a> ClassesStack<'a> {
/// Push an entry to stack.
#[inline]
pub fn push(&mut self, class: ClassDetails<'a>) {
self.stack.push(class);
}

/// Push last entry from stack.
#[inline]
pub fn pop(&mut self) -> ClassDetails<'a> {
self.stack.pop()
}

/// Get details of current class.
#[inline]
pub fn last(&self) -> &ClassDetails<'a> {
self.stack.last()
}

/// Get details of current class as `&mut` reference.
#[inline]
pub fn last_mut(&mut self) -> &mut ClassDetails<'a> {
self.stack.last_mut()
}

/// Lookup details of private property referred to by `ident`.
pub fn find_private_prop<'b>(
&'b mut self,
ident: &PrivateIdentifier<'a>,
) -> Option<ResolvedPrivateProp<'a, 'b>> {
// Check for binding in closest class first, then enclosing classes.
// We skip the first, because this is a `NonEmptyStack` with dummy first entry.
// TODO: Check there are tests for bindings in enclosing classes.
for class in self.stack[1..].iter_mut().rev() {
if let Some(private_props) = &mut class.private_props {
if let Some(prop) = private_props.get(&ident.name) {
return Some(ResolvedPrivateProp {
prop_binding: &prop.binding,
class_bindings: &mut class.bindings,
is_static: prop.is_static,
is_declaration: class.is_declaration,
});
}
}
}
// TODO: This should be unreachable. Only returning `None` because implementation is incomplete.
None
}
}

/// Details of a private property resolved for a private field.
///
/// This is the return value of [`ClassesStack::find_private_prop`].
pub(super) struct ResolvedPrivateProp<'a, 'b> {
/// Binding for temp var representing the property
pub prop_binding: &'b BoundIdentifier<'a>,
/// Bindings for class name and temp var for class
pub class_bindings: &'b mut ClassBindings<'a>,
/// `true` if is a static property
pub is_static: bool,
/// `true` if class which defines this property is a class declaration
pub is_declaration: bool,
}

// Shortcut methods to get current class
impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
/// Get details of current class.
pub(super) fn current_class(&self) -> &ClassDetails<'a> {
self.classes_stack.last()
}

/// Get details of current class as `&mut` reference.
pub(super) fn current_class_mut(&mut self) -> &mut ClassDetails<'a> {
self.classes_stack.last_mut()
}
}
Loading

0 comments on commit 98340bb

Please sign in to comment.