land #8833: ECS specializations, with the argument-route guard restored - #8837
Merged
Conversation
added 4 commits
August 25, 2026 22:44
…ll is bypassed #8833 widened the `$pshape_args` route in three directions at once and removed the runtime class+ShapeId guard at the same time. Individually each widening is arguable; composed, they left an unguarded fixed-offset read of an object the compiler itself had recorded as published to an alias it cannot see. Reproduced as a codegen ratchet: a module with an unattributable `Object.defineProperty`, a callee that publishes its parameter after its licensed read, and two route sites on the same caller local emitted two unguarded direct calls into the clone (2 clone calls, 0 guard blocks). Two changes restore the invariant the PR's own doc comments assert: - The route-only fact is collected with rule 5's module-wide shape-barrier kill bypassed, so it must never license guard-free field access. Its route now keeps the runtime guard plus the generic fallback. Guard elision is retained only where the caller holds the broad `Ptr<Shape>` representation fact, which was proven in a barrier-free module under full containment and where the guard is therefore tautological. - Route admission requires the clone to preserve containment for the parameter's whole lifetime. `PrefixContainedParamUse` proves a temporal property ("the reads happen before the publication"), but the fact map that carries a caller-side route is keyed by local id and is flow-insensitive, so a fact kept past a publishing call is consulted again at every later route site. The `require_post_call_containment` knob, whose only other mode was unsound, is deleted rather than left selectable. Also refreshes the `local_binding_type_audit` allowlist: #8833 hoisted `unique_global` out of `codegen/mod.rs`, moving the attribution of an unchanged `module_local_types` read to `compile_module`. The measured `perform-ecs` and Wolf routes are unaffected (fresh contained locals in barrier-free modules); the #8774 slice carried no speed claim.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (96)
📝 WalkthroughWalkthroughThe change adds guarded argument-shape routing, cross-module method capability propagation, live packed-loop revalidation, iterator-aware spread fallback, and regression coverage across code generation, runtime helpers, cache keys, and integration fixtures. ChangesArgument-route guards
Cross-module method capabilities
Packed-loop revalidation
Spread lowering and integration coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lands #8833 (
perf: complete ECS benchmark specializations) with a real soundness bug fixed, not a test updated.The two failing tests were a symptom, and the fix was in the producing code
#8833 as submitted failed two
perry-codegentests. The second one —— was not a stale assertion. The reword was the visible surface of a substantive change: the PR widened the
$pshape_argsroute in four mutually load-bearing ways and removed the runtime class+ShapeId guard in the same change.ReadOnlyParamUse→PrefixContainedParamUse)collectors/proven_args.rsrequire_post_call_containment = false)collectors/ptr_shape.rsCollectionPurpose::GuardedArgumentRoute)collectors/ptr_shape_entry.rsptr_shape_argument_route_factreturned(fact, false))expr/mod.rsptr_shape.rs's own doc says rule 5 is "belt-and-braces against analysis blind spots", justified by rule 2's containment — and the PR removed containment in the same breath. Its doc comments contradicted its code in three further places (hir_facts.rs: "must never license guard-free field access";ptr_shape_entry.rs: "consumable only beside a live guard";proven_args.rs: "every route revalidates the live class and shape").Root cause
PrefixContainedParamUseproves a temporal property — reads happen before publication. Butguarded_argument_route_localsis aHashMap<u32, PtrShapeLocal>keyed by local id, i.e. flow-insensitive, and is consulted at every route site for that local, including sites executing after the alias exists. A per-local map cannot express "before". The PR's own wording — "the caller cannot retain an unguardedPtr<Shape>fact after that call" — states a constraint its data structure cannot enforce. This is the #7773 family: a declared property is not a proof; only a live guard is.Demonstrated, not argued
A probe fixture (module with an unattributable
Object.defineProperty; callee readingentity.idthen returningentity; tworegistry.read(entity)sites) made the pre-fix compiler emit:Two unguarded direct calls into a clone performing fixed-offset field loads — the second on an object the compiler itself recorded as published to an alias it cannot see, in a module its own rule 5 says carries an unbounded shape barrier. On clean
mainthat fixture routes nowhere.The fix
expr/mod.rs— route-only facts now require the guard ((fact, true)). Lookup order isproven_shape_params(guard) →shape_proven_ptr_local(elide) →guarded_argument_route_local(guard). Elision survives only where it is justified: the caller already holds the broadPtr<Shape>fact, proven in a barrier-free module under full containment, so it is already licensed to read those fields at fixed offsets and the guard is tautological.proven_args.rs/ptr_shape.rs— route admission always requirespreserves_containment; therequire_post_call_containmentknob, whose only other mode was unsound, is deleted rather than left selectable (CLAUDE.md's kill policy: a mode that still exists is a decision that hasn't been made).ptr_shape_report.rs— restored"passed as a call argument"; the test assertion is untouched and its discrimination intact.unrelated_module_shape_barrier_keeps_guarded_argument_routenow asserts the guard/fallback is retained;field_read_before_terminal_publication_gets_only_the_guarded_route→publishing_clone_gets_no_caller_side_route; newpublished_argument_in_a_barrier_module_never_reaches_an_unguarded_clone, which asserts its own subject is live (the clone must still be emitted) and was sabotage-checked against the pre-fix code, where it fails with 2 clone calls / 0 guard blocks.Failure 1 — allowlist deliberately not widened
The
$pshape_argsoffenders (hir_facts.rs,ptr_shape_report.rs, andptr_shape_entry.rswhich the merge exposed) mention the fragment only in prose — a doc comment and a diagnostic string. A doc comment cannot register a vtable entry, and any code that builds the name goes throughpshape_args_method_namein the already-allowlistedproven_args.rs. Allowlisting three prose files would permanently exempt them from the real check, so the prose was reworded instead. The allowlist stays at 10 entries.A fifth gate failure, not in the original report
local_binding_type_auditwas also red: #8833 hoistedunique_globalout ofcodegen/mod.rsintohelpers::unique_class_keys_global, moving the attribution of an unchangedmodule_local_typesread tocompile_module. Allowlist entry refreshed; count, classification and reason unchanged.Validation
addr_class_inventory --self-testandlocal_binding_type_audit --self-testperry-codegen1265,perry-runtime2697,perry-stdlib120,perry-hir337 — all 0 failedissue_8774_argument_shape_clones(Node parity + forced-moving-GC arms): 2 passed, 0 failedperform-ecs/Wolf fixtures are fresh contained locals in barrier-free modules and keep the elided guard, so the e2e's!ir.contains("pshape_arg.fallback")assertions still hold. The perf(codegen): propagate Ptr<Shape> facts into guarded argument clones #8774 slice carried no speed claim ("0.24% slower (flat), 4/11 wins"), so removing the widening costs nothing measurable.An earlier run of the e2e failed with ENOSPC at 412Mi free; those results were discarded rather than interpreted, and the numbers above come from a clean re-run with 18Gi, verified by
dfbefore and after.Summary by CodeRabbit
Math.minandMath.maxcalls.