Skip to content

fix(object/ module/): convert remaining panics to EngineError::Panic#5311

Draft
KaustubhOG wants to merge 1 commit intoboa-dev:mainfrom
KaustubhOG:fix/object-followup
Draft

fix(object/ module/): convert remaining panics to EngineError::Panic#5311
KaustubhOG wants to merge 1 commit intoboa-dev:mainfrom
KaustubhOG:fix/object-followup

Conversation

@KaustubhOG
Copy link
Copy Markdown
Contributor

@KaustubhOG KaustubhOG commented Apr 9, 2026

Part of #3241. Follow-up to #5063 addressing review comment.

Changes:

  • operations.rs: converted 5 panics — ToObject, CreateDataPropertyOrThrow (js_expect), private_field_add, private_method_or_accessor_add, private_set (try_borrow_mut)
  • internal_methods/mod.rs: migrated 6 borrow call sites to try_borrow/try_borrow_mut
  • internal_methods/string.rs: migrated 3 borrow call sites, return type of string_get_own_property changed to JsResult<Option>
  • builtins/jsdataview.rs: migrated 2 borrow call sites to try_borrow

@github-actions github-actions bot added the Waiting On Review Waiting on reviews from the maintainers label Apr 9, 2026
@github-actions github-actions bot added this to the v1.0.0 milestone Apr 9, 2026
@KaustubhOG
Copy link
Copy Markdown
Contributor Author

@jedel1043 Following up on your review in #5063 regarding the borrow()/borrow_mut() guards in jsobject.rs (lines 820, 850).

Changing their return types from Ref<'_, Object<T>>/RefMut<'_, Object<T>> to JsResult<...> would cascade into hundreds of call sites across object/, shape/, and internal_methods/. Two possible approaches:

  1. Change the foundation methods themselves and update all call sites
  2. Leave the foundation methods as-is and only convert specific call sites inside JsResult-returning functions to use try_borrow + js_expect directly

does thiss idea looks fine ?

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

Test262 conformance changes

Test result main count PR count difference
Total 53,125 53,125 0
Passed 51,049 51,049 0
Ignored 1,482 1,482 0
Failed 594 594 0
Panics 0 0 0
Conformance 96.09% 96.09% 0.00%

Tested main commit: d6d76d86e9e18fca07f318f1db434cc8abffaf14
Tested PR commit: ab5705d887e525fd5aa4e066f0abd1623b4c8d20
Compare commits: d6d76d8...ab5705d

@jedel1043
Copy link
Copy Markdown
Member

Changing their return types from Ref<', Object>/RefMut<', Object> to JsResult<...> would cascade into hundreds of call sites across object/, shape/, and internal_methods/.

I was more thinking of a progressive migration to try_borrow and try_borrow_mut, leaving the panicking methods there. Users would still wanna panic sometimes, so having that API in place is still useful.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 9, 2026

Codecov Report

❌ Patch coverage is 56.25000% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.77%. Comparing base (6ddc2b4) to head (ab5705d).
⚠️ Report is 947 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/object/internal_methods/string.rs 25.00% 12 Missing ⚠️
core/engine/src/object/builtins/jsdataview.rs 0.00% 5 Missing ⚠️
core/engine/src/object/operations.rs 50.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5311       +/-   ##
===========================================
+ Coverage   47.24%   59.77%   +12.52%     
===========================================
  Files         476      589      +113     
  Lines       46892    63690    +16798     
===========================================
+ Hits        22154    38069    +15915     
- Misses      24738    25621      +883     

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

@KaustubhOG
Copy link
Copy Markdown
Contributor Author

Changing their return types from Ref<', Object>/RefMut<', Object> to JsResult<...> would cascade into hundreds of call sites across object/, shape/, and internal_methods/.

I was more thinking of a progressive migration to try_borrow and try_borrow_mut, leaving the panicking methods there. Users would still wanna panic sometimes, so having that API in place is still useful.

Ok. Will migrate call sites inside JsResult-returning functions across object/ to use try_borrow/try_borrow_mut directly, leaving the panicking foundation methods in place. Will update this PR shortly.

@KaustubhOG KaustubhOG force-pushed the fix/object-followup branch from 242be67 to ab5705d Compare April 9, 2026 22:56
@github-actions github-actions bot added the C-Builtins PRs and Issues related to builtins/intrinsics label Apr 9, 2026
@KaustubhOG
Copy link
Copy Markdown
Contributor Author

@jedel1043 Updated. Migrated all eligible borrow/borrow_mut call sites inside JsResult-returning functions across object/ to use try_borrow/try_borrow_mut directly.

Remaining .borrow()/.borrow_mut() calls in object/ are either in non-JsResult contexts (bool, Option, () returns), test blocks, or the foundation borrow()/borrow_mut() methods themselves ,all left as-is per suggestion.

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

Labels

C-Builtins PRs and Issues related to builtins/intrinsics Waiting On Review Waiting on reviews from the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants