fix(codegen): evaluate an assignment's member base once, before the RHS, for every base shape (#11150) - #11173
Conversation
|
Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
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. Comment |
|
Ready for a train. Fixes #11150, a redis blocker. It is a codegen change: when a member assignment's base is not in |
|
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. |
Fixes #11150
Root cause
HIR lowers
obj.k = v/obj[k] = vtoPutValueSet { target, key, value, receiver }. It fillstargetandreceiverwith clones of one lowered base, so the two trees are one source evaluation. Codegen decides whether it can evaluate the base once withsame_put_value_receiver_expr. That check only recognised a closed set of shapes: locals,this, literals,PropertyGet/IndexGetchains over those, and calls. Any other base shape fell out as "distinct receiver" and took the explicit-receiver lowering. That lowering evaluatesreceivera second time, after the RHS.this.#tail.next = this.#tail = nodehas the basePropertyGet { object: PrivateGuard { This } }.PrivateGuardwasn't in the set, so the base was re-read after the RHS had already moved#tail, andnextwas written onto the new node. The public-field spelling isPropertyGet { 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)this.#g.x = …: the getter ran twicesuper.x.y = …: thesuper.xgetter ran twice and the write landed on the new objectnew C().x = …: constructed twice(cond ? p : q).z = …: the condition was evaluated twiceAll operator forms were affected, because compound (
+=,*=) and logical (??=,||=,&&=) assignments on these bases reach the samePutValueSet.Fix
In
same_put_value_receiver_expr, the old catch-all_ => falsebecomes a structural comparison: the same variant, then a byte-exact comparison of the two trees'stable_hashserialization. That serialization is exhaustive, uses one tag per variant, and length-prefixes strings. The newperry_hir::stable_hash::same_expr_structuredoes the comparison. It deliberately ignores the same derived metadata the object cache ignores (byte_offset,cap_args_appended).A structurally identical
targetandreceiveris always one clone. No HIR producer builds aPutValueSetwith an independently re-evaluated receiver: the eightPutValueSetconstructors inperry-hireither clone one base or useGlobalThisExpron 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. TheRootedGrouprooting of the base across the RHS is unchanged.Tests
test-files/test_gap_private_member_target_eval_order.ts, 9 sections:SinglyLinkedList/DoublyLinkedListpush/unshift idioms=, deep=,+=,*=,??=,||=,&&=,[k] +=, chained= =++/--on private basesobj.#a.xthis.#a.#b=/+=super.x.y(with the getter log)new C().x,(cond ? p : q).z, and statement-position compound/logical assignmentscrates/perry-hir/src/stable_hash/tests.rs:same_expr_structure_matches_clones_and_rejects_differencesValidation (perrymaster, Linux x86_64, perry-dev,
PERRY_NO_AUTO_OPTIMIZE=1, Node 26.5.1 from/opt/node-v26.5.1-linux-x64)2754cb0fadiffers from Node in 16 lines: the singly list gives1,,,instead of1,2,3,4; every operator row writes to the new object; the getter runs twice;newruns twice;super.xruns twice; the condition runs twice.cmpclean).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 notrun_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 producedText file busyartifacts. 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 xwinis not installed on the host, and "Public benchmark evidence freshness" is known-red on main.redis-server 7.0.15. Both arms are origin/maine6a3bd8f5(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:connectevent, thenclient error: Cannot read properties of undefined (reading 'reject'). The client never gets toready. This is the failure from this.#tail.next = this.#tail = node evaluates the RHS before the private-field target object (redis linked-list push; next redis blocker) #11150.connect→ready→connect()resolves. The first command (SET) then rejects withCannot access private member ...from*[Symbol.iterator]()on the linked list. That is the next blocker, filed as A #private read inside a *[Symbol.iterator]() generator method throws "Cannot access private member" (next redis@6.1.0 blocker after #11150) #11170 with a package-free repro. The set/get round trip therefore doesn't complete yet, over either socket or url.Not run
run_parity_tests.shitself. I used the targeted compile-and-diff loop described above.cargo testforperry-codegen/perryintegration suites. The change is one match arm inperry-codegen/src/expr/proxy_reflect.rs. No test file in another suite is known to depend on it.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.Side finding while triaging, which does not block redis: #11172. A default-imported
EventEmittercreated in a class-field initializer is inert.