-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(runtime): support class expressions in dyn_eval new Function interpreter (#10661) #10675
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
Changes from all commits
c85ce67
1538c92
ed63068
ccdd1f0
255e1c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| Fixed `new Function`-string interpreter (#6559) to support **class expressions** in a | ||
| deliberately narrow subset: a constructor plus regular instance/`static` methods | ||
| (identifier/string/numeric keys), no `extends`/decorators/getters-setters/fields/private | ||
| members/computed keys/static blocks. This was the sole runtime blocker for `mysql2`, whose | ||
| `generate-function`-built row parsers (`text_parser.js`/`binary_parser.js`) return a class | ||
| expression from a `Function.apply(...).apply(...)` call; `generate-function` is used well beyond | ||
| mysql2, so this likely unblocks other packages too. Desugars onto machinery the interpreter | ||
| already had (closure expando writes + the generic `new <closure>` path that already reads a | ||
| `"prototype"` dynamic prop), so no new runtime mechanism was added. mysql2 now runs an | ||
| end-to-end `CREATE`/`INSERT`/`SELECT`/`DROP` round trip against a real server; the hand-written | ||
| native mysql2 binding now looks deletable as a follow-up. See #10661. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1089,3 +1089,200 @@ fn exec_try_catch(ctx: &Ctx, t: &ast::TryStmt, env_idx: usize) -> Flow { | |
| fn protected_block(ctx: &Ctx, t: &ast::TryStmt, env_idx: usize) -> Flow { | ||
| exec_block_scope(ctx, &t.block, env_idx) | ||
| } | ||
|
|
||
| // ── class expressions (#10661) ────────────────────────────────────────────── | ||
|
|
||
| /// `class [Name] { constructor(...) { ... } method(...) { ... } ... }` as a | ||
| /// standalone expression — the shape `generate-function` emits (mysql2's row | ||
| /// parsers: `return class TextRow { constructor(fields) {...} next(...) {...} }`). | ||
| /// | ||
| /// **Supported subset, deliberately narrow** (matches what the schema/codegen | ||
| /// corpus behind #6559 actually emits, not general ES2022 class syntax): | ||
| /// * an optional `constructor`; missing one synthesizes an empty no-op | ||
| /// constructor (there is no `extends`, so there is nothing to forward to | ||
| /// a super constructor); | ||
| /// * regular (non-getter/setter, non-generator/async) methods, instance or | ||
| /// `static`, keyed by identifier / string / numeric literal; | ||
| /// * a named class expression sees its own name inside its body, exactly | ||
| /// like a named function expression. | ||
| /// | ||
| /// **Explicitly unsupported** (throws the #6559 diagnostic naming the | ||
| /// construct, same as every other out-of-subset form in this interpreter): | ||
| /// `extends` (no superclass chain — no `super()`/`super.foo` machinery | ||
| /// exists here), decorators, getters/setters, generator/async methods, | ||
| /// class fields (public or private), private methods, static blocks, | ||
| /// auto-accessors, TS index signatures, TS parameter properties, and | ||
| /// computed member keys. | ||
| /// | ||
| /// **Why this is sugar, not a new mechanism.** The interpreter already | ||
| /// supports the ES5 pattern this desugars to — `function Foo(){}` plus | ||
| /// `Foo.prototype.bar = function(){}` plus `new Foo()` — because ordinary | ||
| /// property writes on an interpreted closure already land in its dynamic | ||
| /// expando table (ajv's `validate.errors = ...` already exercises that path), | ||
| /// and `new` on ANY closure (host or interpreted) already goes through | ||
| /// `js_new_function_construct`'s generic path, which specifically looks for a | ||
| /// `"prototype"` dynamic prop to link the new instance's `[[Prototype]]` | ||
| /// (`crates/perry-runtime/src/object/class_registry/construct.rs`). So this | ||
| /// function does nothing runtime-side that wasn't already reachable from | ||
| /// interpreted code — it just builds a constructor closure, a plain prototype | ||
| /// object, and wires them together the same way hand-written ES5 would. | ||
| /// Nothing new is added to `js_new_function_construct`, method dispatch, or | ||
| /// `instanceof` — an instance built this way is an ordinary object whose | ||
| /// `[[Prototype]]` happens to be the class's prototype object, found by the | ||
| /// same prototype-chain walk any plain object uses. | ||
| pub(crate) fn eval_class_expr(ctx: &Ctx, class_expr: &ast::ClassExpr, env_idx: usize) -> f64 { | ||
| let class = class_expr.class.as_ref(); | ||
| if class.super_class.is_some() { | ||
| throw_unsupported("class expression with `extends`"); | ||
| } | ||
| if !class.decorators.is_empty() { | ||
| throw_unsupported("class decorator"); | ||
| } | ||
|
|
||
| let base = roots_len(); | ||
|
|
||
| // Named class expressions see their own name inside constructor AND | ||
| // method bodies — same pattern `make_function_value` uses for named | ||
| // function expressions: chain a one-binding scope, alloc the closure | ||
| // over it, then backfill the binding once the closure value exists. | ||
| let name = class_expr.ident.as_ref().map(|i| i.sym.to_string()); | ||
| let body_env_idx = if name.is_some() { | ||
| let name_env = env::env_new(root_get(env_idx)); | ||
| root_push(name_env) | ||
| } else { | ||
| env_idx | ||
| }; | ||
|
|
||
| let ctor_member = class.body.iter().find_map(|m| match m { | ||
| ast::ClassMember::Constructor(c) => Some(c), | ||
| _ => None, | ||
| }); | ||
| let ctor_fn_id = match ctor_member { | ||
| Some(c) => { | ||
| let mut params = Vec::with_capacity(c.params.len()); | ||
| for p in &c.params { | ||
| match p { | ||
| ast::ParamOrTsParamProp::Param(p) => params.push(p.pat.clone()), | ||
| ast::ParamOrTsParamProp::TsParamProp(_) => { | ||
| throw_unsupported("TypeScript parameter property in class constructor") | ||
| } | ||
| } | ||
| } | ||
| let body = | ||
| InterpBody::Block(c.body.as_ref().map(|b| b.stmts.clone()).unwrap_or_default()); | ||
| fn_id_for_node(c as *const ast::Constructor as usize, || { | ||
| build_interp_fn(params, body, ctx.strict) | ||
| }) | ||
| } | ||
| None => { | ||
| // No constructor written: synthesize an empty one. Keyed on the | ||
| // `Class` node itself (there is no dedicated AST node for a | ||
| // synthesized constructor) — only used as a cache key, stable | ||
| // for the same reason every other node-address key here is: | ||
| // `FN_REGISTRY` keeps the owning `InterpFn` (and therefore this | ||
| // address) alive for the program's lifetime. | ||
| fn_id_for_node(class as *const ast::Class as usize, || { | ||
| build_interp_fn(Vec::new(), InterpBody::Block(Vec::new()), ctx.strict) | ||
| }) | ||
| } | ||
| }; | ||
|
|
||
| let ctor_closure = alloc_interp_closure( | ||
| ctor_fn_id, | ||
| root_get(body_env_idx), | ||
| None, | ||
| root_get(ctx.global_idx), | ||
| root_get(ctx.intrinsics_idx), | ||
| ctx.strings_allowed, | ||
| ctx.wasm_allowed, | ||
| ); | ||
| let ctor_idx = root_push(ctor_closure); | ||
|
|
||
| if let Some(name) = &name { | ||
| env::define(root_get(body_env_idx), name, root_get(ctor_idx)); | ||
| } | ||
|
|
||
| // Plain object, `Object.prototype`-rooted — same as any object literal. | ||
| let prototype = bridge::attach_intrinsic_prototype( | ||
| bridge::object_new(), | ||
| root_get(ctx.intrinsics_idx), | ||
| "Object", | ||
| ); | ||
| let proto_idx = root_push(prototype); | ||
| bridge::set_member(root_get(proto_idx), "constructor", root_get(ctor_idx)); | ||
|
|
||
| for member in &class.body { | ||
| match member { | ||
| ast::ClassMember::Constructor(_) => {} | ||
| ast::ClassMember::Method(m) => { | ||
| if m.kind != ast::MethodKind::Method { | ||
| throw_unsupported("getter/setter in class body"); | ||
| } | ||
| if m.function.is_generator || m.function.is_async { | ||
| throw_unsupported("generator/async method in class body"); | ||
| } | ||
| let value = make_function_value( | ||
| ctx, | ||
| m.function.params.iter().map(|p| p.pat.clone()).collect(), | ||
| InterpBody::Block( | ||
| m.function | ||
| .body | ||
| .as_ref() | ||
| .map(|b| b.stmts.clone()) | ||
| .unwrap_or_default(), | ||
| ), | ||
| false, | ||
| None, | ||
| m.function.as_ref() as *const ast::Function as usize, | ||
| body_env_idx, | ||
|
Comment on lines
+1173
to
+1237
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '320,370p' crates/perry-runtime/src/dyn_eval/interp.rs
sed -n '1080,1300p' crates/perry-runtime/src/dyn_eval/interp.rs
rg -n 'build_interp_fn|invoke_interp_fn|strict' crates/perry-runtime/src/dyn_evalRepository: PerryTS/perry Length of output: 16096 🏁 Script executed: sed -n '60,110p' crates/perry-runtime/src/dyn_eval/interp.rs
sed -n '285,410p' crates/perry-runtime/src/dyn_eval/interp.rs
sed -n '420,475p' crates/perry-runtime/src/dyn_eval/interp.rs
rg -n -C 5 'invoke_interp_fn|make_function_value|is_arrow|call.*interp|interp.*call' crates/perry-runtime/src/dyn_eval crates/perry-runtime/src | head -240Repository: PerryTS/perry Length of output: 28296 🏁 Script executed: sed -n '390,465p' crates/perry-runtime/src/dyn_eval/env.rs
rg -n -C 8 'js_implicit_this_get|implicit_this|NO_LEXICAL_THIS|call.*closure|closure.*call' crates/perry-runtime/src/dyn_eval crates/perry-runtime/src | head -220Repository: PerryTS/perry Length of output: 20733 Force strict mode for class constructors and methods. Explicit and synthesized constructors pass A plain call supplies Use a class-specific strict override when building all class constructors and methods. Add regression tests for a detached method and an undeclared assignment. 🤖 Prompt for AI Agents |
||
| ); | ||
| let target_idx = if m.is_static { ctor_idx } else { proto_idx }; | ||
| set_class_member(target_idx, &m.key, value); | ||
| } | ||
| ast::ClassMember::PrivateMethod(_) => throw_unsupported("private method (#field)"), | ||
| ast::ClassMember::ClassProp(_) => throw_unsupported("class field"), | ||
| ast::ClassMember::PrivateProp(_) => throw_unsupported("private class field (#field)"), | ||
| ast::ClassMember::TsIndexSignature(_) => { | ||
| throw_unsupported("TypeScript index signature in class body") | ||
| } | ||
| ast::ClassMember::Empty(_) => {} | ||
| ast::ClassMember::StaticBlock(_) => throw_unsupported("static initialization block"), | ||
| ast::ClassMember::AutoAccessor(_) => throw_unsupported("auto-accessor class member"), | ||
| } | ||
| } | ||
|
|
||
| // Wire the two together last: `Ctor.prototype = proto` is the dynamic | ||
| // expando write `js_new_function_construct` specifically looks for | ||
| // (`closure_get_dynamic_prop(fp, "prototype")`) to link a `new`-built | ||
| // instance's `[[Prototype]]` to `proto` instead of the closure's default | ||
| // (empty, per-function) prototype object. | ||
| bridge::set_member(root_get(ctor_idx), "prototype", root_get(proto_idx)); | ||
|
|
||
| let result = root_get(ctor_idx); | ||
| roots_truncate(base); | ||
| result | ||
| } | ||
|
|
||
| /// Set a class member (method) by its `PropName` onto the target (prototype | ||
| /// or constructor, for instance vs. `static`). Rejects computed and bigint | ||
| /// keys — see `eval_class_expr`'s documented subset. | ||
| fn set_class_member(target_idx: usize, key: &ast::PropName, value: f64) { | ||
| let value_idx = root_push(value); | ||
| match key { | ||
| ast::PropName::Ident(i) => { | ||
| bridge::set_member(root_get(target_idx), &i.sym, root_get(value_idx)) | ||
| } | ||
| ast::PropName::Str(s) => bridge::set_member( | ||
| root_get(target_idx), | ||
| &String::from_utf8_lossy(s.value.as_bytes()), | ||
| root_get(value_idx), | ||
| ), | ||
| ast::PropName::Num(n) => { | ||
| let k = bridge::make_number(n.value); | ||
| bridge::set_index(root_get(target_idx), k, root_get(value_idx), false); | ||
| } | ||
| ast::PropName::Computed(_) => throw_unsupported("computed method name in class body"), | ||
| ast::PropName::BigInt(_) => throw_unsupported("bigint method name in class body"), | ||
| } | ||
| roots_truncate(value_idx); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 30480
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 48628
Enforce construct-only invocation for class constructors.
eval_class_exprreturnsctor_closureas an ordinary callable closure. A plain call reachesbridge::call_functionwiththis = undefined, so this reachable dynamic-eval code can execute the constructor body and return7:The current result can set
box.valueand return7. A JavaScript class constructor called withoutnewmust throw aTypeErrorwithout executing its body. Add construct-only enforcement to the created class value. Keep the existing construction path valid, and add a regression test for the ordinary call.🤖 Prompt for AI Agents