Skip to content

fix(release): clear field-shadow and WebAssembly parity blockers - #9242

Merged
proggeramlug merged 4 commits into
mainfrom
fix/release-parity-blockers
Aug 31, 2026
Merged

fix(release): clear field-shadow and WebAssembly parity blockers#9242
proggeramlug merged 4 commits into
mainfrom
fix/release-parity-blockers

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Fixes #9240.
Fixes #9241.

Clears the two new failures from the v0.5.1519 full CI run:

  • guarded direct method calls now fall back to own-property lookup when the class or an ancestor declares a same-named instance field (and conservatively for computed fields), preserving JavaScript field-over-prototype semantics;
  • the default-runtime WebAssembly fixture now computes its namespace key at runtime, preventing constant folding from accidentally enabling the real wasm host.

Release evidence on the exact candidate base 01463b81610f089cc566011d16ea5e5b08ec70a8:

  • test_issue_945_scalar_method_guards: PASS, 100% parity
  • test_parity_webassembly_graceful_fail_default: PASS, 100% expected-output parity
  • scripts/run_issue_945_scalar_method_ir_guard.sh: PASS (safe scalar fast path retained)
  • cargo fmt --check --all: PASS

Failed release jobs:

Summary by CodeRabbit

  • Bug Fixes
    • Improved method dispatch when inherited or computed instance fields share a name with a method.
    • Prevented direct method calls from bypassing own-property overrides, ensuring the correct implementation is selected.
    • Improved compatibility for applications that use fields and methods with the same name across class hierarchies.
    • Corrected WebAssembly feature detection in the default runtime, restoring reliable graceful degradation when the feature is unavailable.

@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 17bb9638-97f7-4fb8-8e6f-dab6fec4acd1

📥 Commits

Reviewing files that changed from the base of the PR and between 7acf3c7 and 2426e1c.

📒 Files selected for processing (1)
  • crates/perry-codegen/tests/native_proof_regressions.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Parity blocker fixes

