Skip to content

fix: implement TryIntoJs and TryFromJs for common Rust wrapper types#4727

Open
jakharmonika364 wants to merge 3 commits intoboa-dev:mainfrom
jakharmonika364:fix/try-into-js-coercion-4360
Open

fix: implement TryIntoJs and TryFromJs for common Rust wrapper types#4727
jakharmonika364 wants to merge 3 commits intoboa-dev:mainfrom
jakharmonika364:fix/try-into-js-coercion-4360

Conversation

@jakharmonika364
Copy link

@jakharmonika364 jakharmonika364 commented Feb 25, 2026

Closes #4360

This PR implements TryIntoJs and TryFromJs for common Rust wrapper types and refactors class conversions to resolve trait conflicts.

Key Changes:

  • Wrapper Support: Added TryIntoJs and TryFromJs support for Box, Rc, Arc, and &T.
  • Architectural Refactor: Removed the blanket TryIntoJs implementation for Class + Clone and moved it into the boa_class macro. This resolves a trait conflict that was previously blocking the new wrapper implementations.
  • Testing: Added a unit test manual_repro_4360 in try_into_js.rs to verify derived objects handle standard string coercion correctly.

@jakharmonika364 jakharmonika364 requested a review from a team as a code owner February 25, 2026 16:55
@jakharmonika364 jakharmonika364 force-pushed the fix/try-into-js-coercion-4360 branch from e1b538a to becb9be Compare February 25, 2026 16:55
@github-actions
Copy link

github-actions bot commented Feb 25, 2026

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 50,073 50,073 0
Ignored 2,072 2,072 0
Failed 818 818 0
Panics 0 0 0
Conformance 94.54% 94.54% 0.00%

Tested main commit: daf86d30277a5c1fb353825347f75c1d83516905
Tested PR commit: 4721a96d8d2c1fece6add7cf0cc64211a6d64ce5
Compare commits: daf86d3...4721a96

@hansl
Copy link
Contributor

hansl commented Feb 25, 2026

Just want to clarify, this PR does not change the TryIntoJs macro, what am I missing?

@jakharmonika364
Copy link
Author

I noticed after pushing that the prototype fix in the macro was already there in main. The main point of this PR now is implementing TryIntoJs and TryFromJs from wrapper types. I had to refactor how native classes handle conversion to resolve a trait conflict that was previously blocking those wrapper implementations. I'll update the PR description to reflect the actual changes.

@jakharmonika364 jakharmonika364 force-pushed the fix/try-into-js-coercion-4360 branch from becb9be to 2fd34c9 Compare February 26, 2026 04:27
@jedel1043 jedel1043 requested review from hansl and removed request for a team February 27, 2026 20:23
@jedel1043 jedel1043 changed the title fix: ensure TryIntoJs derived objects have Object.prototype fix: implement TryIntoJs and TryFromJs for common Rust wrapper types Feb 27, 2026
@jedel1043 jedel1043 added A-Enhancement New feature or request A-API Changes related to public APIs waiting-on-review labels Feb 27, 2026
@jedel1043 jedel1043 enabled auto-merge March 11, 2026 05:25
@codecov
Copy link

codecov bot commented Mar 11, 2026

Codecov Report

❌ Patch coverage is 12.50000% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.37%. Comparing base (6ddc2b4) to head (4721a96).
⚠️ Report is 884 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/value/conversions/try_into_js.rs 0.00% 8 Missing ⚠️
core/engine/src/value/conversions/try_from_js.rs 0.00% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4727       +/-   ##
===========================================
+ Coverage   47.24%   59.37%   +12.13%     
===========================================
  Files         476      580      +104     
  Lines       46892    63161    +16269     
===========================================
+ Hits        22154    37504    +15350     
- Misses      24738    25657      +919     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jedel1043 jedel1043 added Waiting On Review Waiting on reviews from the maintainers Waiting On Author Waiting on PR changes from the author and removed waiting-on-review Waiting On Review Waiting on reviews from the maintainers labels Mar 15, 2026
@jedel1043
Copy link
Member

Needs a cargo clippy fix

auto-merge was automatically disabled March 17, 2026 11:38

Head branch was pushed to by a user without write access

@jakharmonika364 jakharmonika364 force-pushed the fix/try-into-js-coercion-4360 branch from b6a4632 to 29287f7 Compare March 17, 2026 11:38
@github-actions github-actions bot added Waiting On Review Waiting on reviews from the maintainers C-Builtins PRs and Issues related to builtins/intrinsics labels Mar 17, 2026
@jakharmonika364 jakharmonika364 force-pushed the fix/try-into-js-coercion-4360 branch from 29287f7 to f131f6e Compare March 17, 2026 11:43
@jakharmonika364
Copy link
Author

Needs a cargo clippy fix

Fixed the cargo clippy warning by replacing str::replace with cow_utils::CowUtils::cow_replace to avoid unnecessary heap allocation when the specifier doesn't contain /.

jakharmonika364 and others added 3 commits March 18, 2026 21:35
Use `&*specifier` to deref Cow<str> for Path::new and join calls,
and use the full path `cow_utils::CowUtils::cow_replace` without import.
@jakharmonika364 jakharmonika364 force-pushed the fix/try-into-js-coercion-4360 branch from 13271f3 to 4721a96 Compare March 18, 2026 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-API Changes related to public APIs A-Enhancement New feature or request C-Builtins PRs and Issues related to builtins/intrinsics Waiting On Author Waiting on PR changes from the author Waiting On Review Waiting on reviews from the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

toString missing on TryIntoJs

3 participants