fix(release): clear field-shadow and WebAssembly parity blockers - #9242
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe code generator now avoids direct method guards when instance fields may shadow the method. The regression test checks own-field lookup and value invocation. The WebAssembly parity fixture now computes its namespace key at runtime. The changelog records both fixes. ChangesParity blocker fixes
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR corrects field-shadow method dispatch and default-runtime WebAssembly parity behavior, with the reported release checks passing. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description explains the purpose, lists the concrete changes, references issues Full details: Linked Issues checkExplanation The changes satisfy the coding objectives for [ Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 too large.) ✨ 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 |
|
Not merging yet — this regresses a pre-existing native-proof guard, and the PR's cited evidence is a different test. The behaviour change itself looks right; it's the IR proof that moved. The regression. A/B on the same tree, only
The assertion wants The specific question for you: with What checks out. The behaviour is right, and I verified it independently rather than taking the issue's word:
So the fix does what it says, the private-field and no-shadow cases are correctly left alone, and the loop rows confirm it holds on the guarded direct-call path rather than only the first call.
Two smaller notes. The The WebAssembly fixture is at exactly its baseline cap: Validation run: |
|
The 4 Verified locally with a 4-arm A/B on the same build and harness config (
Column 3 is this PR's codegen change fully reverted on top of its own merge-base: all four still fail, so the PR causes none of them. Its only effect on this set is The four regress at #9169 and pass at its parent (#9244). They reached The two One follow-up worth filing (not blocking)
|
2426e1c to
7160a34
Compare
|
Rebased onto The 4 For the record, #9247's CI confirmed the diagnosis: |
|
Correction to my earlier comment on this PR. I stated that My evidence was a window correlation: the test printed Measured since, on a coherent tree (static wrappers rebuilt at the tested commit — my first two attempts were invalid, one from a stale runtime archive that failed the compiler/runtime coherence check in 19s rather than running):
The last row is decisive: the test fails identically with this PR's change present, narrowed, or absent. This PR does not cause it. The cause lies elsewhere in main's churn between the two candidates, and the failure is currently unattributed. I had also proposed narrowing the predicate (unknown class → |
Fixes #9240.
Fixes #9241.
Clears the two new failures from the v0.5.1519 full CI run:
Release evidence on the exact candidate base
01463b81610f089cc566011d16ea5e5b08ec70a8:test_issue_945_scalar_method_guards: PASS, 100% paritytest_parity_webassembly_graceful_fail_default: PASS, 100% expected-output parityscripts/run_issue_945_scalar_method_ir_guard.sh: PASS (safe scalar fast path retained)cargo fmt --check --all: PASSFailed release jobs:
Summary by CodeRabbit