Representation-aware type lowering + native-ABI material evidence gate#5466
Draft
proggeramlug wants to merge 23 commits into
Draft
Representation-aware type lowering + native-ABI material evidence gate#5466proggeramlug wants to merge 23 commits into
proggeramlug wants to merge 23 commits into
Conversation
* type lowering integration baseline * format type lowering baseline * Add packed i32 array loop guard * guard packed i32 loop helper symbol * Broaden typed ABI clone reps * materialize scalar receivers with typed slots * report typed path decisions in lowering artifacts * harden native abi evidence packet flags * Specialize numeric-key map string values * add async i32 slots and packed u32 loop facts * harden material type lowering evidence gate * Broaden typed f64 clones for raw i32 locals * Extend scalar method summaries through local temps * Lower Set string values through StringRef * Lower small BigInt literals to native i128 * lower packed i32 array store loops * Harden native ABI evidence gates * Land type lowering material evidence gate
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds typed clone dispatch for functions, methods, and closures; expands representation-first lowering and fact tracking; switches boxed and captured values to bit-based APIs; broadens array and collection specialization; and updates CI evidence, benchmarks, and docs. ChangesTyped ABI and native representation rollout
Sequence Diagram(s)sequenceDiagram
participant compile_module
participant typed_clone
participant call_lowering
participant runtime_fallback
compile_module->>typed_clone: classify eligible functions/methods/closures
call_lowering->>typed_clone: emit guards and call typed body
call_lowering->>runtime_fallback: call generic body when guards fail
typed_clone-->>call_lowering: return typed result
runtime_fallback-->>call_lowering: return generic result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/perry-codegen/src/lower_call/field_init.rs (1)
233-264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the patched
thisslot aligned with closure layout.The general closure layout reserves
new.targetbeforethis, but this field-initializer patch still writesthisatauto_caps.len(). If the initializer closure captures bothnew.targetandthis, this overwrites thenew.targetslot and leaves the realthisslot unpatched.🐛 Proposed fix
if let Expr::Closure { params: cparams, body: cbody, captures: ccaps, + captures_new_target, captures_this: true, .. } = &init_expr { let auto_caps = crate::type_analysis::compute_auto_captures(ctx, cparams, cbody, ccaps); - let this_idx = auto_caps.len() as u32; + let this_idx = (auto_caps.len() + usize::from(*captures_new_target)) as u32;🤖 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-codegen/src/lower_call/field_init.rs` around lines 233 - 264, The calculation of this_idx is incorrectly using auto_caps.len() without accounting for the new.target slot that precedes this in the closure layout. When the closure captures both new.target and this, the this slot should be positioned after new.target, not at auto_caps.len(). Update the this_idx calculation to check whether new.target is captured (via the closure's captures_new_target field or similar indicator), and if so, increment the index by 1 to account for the new.target slot position before this in the closure layout.crates/perry-codegen/src/expr/buffer_access.rs (1)
203-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the non-native fallback before forcing
ExpectedNativeRep::I32/U32.Lines 215 and 217 are identical, and lines 234 and 236 are identical, so
can_lower_expr_as_i32no longer gates anything. This also makes the range-only path from line 251 unsafe for typed stores: it can admit an expression that is range-proven but not native-lowerable, then lines 671/680/689/697 still forcelower_expr_native.🐛 Proposed direction
fn lower_index_i32_value(ctx: &mut FnCtx<'_>, index: &Expr) -> Result<LoweredValue> { - let value = if can_lower_expr_as_i32( + let can_lower_native_i32 = can_lower_expr_as_i32( index, &ctx.i32_counter_slots, ctx.flat_const_arrays, &ctx.array_row_aliases, ctx.native_facts.integer_locals(), ctx.clamp3_functions, ctx.clamp_u8_functions, ctx.integer_returning_functions, ctx.i32_identity_functions, - ) { + ); + let value = if can_lower_native_i32 { lower_expr_native(ctx, index, crate::native_value::ExpectedNativeRep::I32)?.value } else { - lower_expr_native(ctx, index, crate::native_value::ExpectedNativeRep::I32)?.value + let value = lower_expr(ctx, index)?; + ctx.block().fptosi(DOUBLE, &value, I32) }; Ok(LoweredValue::i32(value)) } fn lower_value_i32(ctx: &mut FnCtx<'_>, value: &Expr) -> Result<String> { - if can_lower_expr_as_i32( + let can_lower_native_i32 = can_lower_expr_as_i32( value, &ctx.i32_counter_slots, ctx.flat_const_arrays, &ctx.array_row_aliases, ctx.native_facts.integer_locals(), ctx.clamp3_functions, ctx.clamp_u8_functions, ctx.integer_returning_functions, ctx.i32_identity_functions, - ) { + ); + if can_lower_native_i32 { Ok(lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::I32)?.value) } else { - Ok(lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::I32)?.value) + let value = lower_expr(ctx, value)?; + Ok(ctx.block().fptosi(DOUBLE, &value, I32)) } }Then route range-only integer typed-array stores through the same fallback helper instead of directly calling
lower_expr_native(... I32/U32).Also applies to: 657-698
🤖 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-codegen/src/expr/buffer_access.rs` around lines 203 - 253, The functions lower_index_i32_value and lower_value_i32 have identical if and else branches that both call lower_expr_native with ExpectedNativeRep::I32, making the can_lower_expr_as_i32 check ineffective. Additionally, can_lower_integer_typed_array_store_value allows range-only expressions to bypass native-lowering validation, which can cause unsafe forcing of lower_expr_native at the call sites. Fix lower_index_i32_value and lower_value_i32 by implementing a proper fallback path in the else branch instead of duplicating the native lowering call, and route range-only expressions in can_lower_integer_typed_array_store_value through a fallback helper function rather than directly forcing ExpectedNativeRep::I32 or U32 lowering.crates/perry-codegen/src/expr/index.rs (1)
222-259:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the native evidence record aligned with the emitted store.
The numeric fast path now canonicalizes and directly stores raw
f64, but the evidence record still reportsjs_array_numeric_set_f64_unboxedand records the pre-canonicalizedval_double. That can make the native-representation evidence packet misrepresent this site.Proposed metadata alignment
// FASTEST: in-bounds path. Store directly, jump to merge. ctx.current_block = inbounds_idx; + let mut numeric_store_value_for_record = val_double.to_string(); { let blk = ctx.block(); let (element_addr, element_ptr) = element_slot(blk, &arr_handle, &idx_i32); if require_numeric_layout { let numeric_value = canonicalize_raw_f64_numeric_store_value(blk, val_double); + numeric_store_value_for_record = numeric_value.clone(); blk.store(DOUBLE, &numeric_value, &element_ptr); } else { @@ let stored = LoweredValue { semantic: SemanticKind::JsNumber, rep: NativeRep::F64, llvm_ty: DOUBLE, - value: val_double.to_string(), + value: numeric_store_value_for_record, }; ctx.record_lowered_value_with_access_mode_and_facts( "NumericArrayIndexSet", Some(local_id), - "js_array_numeric_set_f64_unboxed", + "array_index_set.raw_f64_direct_store", &stored,🤖 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-codegen/src/expr/index.rs` around lines 222 - 259, In the numeric fast path where the array element is stored, the code canonicalizes the raw f64 value using canonicalize_raw_f64_numeric_store_value and stores the numeric_value, but the evidence record created in record_lowered_value_with_access_mode_and_facts still references the pre-canonicalized val_double and reports js_array_numeric_set_f64_unboxed. Update the evidence record to use the actual canonicalized numeric_value that was stored instead of val_double, and adjust the evidence string to accurately reflect the canonicalized store operation that was emitted.
🧹 Nitpick comments (6)
crates/perry-codegen/src/boxed_vars.rs (1)
1463-1465: 💤 Low valueMinor:
Stmt::Thrownot walked for closure expressions.The expression extraction at line 1463 handles
Stmt::Expr,Stmt::Return, andStmt::Let, but missesStmt::Throw(e). If a throw expression contained a closure that defined async control locals, those wouldn't be collected.This is likely safe in practice since
throwexpressions rarely contain closure definitions, but for completeness:Suggested fix
- if let Stmt::Expr(e) | Stmt::Return(Some(e)) | Stmt::Let { init: Some(e), .. } = s { + if let Stmt::Expr(e) | Stmt::Throw(e) | Stmt::Return(Some(e)) | Stmt::Let { init: Some(e), .. } = s { collect_compiler_private_async_control_locals_in_expr(e, i32_out, i1_out); }🤖 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-codegen/src/boxed_vars.rs` around lines 1463 - 1465, The pattern match in the if let statement starting with Stmt::Expr is missing Stmt::Throw(e) from its alternatives, which means throw expressions containing closures with async control locals would not be processed. Add Stmt::Throw(e) as an additional alternative in the pattern match alongside Stmt::Expr, Stmt::Return, and Stmt::Let so that the collect_compiler_private_async_control_locals_in_expr function is called for throw expressions as well.crates/perry-codegen/src/collectors/escape_check.rs (1)
433-467: 💤 Low valueRedundant re-match on callee.
The outer match at line 440 already destructures
Expr::PropertyGet { object, .. }, but then line 443-444 re-matches callee to extractproperty. This could be simplified by capturingpropertyin the outer match.Suggested simplification
Expr::Call { callee, args, .. } => { // Method-call form: `local.method(...)` needs a real heap `this` // pointer unless a conservative method summary proves that the // body is just a numeric expression over `this.field`, literals, // and fixed numeric params. That summary lets codegen inline the // body against scalar field slots instead of dispatching with a // heap receiver. - if let Expr::PropertyGet { object, .. } = callee.as_ref() { + if let Expr::PropertyGet { object, property } = callee.as_ref() { if let Expr::LocalGet(id) = object.as_ref() { if candidates.contains_key(id) { - let is_summarized = if let Expr::PropertyGet { property, .. } = - callee.as_ref() - { - candidates.get(id).is_some_and(|class_name| { - crate::collectors::simple_scalar_method_summary( - classes, - class_name, - property, - args.len(), - ) - .is_some() - }) - } else { - false - }; + let is_summarized = candidates.get(id).is_some_and(|class_name| { + crate::collectors::simple_scalar_method_summary( + classes, + class_name, + property, + args.len(), + ) + .is_some() + }); if !is_summarized { escaped.insert(*id); } } } }🤖 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-codegen/src/collectors/escape_check.rs` around lines 433 - 467, The code in the Expr::Call handler performs redundant matching on the callee variable. The outer if-let statement already matches Expr::PropertyGet on callee but then immediately re-matches it at line 443 to extract the property field. Update the outer match pattern to capture property directly by changing it from Expr::PropertyGet { object, .. } to Expr::PropertyGet { object, property, .. }, then remove the inner if-let block and use the captured property variable directly in the candidates.get(id).is_some_and() closure..github/workflows/test.yml (1)
546-608: 💤 Low valueStatic analysis flags apply to existing workflow patterns.
The zizmor hints (unpinned-uses, cache-poisoning, artipacked) apply to this new job, but the same patterns exist throughout this workflow file. If the team decides to address these security hygiene items, it should be done holistically across the entire workflow, not just for this new job.
For reference, pinning to commit SHAs and setting
persist-credentials: falseon checkout would address the flagged issues.🤖 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 @.github/workflows/test.yml around lines 546 - 608, The native-abi-evidence-packet job uses several GitHub actions that are not pinned to specific commit SHAs, which creates security vulnerabilities around unpinned-uses and cache-poisoning as flagged by zizmor. Pin each action to its specific commit SHA by replacing version tags (like `@v6`, `@v0.0.10`, `@v4`, `@v7`) with full commit hashes for actions/checkout, dtolnay/rust-toolchain, mozilla-actions/sccache-action, actions/cache, Swatinem/rust-cache, and actions/upload-artifact. Additionally, add persist-credentials: false to the actions/checkout step to prevent credential exposure. Note that similar unpinned patterns exist throughout the entire workflow file and should ideally be addressed holistically across all jobs.Source: Linters/SAST tools
crates/perry-codegen/src/codegen/typed_abi.rs (1)
519-523: 💤 Low valueRedundant disjunction in mutable capture check.
The condition
!mutable_captures.is_empty() || captures.iter().any(|id| mutable_captures.contains(id))has redundant logic: ifmutable_capturesis empty, the second operand will always be false (no ID can be in an empty set). The second operand only adds value ifmutable_capturescontains IDs that are NOT incaptures.If the intent is simply to reject any closure with mutable captures,
!mutable_captures.is_empty()alone suffices. If the intent is to check overlap betweencapturesandmutable_captures, only the second operand is needed.🤖 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-codegen/src/codegen/typed_abi.rs` around lines 519 - 523, The condition checking mutable captures contains redundant logic that should be simplified. In the if statement where you're checking for mutable captures before the TypedCloneRejectionReason::Captures return, the first part `!mutable_captures.is_empty()` combined with the second part `captures.iter().any(|id| mutable_captures.contains(id))` is redundant because if mutable_captures is empty the second condition will always be false. Determine the intended behavior: if you only need to reject closures that have any mutable captures at all, keep only `!mutable_captures.is_empty()`. If you only need to reject when there's an overlap between the captures set and mutable_captures set, keep only `captures.iter().any(|id| mutable_captures.contains(id))`. Simplify the condition to reflect the actual intent without the redundant operand.crates/perry-codegen/src/expr/bigint_set.rs (1)
97-298: 🏗️ Heavy liftFactor the typed Set dispatch into a shared operation helper.
SetAdd,SetHas, andSetDeletenow duplicate the same receiver classification, native value lowering, helper selection, guarded-number fallback, and telemetry wiring. A smallSetOpdispatch table/helper would reduce drift as more native reps are added.Also applies to: 602-856, 884-1104, 1119-1339
🤖 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-codegen/src/expr/bigint_set.rs` around lines 97 - 298, The three functions guarded_set_number_add, guarded_set_number_has, and guarded_set_number_delete contain nearly identical code for guard checking, block creation, fast path native calling, and fallback path handling. Extract the common dispatcher logic into a shared helper function that takes operation-specific parameters (operation name, native function name, generic function name, return type, and telemetry parameters). Refactor all three functions to call this shared helper instead of duplicating the guard dispatch, block management, and phi node creation code. This will reduce code drift as new native representations are added.crates/perry-codegen/src/expr/arrays_finds.rs (1)
72-105: ⚡ Quick winMove the integer-index proof into one shared helper.
The same
numeric_index_has_integer_array_index_proof/ range / mask proof shape is referenced in the array index get/set modules too. Keeping a private copy here makes Uint8Array and generic index fast-path eligibility easy to drift apart.🤖 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-codegen/src/expr/arrays_finds.rs` around lines 72 - 105, The numeric_index_has_integer_array_index_proof function and its nested range_is_nonnegative_i32 helper contain proof logic that is duplicated across the array index get and set modules. Extract this shared proof logic into a single, reusable helper function that can be referenced from all locations where integer array index validation is needed (Uint8Array fast-path eligibility and generic index validation). This will ensure the proof shape remains consistent across all modules and prevent the logic from drifting apart. Identify all locations referencing this pattern in the get/set modules and update them to use the new shared helper.
🤖 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 `@benchmarks/compiler_output/fixtures/h1_buffer_alias_negative.ts`:
- Around line 45-46: In the loop at lines 45-46, the code directly reads
`owned[i]` but should instead use a closure-captured `read` closure to maintain
the intended closure-capture alias-negative testing scenario. Replace the direct
`owned[i]` access in the total assignment with a call to the `read` closure that
wraps this access, ensuring the fixture properly validates the closure-capture
alias behavior as intended.
In `@benchmarks/compiler_output/README.md`:
- Around line 69-73: Add documentation to the README section describing
scripts/native_abi_evidence_packet.sh --gate to explicitly state that the --gate
flag requires a minimum of 5 runs (--runs >= 5). This requirement should be
documented clearly in the existing paragraph at line 69-73 to prevent
contributors from encountering confusing gate failures when running the command
without meeting this minimum-runs contract.
In `@crates/perry-codegen/src/codegen/closure.rs`:
- Around line 218-254: The load_typed_capture function currently assumes all
captures are stored as JSValue bits, but boxed captures are now stored as raw
box-pointer bits according to expr/closure.rs. This causes typed closures
capturing boxed locals to incorrectly convert the box pointer itself as
f64/i32/string instead of reading the actual box contents. Modify
load_typed_capture to either reject boxed captures for typed closures or add
logic to first load the box value bits from the box pointer before applying the
typed argument conversion via the emit_typed_arg_to_raw pattern. You may need to
plumb boxed-capture metadata through the typed closure compilation pipeline to
identify which captures are boxed.
In `@crates/perry-codegen/src/expr/array_push.rs`:
- Around line 428-439: After calling js_box_set_bits or
js_closure_set_capture_bits in the array_push function, emit appropriate write
barriers to track parent-to-child references. Specifically, add write barrier
calls following the js_box_set_bits invocation in both the closure capture path
(where js_closure_get_capture_bits is called) and the local slot path (where
ctx.locals.get is used), as well as in the other similar paths at lines 449-452,
494-505, and 514-517. The write barriers should register the parent object
(box_ptr or closure_ptr) and the newly allocated array pointer to ensure the
garbage collector properly tracks references when reallocations occur.
In `@crates/perry-codegen/src/expr/buffer_access.rs`:
- Around line 273-278: The current check for ClosureCapture only examines
buffer_hazard_reasons but does not directly check ctx.closure_captures. A
buffer_local_id can exist in closure_captures without an entry in
buffer_hazard_reasons, allowing unchecked proofs to be emitted. Add an
additional check to verify if buffer_local_id is present directly in
ctx.closure_captures and return Ok(None) if it is, ensuring all captured buffers
are properly gated before emitting unchecked native pointer proofs.
In `@crates/perry-codegen/src/expr/i32_fast_path.rs`:
- Around line 891-896: The NativeRep::F64 branch uses is_known_finite to
conditionally call toint32_fast, but is_known_finite only checks for
NaN/Infinity and does not verify that the value is within the i32 representable
range [-2^31, 2^31-1]. Finite values like 1e20 can pass the finite check but
still require JS ToInt32 modulo wrapping semantics that toint32_fast does not
provide. Either add an explicit i32-range proof (e.g., an integer analysis or
bound check) to confirm the lowered.value is safely representable as i32 before
calling toint32_fast, or conservatively replace the conditional and always use
ctx.block().toint32 for F64 conversions to ensure correctness.
In `@crates/perry-codegen/src/expr/index_set.rs`:
- Line 311: The PackedNumericLoopKind::U32 case at line 311 currently aborts
codegen with a bail! call instead of gracefully falling back to the existing
bounded/generic store path. Replace the bail! statement with code that falls
back to the generic path (similar to how other unimplemented packed loop kinds
should degrade gracefully). Additionally, apply the same fallback fix to the
similar U32 packed loop store case in the 760-771 range that also currently
bails out, ensuring both locations handle U32 stores by delegating to the
existing bounded/generic implementation rather than aborting.
- Around line 239-254: The TypedFeedbackContract registered in the
emit_typed_feedback_register_site call on line 243 uses array_set_index() but
the actual runtime helper invoked on line 246 is
js_typed_feedback_array_set_index_or_string, causing a mismatch between the
registered feedback contract and the actual helper being used. Create a new
constant contract method array_set_index_or_string() in the
TypedFeedbackContract that specifies the correct helper name
js_typed_feedback_array_set_index_or_string, then replace the array_set_index()
call in emit_typed_feedback_register_site with this new
array_set_index_or_string() method to align the registered contract with the
actual runtime helper.
- Around line 277-285: The Math.abs optimization in the MathAbs pattern matching
block is too broad and applies the llvm.fabs.f64 shortcut to any IndexGet from
the same array, including string keys and unbounded indices that may be
NaN-boxed. Add an additional constraint within the matches! condition to verify
that the IndexGet represents a proven packed-index load before allowing the
llvm.fabs.f64 shortcut. This check should ensure the index operation is
guaranteed to produce a raw f64 value from a packed loop fact, not a boxed or
runtime-fallback value.
In `@crates/perry-codegen/src/expr/literals_vars.rs`:
- Around line 611-613: The boxed/update write paths storing v_bits or new_bits
via js_box_set_bits are missing write barrier emissions after their calls. These
write barriers are necessary because the stored bits could contain BigInt
pointers (from js_numeric_step in increment/decrement operations) or
object/string/array bits that must be tracked by the garbage collector. Add
emit_write_barrier calls after each js_box_set_bits function call at the
identified locations (611-613, 713-715, 726-730, and 744-746) to match the
pattern already implemented in LocalSet, ensuring young children stored in boxed
values are properly tracked on the GC barrier path.
In `@crates/perry-codegen/src/expr/logical_collections.rs`:
- Around line 462-508: The issue is that when a static numeric key map is
encountered with an unproven key type, the else block containing the
`record_collection_string_key_fallback` call only records string-key fallback
facts, but it should record numeric-key fallback facts instead. In the else
block that handles the generic map delete case, add a check for
`is_static_number_key_map(ctx, map)` before calling
`record_collection_string_key_fallback`, and when true, call the appropriate
numeric-key fallback recording function instead to preserve the evidence that
the numeric-key helper fallback was actually selected.
In `@crates/perry-codegen/src/expr/math_simple.rs`:
- Around line 514-811: The code is incorrectly recording string-key fallback
facts in the generic fallback branches when dealing with static number-key maps
that have non-numeric keys. In the final else block where
record_collection_string_key_fallback is called, add a conditional check for
is_static_number_key_map(ctx, map) to determine whether to record string-key or
number-key fallback facts. When a static number-key map is present but the key
is not provably numeric, record number-key fallback facts instead of string-key
fallback facts to properly count numeric helper rejections in the evidence
stream.
- Around line 241-247: The fast-path Map helpers js_map_set_number_key,
js_map_get_number_key, js_map_has_number_key, and js_map_delete_number_key
currently canonicalize -0 to +0 using normalize_zero() but do not canonicalize
NaN values. Different NaN bitpatterns will hash to different buckets and fail
equality checks, violating SameValueZero semantics. Add NaN canonicalization
(e.g., converting any NaN to a canonical NaN representation) in all four
fast-path helpers immediately after the normalize_zero() call, and also apply
the same canonicalization to the generic Map operations js_map_set, js_map_get,
js_map_has, and js_map_delete to ensure consistent behavior across both code
paths.
In `@crates/perry-codegen/src/expr/misc_methods.rs`:
- Around line 54-55: The match expression on `lowered.rep` at lines 54-55 and
the `matches!` macro calls at lines 137, 143, and 154 are causing partial moves
of `LoweredValue` because `NativeRep` is non-Copy and contains owned `String`
fields. Fix this by using references in all pattern matching operations: change
`match lowered.rep` to `match &lowered.rep` in the main match expression, and
change all `matches!(lowered.rep, ...)` calls to `matches!(&lowered.rep, ...)`
in the subsequent conditional checks. This prevents the partial move and allows
`lowered` to be used or returned later in the function.
In `@crates/perry-codegen/src/expr/mod.rs`:
- Around line 2253-2260: The shift operations at BinaryOp::Shl, BinaryOp::Shr,
and BinaryOp::UShr are emitting LLVM shifts with unmasked RHS operands, which
causes undefined behavior when the shift count is >= 32. To match JavaScript
semantics, mask the right_i32 operand to 5 bits (using a bitwise AND with 31)
before passing it to the shl, ashr, and lshr operations. Create a masked right
operand by calling ctx.block().and(I32, &right_i32, &constant_31) and use this
masked value in all three shift operation calls.
In `@crates/perry-codegen/src/expr/property_get.rs`:
- Around line 73-80: The `js_object_get_field_by_property_id_f64` function call
in the property_get code path is passing `obj_bits` directly, which still
contains the NaN-box tag. Extract the actual unboxed pointer from `obj_bits` by
applying `POINTER_MASK_I64` (similar to how `property_id` is extracted from
`key_bits`) before passing it as the first argument to the
`js_object_get_field_by_property_id_f64` function call to ensure the runtime
receives a canonical ObjectHeader pointer instead of a tagged value.
In `@crates/perry-codegen/src/expr/range_facts.rs`:
- Around line 355-357: The invalidate_local_write_facts function only removes
the forward alias mapping for the given id but leaves reverse aliases (entries
where another variable points to id) intact. After a local variable is
reassigned, these stale reverse aliases can propagate incorrect alias and range
facts through the alias resolution chain. In addition to removing the direct
ctx.local_value_aliases.remove(&id), iterate through ctx.local_value_aliases and
remove any entries where the aliased value equals id to clean up all reverse
references pointing to the invalidated variable.
In `@crates/perry-codegen/src/lower_call/early_branches.rs`:
- Around line 357-386: The three parameter reputation blocks for f64, i32, and
string typed closures are checking their respective typed closure sets
(ctx.typed_f64_closures, ctx.typed_i32_closures, ctx.typed_string_closures) but
all incorrectly read from ctx.typed_i1_closure_param_reps. Fix this by ensuring
each block reads from its matching registry: typed_f64_param_reps should read
from ctx.typed_f64_closure_param_reps, typed_i32_param_reps should read from
ctx.typed_i32_closure_param_reps, and typed_string_param_reps should read from
ctx.typed_string_closure_param_reps.
In `@crates/perry-codegen/src/lower_call/func_ref.rs`:
- Around line 285-335: Each typed function variant (f64, i32, string, i1) is not
reading from its corresponding parameter representations map. The
typed_f64_call_param_reps block checks ctx.typed_f64_functions but reads
ctx.typed_i1_function_param_reps; the typed_i32_call_param_reps block checks
ctx.typed_i32_functions but reads ctx.typed_i1_function_param_reps; the
typed_string_call_param_reps block checks ctx.typed_string_functions but reads
ctx.typed_i1_function_param_reps. Additionally, the typed_i1_call_param_reps
block is missing the gate check ctx.typed_i1_functions.contains(fid). Fix this
by having each block read from its matching context map
(ctx.typed_f64_function_param_reps for f64, ctx.typed_i32_function_param_reps
for i32, ctx.typed_string_function_param_reps for string) and add the missing
gate to the i1 block's condition.
---
Outside diff comments:
In `@crates/perry-codegen/src/expr/buffer_access.rs`:
- Around line 203-253: The functions lower_index_i32_value and lower_value_i32
have identical if and else branches that both call lower_expr_native with
ExpectedNativeRep::I32, making the can_lower_expr_as_i32 check ineffective.
Additionally, can_lower_integer_typed_array_store_value allows range-only
expressions to bypass native-lowering validation, which can cause unsafe forcing
of lower_expr_native at the call sites. Fix lower_index_i32_value and
lower_value_i32 by implementing a proper fallback path in the else branch
instead of duplicating the native lowering call, and route range-only
expressions in can_lower_integer_typed_array_store_value through a fallback
helper function rather than directly forcing ExpectedNativeRep::I32 or U32
lowering.
In `@crates/perry-codegen/src/expr/index.rs`:
- Around line 222-259: In the numeric fast path where the array element is
stored, the code canonicalizes the raw f64 value using
canonicalize_raw_f64_numeric_store_value and stores the numeric_value, but the
evidence record created in record_lowered_value_with_access_mode_and_facts still
references the pre-canonicalized val_double and reports
js_array_numeric_set_f64_unboxed. Update the evidence record to use the actual
canonicalized numeric_value that was stored instead of val_double, and adjust
the evidence string to accurately reflect the canonicalized store operation that
was emitted.
In `@crates/perry-codegen/src/lower_call/field_init.rs`:
- Around line 233-264: The calculation of this_idx is incorrectly using
auto_caps.len() without accounting for the new.target slot that precedes this in
the closure layout. When the closure captures both new.target and this, the this
slot should be positioned after new.target, not at auto_caps.len(). Update the
this_idx calculation to check whether new.target is captured (via the closure's
captures_new_target field or similar indicator), and if so, increment the index
by 1 to account for the new.target slot position before this in the closure
layout.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 546-608: The native-abi-evidence-packet job uses several GitHub
actions that are not pinned to specific commit SHAs, which creates security
vulnerabilities around unpinned-uses and cache-poisoning as flagged by zizmor.
Pin each action to its specific commit SHA by replacing version tags (like `@v6`,
`@v0.0.10`, `@v4`, `@v7`) with full commit hashes for actions/checkout,
dtolnay/rust-toolchain, mozilla-actions/sccache-action, actions/cache,
Swatinem/rust-cache, and actions/upload-artifact. Additionally, add
persist-credentials: false to the actions/checkout step to prevent credential
exposure. Note that similar unpinned patterns exist throughout the entire
workflow file and should ideally be addressed holistically across all jobs.
In `@crates/perry-codegen/src/boxed_vars.rs`:
- Around line 1463-1465: The pattern match in the if let statement starting with
Stmt::Expr is missing Stmt::Throw(e) from its alternatives, which means throw
expressions containing closures with async control locals would not be
processed. Add Stmt::Throw(e) as an additional alternative in the pattern match
alongside Stmt::Expr, Stmt::Return, and Stmt::Let so that the
collect_compiler_private_async_control_locals_in_expr function is called for
throw expressions as well.
In `@crates/perry-codegen/src/codegen/typed_abi.rs`:
- Around line 519-523: The condition checking mutable captures contains
redundant logic that should be simplified. In the if statement where you're
checking for mutable captures before the TypedCloneRejectionReason::Captures
return, the first part `!mutable_captures.is_empty()` combined with the second
part `captures.iter().any(|id| mutable_captures.contains(id))` is redundant
because if mutable_captures is empty the second condition will always be false.
Determine the intended behavior: if you only need to reject closures that have
any mutable captures at all, keep only `!mutable_captures.is_empty()`. If you
only need to reject when there's an overlap between the captures set and
mutable_captures set, keep only `captures.iter().any(|id|
mutable_captures.contains(id))`. Simplify the condition to reflect the actual
intent without the redundant operand.
In `@crates/perry-codegen/src/collectors/escape_check.rs`:
- Around line 433-467: The code in the Expr::Call handler performs redundant
matching on the callee variable. The outer if-let statement already matches
Expr::PropertyGet on callee but then immediately re-matches it at line 443 to
extract the property field. Update the outer match pattern to capture property
directly by changing it from Expr::PropertyGet { object, .. } to
Expr::PropertyGet { object, property, .. }, then remove the inner if-let block
and use the captured property variable directly in the
candidates.get(id).is_some_and() closure.
In `@crates/perry-codegen/src/expr/arrays_finds.rs`:
- Around line 72-105: The numeric_index_has_integer_array_index_proof function
and its nested range_is_nonnegative_i32 helper contain proof logic that is
duplicated across the array index get and set modules. Extract this shared proof
logic into a single, reusable helper function that can be referenced from all
locations where integer array index validation is needed (Uint8Array fast-path
eligibility and generic index validation). This will ensure the proof shape
remains consistent across all modules and prevent the logic from drifting apart.
Identify all locations referencing this pattern in the get/set modules and
update them to use the new shared helper.
In `@crates/perry-codegen/src/expr/bigint_set.rs`:
- Around line 97-298: The three functions guarded_set_number_add,
guarded_set_number_has, and guarded_set_number_delete contain nearly identical
code for guard checking, block creation, fast path native calling, and fallback
path handling. Extract the common dispatcher logic into a shared helper function
that takes operation-specific parameters (operation name, native function name,
generic function name, return type, and telemetry parameters). Refactor all
three functions to call this shared helper instead of duplicating the guard
dispatch, block management, and phi node creation code. This will reduce code
drift as new native representations are added.
🪄 Autofix (Beta)
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: f89c17ad-b226-45b9-9eef-227f44077cf1
📒 Files selected for processing (146)
.github/workflows/test.ymlTYPE_LOWERING.mdTYPE_LOWERING_GUIDANCE.mdbenchmarks/compiler_output/README.mdbenchmarks/compiler_output/fixtures/dynamic_fractional_array_index.tsbenchmarks/compiler_output/fixtures/h1_buffer_alias_negative.tsbenchmarks/compiler_output/fixtures/loop_bound_semantics.tsbenchmarks/compiler_output/fixtures/packed_f64_loop_versioning.tsbenchmarks/compiler_output/fixtures/packed_f64_loop_versioning_negative.tsbenchmarks/compiler_output/fixtures/raw_numeric_object_fields.tsbenchmarks/compiler_output/workloads.tomlbenchmarks/suite/17_loop_data_dependent.tscrates/perry-codegen/docs/native-representation.mdcrates/perry-codegen/src/boxed_vars.rscrates/perry-codegen/src/codegen/arguments.rscrates/perry-codegen/src/codegen/artifacts.rscrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/codegen/opts.rscrates/perry-codegen/src/codegen/typed_abi.rscrates/perry-codegen/src/collectors/escape_check.rscrates/perry-codegen/src/collectors/hir_facts.rscrates/perry-codegen/src/collectors/index_uses.rscrates/perry-codegen/src/collectors/mod.rscrates/perry-codegen/src/collectors/scalar_methods.rscrates/perry-codegen/src/expr/array_push.rscrates/perry-codegen/src/expr/arrays_finds.rscrates/perry-codegen/src/expr/bigint_set.rscrates/perry-codegen/src/expr/binary.rscrates/perry-codegen/src/expr/buffer_access.rscrates/perry-codegen/src/expr/buffer_views.rscrates/perry-codegen/src/expr/call_spread.rscrates/perry-codegen/src/expr/calls.rscrates/perry-codegen/src/expr/closure.rscrates/perry-codegen/src/expr/compare.rscrates/perry-codegen/src/expr/i32_fast_path.rscrates/perry-codegen/src/expr/index.rscrates/perry-codegen/src/expr/index_get.rscrates/perry-codegen/src/expr/index_set.rscrates/perry-codegen/src/expr/instance_misc1.rscrates/perry-codegen/src/expr/js_runtime.rscrates/perry-codegen/src/expr/literals_vars.rscrates/perry-codegen/src/expr/logical_collections.rscrates/perry-codegen/src/expr/math_simple.rscrates/perry-codegen/src/expr/misc_methods.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/nanbox_inline.rscrates/perry-codegen/src/expr/native_record.rscrates/perry-codegen/src/expr/new_dynamic.rscrates/perry-codegen/src/expr/object_literal.rscrates/perry-codegen/src/expr/pod_record.rscrates/perry-codegen/src/expr/property_get.rscrates/perry-codegen/src/expr/property_set.rscrates/perry-codegen/src/expr/proxy_reflect.rscrates/perry-codegen/src/expr/range_facts.rscrates/perry-codegen/src/expr/static_method.rscrates/perry-codegen/src/expr/typed_feedback.rscrates/perry-codegen/src/expr/unary.rscrates/perry-codegen/src/expr/write_barrier.rscrates/perry-codegen/src/lower_call/console_promise.rscrates/perry-codegen/src/lower_call/early_branches.rscrates/perry-codegen/src/lower_call/field_init.rscrates/perry-codegen/src/lower_call/func_ref.rscrates/perry-codegen/src/lower_call/method_override.rscrates/perry-codegen/src/lower_call/mod.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/lower_call/property_get.rscrates/perry-codegen/src/lower_call/scalar_method.rscrates/perry-codegen/src/lower_conditional.rscrates/perry-codegen/src/native_value/artifact.rscrates/perry-codegen/src/native_value/materialize.rscrates/perry-codegen/src/native_value/mod.rscrates/perry-codegen/src/native_value/rep.rscrates/perry-codegen/src/native_value/verify.rscrates/perry-codegen/src/runtime_decls/arrays.rscrates/perry-codegen/src/runtime_decls/mod.rscrates/perry-codegen/src/runtime_decls/objects.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-codegen/src/runtime_decls/strings_part2.rscrates/perry-codegen/src/stmt/if_stmt.rscrates/perry-codegen/src/stmt/let_stmt.rscrates/perry-codegen/src/stmt/loops.rscrates/perry-codegen/src/stmt/mod.rscrates/perry-codegen/src/type_analysis.rscrates/perry-codegen/src/types.rscrates/perry-codegen/tests/native_proof_buffer_views.rscrates/perry-codegen/tests/native_proof_regressions.rscrates/perry-codegen/tests/native_proof_regressions/invalidation.rscrates/perry-codegen/tests/typed_feedback.rscrates/perry-codegen/tests/typed_shape_descriptors.rscrates/perry-runtime/src/array/header.rscrates/perry-runtime/src/array/indexing.rscrates/perry-runtime/src/array/push_pop.rscrates/perry-runtime/src/bigint.rscrates/perry-runtime/src/box.rscrates/perry-runtime/src/closure/alloc.rscrates/perry-runtime/src/closure/mod.rscrates/perry-runtime/src/closure/tests.rscrates/perry-runtime/src/gc/tests/runtime_roots/callback_scanners.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/native_abi.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/polymorphic_index.rscrates/perry-runtime/src/promise/mod.rscrates/perry-runtime/src/set.rscrates/perry-runtime/src/string/mod.rscrates/perry-runtime/src/string/tests.rscrates/perry-runtime/src/typed_feedback.rscrates/perry-runtime/src/typed_feedback/guards.rscrates/perry-runtime/src/typed_feedback/tests.rscrates/perry-runtime/src/typed_feedback/trace.rscrates/perry-runtime/src/typedarray/mod.rscrates/perry-runtime/src/value/dyn_index.rscrates/perry/src/commands/compile.rscrates/perry/src/commands/compile/build_cache.rscrates/perry/src/commands/compile/lowering_report.rscrates/perry/src/commands/compile/optimized_libs.rscrates/perry/src/commands/compile/types.rscrates/perry/src/commands/dev.rscrates/perry/src/commands/run/mod.rscrates/perry/src/main.rscrates/perry/tests/local_bound_loop_semantics.rsscripts/check_runtime_symbols.shscripts/compiler_output_harness/analyzers.pyscripts/compiler_output_harness/capture.pyscripts/compiler_output_harness/common.pyscripts/compiler_output_harness/spec.pyscripts/compiler_output_harness/verification.pyscripts/native_abi_evidence_packet.shscripts/native_abi_evidence_report.pyscripts/release_sweep.shscripts/release_sweep_tiers/tier13_native_abi_evidence.shtest-files/test_typed_array_fractional_numeric_index.tstests/raw_numeric_object_fields.tstests/test_compiler_output_regression.pytests/test_native_abi_contract.shtests/test_native_abi_evidence_packet_smoke.shtests/test_native_abi_evidence_report.pytests/test_typed_feedback_runtime_evidence.pytests/typed_feedback_runtime_evidence.ts
Comment on lines
+69
to
+73
| `scripts/native_abi_evidence_packet.sh --gate` aggregates the | ||
| `native-abi-proof` compiler-output suite into | ||
| `native-abi-evidence.json` and `native-abi-evidence.md`. The packet is the | ||
| representative material type-lowering gate for PRs and release sweeps. | ||
|
|
There was a problem hiding this comment.
Document the --gate minimum-runs contract.
Line 69 introduces the gate entrypoint, but this section does not mention that scripts/native_abi_evidence_packet.sh --gate requires --runs >= 5. That omission can cause confusing gate failures for contributors running the command directly.
Suggested doc patch
-`scripts/native_abi_evidence_packet.sh --gate` aggregates the
+`scripts/native_abi_evidence_packet.sh --gate` (with `--runs >= 5`) aggregates the
`native-abi-proof` compiler-output suite into
`native-abi-evidence.json` and `native-abi-evidence.md`. The packet is the
representative material type-lowering gate for PRs and release sweeps.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `scripts/native_abi_evidence_packet.sh --gate` aggregates the | |
| `native-abi-proof` compiler-output suite into | |
| `native-abi-evidence.json` and `native-abi-evidence.md`. The packet is the | |
| representative material type-lowering gate for PRs and release sweeps. | |
| `scripts/native_abi_evidence_packet.sh --gate` (with `--runs >= 5`) aggregates the | |
| `native-abi-proof` compiler-output suite into | |
| `native-abi-evidence.json` and `native-abi-evidence.md`. The packet is the | |
| representative material type-lowering gate for PRs and release sweeps. |
🤖 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 `@benchmarks/compiler_output/README.md` around lines 69 - 73, Add documentation
to the README section describing scripts/native_abi_evidence_packet.sh --gate to
explicitly state that the --gate flag requires a minimum of 5 runs (--runs >=
5). This requirement should be documented clearly in the existing paragraph at
line 69-73 to prevent contributors from encountering confusing gate failures
when running the command without meeting this minimum-runs contract.
Comment on lines
+218
to
+254
| fn load_typed_capture( | ||
| blk: &mut crate::block::LlBlock, | ||
| capture_index: usize, | ||
| rep: TypedParamRep, | ||
| ) -> String { | ||
| let idx = capture_index.to_string(); | ||
| let captured_bits = blk.call( | ||
| I64, | ||
| "js_closure_get_capture_bits", | ||
| &[(I64, "%this_closure"), (I32, &idx)], | ||
| ); | ||
| let captured = blk.bitcast_i64_to_double(&captured_bits); | ||
| match rep { | ||
| TypedParamRep::F64 => blk.call( | ||
| DOUBLE, | ||
| "js_typed_f64_arg_to_raw", | ||
| &[(DOUBLE, captured.as_str())], | ||
| ), | ||
| TypedParamRep::I32 => blk.call( | ||
| I32, | ||
| "js_typed_i32_arg_to_raw", | ||
| &[(DOUBLE, captured.as_str())], | ||
| ), | ||
| TypedParamRep::I1 => { | ||
| let raw_i32 = blk.call( | ||
| I32, | ||
| "js_typed_i1_arg_to_raw", | ||
| &[(DOUBLE, captured.as_str())], | ||
| ); | ||
| blk.icmp_ne(I32, &raw_i32, "0") | ||
| } | ||
| TypedParamRep::StringRef => blk.call( | ||
| I64, | ||
| "js_typed_string_arg_to_raw", | ||
| &[(DOUBLE, captured.as_str())], | ||
| ), | ||
| } |
There was a problem hiding this comment.
Handle boxed captures before typed capture conversion.
load_typed_capture treats every capture slot as JSValue bits, but boxed captures are now stored as raw box-pointer bits by expr/closure.rs. A typed closure that captures a boxed local will convert the box pointer itself as f64/i32/string instead of reading the box contents. Plumb boxed-capture metadata into typed closure compilation and either reject boxed captures for typed clones or load the box value bits before emit_typed_arg_to_raw/guard conversion.
Also applies to: 257-283, 285-468
🤖 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-codegen/src/codegen/closure.rs` around lines 218 - 254, The
load_typed_capture function currently assumes all captures are stored as JSValue
bits, but boxed captures are now stored as raw box-pointer bits according to
expr/closure.rs. This causes typed closures capturing boxed locals to
incorrectly convert the box pointer itself as f64/i32/string instead of reading
the actual box contents. Modify load_typed_capture to either reject boxed
captures for typed closures or add logic to first load the box value bits from
the box pointer before applying the typed argument conversion via the
emit_typed_arg_to_raw pattern. You may need to plumb boxed-capture metadata
through the typed closure compilation pipeline to identify which captures are
boxed.
Comment on lines
+273
to
+278
| if matches!( | ||
| ctx.buffer_hazard_reasons.get(&buffer_local_id), | ||
| Some(MaterializationReason::ClosureCapture) | ||
| ) { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
Check closure_captures directly before emitting unchecked buffer proofs.
buffer_access_materialization_reason derives ClosureCapture from ctx.closure_captures, but this gate only checks buffer_hazard_reasons. A captured buffer local without an explicit hazard-map entry can still get an unchecked native pointer proof.
🛡️ Proposed fix
- if matches!(
- ctx.buffer_hazard_reasons.get(&buffer_local_id),
- Some(MaterializationReason::ClosureCapture)
- ) {
+ if ctx.closure_captures.contains_key(&buffer_local_id)
+ || matches!(
+ ctx.buffer_hazard_reasons.get(&buffer_local_id),
+ Some(MaterializationReason::ClosureCapture)
+ )
+ {
+ ctx.buffer_hazard_reasons
+ .entry(buffer_local_id)
+ .or_insert(MaterializationReason::ClosureCapture);
return Ok(None);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if matches!( | |
| ctx.buffer_hazard_reasons.get(&buffer_local_id), | |
| Some(MaterializationReason::ClosureCapture) | |
| ) { | |
| return Ok(None); | |
| } | |
| if ctx.closure_captures.contains_key(&buffer_local_id) | |
| || matches!( | |
| ctx.buffer_hazard_reasons.get(&buffer_local_id), | |
| Some(MaterializationReason::ClosureCapture) | |
| ) | |
| { | |
| ctx.buffer_hazard_reasons | |
| .entry(buffer_local_id) | |
| .or_insert(MaterializationReason::ClosureCapture); | |
| return Ok(None); | |
| } |
🤖 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-codegen/src/expr/buffer_access.rs` around lines 273 - 278, The
current check for ClosureCapture only examines buffer_hazard_reasons but does
not directly check ctx.closure_captures. A buffer_local_id can exist in
closure_captures without an entry in buffer_hazard_reasons, allowing unchecked
proofs to be emitted. Add an additional check to verify if buffer_local_id is
present directly in ctx.closure_captures and return Ok(None) if it is, ensuring
all captured buffers are properly gated before emitting unchecked native pointer
proofs.
Comment on lines
355
to
+357
| pub(crate) fn invalidate_local_write_facts(ctx: &mut FnCtx<'_>, id: u32) { | ||
| ctx.local_value_aliases.remove(&id); | ||
|
|
There was a problem hiding this comment.
Invalidate reverse local-value aliases on write.
Line 356 removes only id -> *, but leaves aliases like other -> id intact. After reassigning id, those links become stale and can propagate incorrect alias/range facts through local_value_alias_root.
Suggested fix
pub(crate) fn invalidate_local_write_facts(ctx: &mut FnCtx<'_>, id: u32) {
ctx.local_value_aliases.remove(&id);
+ let aliases = ctx.local_value_aliases.clone();
+ ctx.local_value_aliases.retain(|alias_id, _| {
+ *alias_id != id && local_value_alias_root_from_map(&aliases, *alias_id) != id
+ });fn local_value_alias_root_from_map(aliases: &HashMap<u32, u32>, mut id: u32) -> u32 {
let mut seen = std::collections::HashSet::new();
while let Some(next) = aliases.get(&id).copied() {
if !seen.insert(id) {
break;
}
id = next;
}
id
}🤖 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-codegen/src/expr/range_facts.rs` around lines 355 - 357, The
invalidate_local_write_facts function only removes the forward alias mapping for
the given id but leaves reverse aliases (entries where another variable points
to id) intact. After a local variable is reassigned, these stale reverse aliases
can propagate incorrect alias and range facts through the alias resolution
chain. In addition to removing the direct ctx.local_value_aliases.remove(&id),
iterate through ctx.local_value_aliases and remove any entries where the aliased
value equals id to clean up all reverse references pointing to the invalidated
variable.
Comment on lines
+357
to
+386
| let typed_f64_param_reps = if ctx.typed_f64_closures.contains(&func_id) { | ||
| ctx.typed_i1_closure_param_reps | ||
| .get(&func_id) | ||
| .filter(|reps| { | ||
| crate::codegen::typed_param_reps_match_args(ctx, reps, args) | ||
| }) | ||
| .cloned() | ||
| } else { | ||
| None | ||
| }; | ||
| let typed_i32_param_reps = if ctx.typed_i32_closures.contains(&func_id) { | ||
| ctx.typed_i1_closure_param_reps | ||
| .get(&func_id) | ||
| .filter(|reps| { | ||
| crate::codegen::typed_param_reps_match_args(ctx, reps, args) | ||
| }) | ||
| .cloned() | ||
| } else { | ||
| None | ||
| }; | ||
| let typed_string_param_reps = if ctx.typed_string_closures.contains(&func_id) { | ||
| ctx.typed_i1_closure_param_reps | ||
| .get(&func_id) | ||
| .filter(|reps| { | ||
| crate::codegen::typed_param_reps_match_args(ctx, reps, args) | ||
| }) | ||
| .cloned() | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
Use the matching typed-closure parameter-rep registry.
The f64, i32, and string branches are gated by their own ctx.typed_*_closures sets but all read ctx.typed_i1_closure_param_reps. That can skip valid typed clones or feed the wrong ABI reps into the selected clone.
Suggested fix
let typed_f64_param_reps = if ctx.typed_f64_closures.contains(&func_id) {
- ctx.typed_i1_closure_param_reps
+ ctx.typed_f64_closure_param_reps
.get(&func_id)
.filter(|reps| {
crate::codegen::typed_param_reps_match_args(ctx, reps, args)
})
.cloned()
@@
};
let typed_i32_param_reps = if ctx.typed_i32_closures.contains(&func_id) {
- ctx.typed_i1_closure_param_reps
+ ctx.typed_i32_closure_param_reps
.get(&func_id)
.filter(|reps| {
crate::codegen::typed_param_reps_match_args(ctx, reps, args)
})
.cloned()
@@
};
let typed_string_param_reps = if ctx.typed_string_closures.contains(&func_id) {
- ctx.typed_i1_closure_param_reps
+ ctx.typed_string_closure_param_reps
.get(&func_id)
.filter(|reps| {
crate::codegen::typed_param_reps_match_args(ctx, reps, args)
})
.cloned()🤖 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-codegen/src/lower_call/early_branches.rs` around lines 357 -
386, The three parameter reputation blocks for f64, i32, and string typed
closures are checking their respective typed closure sets
(ctx.typed_f64_closures, ctx.typed_i32_closures, ctx.typed_string_closures) but
all incorrectly read from ctx.typed_i1_closure_param_reps. Fix this by ensuring
each block reads from its matching registry: typed_f64_param_reps should read
from ctx.typed_f64_closure_param_reps, typed_i32_param_reps should read from
ctx.typed_i32_closure_param_reps, and typed_string_param_reps should read from
ctx.typed_string_closure_param_reps.
Comment on lines
+285
to
+335
| let typed_f64_call_param_reps = if !resets_this | ||
| && !has_rest | ||
| && !ctx.func_synthetic_arguments.contains(fid) | ||
| && ctx.typed_f64_functions.contains(fid) | ||
| && declared_count == args.len() | ||
| { | ||
| ctx.typed_i1_function_param_reps | ||
| .get(fid) | ||
| .filter(|reps| typed_i1_param_reps_match_args(ctx, reps, args)) | ||
| .cloned() | ||
| } else { | ||
| None | ||
| }; | ||
| let typed_i32_call_param_reps = if !resets_this | ||
| && !has_rest | ||
| && !ctx.func_synthetic_arguments.contains(fid) | ||
| && ctx.typed_i32_functions.contains(fid) | ||
| && declared_count == args.len() | ||
| { | ||
| ctx.typed_i1_function_param_reps | ||
| .get(fid) | ||
| .filter(|reps| typed_i1_param_reps_match_args(ctx, reps, args)) | ||
| .cloned() | ||
| } else { | ||
| None | ||
| }; | ||
| let typed_string_call_param_reps = if !resets_this | ||
| && !has_rest | ||
| && !ctx.func_synthetic_arguments.contains(fid) | ||
| && ctx.typed_string_functions.contains(fid) | ||
| && declared_count == args.len() | ||
| { | ||
| ctx.typed_i1_function_param_reps | ||
| .get(fid) | ||
| .filter(|reps| typed_i1_param_reps_match_args(ctx, reps, args)) | ||
| .cloned() | ||
| } else { | ||
| None | ||
| }; | ||
| let typed_i1_call_param_reps = if !resets_this | ||
| && !has_rest | ||
| && !ctx.func_synthetic_arguments.contains(fid) | ||
| && declared_count == args.len() | ||
| { | ||
| ctx.typed_i1_function_param_reps | ||
| .get(fid) | ||
| .filter(|reps| typed_i1_param_reps_match_args(ctx, reps, args)) | ||
| .cloned() | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
Select typed function reps from the same clone family.
The f64/i32/string branches are gated by their matching typed_*_functions sets, but then read ctx.typed_i1_function_param_reps; the i1 branch also lacks a ctx.typed_i1_functions.contains(fid) gate. This can skip emitted typed clones or generate a call to a clone with the wrong/missing ABI.
Suggested fix
{
- ctx.typed_i1_function_param_reps
+ ctx.typed_f64_function_param_reps
.get(fid)
.filter(|reps| typed_i1_param_reps_match_args(ctx, reps, args))
.cloned()
@@
{
- ctx.typed_i1_function_param_reps
+ ctx.typed_i32_function_param_reps
.get(fid)
.filter(|reps| typed_i1_param_reps_match_args(ctx, reps, args))
.cloned()
@@
{
- ctx.typed_i1_function_param_reps
+ ctx.typed_string_function_param_reps
.get(fid)
.filter(|reps| typed_i1_param_reps_match_args(ctx, reps, args))
.cloned()
@@
&& !has_rest
&& !ctx.func_synthetic_arguments.contains(fid)
+ && ctx.typed_i1_functions.contains(fid)
&& declared_count == args.len()🤖 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-codegen/src/lower_call/func_ref.rs` around lines 285 - 335, Each
typed function variant (f64, i32, string, i1) is not reading from its
corresponding parameter representations map. The typed_f64_call_param_reps block
checks ctx.typed_f64_functions but reads ctx.typed_i1_function_param_reps; the
typed_i32_call_param_reps block checks ctx.typed_i32_functions but reads
ctx.typed_i1_function_param_reps; the typed_string_call_param_reps block checks
ctx.typed_string_functions but reads ctx.typed_i1_function_param_reps.
Additionally, the typed_i1_call_param_reps block is missing the gate check
ctx.typed_i1_functions.contains(fid). Fix this by having each block read from
its matching context map (ctx.typed_f64_function_param_reps for f64,
ctx.typed_i32_function_param_reps for i32, ctx.typed_string_function_param_reps
for string) and add the missing gate to the i1 block's condition.
Resolves conflicts across 33 files (codegen type-lowering + runtime typed-feedback) by unioning the feature branch's representation-aware lowering with main's evolution. Notable folds from main: Set.add write-back SIGSEGV fix, #5334 class-field-set barrier elision/outlining, size-optimize feature gating. Post-merge fixes: - restore imports dropped during import-union resolution (bail in index_set; double_literal + ExpectedNativeRep in expr/mod). - boxed_local box-bits IR test now asserts the ABI invariant (no raw double reaches a bits helper) instead of one exact instruction sequence; main's constant lowering folds the undefined slot default straight to i64 bits. - realign typed_feedback LTO keepalive anchor test K29/K30 -> K30/K31 (stale on the codex branch since maybe_dump_trace took K29; trace.rs is canonical). Version 0.5.1196 -> 0.5.1197 + CHANGELOG entry. Validation: build green (codegen/runtime/perry/stdlib); native_proof_regressions 232, typed_feedback 15, python evidence gate 85, perry-runtime lib 1096 (single-threaded) all pass. Remaining parallel-suite failures are pre-existing non-deterministic flakes (set varies per run; green single-threaded). Claude-Session: https://claude.ai/code/session_019hwNPXCAWnjv5vddcznhFA
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@benchmarks/compiler_output/workloads.toml`:
- Around line 1179-1198: Remove the duplicate
`[[workloads.raw_numeric_object_fields.native_rep_checks.require_records]]`
table entry for "raw_scalar_field_load_raw_f64" (the second entry shown in the
diff). This entry appears to be duplicated elsewhere in the workloads.toml file
and removing it will eliminate the redundant declaration that creates ambiguous
gate semantics and makes the workload contract clearer to maintain.
In `@CHANGELOG.md`:
- Around line 1-28: Remove the entire changelog entry from CHANGELOG.md (the
section starting with v0.5.1197 and all associated details). External
contributors should not edit CHANGELOG.md according to project guidelines;
maintainers handle changelog additions at merge time. Delete all content shown
in the diff to allow this responsibility to be handled by maintainers during the
merge process.
- Around line 1-621: Four markdown headings in the CHANGELOG.md file are missing
the required space after the hash symbol(s), violating the MD018 linting rule.
Locate the atx-style headings at lines 24, 47, 564, and 565 that currently
follow the pattern `##heading` (no space) or similar, and modify each to include
a space after the hash(es), so they follow the correct pattern `## heading`.
Ensure all heading levels (single or multiple `#` characters) have exactly one
space between the last `#` and the heading text.
In `@crates/perry-codegen/src/lower_call/new.rs`:
- Around line 185-190: The truncate call in the constructor argument padding
logic uses `param_count.max(out.len())` which will always be greater than or
equal to `out.len()`, meaning truncate never actually removes extra arguments.
This violates the helper's contract to return exactly `param_count` arguments.
Change the truncate call to use `param_count` directly instead of
`param_count.max(out.len())` so that any extra arguments beyond the parameter
count are properly removed for fixed-arity imported constructors.
- Around line 1449-1455: In the ancestor constructor handling block (around the
ctx.block().call(DOUBLE, &ctor.symbol, &ctor_args) statement), the return value
from the imported constructor is being discarded with let _ instead of being
captured and stored. Refactor this section to capture the return value into a
variable and store it to ctor_result_slot using ctx.block().store(), similar to
how the leaf-constructor path correctly handles it in the else branch (lines
1470-1481). This ensures that inherited imported constructors whose return
values override the instance are properly propagated to ctor_result_slot,
allowing js_ctor_return_override to apply ECMAScript semantics. Additionally,
add a test case for a subclass with no own constructor that inherits from an
imported parent class whose constructor explicitly returns an object.
In `@TYPE_LOWERING.md`:
- Line 499: The markdown fragment link [3](`#0-2`) at line 499 references a
non-existent anchor `0-2` which prevents the link from resolving properly.
Either locate an existing heading in the document that should be the target of
this footnote reference and update the fragment identifier to match that
heading's anchor (e.g., change `#0-2` to the correct anchor name), or remove the
link entirely if the referenced footnote no longer exists in the document.
🪄 Autofix (Beta)
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: 726053e3-1dab-42bb-b983-96ad77b6d4bc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (59)
.github/workflows/test.ymlCHANGELOG.mdCLAUDE.mdCargo.tomlTYPE_LOWERING.mdbenchmarks/compiler_output/workloads.tomlcrates/perry-codegen/src/boxed_vars.rscrates/perry-codegen/src/codegen/artifacts.rscrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/codegen/opts.rscrates/perry-codegen/src/collectors/escape_check.rscrates/perry-codegen/src/collectors/hir_facts.rscrates/perry-codegen/src/collectors/index_uses.rscrates/perry-codegen/src/collectors/mod.rscrates/perry-codegen/src/expr/array_push.rscrates/perry-codegen/src/expr/arrays_finds.rscrates/perry-codegen/src/expr/bigint_set.rscrates/perry-codegen/src/expr/binary.rscrates/perry-codegen/src/expr/buffer_access.rscrates/perry-codegen/src/expr/call_spread.rscrates/perry-codegen/src/expr/closure.rscrates/perry-codegen/src/expr/index_get.rscrates/perry-codegen/src/expr/index_set.rscrates/perry-codegen/src/expr/instance_misc1.rscrates/perry-codegen/src/expr/literals_vars.rscrates/perry-codegen/src/expr/logical_collections.rscrates/perry-codegen/src/expr/math_simple.rscrates/perry-codegen/src/expr/misc_methods.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/property_get.rscrates/perry-codegen/src/expr/property_set.rscrates/perry-codegen/src/lower_call/early_branches.rscrates/perry-codegen/src/lower_call/mod.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/lower_call/property_get.rscrates/perry-codegen/src/native_value/mod.rscrates/perry-codegen/src/runtime_decls/objects.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-codegen/src/stmt/let_stmt.rscrates/perry-codegen/src/stmt/loops.rscrates/perry-codegen/src/type_analysis.rscrates/perry-codegen/tests/native_proof_regressions.rscrates/perry-codegen/tests/typed_feedback.rscrates/perry-runtime/src/array/header.rscrates/perry-runtime/src/array/indexing.rscrates/perry-runtime/src/bigint.rscrates/perry-runtime/src/box.rscrates/perry-runtime/src/closure/mod.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/native_abi.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/polymorphic_index.rs
💤 Files with no reviewable changes (1)
- crates/perry-codegen/src/collectors/index_uses.rs
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (28)
- crates/perry-codegen/src/lower_call/mod.rs
- crates/perry-codegen/src/collectors/escape_check.rs
- crates/perry-codegen/src/expr/logical_collections.rs
- crates/perry-codegen/src/expr/closure.rs
- crates/perry-codegen/src/boxed_vars.rs
- crates/perry-codegen/src/codegen/entry.rs
- crates/perry-codegen/src/expr/buffer_access.rs
- crates/perry-codegen/src/codegen/opts.rs
- .github/workflows/test.yml
- crates/perry-codegen/src/codegen/function.rs
- crates/perry-codegen/src/expr/instance_misc1.rs
- crates/perry-codegen/src/expr/array_push.rs
- crates/perry-codegen/src/expr/misc_methods.rs
- crates/perry-codegen/src/expr/literals_vars.rs
- crates/perry-codegen/src/expr/arrays_finds.rs
- crates/perry-codegen/src/expr/index_set.rs
- crates/perry-codegen/src/codegen/artifacts.rs
- crates/perry-codegen/src/codegen/method.rs
- crates/perry-codegen/src/lower_call/early_branches.rs
- crates/perry-codegen/src/expr/property_get.rs
- crates/perry-codegen/src/codegen/mod.rs
- crates/perry-codegen/src/expr/bigint_set.rs
- crates/perry-codegen/src/expr/math_simple.rs
- crates/perry-codegen/src/expr/binary.rs
- crates/perry-codegen/src/codegen/closure.rs
- crates/perry-codegen/src/collectors/hir_facts.rs
- crates/perry-codegen/src/expr/mod.rs
- crates/perry-codegen/src/expr/index_get.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🤖 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 `@benchmarks/compiler_output/workloads.toml`:
- Around line 1179-1198: Remove the duplicate
`[[workloads.raw_numeric_object_fields.native_rep_checks.require_records]]`
table entry for "raw_scalar_field_load_raw_f64" (the second entry shown in the
diff). This entry appears to be duplicated elsewhere in the workloads.toml file
and removing it will eliminate the redundant declaration that creates ambiguous
gate semantics and makes the workload contract clearer to maintain.
In `@CHANGELOG.md`:
- Around line 1-28: Remove the entire changelog entry from CHANGELOG.md (the
section starting with v0.5.1197 and all associated details). External
contributors should not edit CHANGELOG.md according to project guidelines;
maintainers handle changelog additions at merge time. Delete all content shown
in the diff to allow this responsibility to be handled by maintainers during the
merge process.
- Around line 1-621: Four markdown headings in the CHANGELOG.md file are missing
the required space after the hash symbol(s), violating the MD018 linting rule.
Locate the atx-style headings at lines 24, 47, 564, and 565 that currently
follow the pattern `##heading` (no space) or similar, and modify each to include
a space after the hash(es), so they follow the correct pattern `## heading`.
Ensure all heading levels (single or multiple `#` characters) have exactly one
space between the last `#` and the heading text.
In `@crates/perry-codegen/src/lower_call/new.rs`:
- Around line 185-190: The truncate call in the constructor argument padding
logic uses `param_count.max(out.len())` which will always be greater than or
equal to `out.len()`, meaning truncate never actually removes extra arguments.
This violates the helper's contract to return exactly `param_count` arguments.
Change the truncate call to use `param_count` directly instead of
`param_count.max(out.len())` so that any extra arguments beyond the parameter
count are properly removed for fixed-arity imported constructors.
- Around line 1449-1455: In the ancestor constructor handling block (around the
ctx.block().call(DOUBLE, &ctor.symbol, &ctor_args) statement), the return value
from the imported constructor is being discarded with let _ instead of being
captured and stored. Refactor this section to capture the return value into a
variable and store it to ctor_result_slot using ctx.block().store(), similar to
how the leaf-constructor path correctly handles it in the else branch (lines
1470-1481). This ensures that inherited imported constructors whose return
values override the instance are properly propagated to ctor_result_slot,
allowing js_ctor_return_override to apply ECMAScript semantics. Additionally,
add a test case for a subclass with no own constructor that inherits from an
imported parent class whose constructor explicitly returns an object.
In `@TYPE_LOWERING.md`:
- Line 499: The markdown fragment link [3](`#0-2`) at line 499 references a
non-existent anchor `0-2` which prevents the link from resolving properly.
Either locate an existing heading in the document that should be the target of
this footnote reference and update the fragment identifier to match that
heading's anchor (e.g., change `#0-2` to the correct anchor name), or remove the
link entirely if the referenced footnote no longer exists in the document.
🪄 Autofix (Beta)
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: 726053e3-1dab-42bb-b983-96ad77b6d4bc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (59)
.github/workflows/test.ymlCHANGELOG.mdCLAUDE.mdCargo.tomlTYPE_LOWERING.mdbenchmarks/compiler_output/workloads.tomlcrates/perry-codegen/src/boxed_vars.rscrates/perry-codegen/src/codegen/artifacts.rscrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/codegen/opts.rscrates/perry-codegen/src/collectors/escape_check.rscrates/perry-codegen/src/collectors/hir_facts.rscrates/perry-codegen/src/collectors/index_uses.rscrates/perry-codegen/src/collectors/mod.rscrates/perry-codegen/src/expr/array_push.rscrates/perry-codegen/src/expr/arrays_finds.rscrates/perry-codegen/src/expr/bigint_set.rscrates/perry-codegen/src/expr/binary.rscrates/perry-codegen/src/expr/buffer_access.rscrates/perry-codegen/src/expr/call_spread.rscrates/perry-codegen/src/expr/closure.rscrates/perry-codegen/src/expr/index_get.rscrates/perry-codegen/src/expr/index_set.rscrates/perry-codegen/src/expr/instance_misc1.rscrates/perry-codegen/src/expr/literals_vars.rscrates/perry-codegen/src/expr/logical_collections.rscrates/perry-codegen/src/expr/math_simple.rscrates/perry-codegen/src/expr/misc_methods.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/property_get.rscrates/perry-codegen/src/expr/property_set.rscrates/perry-codegen/src/lower_call/early_branches.rscrates/perry-codegen/src/lower_call/mod.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/lower_call/property_get.rscrates/perry-codegen/src/native_value/mod.rscrates/perry-codegen/src/runtime_decls/objects.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-codegen/src/stmt/let_stmt.rscrates/perry-codegen/src/stmt/loops.rscrates/perry-codegen/src/type_analysis.rscrates/perry-codegen/tests/native_proof_regressions.rscrates/perry-codegen/tests/typed_feedback.rscrates/perry-runtime/src/array/header.rscrates/perry-runtime/src/array/indexing.rscrates/perry-runtime/src/bigint.rscrates/perry-runtime/src/box.rscrates/perry-runtime/src/closure/mod.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/native_abi.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/polymorphic_index.rs
💤 Files with no reviewable changes (1)
- crates/perry-codegen/src/collectors/index_uses.rs
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (28)
- crates/perry-codegen/src/lower_call/mod.rs
- crates/perry-codegen/src/collectors/escape_check.rs
- crates/perry-codegen/src/expr/logical_collections.rs
- crates/perry-codegen/src/expr/closure.rs
- crates/perry-codegen/src/boxed_vars.rs
- crates/perry-codegen/src/codegen/entry.rs
- crates/perry-codegen/src/expr/buffer_access.rs
- crates/perry-codegen/src/codegen/opts.rs
- .github/workflows/test.yml
- crates/perry-codegen/src/codegen/function.rs
- crates/perry-codegen/src/expr/instance_misc1.rs
- crates/perry-codegen/src/expr/array_push.rs
- crates/perry-codegen/src/expr/misc_methods.rs
- crates/perry-codegen/src/expr/literals_vars.rs
- crates/perry-codegen/src/expr/arrays_finds.rs
- crates/perry-codegen/src/expr/index_set.rs
- crates/perry-codegen/src/codegen/artifacts.rs
- crates/perry-codegen/src/codegen/method.rs
- crates/perry-codegen/src/lower_call/early_branches.rs
- crates/perry-codegen/src/expr/property_get.rs
- crates/perry-codegen/src/codegen/mod.rs
- crates/perry-codegen/src/expr/bigint_set.rs
- crates/perry-codegen/src/expr/math_simple.rs
- crates/perry-codegen/src/expr/binary.rs
- crates/perry-codegen/src/codegen/closure.rs
- crates/perry-codegen/src/collectors/hir_facts.rs
- crates/perry-codegen/src/expr/mod.rs
- crates/perry-codegen/src/expr/index_get.rs
🛑 Comments failed to post (6)
benchmarks/compiler_output/workloads.toml (1)
1179-1198:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicated
require_recordstable declarations.At Line 1179 and Line 1189, the double
[[workloads.raw_numeric_object_fields.native_rep_checks.require_records]]declarations introduce redundant/ambiguous entries, and theraw_scalar_field_load_raw_f64requirement is duplicated. This can skew gate semantics and makes the workload contract harder to trust.Suggested cleanup
-[[workloads.raw_numeric_object_fields.native_rep_checks.require_records]] [[workloads.raw_numeric_object_fields.native_rep_checks.require_records]] name = "raw_scalar_ctor_field_store_raw_f64" source_function = "rawNumericObjectFieldsChecksum" expr_kind = "ScalarThisFieldSet" consumer = "scalar_object_field_store.raw_f64" native_rep_name = "f64" access_mode = "none" consumed_fact_kind = "representation" consumed_fact_state = "consumed" -[[workloads.raw_numeric_object_fields.native_rep_checks.require_records]] -[[workloads.raw_numeric_object_fields.native_rep_checks.require_records]] -name = "raw_scalar_field_load_raw_f64" -source_function = "rawNumericObjectFieldsChecksum" -expr_kind = "ScalarObjectFieldGet" -consumer = "scalar_object_field_load.raw_f64" -native_rep_name = "f64" -access_mode = "none" -consumed_fact_kind = "representation" -consumed_fact_state = "consumed"🤖 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 `@benchmarks/compiler_output/workloads.toml` around lines 1179 - 1198, Remove the duplicate `[[workloads.raw_numeric_object_fields.native_rep_checks.require_records]]` table entry for "raw_scalar_field_load_raw_f64" (the second entry shown in the diff). This entry appears to be duplicated elsewhere in the workloads.toml file and removing it will eliminate the redundant declaration that creates ambiguous gate semantics and makes the workload contract clearer to maintain.CHANGELOG.md (2)
1-28:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove this changelog entry per contributor guidelines — maintainers handle CHANGELOG.md at merge time.
External contributors should not edit CHANGELOG.md; maintainers add changelog entries when merging. The PR is correctly marked "NOT YET MERGEABLE" with changelog addition as a pre-merge requirement, indicating this should be deferred to merge time, not added in the PR.
However, the format is correct:
## v0.5.x — <one-line summary>follows the coding guideline pattern and is properly prepended at the top.🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 24-24: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 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 `@CHANGELOG.md` around lines 1 - 28, Remove the entire changelog entry from CHANGELOG.md (the section starting with v0.5.1197 and all associated details). External contributors should not edit CHANGELOG.md according to project guidelines; maintainers handle changelog additions at merge time. Delete all content shown in the diff to allow this responsibility to be handled by maintainers during the merge process.Sources: Coding guidelines, Learnings
1-621:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix 4 markdown linting warnings (MD018: missing space after
#in headings).Static analysis detected 4 instances of atx-style headings with no space after the
#character at lines 24, 47, 564, 565. Ensure all headings follow the pattern## heading(space required) instead of##heading. This may be in pre-existing content shown in context; verify and fix all instances.🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 24-24: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 47-47: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 564-564: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 565-565: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 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 `@CHANGELOG.md` around lines 1 - 621, Four markdown headings in the CHANGELOG.md file are missing the required space after the hash symbol(s), violating the MD018 linting rule. Locate the atx-style headings at lines 24, 47, 564, and 565 that currently follow the pattern `##heading` (no space) or similar, and modify each to include a space after the hash(es), so they follow the correct pattern `## heading`. Ensure all heading levels (single or multiple `#` characters) have exactly one space between the last `#` and the heading text.Source: Linters/SAST tools
crates/perry-codegen/src/lower_call/new.rs (2)
185-190:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winTruncate fixed-arity imported constructor args to
param_count.
param_count.max(out.len())is never smaller thanout.len(), so fixed-arity imported constructors still receive every extra call-site arg. That contradicts this helper’s “exactlyctor.param_count” contract and can emit mismatched imported-constructor call signatures.Proposed fix
- out.truncate(param_count.max(out.len())); + out.truncate(param_count);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let mut out: Vec<String> = lowered_args.to_vec(); while out.len() < param_count { out.push(undef.clone()); } out.truncate(param_count); out🤖 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-codegen/src/lower_call/new.rs` around lines 185 - 190, The truncate call in the constructor argument padding logic uses `param_count.max(out.len())` which will always be greater than or equal to `out.len()`, meaning truncate never actually removes extra arguments. This violates the helper's contract to return exactly `param_count` arguments. Change the truncate call to use `param_count` directly instead of `param_count.max(out.len())` so that any extra arguments beyond the parameter count are properly removed for fixed-arity imported constructors.
1449-1455:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and locate the relevant code cd crates/perry-codegen/src/lower_call && ls -laRepository: PerryTS/perry
Length of output: 1807
🏁 Script executed:
# Read the specific lines mentioned in the review sed -n '1440,1470p' crates/perry-codegen/src/lower_call/new.rsRepository: PerryTS/perry
Length of output: 2063
🏁 Script executed:
# Get broader context around the issue sed -n '1400,1500p' crates/perry-codegen/src/lower_call/new.rs | head -150Repository: PerryTS/perry
Length of output: 6110
🏁 Script executed:
# Search for ctor_result_slot usage to understand how it's managed rg "ctor_result_slot" crates/perry-codegen/src/lower_call/new.rs -n -C 3Repository: PerryTS/perry
Length of output: 2092
🏁 Script executed:
# Check for existing tests related to inherited constructors with object returns fd -e rs test -type f | xargs grep -l "extends.*Base\|inherited.*ctor\|super.*return" 2>/dev/null | head -5Repository: PerryTS/perry
Length of output: 228
🏁 Script executed:
# Search for test cases related to constructors and inheritance find . -path ./target -prune -o -type f -name "*.rs" -print | xargs grep -l "class.*extends\|inherit.*constructor\|return.*object.*constructor" 2>/dev/null | head -10Repository: PerryTS/perry
Length of output: 529
🏁 Script executed:
# Look for test files in the perry repository find . -path ./target -prune -o -type f -name "*test*.rs" -print | head -20Repository: PerryTS/perry
Length of output: 1081
🏁 Script executed:
# Check the context around the ancestor constructor call to understand the flow better sed -n '1420,1490p' crates/perry-codegen/src/lower_call/new.rsRepository: PerryTS/perry
Length of output: 4285
🏁 Script executed:
# Look for how ctor_result_slot is used at the end of lower_new rg "ctor_result_slot" crates/perry-codegen/src/lower_call/new.rs -A 5 -B 5Repository: PerryTS/perry
Length of output: 2371
🏁 Script executed:
# Check if there are any ECMAScript constructor return-override tests fd -type f -name "*.ts" | xargs grep -l "class.*extends.*constructor.*return\|return.*object.*new" 2>/dev/null | head -10Repository: PerryTS/perry
Length of output: 228
🏁 Script executed:
# Check the ECMAScript spec comments and references in the code rg "return-override|class.*return.*function|Chalk" crates/perry-codegen/src/lower_call/new.rs -B 2 -A 2Repository: PerryTS/perry
Length of output: 3939
🏁 Script executed:
# Look for test files specifically for constructor behavior fd -type f -name "*.ts" | xargs grep -l "extends.*constructor\|class.*constructor.*return" 2>/dev/null | head -5Repository: PerryTS/perry
Length of output: 228
🏁 Script executed:
# Search for any existing handling of inherited constructor returns rg "ancestor.*return\|inherited.*return\|parent.*constructor.*return" crates/perry-codegen/src/ -nRepository: PerryTS/perry
Length of output: 39
🏁 Script executed:
# Check the structure of the code more - look at the whole context of this block sed -n '1380,1500p' crates/perry-codegen/src/lower_call/new.rsRepository: PerryTS/perry
Length of output: 7201
🏁 Script executed:
# Look for how js_ctor_return_override is used rg "js_ctor_return_override" crates/perry-codegen/src/ -n -B 2 -A 2Repository: PerryTS/perry
Length of output: 3967
🏁 Script executed:
# Check if there's special handling for ancestor vs leaf constructors rg "ancestor\|inherited" crates/perry-codegen/src/lower_call/new.rs -n -B 1 -A 1Repository: PerryTS/perry
Length of output: 39
🏁 Script executed:
# Search for test cases with 'extends' and 'constructor' and 'return' find . -path ./target -prune -o -type f -name "*.rs" -exec grep -l "class.*extends.*constructor.*return\|derived.*class.*return" {} \; 2>/dev/null | head -5Repository: PerryTS/perry
Length of output: 243
🏁 Script executed:
# Look at the end of lower_new to see how ctor_result_slot is used sed -n '1500,1600p' crates/perry-codegen/src/lower_call/new.rsRepository: PerryTS/perry
Length of output: 3823
Propagate return values from inherited imported constructors to
ctor_result_slot.For
class Sub extends Base {}whereSubhas no own constructor andBase's imported constructor returns an object/function,new Sub()currently discards that return value at line 1449-1455. The ancestor-constructor path useslet _ = ctx.block().call(...), dropping the return, while the leaf-constructor path (lines 1470-1481) correctly captures it vialet ctor_ret = ... ctx.block().store(DOUBLE, &ctor_ret, &ctor_result_slot).ECMAScript return-override semantics require that an object/function returned from any constructor (base or derived) replaces the allocated instance. Capture the ancestor constructor's return into
ctor_result_slotsojs_ctor_return_overrideat the end oflower_newcan apply the override correctly. Add a test case for a no-own-constructor subclass of an imported parent whose constructor explicitly returns an object.🤖 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-codegen/src/lower_call/new.rs` around lines 1449 - 1455, In the ancestor constructor handling block (around the ctx.block().call(DOUBLE, &ctor.symbol, &ctor_args) statement), the return value from the imported constructor is being discarded with let _ instead of being captured and stored. Refactor this section to capture the return value into a variable and store it to ctor_result_slot using ctx.block().store(), similar to how the leaf-constructor path correctly handles it in the else branch (lines 1470-1481). This ensures that inherited imported constructors whose return values override the instance are properly propagated to ctor_result_slot, allowing js_ctor_return_override to apply ECMAScript semantics. Additionally, add a test case for a subclass with no own constructor that inherits from an imported parent class whose constructor explicitly returns an object.TYPE_LOWERING.md (1)
499-499:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix invalid markdown fragment link at Line 499.
[3](#0-2)is currently flagged as an invalid fragment, so the in-doc reference won’t resolve reliably. Point it to an existing heading anchor (or remove the footnote-style link if the target no longer exists).🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 499-499: Link fragments should be valid
(MD051, link-fragments)
🤖 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 `@TYPE_LOWERING.md` at line 499, The markdown fragment link [3](`#0-2`) at line 499 references a non-existent anchor `0-2` which prevents the link from resolving properly. Either locate an existing heading in the document that should be the target of this footnote reference and update the fragment identifier to match that heading's anchor (e.g., change `#0-2` to the correct anchor name), or remove the link entirely if the referenced footnote no longer exists in the document.Source: Linters/SAST tools
added 3 commits
June 19, 2026 20:26
…type-lowering files PR #5466 gate fixes (the type-lowering branch failed its own evidence gate, unenforced on the codex branch; main enforces it): - string_pool.rs: module string-handle init used emit_root_nanbox_store_on_block, emitting a js_write_barrier_root_nanbox per interned string. The slot is registered as a permanent global root on the next line (always scanned), so the remembered-set barrier is redundant; no allocation runs in the gap. Replaced with a plain store. This is what the native-region-proof write_barriers_static budgets (image_convolution=0, loop_data_dependent=1) always expected — the barriers were one-time init noise, never the hot path. image_convolution 5->0, loop_data_dependent 6->0. The branch's own analyzer counts root_nanbox barriers; main's didn't, which is why this only surfaced on the PR to main. - check_file_size.sh: allowlist the 7 type-lowering files already over the 2000-line cap (loops.rs, hir_facts.rs, map.rs, set.rs, typed_feedback.rs, typed_feedback/tests.rs, lowering_report.rs) — pre-existing on the branch (lint was unenforced there). Topical splits are reasonable follow-ups. Validation: native-region-proof gate EXIT 0 (all 11 workloads pass, 3 runs); check_file_size.sh OK; perry-codegen full suite green (incl. shadow_slot_hygiene). Claude-Session: https://claude.ai/code/session_019hwNPXCAWnjv5vddcznhFA
…ring branch Second main sync to clear the version-bump re-conflict that left PR #5466 DIRTY (no CI runs on a conflicting PR). Conflicts: CHANGELOG (kept both, my entry -> v0.5.1198 above main's WeakMap 1197); strings_part2.rs auto-unioned the FFI decls; version 0.5.1197 -> 0.5.1198 (main already shipped 1197). Build green; no type-lowering code reconflicted. Claude-Session: https://claude.ai/code/session_019hwNPXCAWnjv5vddcznhFA
…tore-sites The lint job's GC store-site inventory (and the addr-class audit after it) only ran once the file-size step stopped failing — surfacing two pre-existing branch store-sites that lack the required GC_STORE_AUDIT marker: - index_set.rs packed numeric-array element store (slot_value is a raw numeric f64 via js_array_numeric_value_to_raw_f64 / sitofp of an i32) - scalar_method.rs raw-f64 class-field store (raw via js_array_numeric_value_to_raw_f64) Both write a number into a numeric-layout slot — never a GC pointer — so they are POINTER_FREE and need no write barrier; added the markers documenting that. Comment-only; no behavior change. gc_store_site_inventory + addr_class_inventory + self-tests pass; fmt/file-size clean. Claude-Session: https://claude.ai/code/session_019hwNPXCAWnjv5vddcznhFA
Contributor
Author
Status after sync + fixes — one pre-existing gate failure remains for the authorI synced
What I already fixed (pushed)
The remaining failure (for @andrewtdiz)
It is platform-specific (Linux CI fails; macOS passes). Root cause:
Suggested directions (author's call):
I held off on these because (a) I can't reproduce the Linux-only failure locally to validate a fix, and (b) the guard-ABI rework is a design call best made by the type-lowering author. Everything else is green and the branch is mergeable once this check is resolved. Follow-ups also tracked in #5464. |
Sync the representation-aware type-lowering branch with current main (clean merge — no conflicts). Addresses the 'sync with main' item on the PR's pre-merge checklist. Version bump + CHANGELOG are intentionally left for the maintainer at merge time, per the external-contributor policy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ4RdAWQhxvqyS9vWeLoTB
…re lowering Correctness: - expr/mod.rs: mask i32 shift counts to 5 bits (`& 31`) before emitting shl/ashr/lshr — JS masks shift counts and LLVM i32 shifts >= 32 are UB (`x << 40` etc.). - map.rs/set.rs: canonicalize genuine number NaN to a single bit pattern in `normalize_zero` so SameValueZero treats all NaN as one key (the bits-keyed side-table and `jsvalue_eq` bit fast-path otherwise bucket distinct NaN payloads separately). Gated on `is_number()` so NaN-boxed tagged values (objects/strings/bigints) keep their payloads. - property_get.rs: strip the NaN-box POINTER_TAG before passing the receiver to `js_object_get_field_by_property_id_f64` (it takes a raw `*ObjectHeader`). GC write barriers (mirror the established LocalSet path): - array_push.rs: barrier after the bits-based box/closure-capture write-backs that store the realloc'd array head. - literals_vars.rs: barrier after the boxed-assignment and `++`/`--` box/ closure write-backs (BigInt `++` yields a heap pointer). Graceful fallback (PR #5464 follow-up): - index_set.rs: a packed-U32 loop store fact now falls through to the generic array-store path instead of aborting codegen via `bail!`. Benchmark: - h1_buffer_alias_negative.ts: the closureCapture hot loop now reads through the captured `read` closure so the alias-negative scenario is exercised. Skipped as false positives (verified against current code): the per-type typed-function/closure param-rep registries CodeRabbit suggested don't exist (`typed_i1_*_param_reps` is a shared registry keyed by id, populated for all clone kinds; the set-gate selects the clone), and the LoweredValue "partial move" finding doesn't apply (the match arms are unit patterns, no binding — the crate compiles). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ4RdAWQhxvqyS9vWeLoTB
… directly CodeRabbit suggested routing the hot loop through the captured `read` closure, but that removes the direct buffer index from `closureCapture`'s own body — so the native-region proof no longer records the "closure-capture → unchecked access denied" signal, flipping `closure_capture` from true to false and failing the `native_reps_negative_closure_capture_access_denied` golden assertion in the compiler-output-regression suite. The direct `owned[i]` read while `owned` is closure-captured is the intentional shape of this negative fixture, so revert to it. (CodeRabbit finding #5 conflicts with the fixture's design.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ4RdAWQhxvqyS9vWeLoTB
… index-get hot path
The compiler-output-regression `loop_data_dependent` workload was red
(pre-existing on this branch): its numeric-loop named region contained a
`sitofp i32 -> double` because every array index-get fed the i32 index to
`js_typed_feedback_{numeric,plain}_array_index_get_guard` as an f64
`index_value`. That f64 was used by the guard only for an `is_plain_number`
check that is tautological — every emit site supplies a statically-proven
non-negative i32 index (proven via `numeric_index_has_integer_array_index_proof`
or a bounded loop counter).
Change the two guards to take the i32 index directly (no f64 `index_value`,
no `is_plain_number` check) and materialize the f64 index lazily inside the
(cold) boxed-fallback block, where it is actually needed. The proven-i32
callers now lower the index via `lower_expr_as_i32` instead of
`fptosi(lower_expr(..))`, so no conversion is emitted in the hot region at
all. Guard names are unchanged, so the evidence-gate allowlists and IR
contains/regex checks still match.
native-region-proof suite is now fully green (was: loop_data_dependent fail);
runtime typed_feedback tests, perry-codegen tests, and the
test_compiler_output_regression / test_native_abi_evidence_report harnesses
all pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQ4RdAWQhxvqyS9vWeLoTB
…64 loads CodeRabbit: the packed-f64 loop store fast path folded `Math.abs(arr[..])` to `llvm.fabs.f64` whenever the inner read was from the same array, ignoring the index. A non-packed index (string key / unbounded) lowers through the boxed/runtime fallback to a NaN-boxed JS value, and `fabs` (a bare sign-bit clear) would skip Math.abs's ToNumber coercion on it. Require the inner index-get to be a proven packed-f64 load (index is the packed-loop counter) before folding; everything else falls through to the normal value lowering. native-region-proof suite stays green (the legitimate packed_f64_loop_versioning Math.abs path still folds); perry-codegen tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ4RdAWQhxvqyS9vWeLoTB
Contributor
Author
⛔ Local verification found 3 runtime regressions vs
|
| gap test | main (8de2333) |
PR head (9af541c) |
|---|---|---|
test_gap_array_methods |
✅ MATCH | ❌ DIFFERS |
test_gap_dynamic_import_literal |
✅ MATCH | ❌ DIFFERS |
test_gap_fetch_response |
✅ MATCH | ❌ DIFFERS |
Symptoms (< = PR, > = node)
test_gap_array_methods
< Array.fromAsync: NaN
< Array.fromAsync promises: NaN
> Array.fromAsync: [ 1, 2, 3 ]
> Array.fromAsync promises: [ 10, 20, 30 ]
test_gap_dynamic_import_literal
< undefined
< Uncaught (in promise) TypeError: (number).greet is not a function
> 42
> hello from a
test_gap_fetch_response
< headers forEach: content-type=text/plain
> headers forEach: content-type=text/plain x-custom=123
These are intrinsic to the PR — 9af541ce7 (the commit GitHub marks "all checks passing") fails all three; merging latest main in does not change it. All three look like the representation-aware lowering mis-boxing a non-number value as a number (async-array result → NaN, module export → number, header iteration → dropped entries) — likely a shared root in value flow.
Repro
target/release/perry test-files/test_gap_<name>.ts -o /tmp/x && /tmp/x
node --experimental-strip-types test-files/test_gap_<name>.ts
Next steps
- Root-cause the three (start with
array_methods/Array.fromAsync— it's aruntime-onlylink, so the bug is in the runtime/codegen path, not stdlib auto-opt). - Re-run the gap suite as part of the fix.
- Recommend promoting the gap suite to a required PR gate so this regression class can't merge green again.
Converting to draft until these are resolved.
🤖 Generated with Claude Code
…-aware lowering) Root cause of two pre-existing runtime regressions the PR author found (`await import()` namespace → number; `Array.fromAsync` → NaN), and more broadly: EVERY `await`/`yield` of a non-numeric value was being coerced to a number. `lower_expr` speculatively tries `lower_expr_value` (the native-rep path) first for every expression. Its `Expr::IterResultGetValue` arm unconditionally lowered to the coercing `js_iter_result_get_value_f64` runtime variant (which runs `js_number_coerce` on any generic JSValue). Since the value carried by `AsyncStepChain` / `AsyncStepDone` (the awaited/resolved value threaded into the next async step) flows through `lower_expr`, every await result was numerically coerced — `await Promise.resolve(42)` returned NaN, objects/ strings/arrays became NaN/undefined. `IterResultGetValue` carries an arbitrary JSValue, so it must not be speculatively lowered as f64. Return `Ok(None)` from the native path so `lower_expr` falls through to the boxed `js_iter_result_get_value` (misc_methods). Genuinely-numeric consumers (bitwise operands, i32_fast_path) still request a native rep explicitly via `lower_expr_native`, which keeps its own raw-f64/i32/i1 reads — so numeric for-await stays fast. Validation: `await` of number/string/object/array now returns the correct value; test_gap_array_methods and test_gap_dynamic_import_literal match node; native-region-proof suite green; perry-codegen + 52 runtime promise tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ4RdAWQhxvqyS9vWeLoTB
…aptured array Root cause of the third runtime regression the PR author found (fetch `headers.forEach` dropping an entry): inside an `async` function every local is a boxed capture of the async step closure, so an array captured by a nested closure (the `forEach` callback) is BOTH `boxed_vars` and `closure_captures`. `ArrayPush`/`ArrayPushSpread` handled the boxed case by writing the realloc'd array head through `js_box_set_bits` (the box is the shared storage) — but then fell through to the capture-slot store, calling `js_closure_set_capture_bits(slot, array)`. That overwrote the capture slot, which holds the BOX pointer, with the array pointer. The next push then read the array as if it were the box (`js_box_get_bits(array)`), so the real box never received the second realloc and the pushed element was lost (`parts.length` came back 1 instead of 2). Return after the box write-back in the captured/slot sub-branches so the capture slot keeps pointing at the box. Only the genuine #5459 case (in `boxed_vars` but no box location here — a module global accessed from a nested function) still falls through to the module-global store. Validation: test_gap_fetch_response now matches node; push-in-async repros fixed (boxed captured array, for-await of objects); native-region-proof suite green; perry-codegen tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ4RdAWQhxvqyS9vWeLoTB
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
3 participants
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.
Brings the representation-aware type-lowering track into `main`. Builds on the earlier #5291 landing; the most recent integration was #5462 (the material evidence gate), squash-merged into this branch.
What this adds
Audit summary (from #5462 review)
The evidence gate is legitimate (not self-gaming): headline numbers are observed, thresholds are modest and real, the gate fails on regression. Runtime changes (map/set/bigint/promise/typed_feedback) are GC-root- and memory-safe. No confirmed correctness bugs; fast i32 paths are gated on declared `Int32[]` element type, so plain `number[]` is unaffected.
Tracking: #5464
https://claude.ai/code/session_019hwNPXCAWnjv5vddcznhFA
Summary by CodeRabbit
New Features
Improvements
Tests