Layer / File(s) Summary
Shadow detection and guarded-call fallback
crates/perry-codegen/src/lower_call/method_override.rs, crates/perry-codegen/tests/native_proof_regressions.rs
The code generator scans the inheritance chain for matching or computed instance fields. It skips guarded direct calls when a field may shadow the method. The regression test checks the own-field lookup and value-call fallback.
Runtime namespace lookup and release notes
test-files/test_parity_webassembly_graceful_fail_default.ts, changelog.d/9242-release-parity-blockers.md
The fixture builds the WebAssembly namespace key with join("") and explains why string concatenation must not be constant-folded. The changelog records both behavioral fixes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 2426e

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: jdalton, thehypnoo

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies both release-blocking fixes: field-shadow handling and WebAssembly parity behavior.
Description check ✅ Passed The description explains the purpose, lists the concrete changes, references issues #9240 and #9241, and provides test evidence. It omits the template headings and checklist, but the required informat…
Linked Issues check ✅ Passed The changes satisfy the coding objectives for [#9240] and [#9241]. The method lowering now detects same-class, inherited, and computed-field shadows and falls back safely while preserving the scalar f…
Out of Scope Changes check ✅ Passed All changes are related to the linked release blockers. The code, regression test, parity fixture, explanatory comment, and changelog fragment support the objectives for [#9240] and [#9241].
Docstring Coverage ✅ Passed 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 …
Full details: Description check

Explanation

The description explains the purpose, lists the concrete changes, references issues #9240 and #9241, and provides test evidence. It omits the template headings and checklist, but the required information is mostly complete.

Full details: Linked Issues check

Explanation

The changes satisfy the coding objectives for [#9240] and [#9241]. The method lowering now detects same-class, inherited, and computed-field shadows and falls back safely while preserving the scalar fast path. The WebAssembly fixture now uses runtime namespace construction, updates the explanation, and preserves graceful-failure coverage. The reported parity tests and formatting check pass.

Full details: Docstring Coverage

Explanation

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
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/release-parity-blockers

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

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. perry-codegen --test native_proof_regressions scalar_method_boolean_predicate_rejects_mutation_call_accessor_and_dynamic_property fails on the inherited_field_shadow case:

inherited_field_shadow must keep dynamic method dispatch fallback

A/B on the same tree, only method_override.rs swapped:

  • main: test result: ok. 3 passed, exit 0
  • this PR: exit 101

The assertion wants call double @js_native_call_method in the emitted IR. With your early return the module instead emits js_object_get_field_by_name_f64 + js_native_call_value — a lookup-then-call rather than a single dynamic dispatch. Both are dynamic, so this may well be a case of the proof test pinning one specific safe form and your change picking a different safe form. But that IR also contains a direct call double @perry_method_…__Point__isAbove, and I could not establish whether that is a second call site in the fixture or the scalar fast path still being taken. That guard exists precisely to assert the fast path isn't taken for these cases, so I'm not willing to widen the assertion on the assumption it's the former.

The specific question for you: with emit_guarded_direct_method_call returning None, does the caller reach the same safety point it reached before, or a different one that happens to be correct for a different reason? If the former, the test's assertion should accept either dynamic form and this is ready. If the latter, that's worth understanding before it lands in a release.

What checks out. The behaviour is right, and I verified it independently rather than taking the issue's word:

case main this PR node 26.5.1
class A { m = () => … } field-A ✓ field-A ✓ field-A
class C extends B { m = … } over B.m() method-B ✗ field-C ✓ field-C
same, 200-iteration loop (guarded path) method-B ✗ field-C ✓ field-C
class D extends B {} (no shadow) method-B ✓ method-B ✓ method-B
computed ["m"] = … computed-E ✓ computed-E ✓ computed-E
private #m + public m() method-F:1 ✓ method-F:1 ✓ method-F:1
field on a grandparent (I extends H extends G) field-G ✓ field-G ✓ field-G
runtime b.m = … assignment own-assigned ✓ own-assigned ✓ own-assigned

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.

scripts/run_issue_945_scalar_method_ir_guard.sh also passes, so the scalar fast path is retained where it should be — the conservative bail isn't over-broad.

Two smaller notes.

The class_chain_may_declare_method_field design is the right shape: every uncertain case (cycle, unresolved class, computed key, 64-deep cap) returns true and disables the optimization, so it fails safe. Worth keeping that property explicit in a comment — it's the kind of thing a later "simplification" quietly inverts.

The WebAssembly fixture is at exactly its baseline cap: max_diff_lines: 11 and it produces 11. Within budget, but with zero headroom, so any future line added to that fixture's output turns parity_matrix_baseline.json red without anyone changing behaviour. Might be worth raising the cap or trimming a line while you're here.

Validation run: perry-runtime 2867 passed / 0 failed at RUST_TEST_THREADS=1; perry-codegen 14 suites clean and the one above failing.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

The 4 gap-suite (1)/(2) failures on this PR are not caused by it — they are pre-existing on main, tracked as #9244.

Verified locally with a 4-arm A/B on the same build and harness config (perry-dev, PERRY_SKIP_BUILD=1, PERRY_NO_AUTO_OPTIMIZE=1, node 26.5.1):

test this PR PR + narrowed predicate 89cde4ff14 (main, PR reverted) a722b4cca2 (pre-#9169)
test_gap_iterator_helpers_2874 FAIL FAIL FAIL PASS
test_gap_language_types_object_part_a FAIL FAIL FAIL PASS
test_gap_object_string_wrappers FAIL FAIL FAIL PASS
test_gap_5592_class_expr_rebind_computed_accessor FAIL FAIL FAIL PASS
test_issue_945_scalar_method_guards (#9240) PASS PASS FAIL FAIL
test_parity_webassembly_graceful_fail_default (#9241) PASS PASS PASS PASS

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 test_issue_945_scalar_method_guards FAIL → PASS, which is exactly what #9240 asks for.

The four regress at #9169 and pass at its parent (#9244). They reached main because gap-suite-build on #9169 was cancelled, so every dependent gap shard was skipped.

The two self-test-checker reds are also cancellations, not failures.

One follow-up worth filing (not blocking)

class_chain_may_declare_method_field returns true for a receiver class that is not in ctx.classes, and ctx.classes holds only user-declared classes. Every builtin receiver (Object, Iterator, Number) and every anonymous class expression therefore takes the bail-out, losing the guarded direct-method fast path — a perf deopt on a very hot path, not a correctness issue. The pre-existing canonical_shape_excludes_own_property (dynamic_dispatch.rs:41) handles the same question in the safe polarity: unknown class → "cannot prove" → retain the runtime own-property probe, rather than abandoning the fast path. Narrowing to ctx.classes.get(..) else { return false } plus field.key_expr.is_none() measured identical on all 6 tests above, so it is a clean follow-up rather than a change this PR needs.

@proggeramlug
proggeramlug force-pushed the fix/release-parity-blockers branch from 2426e1c to 7160a34 Compare August 31, 2026 07:40
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Rebased onto main @ fad4cfb460, which now carries the #9244 fix (#9247).

The 4 gap-suite failures previously attributed to this PR were pre-existing on main — regressions from #9169, which merged with its gap shards cancelled. With #9247 landed they should clear, and this PR's own content is unchanged by the rebase (still exactly the same 4 files, 58 insertions / 4 deletions).

For the record, #9247's CI confirmed the diagnosis: gap-suite (1) and (2) — the two shards that reported these failures — are green on main now, as is e2e-scoped.

@proggeramlug
proggeramlug merged commit 8375481 into main Aug 31, 2026
52 checks passed
@proggeramlug
proggeramlug deleted the fix/release-parity-blockers branch August 31, 2026 08:27
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Correction to my earlier comment on this PR.

I stated that guard_failures_match_node_and_unsafe_parameters_stay_generic (crates/perry/tests/issue_8774_argument_shape_clones.rs) was fallout from this PR. That was wrong, and I am retracting it.

My evidence was a window correlation: the test printed ... ok at 01463b8161, FAILED at 83754818ea, and this PR was the only commit touching method_override.rs between them. I called that "confirmed" when it was a hypothesis.

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):

class_chain_may_declare_method_field bail result
as merged FAILED — all selected semantic-fixture routes originate at fresh contained locals
narrowed to visible declarations only FAILED, same assertion
fully disabled (pre-#9242 behaviour) FAILED, same assertion

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 → false, requiring key_expr.is_none()). That change is no longer justified by this evidence and I am not pursuing it — the over-broad arms remain a reasonable cleanup on their own merits, but nothing measured here requires them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant