Skip to content

fix(hir): give each evaluation of a dynamic-heritage class expression its own class (#11042) - #11122

Closed
proggeramlug wants to merge 3 commits into
mainfrom
fix/11042-class-expr-dynamic-heritage
Closed

proggeramlug wants to merge 3 commits into
mainfrom
fix/11042-class-expr-dynamic-heritage

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Part of #11042 — fixes the reported client.on("error", …) TypeError; the real package then stops on #11120 and #11121 (see Tests), so this does not close the issue.

Root cause

A class expression with dynamic heritage (class extends <runtime value> {}) evaluated inside a function was one shared class across every evaluation. It had one class id, one parent edge (the last evaluation's extends won) and one Class.prototype[name] = … table. arm_class.rs sent a function-body class expression through the per-evaluation ClassExprFresh path only when it also carried statics, computed keys, captures, private elements or a used self-binding. #10622 widened that to one shape: an exported factory whose whole body is return class extends X {}. Same-module direct call sites are cloned by specialize_captured_class_factories. Everything else kept the shared template.

@redis/client's commander.js attachConfig falls outside all of those. It is a comma declarator (const RESP = …, Class = class extends BaseClass {}) followed by a Class.prototype[name] = … loop, and it is called across modules. RedisClient.factory evaluates it for RedisClient, then for RedisClientMultiCommand (Client.prototype.Multi = …extend(config)), and only then runs new Client(options). So:

  • The client ran the Multi class's constructor.
  • It lost its EventEmitter ancestry.
  • client.on("error", cb) resolved to the events module's static on(emitter, name), which threw The "emitter" argument must be an instance of EventEmitter. Received type string ('error').

The earlier diagnosis on the issue was right about the symptom: the receiver reaching js_native_call_method's namespace fast path was the real events namespace object. The fast path is correct for a genuine namespace. The defect is upstream, in the clobbered class.

Fix

Tests

Command and result for each check:

Known remaining (pre-existing, not changed here)

Instances still carry their template's class id. A class-id-keyed lookup can therefore still see a sibling evaluation's parent:

  • A mk(A) instance reading a method that only mk(B)'s base defines gets it, where Node gives undefined.
  • Object.create(new (mk(R))()) instanceof R is false after a later evaluation.

These were already true for fresh class expressions with statics. Giving each evaluation its own class id would fix them, but that touches every cid-keyed runtime table, so it is not attempted here.

Not run

  • cargo test --workspace and perry-runtime's own suite (only a formatter branch changed there).
  • The full gap sweep.
  • The run_lint_gates.sh compile tier and the Windows cargo xwin check.
  • The perf/instruction-count A/B. The change is a lowering-route change for dynamic-heritage class expressions only. Construction of such classes now goes through the class-object replay path instead of the shared template, which was the wrong class anyway.
  • No other integration suite should be affected. The perry-transform factory-specialization tests pass unchanged.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Heads-up while this is being reduced: #11014's version of the util.inspect fix landed on main in merge train 268 (v0.5.1651, 36892b7194). So dropping this PR's formatting.rs half is now a rebase requirement, not just a courtesy — keeping it will conflict.

The landed version is worth a look before you rebase, because the diagnosis went past "hide two keys":

  • The leak pre-dates both PRs. format_object_as_json walks object_keys_array directly and was the only enumeration path that never applied is_internal_runtime_keyObject.keys, for…in, getOwnPropertyNames, JSON.stringify, hasOwnProperty and spread all do. The fix is "apply the existing allowlist here too", not a new hide-list.
  • Neither PR caused it; both merely route a covered fixture onto the latent bug. Yours via ClassExprFresh, fix: preserve Effect tagged error names across class evaluations #11014's via its arm_class.rs hunk.
  • showHidden deliberately still does not reveal them — it exposes non-enumerable JS properties, and these are runtime bookkeeping.
  • It came with formatting/internal_key_hiding_tests.rs: three cases, each asserting its key spelling really is in the allowlist first, so a renamed constant fails the precondition rather than passing vacuously.

Your perry-hir class-expression half is independent and still wanted — send the reduced head and it goes in the next train.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Closing as superseded. Main fixed the #11042 client.on error independently in 86cb666 ("preserve tagged error names across class evaluations"), which changes the same arm_class.rs condition and deliberately excludes class expressions with static methods.

Rebased onto main, this PR's extra class_value_template_name helper would make a C.prototype[k] = v write unreadable on instances, so it should not land.

The remaining per-evaluation prototype leak is filed as #11134, and the fix will carry this PR's gap test as coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant