Skip to content

fix(runtime): coerce class refs before dynamic addition - #9097

Merged
proggeramlug merged 1 commit into
PerryTS:mainfrom
proggeramlug:fix/9087-class-ref-add
Aug 29, 2026
Merged

fix(runtime): coerce class refs before dynamic addition#9097
proggeramlug merged 1 commit into
PerryTS:mainfrom
proggeramlug:fix/9087-class-ref-add

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • recognize registered INT32-tagged class references as Function objects in dynamic + coercion
  • run the ordinary valueOf/toString sequence so default classes concatenate their function source while static overrides remain observable
  • add an end-to-end regression for both operand orders and the looped += reproducer

No version bump.

Tests

Run on root@perrymaster.skelpo.net:

  • cargo fmt --all -- --check
  • cargo check -p perry-runtime
  • cargo test -p perry-runtime dynamic_arith --lib
  • cargo build --release -p perry-runtime-static
  • cargo test -p perry --test issue_9087_class_ref_add -- --nocapture

Closes #9087

Summary by CodeRabbit

  • Bug Fixes

    • Corrected addition and string coercion for declared classes used as values.
    • Class values now behave like function objects during dynamic + operations instead of being treated as numeric identifiers.
    • Preserved custom valueOf behavior and correct results for both operand orders and repeated additions.
  • Tests

    • Added regression coverage for class coercion, accumulator operations, and custom valueOf results.

@coderabbitai

coderabbitai Bot commented Aug 29, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9ef82dfd-7782-41dd-9d60-123a481f9b1f

📥 Commits

Reviewing files that changed from the base of the PR and between 97bfbab and 6678e7e.

📒 Files selected for processing (3)
  • crates/perry-runtime/src/value/dynamic_arith.rs
  • crates/perry-runtime/src/value/to_string.rs
  • crates/perry/tests/issue_9087_class_ref_add.rs

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


📝 Walkthrough

Walkthrough

Registered class-reference INT32 values now use Function-object ToPrimitive handling during dynamic addition. New integration coverage checks string concatenation, repeated +=, operand order, and custom valueOf.

Changes

Class Reference Coercion

Layer / File(s) Summary
Runtime coercion routing
crates/perry-runtime/src/value/dynamic_arith.rs, crates/perry-runtime/src/value/to_string.rs
Registered class references are excluded from primitive classification and routed through ordinary Function-object ToPrimitive handling.
Dynamic addition regression coverage
crates/perry/tests/issue_9087_class_ref_add.rs
The integration test compiles and runs programs that check both operand orders, repeated +=, and a custom valueOf.

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

Merge Risk: 🔵 Low · up to 6678e

Dynamic addition now treats registered class references like functions, but an ordinary integer with the same runtime identifier may also be routed through class valueOf or toString behavior, producing unexpected results or method execution within a compiled program. The PR is mergeable with explicit owner awareness and follow-up to make class-reference identification unambiguous.

Suggested reviewers: thehypnoo

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main runtime change: coercing class references before dynamic addition.
Description check ✅ Passed The description provides a clear summary, identifies issue #9087, lists concrete validation commands, and notes the absence of a version bump. It omits the template's separate Changes and Checklist se…
Linked Issues check ✅ Passed The implementation addresses issue #9087 by excluding registered INT32-tagged class references from primitive numeric handling, applying ordinary object coercion, and adding regression coverage for bo…
Out of Scope Changes check ✅ Passed The runtime changes and the integration test directly support the linked issue and stated objectives. No unrelated code changes are identified.
Full details: Description check

Explanation

The description provides a clear summary, identifies issue #9087, lists concrete validation commands, and notes the absence of a version bump. It omits the template's separate Changes and Checklist sections, but the required information is mostly present.

Full details: Linked Issues check

Explanation

The implementation addresses issue #9087 by excluding registered INT32-tagged class references from primitive numeric handling, applying ordinary object coercion, and adding regression coverage for both operand orders and repeated += operations.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Merged. Closes #9087.

Running the ordinary valueOf/toString sequence rather than special-casing a string is the right shape — it means static valueOf and static toString are both honoured with correct precedence, and an inherited static toString works through extends without extra machinery. A/B against main, 27 shapes, node v26.5.1:

shape node main this PR
4, 5 static valueOf via + (both operand orders) 42 function WithValueOf() { [native code] } 42
6, 7 static toString via + CUSTOM native-code text CUSTOM
8 both defined — valueOf wins 7 native-code text 7
10, 11 1 + C / C + 1 43 4 43
18 looped numeric n += C 126 9 126
22 extends with an inherited static toString BASE native-code text BASE

Rows correct: main 5/27 → this PR 14/27, and nothing regressed — every row the PR touches moves toward node, and the rows still wrong are byte-identical on both arms.

Case 10/11 is the one I'd highlight: main answered 4 for 1 + WithValueOf, which is a plausible-looking number rather than an obvious garbage string, so this was quietly producing wrong arithmetic rather than visibly wrong text.

Four residuals, filed as #9101, none introduced here. The one worth your attention is that the looped += fix is half-landed: the numeric accumulator (case 18) is fixed, but the string accumulator still takes the old path —

class WithToString { static toString() { return "CUSTOM"; } }
let s = ""; for (let i = 0; i < 3; i++) s += WithToString;
// node: "CUSTOMCUSTOMCUSTOM"   here: "function WithToString() { [native code] }…"

— which is notable because the PR description names the looped += reproducer as a target. Probably a narrow gap in the same area rather than a separate mechanism, so it may be a short follow-up. The other three are * and - not coercing at all (WithValueOf * 26, node 84), static Symbol.toPrimitive being ignored despite taking precedence over valueOf/toString in ToPrimitive, and class source text not being retained ("" + Plain gives function Plain() { [native code] }, node gives class Plain { }) — that last one is a lowering-level gap rather than a coercion fix, and I noted in the issue that it may already be tracked elsewhere.

Validation: runtime 2819 passed (RUST_TEST_THREADS=1), codegen 1347, perry --bins 1066, the new issue_9087_class_ref_add integration test passes, fmt clean, run_lint_gates.sh all 60 gates passed; 2 CI-only skipped.

@proggeramlug
proggeramlug merged commit db6df04 into PerryTS:main Aug 29, 2026
22 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

correctness: class reference + number treats the INT32-boxed class ref as a number (should be string concat)

1 participant