refactor(engine): encapsulate Context VM field access behind wrapper methods#5149
refactor(engine): encapsulate Context VM field access behind wrapper methods#5149hansl wants to merge 3 commits intoboa-dev:mainfrom
Conversation
…methods Add `context/vm_access.rs` with 45 wrapper methods that delegate to `Context`'s internal `Vm` field, then replace all ~600 direct `context.vm.*` accesses across 78 files with calls to these wrappers. This is the first step toward extracting Context into its own crate (boa_context). By routing all external access through methods, the internal structure of Context can be changed without touching call sites. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
added
the
Waiting On Review
github-actions
bot
added
C-Builtins
Test262 conformance changes
Tested main commit: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5149 +/- ##
===========================================
+ Coverage 47.24% 59.80% +12.55%
===========================================
Files 476 583 +107
Lines 46892 63496 +16604
===========================================
+ Hits 22154 37971 +15817
- Misses 24738 25525 +787 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| let in_with = context.vm.frame().environments.has_object_environment(); | ||
| let in_with = context.frame().environments.has_object_environment(); |
There was a problem hiding this comment.
I think this shows we would also need to extract the environment related types, but those depend on JsObject, so...
Maybe we should start by extracting JsObject first?
There was a problem hiding this comment.
And JsObject depends on Context.
Regardless, this PR would still be needed. And it's a simple refactor, there should be no behaviour changes. I can do a follow to address the environment types.
There was a problem hiding this comment.
Yeah this doesn't block merging this, but we also need to think what exactly are the steps to properly extract this into its own crate
Test262 conformance changes
Tested main commit: |
github-actions
bot
added
the
C-Builtins
|
|
||
| /// Pop a value from the VM stack. | ||
| #[track_caller] | ||
| pub(crate) fn stack_pop(&mut self) -> JsValue { |
There was a problem hiding this comment.
Do we need to have individual methods to pop and push from the stack here? I would expect that just returning &mut Stack should be enough
There was a problem hiding this comment.
It's the same approach we follow for the EnvironmentStack, so it makes sense to also use the same approach here.
|
|
||
| impl Context { | ||
| /// Get the loop iteration limit. | ||
| pub(crate) fn loop_iteration_limit(&self) -> u64 { |
There was a problem hiding this comment.
Same suggestion here. We should return &RuntimeLimits instead.
2 participants
Add
context/vm_access.rswith 45 wrapper methods that delegate toContext's internalVmfield, then replace all ~600 directcontext.vm.*accesses across 78 files with calls to these wrappers.This is the first step toward extracting Context into its own crate (boa_context). By routing all external access through methods, the internal structure of Context can be changed without touching call sites.