Skip to content

fix(module/source.rs,namespace.rs,synthetic.rs): convert panics to EngineError::Panic using js_expect#5064

Merged
nekevss merged 1 commit intoboa-dev:mainfrom
KaustubhOG:fix/module-panics
Mar 14, 2026
Merged

fix(module/source.rs,namespace.rs,synthetic.rs): convert panics to EngineError::Panic using js_expect#5064
nekevss merged 1 commit intoboa-dev:mainfrom
KaustubhOG:fix/module-panics

Conversation

@KaustubhOG
Copy link
Copy Markdown
Contributor

Part of #3241. It changes the following:

core/engine/src/module/source.rs: converted 24 panics to use js_expect.
core/engine/src/module/namespace.rs: converted 11 panics to use js_expect.
core/engine/src/module/synthetic.rs: converted 4 panics to use js_expect.

This completes all real panics in module/. The remaining .expect() calls in module/ are either in non-JsResult functions (load(), inner_load(), load_link_evaluate(), link(), execute_async()), inside debug_assert! blocks, or in #[cfg(test)] blocks and are left as-is.

@KaustubhOG KaustubhOG requested a review from a team as a code owner March 14, 2026 16:00
@github-actions
Copy link
Copy Markdown

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 49,935 49,935 0
Ignored 2,207 2,207 0
Failed 821 821 0
Panics 0 0 0
Conformance 94.28% 94.28% 0.00%

Tested main commit: 5acb570038007c606d75b17bed6ee9a02117ce97
Tested PR commit: a2a875b326340ee89202991d19340cee4c15393d
Compare commits: 5acb570...a2a875b

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 14, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.98%. Comparing base (6ddc2b4) to head (a2a875b).
⚠️ Report is 853 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/module/source.rs 46.66% 8 Missing ⚠️
core/engine/src/module/namespace.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5064       +/-   ##
===========================================
+ Coverage   47.24%   58.98%   +11.73%     
===========================================
  Files         476      563       +87     
  Lines       46892    62569    +15677     
===========================================
+ Hits        22154    36905    +14751     
- Misses      24738    25664      +926     

☔ 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 A-Enhancement New feature or request A-Internal Changes that don't modify execution behaviour labels Mar 14, 2026
@jedel1043 jedel1043 added this to the v1.0.0 milestone Mar 14, 2026
@nekevss nekevss added this pull request to the merge queue Mar 14, 2026
Merged via the queue into boa-dev:main with commit 3a2f3c3 Mar 14, 2026
19 checks passed
@KaustubhOG
Copy link
Copy Markdown
Contributor Author

image

@KaustubhOG
Copy link
Copy Markdown
Contributor Author

Remaining module/ panics audit — all invariants, zero convertible

File(s) Hits Reason
module/source.rs 5 .expect("should be linking") — state machine invariant, transition only reachable from correct state. .expect("cannot fail for the %Promise% intrinsic") / .expect("%Promise% constructor must always return a Promise object") / .expect("async modules cannot directly throw") — spec-guaranteed %Promise% intrinsic invariants. panic! on exhaustive match arm — unreachable by construction.
module/synthetic.rs 2 .expect("default resolve functions cannot throw and must return a promise") — spec-guaranteed promise invariant. .expect("should have the module environment") — environment set during link phase, guaranteed present at evaluate.
module/namespace.rs 2 Both .expect("6. Assert: binding is a ResolvedBinding Record.") — verbatim spec assert, binding is guaranteed non-empty by the caller's resolve step.
module/loader/embedded.rs 1 .expect("Invalid compress type") — called on a compile-time &'static str constant from the macro, cannot be an invalid value at runtime.
module/mod.rs (tests) 7 All inside #[test] functions — .unwrap() on context builder and module parse. Not in scope for #3241.
Total 17 0 convertible

All remaining panics fall into: spec-mandated assert invariants, module state machine invariants, %Promise% intrinsic guarantees, compile-time constant .expect, or test assertions. None are suitable for EngineError conversion.

@jedel1043
Copy link
Copy Markdown
Member

Uh I'd argue these kind of state transition steps should also be engine panics, since those have the highest probability of being incorrect

Also, I think I've mentioned the spec might also have bugs, so it is safer to engine panic on spec assertions.

@KaustubhOG
Copy link
Copy Markdown
Contributor Author

Uh I'd argue these kind of state transition steps should also be engine panics, since those have the highest probability of being incorrect

Also, I think I've mentioned the spec might also have bugs, so it is safer to engine panic on spec assertions.

Yup , just saw yours comment on the 5063 , will convert the statetranstions and spec assertions ones .

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

Labels

A-Enhancement New feature or request A-Internal Changes that don't modify execution behaviour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants