fitzgen requested wasmtime-wasi-reviewers for a review on PR #12308.
fitzgen opened PR #12308 from fitzgen:polyfill-to-wasmtime-result to bytecodealliance:main:
This commit polyfills
wasmtime_internal_error::ToWasmtimeResultinwasmtime_environ, adds the cargo feature plumbing that will eventually connect to the"wasmtime_internal_error/anyhow"cargo feature but for now just configures this polyfill, and adds uses of the polyfill at all the sites that will be necessary once we swap outanyhowfor
wasmtime_internal_error. Currently, the polyfill is just an identity function, becausewasmtime::Result/wasmtime_environ::error::Resultis just another name foranyhow::Result. Once we do move away fromanyhow, however, that will no longer be the case, and these uses will do the necessary conversion.My goal with landing this as an incremental commit is to make it so that the actual commit that does the error crate swap out can be as close to _just_ the
Cargo.tomldependency change, without any other code changes as much as possible. This, in turn, means that swap out should be super easy to revert, if necessary.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
fitzgen requested pchickey for a review on PR #12308.
fitzgen requested wasmtime-default-reviewers for a review on PR #12308.
fitzgen requested wasmtime-core-reviewers for a review on PR #12308.
alexcrichton commented on PR #12308:
Personally I'd like to see this trait used as an absolute last resort "quick hack before we figure out what's best to do instead" ideally. What would you think about, instead of using this trait internally, instead propagating the usage of
anyhow::Errorto the callers? This looks like it's mostly used in tests and such so I don't think it'll affect our public API or anything like that.Additionally, looking forward a bit, one thing I'm worried about is
bindgen!-generated traits all written withanyhow::Errorpreviously and deeply integrated with the rest of an embedding, and that would all in theory need to change and use this trait. What do you think about a newbindgen!configuration flag which, when enabled, usesto_wasmtime_result()and this trait on the result of all functions? It would generate trait signatures withanyhow::Resultinstead ofwasmtime::Result, for example.
alexcrichton requested alexcrichton for a review on PR #12308.
github-actions[bot] commented on PR #12308:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "wasmtime:api", "wizer"Thus the following users have been cc'd because of the following labels:
- fitzgen: wizer
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
fitzgen commented on PR #12308:
Personally I'd like to see this trait used as an absolute last resort "quick hack before we figure out what's best to do instead" ideally.
This trait is used as an absolute last resort, and not as a quick hack, already in this PR as it is today:
- All crates in the workspace have been migrated to
wasmtime[_environ]::Error. This makes things uniform and gives us a simple rule for development in the workspace. No weird half-migrations or having to think about which kind of error to use/import at a given location.- All
core::error::Errorimpls (including those originating from deps outside the workspace) automatically get converted intowasmtime::Errorin?propagation and viaFrom<E> for wasmtime::Error where E: core::error::Error. This is the same as when we were (or, still are) usinganyhow::Error.- We only invoke
.to_wasmtime_result()when the above cases do not apply: we are using a worskpace-external dep which returns ananyhow::Error(which does not implementcore::error::Errordue to conflicting trait impls). In this last case, we absolutely must use the.to_wasmtime_result()trait method (or some other quivalent explicit conversion) because we cannot implementFrom<anyhow::Error> for wasmtime::Error(for conflicting trait impl reasons similar to why there is noFrom<impl core::error::Error> for anyhow::Error).This is not a quick hack. This is only and exactly the places where we must insert the conversions.
What would you think about, instead of using this trait internally, instead propagating the usage of
anyhow::Errorto the callers? This looks like it's mostly used in tests and such so I don't think it'll affect our public API or anything like that.This would result in a much more confusing situation where sometimes we are using
anyhow::Errorand sometimes we are usingwasmtime::Errorbut either way it is imported locally as justErrorso you always have to check to see which one you're dealing with and at the end of the day you are still going to have to convert between the two everywhere except in tests (which are only ~1/4 of the use sites, if you look at the diff) so we would just be making things more confusing and losing a clear line for determining when to use which error but we wouldn't even avoiding most of the conversions.Additionally, looking forward a bit, one thing I'm worried about is
bindgen!-generated traits all written withanyhow::Errorpreviously and deeply integrated with the rest of an embedding, and that would all in theory need to change and use this trait. What do you think about a newbindgen!configuration flag which, when enabled, usesto_wasmtime_result()and this trait on the result of all functions? It would generate trait signatures withanyhow::Resultinstead ofwasmtime::Result, for example.Makes sense to me, but also probably something that doesn't need to happen in this PR, I don't think? If you feel strongly that it should be part of this PR, I can add it here though.
fitzgen edited a comment on PR #12308:
Personally I'd like to see this trait used as an absolute last resort "quick hack before we figure out what's best to do instead" ideally.
This trait is used as an absolute last resort, and not as a quick hack, already in this PR as it is today:
- All crates in the workspace have been migrated to
wasmtime[_environ]::Error. This makes things uniform and gives us a simple rule for development in the workspace. No weird half-migrations or having to think about which kind of error to use/import at a given location.- All
core::error::Errorimpls (including those originating from deps outside the workspace) automatically get converted intowasmtime::Errorin?propagation and viaFrom<E> for wasmtime::Error where E: core::error::Error. This is the same as when we were (or, still are) usinganyhow::Error.- We only invoke
.to_wasmtime_result()when the above cases do not apply: we are using a worskpace-external dep which returns ananyhow::Error(which does not implementcore::error::Errordue to conflicting trait impls). In this last case, we absolutely must use the.to_wasmtime_result()trait method (or some other quivalent explicit conversion) because we cannot implementFrom<anyhow::Error> for wasmtime::Error(for conflicting trait impl reasons similar to why there is noimpl core::error::Error for anyhow::Error).This is not a quick hack. This is only and exactly the places where we must insert the conversions.
What would you think about, instead of using this trait internally, instead propagating the usage of
anyhow::Errorto the callers? This looks like it's mostly used in tests and such so I don't think it'll affect our public API or anything like that.This would result in a much more confusing situation where sometimes we are using
anyhow::Errorand sometimes we are usingwasmtime::Errorbut either way it is imported locally as justErrorso you always have to check to see which one you're dealing with and at the end of the day you are still going to have to convert between the two everywhere except in tests (which are only ~1/4 of the use sites, if you look at the diff) so we would just be making things more confusing and losing a clear line for determining when to use which error but we wouldn't even avoiding most of the conversions.Additionally, looking forward a bit, one thing I'm worried about is
bindgen!-generated traits all written withanyhow::Errorpreviously and deeply integrated with the rest of an embedding, and that would all in theory need to change and use this trait. What do you think about a newbindgen!configuration flag which, when enabled, usesto_wasmtime_result()and this trait on the result of all functions? It would generate trait signatures withanyhow::Resultinstead ofwasmtime::Result, for example.Makes sense to me, but also probably something that doesn't need to happen in this PR, I don't think? If you feel strongly that it should be part of this PR, I can add it here though.
fitzgen updated PR #12308.
fitzgen requested wasmtime-fuzz-reviewers for a review on PR #12308.
fitzgen edited a comment on PR #12308:
Personally I'd like to see this trait used as an absolute last resort "quick hack before we figure out what's best to do instead" ideally.
This trait is used as an absolute last resort, and not as a quick hack, already in this PR as it is today:
- All (non-cranelift) crates in the workspace have been migrated to
wasmtime[_environ]::Error. This makes things uniform and gives us a simple rule for development in the workspace. No weird half-migrations or having to think about which kind of error to use/import at a given location.- All
core::error::Errorimpls (including those originating from deps outside the workspace) automatically get converted intowasmtime::Errorin?propagation and viaFrom<E> for wasmtime::Error where E: core::error::Error. This is the same as when we were (or, still are) usinganyhow::Error.- We only invoke
.to_wasmtime_result()when the above cases do not apply: we are using a worskpace-external dep which returns ananyhow::Error(which does not implementcore::error::Errordue to conflicting trait impls). In this last case, we absolutely must use the.to_wasmtime_result()trait method (or some other quivalent explicit conversion) because we cannot implementFrom<anyhow::Error> for wasmtime::Error(for conflicting trait impl reasons similar to why there is noimpl core::error::Error for anyhow::Error).This is not a quick hack. This is only and exactly the places where we must insert the conversions.
What would you think about, instead of using this trait internally, instead propagating the usage of
anyhow::Errorto the callers? This looks like it's mostly used in tests and such so I don't think it'll affect our public API or anything like that.This would result in a much more confusing situation where sometimes we are using
anyhow::Errorand sometimes we are usingwasmtime::Errorbut either way it is imported locally as justErrorso you always have to check to see which one you're dealing with and at the end of the day you are still going to have to convert between the two everywhere except in tests (which are only ~1/4 of the use sites, if you look at the diff) so we would just be making things more confusing and losing a clear line for determining when to use which error but we wouldn't even avoiding most of the conversions.Additionally, looking forward a bit, one thing I'm worried about is
bindgen!-generated traits all written withanyhow::Errorpreviously and deeply integrated with the rest of an embedding, and that would all in theory need to change and use this trait. What do you think about a newbindgen!configuration flag which, when enabled, usesto_wasmtime_result()and this trait on the result of all functions? It would generate trait signatures withanyhow::Resultinstead ofwasmtime::Result, for example.Makes sense to me, but also probably something that doesn't need to happen in this PR, I don't think? If you feel strongly that it should be part of this PR, I can add it here though.
alexcrichton commented on PR #12308:
Sorry yeah "quick hack" is not the right term for this, I was trying to emulate a use case of using
wasmtime::Errorin an application embedding and went a bit too far. I agree though with all your points about why this trait generally isn't needed, and it's clear that it's not pervasively needed throughout.I think you're right as well that, in this workspace, trying to sometimes use
wasmtime::Errorand sometimes useanyhow::Errorprobably isn't worth it. That would remove the need for this conversion trait in this workspace but at too high of a conceptual cost.Personally though I'm still not a fan of this trait. Idiomatically in Rust everyone expects
?to handle type conversion of errors, so requiring a trait +.to_wasmtime_result()call is pretty unintuitive. I understand there's no better solution given our technical constraints, however. One learning, perhaps, is that it's the fault of the libraries in question for producing some sort of error that doesn't implement theErrortrait (e.g.anyhow::Error). The libraries in question are apparently all me-inflicting-pain-on-myself:
- wasmprinter
- wit-component
- wasm-compose
- pulley profiling
- json-from-wast
I think it would be reasonable to update these libraries to have a more dedicated error-type-per-library which does indeed implement the
Errortrait for any one particular error. That would mean that the majority of the conversions in this PR aren't necessary.Question for you though, the other instance in this PR is the updates to the
cranelift-{fuzzgen,icache}targets fuzz targets. I think we control the code in question, right? Is that perhaps more code to transition towasmtime::Error, or a concrete error, as opposed toanyhow::Error? Because otherwise the current state of affairs is the confusion we're trying to avoid, where the internal libraries are usinganyhowbut shouldn't?also probably something that doesn't need to happen in this PR, I don't think?
Agreed yeah, the
bindgen!changes should be separate (sorry just wanted to write it down before I forgot and I was writing here).If you're ok with it I'd prefer to block landing the actual switchover until this PR is implemented, but I would hope that this PR would be pretty easy to do.
fitzgen commented on PR #12308:
One learning, perhaps, is that it's the fault of the libraries in question for producing some sort of error that doesn't implement the
Errortrait (e.g.anyhow::Error).Yes, I would agree. We really shouldn't be doing that as much as possible.
Also somewhat Rust-the-language's fault for not allowing some kind of very basic specialization or something to enable both of the following impls to co-exist:
impl<E: core::error::Error> From<E> for wasmtime::Error { ... } impl From<anyhow::Error> for wasmtime::Error { ... }Question for you though, the other instance in this PR is the updates to the
cranelift-{fuzzgen,icache}targets fuzz targets. I think we control the code in question, right? Is that perhaps more code to transition towasmtime::Error, or a concrete error, as opposed toanyhow::Error? Because otherwise the current state of affairs is the confusion we're trying to avoid, where the internal libraries are usinganyhowbut shouldn't?I wasn't totally sure what to do about cranelift code using
anyhow, which is mostly testing/fuzzing/infra stuff, and ultimately opted to leave it usinganyhowrather thanwasmtime_environ::Errorbecause that seemed (logically) like a weird dependency inversion. But then I also movedwasmtime-fuzztowasmtime::Error, so I also moved the cranelift-specific fuzz targets over towasmtime::Erroras well, but not the cranelift fuzzing crates that they depend on.I am not sure these were the right decisions. Maybe better to
- keep the cranelift fuzz targets on
anyhow, or- move the cranelift crates using
anyhowover towasmtime_environ::Error, despite the "inverted" dependency?Any opinions on this? Or we can just leave the cut as-is. Its a bit ambiguous to me.
If you're ok with it I'd prefer to block landing the actual switchover until this PR is implemented, but I would hope that this PR would be pretty easy to do.
Sounds good to me.
alexcrichton commented on PR #12308:
I'd say switch cranelift fuzzing over to
anyhowsince that's what fuzzgen already uses, and then good to land?
alexcrichton submitted PR review.
fitzgen updated PR #12308.
fitzgen has enabled auto merge for PR #12308.
github-actions[bot] commented on PR #12308:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "fuzzing"Thus the following users have been cc'd because of the following labels:
- fitzgen: fuzzing
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
fitzgen updated PR #12308.
fitzgen has enabled auto merge for PR #12308.
fitzgen merged PR #12308.
Last updated: Jan 29 2026 at 13:25 UTC