majit: fail the slice-index block-alias resolver closed on a non-variable incoming argument - #1133
Conversation
…able incoming argument `resolve_block_alias` collected the incoming link arguments of a block input with `filter_map` and `LinkArg::as_variable`, discarding an argument that is not a variable instead of treating it as a disagreement. A block input fed `[Variable(v), Constant(7)]` collapsed to `[v]`, the "all incoming agree" check passed, and the phi resolved to v's root, so the resolver could equate two values that are not equal on all paths. Collect into `Option<Vec<Variable>>` instead: the first non-variable argument short-circuits the collection and `incoming?` returns None. Links out of blocks not reachable from `startblock` cannot execute, so they are excluded from the walk rather than allowed to veto a resolution. `reachable_from_start` is lifted out of `array_len_base_is_stable` and shared, and the set is computed once and threaded through the recursion. The positional `__pos_0` spelling of the `RangeTo.end` field write stays rejected, recorded by a test: `is_construction_write` does not key on the field name, so widening the `end_writes` gate to `__pos_0` admits sites that only `resolve_block_alias` makes visible rather than restoring an earlier rewrite. Assisted-by: Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe translator now resolves block aliases using only reachable predecessors. It rejects mixed or conflicting reachable inputs, reuses reachability analysis for stability checks, and adds ChangesReachability-aware slice alias resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a92ee7c). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
1 participant
Follow-up to #1124, acting on its Codex parity review. §2 is fixed here; §1 is answered with a test
rather than a code change; §3 is out of scope.
Fixed (§2):
resolve_block_aliasdiscarded non-variable incoming argumentsfilter_mapdrops an incoming argument that is not a variable instead of treating it as adisagreement. A block input fed
[Variable(v), Constant(7)]collapsed to[v], the "all incomingagree" check passed, and the phi resolved to
v's root — so the resolver could equate two valuesthat are not equal on all paths. The static-length proof relies on this resolver for the
receiver/
ArrayLenbase identity, and the runtime helperll_listslice_startstopclamps rather thanpanicking, so a false alias is a silent wrong-length slice, not a crash.
resolve_block_aliasdidnot exist before #1124.
The fix collects into
Option<Vec<Variable>>, so the first non-variable argument short-circuits thecollection and
incoming?returnsNone.Links out of blocks not reachable from
startblockcannot execute, so they are excluded from thewalk rather than allowed to veto a resolution.
reachable_from_startis lifted out ofarray_len_base_is_stableand shared; the set is computed once and threaded through the recursion.Not fixed (§1): the
endfield write keeps requiring the"end"spellingThe review notes that
range_feeds_only_indexmatches theendfield write asfield.name == "end", while the aggregate lowering names that write__pos_<i>when the resolvercannot project a schema entry — so a
RangeTo, whose single operand is at index 0, is spelled__pos_0and theend_writes == 1gate declines it.The code reading is right; the conclusion that #1124 narrowed a rewrite the older code performed does
not follow.
is_construction_writehas never keyed on the field name and still does not — theend_writesgate only requires the write to be unique and named. #1124 also addedresolve_block_alias, which widened which sites are recognised at all. So widening the gate to__pos_0would not restore an earlier rewrite; it would admit sites that only the new aliasresolution makes visible. It was implemented and measured, and cleared no additional prepass subject,
so it is not included. A test pins the decline with that reasoning in its doc comment.
Verification
front::slice_index34 passed / 0 failed; full lib 3170 passed / 0 failed / 33 ignored; the--ignoredanchor 2 passed / 0 failed;cargo fmt --checkclean.PYRE_RTYPER skip), working tree vs.this base:
NEWLY BAD 0 / 0 / 0,NEWLY GOOD 0 / 0 / 0.A measurement caveat worth recording
While validating the above I found that the census is not stable for one row,
pyre_interpreter::display::<Impl>::push_onto: on a byte-identical tree it scoredphaseB 15onceand
phaseB 14eight times across independent runs (fresh--forceLLBC extraction, fresh build,and a full re-run of the A/B harness). A single-observation 1-subject delta on that row is therefore
not evidence either way, and any earlier claim in this fold's history that rests on it — in either
direction — should be re-measured with at least two independent arm runs before being relied on.
Summary by CodeRabbit
Bug Fixes
Tests