fix: replace panics with proper error handling in module env and dyna…#5051
Open
tkshsbcue wants to merge 2 commits intoboa-dev:mainfrom
Open
fix: replace panics with proper error handling in module env and dyna…#5051tkshsbcue wants to merge 2 commits intoboa-dev:mainfrom
tkshsbcue wants to merge 2 commits intoboa-dev:mainfrom
Conversation
…mic import - Replace panic!() in load_dyn_import with JsNativeError::TypeError when a synthetic module is unexpectedly used as a referrer - Replace panic!() in ModuleEnvironment::set with JsNativeError::TypeError when attempting to modify an indirect binding reference - Propagate JsResult through the environment set() call chain: DeclarativeEnvironment, put_lexical_value, put_value_if_uninitialized - Update all callers to handle the new Result return type
Test262 conformance changes
Fixed tests (2):Broken tests (175):Tested main commit: |
Contributor
Author
|
@jedel1043 i hope you can review my PR Thanks! |
Member
|
We still want to signal some kind of panic, because if we're trying to get an indirect binding by index, it means that our bytecompiler or our VM has a bug in the bindings resolution code. I would suggest changing the error in |
ModuleEnvironment::set on an indirect binding indicates an internal bug in the bytecompiler or VM binding resolution, not a user-facing error. Use PanicError to signal this correctly.
Contributor
Author
|
hey @jedel1043 i have taken than account and fixed that |
Member
|
Need to rebase |
jedel1043
added
A-Technical Debt
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
2 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.
Description
This PR replaces two
panic!()calls with properJsNativeError::TypeErrorreturns, preventing the engine from crashing on edge cases that should produce JavaScript errors instead.1. Dynamic import with synthetic module referrer
In
load_dyn_import, if a synthetic module is unexpectedly used as a referrer for a dynamicimport()call, the engine panicked. This now returns aTypeError, allowing the promise to reject gracefully instead of crashing the process.File:
vm/opcode/call/mod.rs:3812. Module environment indirect binding mutation
In
ModuleEnvironment::set, attempting to set an indirect binding (a re-export from another module) caused apanic!(). Per the ECMAScript spec, module export bindings are immutable from the importing side. This now returns aTypeError.File:
environments/runtime/declarative/module.rs:108This required propagating
JsResult<()>through the environmentset()call chain:LexicalEnvironment::set,GlobalEnvironment::set,FunctionEnvironment::set(trivially returnOk(()))DeclarativeEnvironmentKind::set,DeclarativeEnvironment::setEnvironmentStack::put_lexical_value,EnvironmentStack::put_value_if_uninitialized?Changes
vm/opcode/call/mod.rspanic!()->Err(JsNativeError::typ())inload_dyn_importenvironments/runtime/declarative/module.rspanic!()->Err(JsNativeError::typ())inset()environments/runtime/declarative/lexical.rsset()returnsJsResult<()>environments/runtime/declarative/global.rsset()returnsJsResult<()>environments/runtime/declarative/function.rsset()returnsJsResult<()>environments/runtime/declarative/mod.rsJsResultthroughset()environments/runtime/mod.rsput_lexical_valueandput_value_if_uninitializedreturnJsResult<()>vm/opcode/define/mod.rsDefVarandPutLexicalValueoperations returnJsResult<()>module/source.rs?onput_lexical_valuecallsmodule/synthetic.rs?onset()andput_lexical_valuecallsbuiltins/function/mod.rs?onput_lexical_valuecallsbuiltins/function/arguments.rsset()result for mapped argumentsTest plan
cargo check-- clean, no warningscargo clippy-- no warningscargo test -p boa_engine --lib-- all 958 tests pass