Skip to content

fix(codegen): evaluate an assignment's member base once, before the RHS, for every base shape (#11150) - #11173

Merged
proggeramlug merged 3 commits into
mainfrom
fix/11150-private-member-target-order
Sep 24, 2026
Merged

proggeramlug merged 3 commits into
mainfrom
fix/11150-private-member-target-order

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #11150

Root cause

HIR lowers obj.k = v / obj[k] = v to PutValueSet { target, key, value, receiver }. It fills target and receiver with clones of one lowered base, so the two trees are one source evaluation. Codegen decides whether it can evaluate the base once with same_put_value_receiver_expr. That check only recognised a closed set of shapes: locals, this, literals, PropertyGet/IndexGet chains over those, and calls. Any other base shape fell out as "distinct receiver" and took the explicit-receiver lowering. That lowering evaluates receiver a second time, after the RHS.

this.#tail.next = this.#tail = node has the base PropertyGet { object: PrivateGuard { This } }. PrivateGuard wasn't in the set, so the base was re-read after the RHS had already moved #tail, and next was written onto the new node. The public-field spelling is PropertyGet { This }, which is in the set, so it was correct. The same defect hit every base the set didn't recognise:

  • this.#o.x = …, this.#o[k] = …, this.#o.a.b = …, p.#a.x = … (another instance's private field)
  • a private-getter base, this.#g.x = …: the getter ran twice
  • super.x.y = …: the super.x getter ran twice and the write landed on the new object
  • new C().x = …: constructed twice
  • (cond ? p : q).z = …: the condition was evaluated twice

All operator forms were affected, because compound (+=, *=) and logical (??=, ||=, &&=) assignments on these bases reach the same PutValueSet.

Fix

In same_put_value_receiver_expr, the old catch-all _ => false becomes a structural comparison: the same variant, then a byte-exact comparison of the two trees' stable_hash serialization. That serialization is exhaustive, uses one tag per variant, and length-prefixes strings. The new perry_hir::stable_hash::same_expr_structure does the comparison. It deliberately ignores the same derived metadata the object cache ignores (byte_offset, cap_args_appended).

A structurally identical target and receiver is always one clone. No HIR producer builds a PutValueSet with an independently re-evaluated receiver: the eight PutValueSet constructors in perry-hir either clone one base or use GlobalThisExpr on both sides. So the effect is that these bases now take the same-receiver lowerings, which evaluate the base once, first. Those lowerings are the ones already used for public fields: the same-receiver dyn-IC / static-write-IC / index fast paths. The RootedGroup rooting of the base across the RHS is unchanged.

Tests

  • test-files/test_gap_private_member_target_eval_order.ts, 9 sections:
    • redis's SinglyLinkedList / DoublyLinkedList push/unshift idioms
    • every operator form on a private-field base: =, deep =, +=, *=, ??=, ||=, &&=, [k] +=, chained = =
    • computed-key order
    • ++/-- on private bases
    • a private getter as base (with the getter-call log)
    • obj.#a.x
    • nested this.#a.#b = / +=
    • super.x.y (with the getter log)
    • a public-field control
    • new C().x, (cond ? p : q).z, and statement-position compound/logical assignments
  • crates/perry-hir/src/stable_hash/tests.rs: same_expr_structure_matches_clones_and_rejects_differences

Validation (perrymaster, Linux x86_64, perry-dev, PERRY_NO_AUTO_OPTIMIZE=1, Node 26.5.1 from /opt/node-v26.5.1-linux-x64)

  • Gap test fails on main: base 2754cb0fa differs from Node in 16 lines: the singly list gives 1,,, instead of 1,2,3,4; every operator row writes to the new object; the getter runs twice; new runs twice; super.x runs twice; the condition runs twice.
  • With the fix, the gap test is byte-identical to Node 26.5.1 (cmp clean).
  • A/B against main, built the same way: 190 related test-files/ tests (private fields, assignment, compound, PutValue, super, getters/setters, proxy/reflect, class expressions, member, eval order), run as a compile-and-diff loop against Node 26.5.1. That harness is not run_parity_tests.sh; I used my own loop and did not use the harness port. Result: 178 pass on base, 179 pass on the fix, and the one extra pass is the new test. No test changed output except the new test and one pre-existing failure, test_gap_json_lazy_defineproperty_index, which fails on both arms and differs only in a printed heap address. 37 of those tests first ran while two of my loops overlapped, which produced Text file busy artifacts. The numbers above use a clean foreground rerun of those 37 (34 pass on each arm, all identical).
  • cargo test -p perry-hir --lib stable_hash: 8 passed.
  • cargo fmt --all -- --check: clean. scripts/check_file_size.sh: OK.
  • SKIP_COMPILE_GATES=1 scripts/run_lint_gates.sh: 86 of 88 script gates passed, compile tier not run. The 2 failures are environmental or pre-existing, not from this change: cargo xwin is not installed on the host, and "Public benchmark evidence freshness" is known-red on main.
  • redis@6.1.0 end-to-end, against my own redis-server 7.0.15. Both arms are origin/main e6a3bd8f5 (which includes fix(runtime): resolve an ancestor's #private brand through the instance's evaluation heritage chain #11141) + fix(hir): new <class value>() no longer folds builtin-named user methods to the Array fast path (#11128) #11137, built with the same runtime stamp:

Not run

  • The full gap sweep, and run_parity_tests.sh itself. I used the targeted compile-and-diff loop described above.
  • The lint compile tier, and cargo test for perry-codegen / perry integration suites. The change is one match arm in perry-codegen/src/expr/proxy_reflect.rs. No test file in another suite is known to depend on it.
  • The instruction-count A/B (perf stat). The old path for these bases evaluated the base twice and gave the wrong answer, so it isn't a valid baseline. The new path is the existing same-receiver lowering that public fields already use.
  • macOS.

Side finding while triaging, which does not block redis: #11172. A default-imported EventEmitter created in a class-field initializer is inert.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 2 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 8 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: e5c1d559-f4d7-4b39-8b90-953f024e48dc

📥 Commits

Reviewing files that changed from the base of the PR and between c7d0963 and dbe2b1b.

📒 Files selected for processing (5)
  • changelog.d/11173-put-value-base-eval-order.md
  • crates/perry-codegen/src/expr/proxy_reflect.rs
  • crates/perry-hir/src/stable_hash/mod.rs
  • crates/perry-hir/src/stable_hash/tests.rs
  • test-files/test_gap_private_member_target_eval_order.ts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Ready for a train. Fixes #11150, a redis blocker. It is a codegen change: when a member assignment's base is not in same_put_value_receiver_expr's list (private-field reads, private getters, super.x, new C(), conditionals), the base is no longer evaluated a second time after the RHS. The new gap test is byte-identical to Node, and a 190-test A/B shows no regressions. With this fix, redis@6.1.0 reaches ready; the next blocker is #11170.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Merge queue: this run's only non-grandfathered red, "Rekeyed side-table custody audit", was main's own break from #11169, which CI merged into this PR's test commit; #11178 fixed it. Re-running doesn't help because a re-run reuses the old merge commit. Stacked on current main together with #11162/#11173/#11176/#10946, the full run_lint_gates script tier fails only the owner-grandfathered public-baseline step, and -D warnings --all-targets on perry-stdlib/perry-runtime/perry-codegen/perry-hir plus cargo check -p perry --bins are clean. Every other CI job on this head passed.

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.

this.#tail.next = this.#tail = node evaluates the RHS before the private-field target object (redis linked-list push; next redis blocker)

1 participant