-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Migrate the wasmtime crate to wasmtime_environ::error::*
#12231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate the wasmtime crate to wasmtime_environ::error::*
#12231
Conversation
a6c7162 to
9b06a14
Compare
Instead of `anyhow::Error`.
This commit re-exports the `wasmtime_environ::error` as the `wasmtime::error`
module, updates the prelude to include these new error-handling types, redirects
our top-level `wasmtime::{Error, Result}` re-exports to re-export
`wasmtime::error::{Error, Result}`, and updates various use sites that were
directly using `anyhow` to use the new `wasmtime` versions.
This process also required updating the component macro and wit-bindgen macro to
use the new error types instead of `anyhow`.
Part of bytecodealliance#12069
9b06a14 to
0b4fae7
Compare
wasmtime crate to wasmtime_internal_error::Errorwasmtime crate to wasmtime_environ::error::*
|
In retrospect, should we bother with |
I think we do want a module because otherwise we would have |
Happy to change it, but also this seems like it shouldn't really matter since it is just in macro-expanded code? |
|
Scattered thought from some more review:
|
I would say it is 100% a deal-breaker for existing projects that already embed Wasmtime, currently use
I agree, which is why I am focusing on not only the Wasmtime developers' ergonomics but also embedders' ergonomics. The best ergonomics is "you don't need to change anything in your existing code and you're already familiar with all the APIs" which is what we get with matching
This is actually another example of why we cannot just export everything at the top level. A ton of our code in examples, tests, and docs do
Having a top-level alias of an item defined in a nested module seems like a very minor blemish to me. Like I guess in a platonic ideal world it would be nice if it didn't exist, and there was only one path for the item, but it also doesn't seem like it actually hurts at all? Simultaneously, it seems pretty common for crates to re-export the most-common functionality from a nested module to the top level: even |
|
Personally I'm not too sold on the downstream churn argument. For example I'd disagree that I would also say that we're teeing up a lot of churn for downstream users because for embeddings it's likely that With this I'd say that the specific context of naming I guess what I'm getting at is:
Given two distinct nominal error types ( r.e.
Agree yeah and I'm not saying it's absolutely imperative we don't have duplication. What I'm saying is that there should be a more-or-less canonical path that everything is used from. For example most crates use Overall:
|
I think we are mostly on the same page. Regarding the last bullet point, I just don't want to give up on migration ergonomics by-default and only want us to do it deliberately when we have motivation that overrides those ergonomics. There's flex but we should be cognizant when we are making that trade off, not just accepting it as a lost cause from the start. Latest commit has replaced all
|
| use crate::component::{ | ||
| ComponentInstanceId, HasData, HasSelf, Instance, Resource, ResourceTable, ResourceTableError, | ||
| }; | ||
| use crate::error::{Context as _, Result, anyhow, bail}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/::error// for non-Context things here
| use crate::component::{ | ||
| ComponentInstanceId, ComponentType, FutureReader, Lift, Lower, StreamReader, | ||
| }; | ||
| use crate::error::{Context, Result, bail}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/::error// for non-Context things here
| AsAccessor, ComponentInstanceId, ComponentType, FutureAny, Instance, Lift, Lower, StreamAny, | ||
| Val, WasmList, | ||
| }; | ||
| use crate::error::{Context as _, Error, Result, anyhow, bail}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/::error// for non-Context things here
| @@ -1,6 +1,6 @@ | |||
| use super::{TypedResource, TypedResourceIndex}; | |||
| use crate::error::{Result, bail}; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/::error//
| use crate::component::func::{LiftContext, LowerContext}; | ||
| use crate::component::matching::InstanceType; | ||
| use crate::component::{ComponentType, Lift, Lower, Val}; | ||
| use crate::error::{Result, anyhow, bail}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/::error//
crates/wasmtime/src/runtime/fiber.rs
Outdated
| @@ -1,9 +1,9 @@ | |||
| use crate::error::{Result, anyhow}; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/::error// (or maybe remove entirely due to the prelude import here)
Instead of
anyhow::Error.This commit re-exports
wasmtime_environ::erroras thewasmtime::errormodule, updates the prelude to include these new error-handling types, redirects our top-levelwasmtime::{Error, Result}re-exports to re-exportwasmtime::error::{Error, Result}, and updates various use sites that were directly usinganyhowto use the newwasmtimeversions.This process also required updating the component macro and wit-bindgen macro to use the new error types instead of
anyhow.Part of #12069