Polyfill ToWasmtimeResult and add eventually-necessary call sites

[fitzgen]

This commit polyfills wasmtime_internal_error::ToWasmtimeResult in wasmtime_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 out anyhow for
wasmtime_internal_error. Currently, the polyfill is just an identity function, because wasmtime::Result/wasmtime_environ::error::Result is just another name for anyhow::Result. Once we do move away from anyhow, 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.toml dependency 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.

[alexcrichton]

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::Error to 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 with anyhow::Error previously 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 new bindgen! configuration flag which, when enabled, uses to_wasmtime_result() and this trait on the result of all functions? It would generate trait signatures with anyhow::Result instead of wasmtime::Result, for example.

[github-actions]

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 .github/subscribe-to-label.json configuration file.

Learn more.

[fitzgen]

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::Error impls (including those originating from deps outside the workspace) automatically get converted into wasmtime::Error in ? propagation and via From<E> for wasmtime::Error where E: core::error::Error. This is the same as when we were (or, still are) using anyhow::Error.
• We only invoke .to_wasmtime_result() when the above cases do not apply: we are using a worskpace-external dep which returns an anyhow::Error (which does not implement core::error::Error due 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 implement From<anyhow::Error> for wasmtime::Error (for conflicting trait impl reasons similar to why there is no 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::Error to 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::Error and sometimes we are using wasmtime::Error but either way it is imported locally as just Error so 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 with anyhow::Error previously 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 new bindgen! configuration flag which, when enabled, uses to_wasmtime_result() and this trait on the result of all functions? It would generate trait signatures with anyhow::Result instead of wasmtime::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]

Sorry yeah "quick hack" is not the right term for this, I was trying to emulate a use case of using wasmtime::Error in 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::Error and sometimes use anyhow::Error probably 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 the Error trait (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 Error trait 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 to wasmtime::Error, or a concrete error, as opposed to anyhow::Error? Because otherwise the current state of affairs is the confusion we're trying to avoid, where the internal libraries are using anyhow but 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]

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 Error trait (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 to wasmtime::Error, or a concrete error, as opposed to anyhow::Error? Because otherwise the current state of affairs is the confusion we're trying to avoid, where the internal libraries are using anyhow but 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 using anyhow rather than wasmtime_environ::Error because that seemed (logically) like a weird dependency inversion. But then I also moved wasmtime-fuzz to wasmtime::Error, so I also moved the cranelift-specific fuzz targets over to wasmtime::Error as 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 anyhow over to wasmtime_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]

I'd say switch cranelift fuzzing over to anyhow since that's what fuzzgen already uses, and then good to land?

[github-actions]

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 .github/subscribe-to-label.json configuration file.

Learn more.