Skip to content

Commit

Permalink
Fix: Store correct class binding for private static props
Browse files Browse the repository at this point in the history
  • Loading branch information
@overlookmotel
overlookmotel committed Nov 18, 2024
1 parent 17cfc27 commit 441ff9b
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 15 deletions.
38 changes: 28 additions & 10 deletions crates/oxc_transformer/src/es2022/class_properties.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,12 @@ enum ClassName<'a> {
}

struct PrivateProps<'a> {
/// Private properties for class. Indexed by property name.
props: FxIndexMap<Atom<'a>, PrivateProp<'a>>,
/// Binding for class name.
class_name_binding: BoundIdentifier<'a>,
}

struct PrivateProp<'a> {
binding: BoundIdentifier<'a>,
is_static: bool,
Expand Down Expand Up @@ -523,7 +527,17 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
if private_props.is_empty() {
self.private_props_stack.push(None);
} else {
self.private_props_stack.push(Some(PrivateProps { props: private_props }));
let class_name_binding = match &self.class_name {
ClassName::Binding(binding) => binding.clone(),
ClassName::Name(_) => {
// Dummy. If binding is not initialized yet, it's not needed because there are no
// static props. Using a dummy rather than storing `Option<BoundIdentifier>`,
// to avoid unnecessary `unwrap` later.
BoundIdentifier::new(Atom::from(""), SymbolId::DUMMY)
}
};
self.private_props_stack
.push(Some(PrivateProps { props: private_props, class_name_binding }));
}

if instance_prop_count == 0 && !has_static_prop_or_static_block {
Expand Down Expand Up @@ -633,11 +647,11 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
self.insert_private_static_init_assignment(ident, value, ctx);
} else {
// Convert to assignment or `_defineProperty` call, depending on `loose` option
let ClassName::Binding(class_binding) = &self.class_name else {
let ClassName::Binding(class_name_binding) = &self.class_name else {
// Binding is initialized in 1st pass in `transform_class` when a static prop is found
unreachable!();
};
let assignee = class_binding.create_read_expression(ctx);
let assignee = class_name_binding.create_read_expression(ctx);
let init_expr = self.create_init_assignment(prop, value, assignee, ctx);
self.insert_expr_after_class(init_expr, ctx);
}
Expand Down Expand Up @@ -998,9 +1012,9 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
field_expr: ArenaBox<'a, PrivateFieldExpression<'a>>,
ctx: &mut TraverseCtx<'a>,
) -> Expression<'a> {
let prop = self.lookup_private_property(&field_expr.field);
let prop_and_class_name_binding = self.lookup_private_property(&field_expr.field);
// TODO: Should never be `None` - only because implementation is incomplete.
let Some(prop) = prop else {
let Some((prop, class_name_binding)) = prop_and_class_name_binding else {
return Expression::PrivateFieldExpression(field_expr);
};
let prop_ident = prop.binding.create_read_expression(ctx);
Expand All @@ -1010,10 +1024,11 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {

let (helper, arguments) = if prop.is_static {
// `_assertClassBrand(Class, this, _prop)`
// TODO: Need to store `class_name` on stack, because could be from an enclosing class.
let ClassName::Binding(class_binding) = &self.class_name else { unreachable!() };
// TODO: Ensure there are tests for nested classes with references to private static props
// of outer class inside inner class, to make sure we're getting the right `class_name_binding`.
debug_assert!(class_name_binding.name != "");
let arguments = ctx.ast.vec_from_array([
Argument::from(class_binding.create_read_expression(ctx)),
Argument::from(class_name_binding.create_read_expression(ctx)),
Argument::from(field_expr.object),
Argument::from(prop_ident),
]);
Expand All @@ -1029,12 +1044,15 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
}

/// Lookup details of private property referred to by `ident`.
fn lookup_private_property(&self, ident: &PrivateIdentifier<'a>) -> Option<&PrivateProp<'a>> {
fn lookup_private_property(
&self,
ident: &PrivateIdentifier<'a>,
) -> Option<(&PrivateProp<'a>, &BoundIdentifier<'a>)> {
// Check for binding in closest class first, then enclosing classes
// TODO: Check there are tests for bindings in enclosing classes.
for private_props in self.private_props_stack.as_slice().iter().rev() {
if let Some(prop) = private_props.props.get(&ident.name) {
return Some(prop);
return Some((prop, &private_props.class_name_binding));
}
}
// TODO: This should be unreachable. Only returning `None` because implementation is incomplete.
Expand Down
8 changes: 4 additions & 4 deletions tasks/coverage/snapshots/semantic_test262.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1347,7 +1347,7 @@ Bindings mismatch:
after transform: ScopeId(1): ["C", "_Class", "_await$", "_await$2", "c"]
rebuilt : ScopeId(1): ["C", "_await$", "_await$2", "c"]
Symbol scope ID mismatch for "_Class":
after transform: SymbolId(5): ScopeId(1)
after transform: SymbolId(3): ScopeId(1)
rebuilt : SymbolId(1): ScopeId(0)

tasks/coverage/test262/test/language/expressions/class/cpn-class-expr-fields-computed-property-name-from-function-expression.js
Expand Down Expand Up @@ -1597,7 +1597,7 @@ Scope parent mismatch:
after transform: ScopeId(4): Some(ScopeId(2))
rebuilt : ScopeId(5): Some(ScopeId(1))
Symbol scope ID mismatch for "_Class":
after transform: SymbolId(5): ScopeId(1)
after transform: SymbolId(3): ScopeId(1)
rebuilt : SymbolId(1): ScopeId(0)

tasks/coverage/test262/test/language/expressions/class/cpn-class-expr-fields-methods-computed-property-name-from-condition-expression-false.js
Expand Down Expand Up @@ -5583,7 +5583,7 @@ Bindings mismatch:
after transform: ScopeId(1): ["C", "_Class", "_await$", "_await$2", "c"]
rebuilt : ScopeId(1): ["C", "_await$", "_await$2", "c"]
Symbol scope ID mismatch for "_Class":
after transform: SymbolId(5): ScopeId(1)
after transform: SymbolId(3): ScopeId(1)
rebuilt : SymbolId(1): ScopeId(0)

tasks/coverage/test262/test/language/statements/class/cpn-class-decl-fields-computed-property-name-from-function-expression.js
Expand Down Expand Up @@ -5833,7 +5833,7 @@ Scope parent mismatch:
after transform: ScopeId(4): Some(ScopeId(2))
rebuilt : ScopeId(5): Some(ScopeId(1))
Symbol scope ID mismatch for "_Class":
after transform: SymbolId(5): ScopeId(1)
after transform: SymbolId(3): ScopeId(1)
rebuilt : SymbolId(1): ScopeId(0)

tasks/coverage/test262/test/language/statements/class/cpn-class-decl-fields-methods-computed-property-name-from-condition-expression-false.js
Expand Down
2 changes: 1 addition & 1 deletion tasks/coverage/snapshots/semantic_typescript.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45457,7 +45457,7 @@ Scope parent mismatch:
after transform: ScopeId(4): Some(ScopeId(3))
rebuilt : ScopeId(5): Some(ScopeId(4))
Symbol scope ID mismatch for "_C":
after transform: SymbolId(5): ScopeId(2)
after transform: SymbolId(3): ScopeId(2)
rebuilt : SymbolId(3): ScopeId(0)

tasks/coverage/typescript/tests/cases/conformance/es6/computedProperties/computedPropertyNames7_ES5.ts
Expand Down

0 comments on commit 441ff9b

Please sign in to comment.