diff --git a/examples/js_dsl/mod.test.ts b/examples/js_dsl/mod.test.ts index 9df6556..9886585 100644 --- a/examples/js_dsl/mod.test.ts +++ b/examples/js_dsl/mod.test.ts @@ -391,7 +391,7 @@ describe("optional parameters", () => { }); describe("class materialization", () => { - it("static class return avoids constructor placeholder allocation", () => { + it("static class return avoids constructor init allocation", () => { const initBefore = mod.getFactoryResourceInitCount(); const deinitBefore = mod.getFactoryResourceDeinitCount(); @@ -402,7 +402,7 @@ describe("class materialization", () => { expect(mod.getFactoryResourceDeinitCount()).toEqual(deinitBefore); }); - it("instance class return avoids constructor placeholder allocation", () => { + it("instance class return avoids constructor init allocation", () => { const base = mod.FactoryResource.withByte(1); const initBefore = mod.getFactoryResourceInitCount(); const deinitBefore = mod.getFactoryResourceDeinitCount(); @@ -425,6 +425,62 @@ describe("class materialization", () => { it("rejects cross-class static factory binding during materialization", () => { expect(() => mod.Point.create.call(mod.Buffer, 1, 2)).toThrow(); }); + + it("preserves normal nested construction inside subclass constructors", () => { + // Same-class materialization may call a JS subclass constructor via the preferred + // receiver constructor. A nested normal `new` inside that constructor must not + // inherit the internal materialization marker, otherwise it skips native wrapping. + let nested: InstanceType | undefined; + class DerivedFactoryResource extends mod.FactoryResource { + constructor() { + super(); + nested = new mod.FactoryResource(); + } + } + + const resource = DerivedFactoryResource.withByte(5); + + expect(resource).toBeInstanceOf(DerivedFactoryResource); + expect(resource.getByte()).toEqual(5); + expect(nested?.getByte()).toEqual(0); + }); + + it("rejects subclass constructors that return a different object", () => { + class ReplacingPoint extends mod.Point { + constructor() { + super(); + return {}; + } + } + + expect(() => ReplacingPoint.create(3, 4)).toThrow(); + }); + + it("does not run cross-class constructors during failed materialization", () => { + const initBefore = mod.getFactoryResourceInitCount(); + const deinitBefore = mod.getFactoryResourceDeinitCount(); + + expect(() => mod.Point.create.call(mod.FactoryResource, 1, 2)).toThrow(); + + expect(mod.getFactoryResourceInitCount()).toEqual(initBefore); + expect(mod.getFactoryResourceDeinitCount()).toEqual(deinitBefore); + }); + + it("rejects non-zapi constructors during materialization", () => { + function FakePoint() {} + + expect(() => mod.Point.create.call(FakePoint, 1, 2)).toThrow(); + }); + + it("deinitializes returned native resources when materialization fails", () => { + const initBefore = mod.getFactoryResourceInitCount(); + const deinitBefore = mod.getFactoryResourceDeinitCount(); + + expect(() => mod.FactoryResource.withByte.call(mod.Point, 7)).toThrow(); + + expect(mod.getFactoryResourceInitCount()).toEqual(initBefore + 1); + expect(mod.getFactoryResourceDeinitCount()).toEqual(deinitBefore + 1); + }); }); // Section 15: Getters and Setters diff --git a/examples/js_dsl/mod.zig b/examples/js_dsl/mod.zig index 1eef6cd..59ee1c8 100644 --- a/examples/js_dsl/mod.zig +++ b/examples/js_dsl/mod.zig @@ -426,7 +426,7 @@ pub const Point = struct { } }; -/// A resource-owning class used to verify placeholder cleanup in factory paths. +/// A resource-owning class used to verify cleanup in class materialization paths. pub const FactoryResource = struct { pub const js_meta = js.class(.{}); data: []u8, diff --git a/src/js/class_runtime.zig b/src/js/class_runtime.zig index 7af0b03..edf52ef 100644 --- a/src/js/class_runtime.zig +++ b/src/js/class_runtime.zig @@ -8,15 +8,11 @@ pub fn typeTag(comptime T: type) napi.c.napi_type_tag { }; } -pub fn wrapTaggedObject(comptime T: type, env: napi.Env, object: napi.Value, native_object: *T, finalize_hint: ?*anyopaque) !void { +pub fn wrapTaggedObject(comptime T: type, env: napi.Env, object: napi.Value, native_object: *T) !void { const tag = typeTag(T); - try env.wrap(object, T, native_object, defaultFinalize(T), finalize_hint, null); + try env.wrap(object, T, native_object, defaultFinalize(T), null, null); errdefer if (env.removeWrap(T, object)) |removed| { - if (isInternalPlaceholderHint(T, finalize_hint)) { - destroyInternalPlaceholder(T, removed); - } else { - destroyNativeObject(T, removed); - } + destroyNativeObject(T, removed); } else |_| {}; if (!(try env.checkObjectTypeTag(object, tag))) { try env.typeTagObject(object, tag); @@ -48,17 +44,9 @@ pub fn destroyNativeObject(comptime T: type, obj: *T) void { std.heap.c_allocator.destroy(obj); } -pub fn destroyInternalPlaceholder(comptime T: type, obj: *T) void { - std.heap.c_allocator.destroy(obj); -} - pub fn defaultFinalize(comptime T: type) napi.FinalizeCallback(T) { return struct { - fn f(_: napi.Env, obj: *T, hint: ?*anyopaque) void { - if (isInternalPlaceholderHint(T, hint)) { - destroyInternalPlaceholder(T, obj); - return; - } + fn f(_: napi.Env, obj: *T, _: ?*anyopaque) void { destroyNativeObject(T, obj); } }.f; @@ -85,28 +73,75 @@ pub fn registerClass(comptime T: type, env: napi.Env, ctor: napi.Value) !void { try env.addEnvCleanupHook(State.Entry, entry, State.cleanupHook); } +/// Per-thread marker set by `materializeClassInstance` to tell the generated +/// constructor "this `new` call comes from the DSL; return the JS instance +/// without running `init`, and materialization will wrap the native object." +/// Compared by identity against `internalCtorMarkerPtr(T)`. +threadlocal var materialize_target: ?*const anyopaque = null; + +/// Captures the exact `this` object whose generated base constructor consumed +/// `materialize_target`. JS derived constructors are allowed to `return {}` +/// after `super()`, causing `napi_new_instance` to return that replacement +/// object. Materialization must reject that case instead of wrapping native +/// state onto an unrelated object with the wrong prototype. +/// +/// Stored as a temporary N-API reference because nested JS construction can run +/// before `napi_new_instance` returns; keeping only the raw constructor callback +/// handle is not stable enough across that nested call stack. +threadlocal var materialized_instance: ?napi.Ref = null; + +pub fn isMaterializing(comptime T: type) bool { + return materialize_target == @as(?*const anyopaque, @ptrCast(internalCtorMarkerPtr(T))); +} + +pub fn hasPendingMaterialization() bool { + return materialize_target != null; +} + +pub fn consumeMaterialization(comptime T: type, env: napi.Env, this_arg: napi.c.napi_value) !bool { + if (!isMaterializing(T)) return false; + const this_val = napi.Value{ .env = env.env, .value = this_arg }; + const this_ref = try env.createReference(this_val, 1); + materialize_target = null; + materialized_instance = this_ref; + return true; +} + pub fn materializeClassInstance(comptime T: type, env: napi.Env, instance: T, preferred_ctor: ?napi.Value) !napi.Value { const ctor = preferred_ctor orelse try getConstructor(T, env); - const internal_arg = try env.createExternal(@ptrCast(internalCtorMarkerPtr(T)), null, null); - var raw_args = [_]napi.c.napi_value{internal_arg.value}; + + const obj_ptr = try std.heap.c_allocator.create(T); + errdefer destroyNativeObject(T, obj_ptr); + obj_ptr.* = instance; + + const prev = materialize_target; + const prev_instance = materialized_instance; + materialize_target = @ptrCast(internalCtorMarkerPtr(T)); + materialized_instance = null; + defer materialize_target = prev; + defer { + if (materialized_instance) |ref| ref.delete() catch {}; + materialized_instance = prev_instance; + } var js_instance_raw: napi.c.napi_value = null; try napi.status.check(napi.c.napi_new_instance( env.env, ctor.value, - 1, - &raw_args, + 0, + null, &js_instance_raw, )); const js_instance = napi.Value{ .env = env.env, .value = js_instance_raw }; - const placeholder = try env.removeWrapChecked(T, js_instance, typeTag(T)); - destroyInternalPlaceholder(T, placeholder); - - const obj_ptr = try std.heap.c_allocator.create(T); - obj_ptr.* = instance; - - try wrapTaggedObject(T, env, js_instance, obj_ptr, null); + if (materialize_target != null) return error.InvalidMaterializationConstructor; + const expected_instance_ref = materialized_instance orelse return error.InvalidMaterializationConstructor; + const expected_instance = try expected_instance_ref.getValue(); + // The generated constructor must be the object that comes back from + // `napi_new_instance`; otherwise a subclass returned a replacement object. + if (!(try expected_instance.strictEquals(js_instance))) return error.InvalidMaterializationConstructor; + + try wrapTaggedObject(T, env, js_instance, obj_ptr); return js_instance; } @@ -120,19 +155,6 @@ fn getConstructor(comptime T: type, env: napi.Env) !napi.Value { return try entry.ctor_ref.getValue(); } -pub fn isInternalCtorArg(comptime T: type, value: napi.Value) bool { - const raw = value.getValueExternal() catch return false; - return raw == @as(*anyopaque, @ptrCast(internalCtorMarkerPtr(T))); -} - -pub fn internalPlaceholderHint(comptime T: type) ?*anyopaque { - return @ptrCast(&markers(T).placeholder_hint); -} - -pub fn isInternalPlaceholderHint(comptime T: type, hint: ?*anyopaque) bool { - return hint == internalPlaceholderHint(T); -} - fn state(comptime T: type) type { return struct { const Class = T; @@ -193,14 +215,9 @@ fn markers(comptime T: type) type { } var ctor_marker: u8 = 0; - var placeholder_hint: u8 = 0; }; } -fn internalCtorMarker(comptime T: type) [*]const u8 { - return internalCtorMarkerPtr(T); -} - fn internalCtorMarkerPtr(comptime T: type) *u8 { return &markers(T).ctor_marker; } diff --git a/src/js/wrap_class.zig b/src/js/wrap_class.zig index 6a34c0c..bb20974 100644 --- a/src/js/wrap_class.zig +++ b/src/js/wrap_class.zig @@ -68,21 +68,15 @@ pub fn wrapClass(comptime T: type) type { /// This callback is invoked when `new Class(...)` is called in JavaScript. /// It handles argument conversion, calls the `pub fn init(...)` method of /// the Zig struct `T`, and wraps the resulting native object in the JS instance. - /// It also handles internal placeholder creation for factory methods. + /// It also supports direct wrapping for DSL-returned class instances. pub const constructor: napi.c.napi_callback = genConstructor(); /// The default N-API finalizer callback for native objects wrapped by instances of `T`. /// /// This function is registered with `napi.Env.wrap()` and is called by the /// JavaScript garbage collector when the wrapped JS object is finalized. - /// It handles cleanup for internal placeholder objects during class - /// materialization and calls the `deinit()` method (if present) and frees - /// the native memory for regular class instances. - pub fn defaultFinalize(_: napi.Env, obj: *T, hint: ?*anyopaque) void { - if (class_runtime.isInternalPlaceholderHint(T, hint)) { - class_runtime.destroyInternalPlaceholder(T, obj); - return; - } + /// It calls the `deinit()` method (if present) and frees the native memory. + pub fn defaultFinalize(_: napi.Env, obj: *T, _: ?*anyopaque) void { class_runtime.destroyNativeObject(T, obj); } @@ -366,22 +360,19 @@ pub fn wrapClass(comptime T: type) type { return null; }; - if (actual_argc == 1) { - const internal_arg = napi.Value{ .env = raw_env, .value = raw_args[0] }; - if ((internal_arg.typeof() catch null) == .external) { - const obj_ptr = std.heap.c_allocator.create(T) catch { - e.throwError("", "Out of memory allocating internal placeholder") catch {}; - return null; - }; - obj_ptr.* = std.mem.zeroes(T); - - const this_val = napi.Value{ .env = raw_env, .value = this_arg }; - class_runtime.wrapTaggedObject(T, e, this_val, obj_ptr, class_runtime.internalPlaceholderHint(T)) catch { - e.throwError("", "Failed to wrap internal placeholder") catch {}; - return null; - }; - return this_arg; - } + // Fast path: materializeClassInstance is creating this instance. + // Skip normal init; materialize will wrap the returned JS instance + // with the real native pointer after napi_new_instance returns. + const did_consume_materialization = class_runtime.consumeMaterialization(T, e, this_arg) catch { + e.throwError("", "Failed to record materialized instance") catch {}; + return null; + }; + if (did_consume_materialization) { + return this_arg; + } + if (class_runtime.hasPendingMaterialization()) { + e.throwTypeError("", "Invalid materialization constructor") catch {}; + return null; } const required_init_argc = comptime wrap_function.requiredArgCount(init_params); @@ -408,7 +399,7 @@ pub fn wrapClass(comptime T: type) type { obj_ptr.* = init_result; const this_val = napi.Value{ .env = raw_env, .value = this_arg }; - class_runtime.wrapTaggedObject(T, e, this_val, obj_ptr, null) catch { + class_runtime.wrapTaggedObject(T, e, this_val, obj_ptr) catch { e.throwError("", "Failed to wrap native object") catch {}; return null; };