fix(codegen): class-typed nullish field read throws TypeError (#7153) - #7430
Merged
Conversation
added 2 commits
August 5, 2026 11:06
The class-field guard diamond's fallback funneled a nullish receiver into js_object_get_field_by_name_f64, which answers undefined - so a field read through a class-annotated binding holding undefined/null kept running on a silent wrong value where Node throws TypeError. Mirror the generic path's nullish check (#462) on the cold fallback arm of both the value-context diamond (property_get.rs) and the raw-f64 number-context variant (property_get/helpers.rs), and in their runtime full-outline js_class_field_get_ic (#5391).
📝 WalkthroughWalkthroughClass-typed field-read fallbacks now check for ChangesNullish class-field access
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/typed_feedback/guards.rs`:
- Around line 764-781: Move the js_typed_feedback_record_fallback_call(site_id)
invocation in the outlined property-get path to after the undefined/null check,
so nullish receivers throw without recording fallback feedback. Keep recording
feedback immediately before the dynamic by-name lookup, matching the inline
paths in property_get and its helpers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de50ec7e-ed07-444d-b6e6-bb5645360395
📒 Files selected for processing (6)
changelog.d/7430-class-typed-nullish-field-read.mdcrates/perry-codegen/src/expr/property_get.rscrates/perry-codegen/src/expr/property_get/helpers.rscrates/perry-runtime/src/typed_feedback/guards.rstest-files/test_gap_7153_class_typed_nullish_field_read.tstest-files/test_gap_repsel_ptr_shape_elements.ts
Comment on lines
+764
to
+781
| // #7153: this function is the full-outline of the codegen class-field-get | ||
| // diamond (#5391), so it must mirror the diamond's nullish-receiver check — | ||
| // a field read on undefined/null throws TypeError instead of answering | ||
| // `undefined` through the by-name lookup. | ||
| let key_raw = key as u64 & crate::value::POINTER_MASK; | ||
| if obj_bits == crate::value::TAG_UNDEFINED || obj_bits == crate::value::TAG_NULL { | ||
| let name = unsafe { | ||
| crate::object::has_own_helpers::str_from_string_header( | ||
| key_raw as *const crate::StringHeader, | ||
| ) | ||
| } | ||
| .unwrap_or(""); | ||
| crate::error::js_throw_type_error_property_access( | ||
| (obj_bits == crate::value::TAG_NULL) as u32, | ||
| name.as_ptr(), | ||
| name.len(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep outlined fallback feedback consistent with inline lowering.
Move js_typed_feedback_record_fallback_call(site_id) after the nullish check. The inline paths in crates/perry-codegen/src/expr/property_get.rs and crates/perry-codegen/src/expr/property_get/helpers.rs record feedback only when dynamic lookup runs. The outlined path currently records a fallback for a throwing nullish access. This makes feedback state depend on PERRY_FULL_OUTLINE_IC.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/typed_feedback/guards.rs` around lines 764 - 781,
Move the js_typed_feedback_record_fallback_call(site_id) invocation in the
outlined property-get path to after the undefined/null check, so nullish
receivers throw without recording fallback feedback. Keep recording feedback
immediately before the dynamic by-name lookup, matching the inline paths in
property_get and its helpers.
proggeramlug
added a commit
that referenced
this pull request
Aug 6, 2026
…ed one failure into five (#7490) (#7492) * fix(test): isolate typed_feedback failures — poisoned ENV_LOCK cascaded one failure into five (#7490) `cargo test -p perry-codegen --test typed_feedback` reported five failures as a suite and a wobbling set under default parallelism. The suspected cause was process-global codegen state leaking between in-process compiles. It was not. Two assertions had genuinely drifted from intentional codegen changes, and both fail when run ALONE (contrary to the issue's premise): * `typed_feedback_guards_direct_class_field_specialization` matched the numeric coercion in the textual window between the `class_field_get_number.fallback` and `.merge` labels. #7430 split that arm: `.fallback` now holds only the nullish-receiver check, and the by-name load plus coercion moved to `.fallback_lookup` — a block RENDERED AFTER `.merge`. The window could never match again. * `typed_feedback_trace_dump_runs_before_entry_return` cut `main`'s body at the literal header `define i32 @main() {`. Since #7370 made native roots the default every emitted function carries `"frame-pointer"="non-leaf"`, so that header never matches. The first of those panics while holding `ENV_LOCK`, which poisons the mutex for the rest of the process; every later `ENV_LOCK.lock().unwrap()` then dies with `PoisonError` regardless of its own subject. That is the whole of the "order dependence": under `--test-threads=1` the alphabetically-early poisoner takes three healthy tests down with it, and under default parallelism the victim set shifts with the scheduler. Fix, in three parts: * `env_lock()` recovers a poisoned guard. Sound because each test declares its `EnvVarGuard` after the lock guard, so the env var is restored during unwind before the mutex is released — the protected state is already consistent at poison time. One test's failure must fail that test alone. * Both drifted assertions are re-pointed at the current IR and made STRONGER, not looser. The class-field one now proves the data flow the positional window stood in for, end to end: `.fallback_lookup` records the fallback, loads by name and coerces; its terminator branches to the numeric merge; and the merge phi's fallback incoming IS the coerced register. `entry_fn_body` matches the exact signature and cuts at that line's opening brace, so unrelated attribute changes can no longer fail the test. * A sabotage test plants the exact #7490 shape — an unwind out of a lock-holding test — asserts it really poisoned the mutex, and demands the accessor still hands out a guard. It fails against the pre-fix `.lock().unwrap()` (9 of 16 red), so a green run is evidence, not decoration. No production codegen state leaks between compiles: `PERRY_TYPED_FEEDBACK` is read live at each call site and `PERRY_FULL_OUTLINE_IC`'s decision is a thread-local set once per `compile_module` — both already correct. * docs(changelog): add fragment for #7490 typed_feedback test isolation * docs(changelog): point the #7490 fragment at the split follow-ups with measured sweep data * chore: bump version to 0.5.1285 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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.
Fixes #7153.
Problem
A field read through a class-annotated binding whose runtime value is nullish returned
undefinedinstead of throwing, so the program kept running on a silent wrong value where Node aborts:An
any-typed binding was unaffected (the generic dispatch path has thrown correctly since #462) — confirming the issue's hypothesis that the typed fast path was the culprit.Root cause
refine_type_from_initrefines the element of aRow[]index read toNamed("Row"), so the read lowers to the class-field guard diamond. The inline shape pre-check andjs_typed_feedback_class_field_get_guardboth correctly reject a non-pointer receiver — but the fallback arm funneled straight intojs_object_get_field_by_name_f64, which answersundefinedfor any unrecognized receiver. No path in the diamond distinguished "shape miss" from "nullish receiver must throw".Fix
Mirror the generic path's nullish check (
js_throw_type_error_property_access, #462) at the head of the cold fallback arm — the fast path is untouched — in all three variants of the diamond:crates/perry-codegen/src/expr/property_get.rs— the value-context class-field GET diamondcrates/perry-codegen/src/expr/property_get/helpers.rs— the raw-f64 number-context variant (which previously coercedundefinedtoNaNand kept going)crates/perry-runtime/src/typed_feedback/guards.rs—js_class_field_get_ic, the codegen: split large modules into parallel codegen units (bound clang peak memory; remove the single-TU wall) #5391 full-outline of the same diamond for oversized modulesOut of scope (verified already correct): class-typed property set on nullish throws, method calls on nullish throw,
any-typed reads throw.Validation
test-files/test_gap_7153_class_typed_nullish_field_read.ts: OOB value context, number context, string field, null (not just undefined) receiver with thenullwording, and an in-bounds control — output byte-identical to Node 26.5.1 locally.test_gap_repsel_ptr_shape_elements.ts(the file that had to drop this case in perf(repsel): open the array-element escape forPtr<Shape>(#7034 §3) #7149) still byte-identical to Node; its stale "NOT covered" note now points at the new test.cargo test -p perry-codegen --lib: 624 passed.cargo test -p perry-runtime --lib typed_feedback: 49 passed.check_file_size.shcap andaddr_class_inventory.pyratchet both pass.No version bump per contributor guidelines — maintainer bumps at merge.
Summary by CodeRabbit
Bug Fixes
nullorundefinednow correctly throw aTypeError.Tests