refactor(crt): add exhaustive wasmtime trap classification#15755
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15755 +/- ##
==========================================
- Coverage 69.99% 69.97% -0.02%
==========================================
Files 938 939 +1
Lines 198599 197841 -758
Branches 198599 197841 -758
==========================================
- Hits 139003 138439 -564
+ Misses 54926 54752 -174
+ Partials 4670 4650 -20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors Wasmtime trap handling in near-vm-runner by introducing an owned, exhaustive trap classification enum to avoid silently swallowing new wasmtime::Trap variants (which is #[non_exhaustive]) on Wasmtime upgrades.
Changes:
- Add
TrapClassification(mirror ofwasmtime::Trap) with explicit mapping of all currently known trap variants and a fallbackUnknown. - Update
IntoVMError for wasmtime::Errorto convert traps viaTrapClassificationinstead of matchingwasmtime::Trapdirectly. - Add a unit test that iterates
Trap::from_u8over all discriminants and asserts no current trap maps toUnknown; extend cspell wordlist.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| runtime/near-vm-runner/src/wasmtime_runner/trap_classification.rs | New owned trap mirror enum + exhaustive classification logic and a regression test to catch new Wasmtime trap variants. |
| runtime/near-vm-runner/src/wasmtime_runner/mod.rs | Switch trap-to-NEAR-error mapping to go through TrapClassification. |
| cspell.json | Add “Waitable” to accommodate new trap variant naming in code/comments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn from(trap: Trap) -> Self { | ||
| match trap { | ||
| Trap::StackOverflow => Self::StackOverflow, | ||
| Trap::MemoryOutOfBounds => Self::MemoryOutOfBounds, |
There was a problem hiding this comment.
The Copilot suggestion has a valid point about readability. Switched to using explicit bindings.
🤖 -- Claude on behalf of Darioush
Introduces a `TrapClassification` mirror enum that we own, replacing the raw `wasmtime::Trap` match in `into_vm_error`. Since `wasmtime::Trap` is `#[non_exhaustive]`, the catch-all arm silently swallows new variants on Wasmtime upgrades. The new `From<Trap>` impl explicitly classifies every current variant, and a unit test iterates all discriminants via `Trap::from_u8` to ensure no variant lands in `Unknown`. Each disabled-proposal variant is annotated with the Wasm proposal it belongs to (fuel, threads, GC/function-references, component model, component-model async, stack switching, pulley).
Introduces a
TrapClassificationmirror enum that we own, replacing the rawwasmtime::Trapmatch ininto_vm_error. Sincewasmtime::Trapis#[non_exhaustive], the catch-all arm silently swallows new variants on Wasmtime upgrades. The newFrom<Trap>impl explicitly classifies every current variant, and a unit test iterates all discriminants viaTrap::from_u8to ensure no variant lands inUnknown. Each disabled-proposal variant is annotated with the Wasm proposal it belongs to (fuel, threads, GC/function-references, component model, component-model async, stack switching, pulley).