feat(wasip3): implement wasi:cli

[rvolosatovs]

follow-up on #11221, this implements wasi:cli, mostly by moving and cleaning up the implementation from https://github.com/bytecodealliance/wasip3-prototyping

[alexcrichton]

Would it be possible to use std::io::IsTerminal instead?

[alexcrichton]

My guess is "no" given the litany of implementations we have, but in such a case could documentation be added that this is basically a mirror of what's in libstd? Also is it worth trying to add a blanket impl<T: std::IsTerminal> wasi::IsTermainal for T impl?

[rvolosatovs]

We cannot use the std::io::IsTerminal, because it's sealed: https://github.com/rust-lang/rust/blob/f8f6997469237299c1d60814c7b9828602a1f8e4/library/std/src/io/stdio.rs#L1197

Unfortunately, largely because of that, blanket implementations for T also do not appear to be possible.

For example:

error[E0119]: conflicting implementations of trait `cli::IsTerminal` for type `tokio::io::Stderr`
   --> crates/wasi/src/cli.rs:126:1
    |
66  | impl<T: std::io::IsTerminal> IsTerminal for T {
    | --------------------------------------------- first implementation here
...
126 | impl IsTerminal for tokio::io::Stderr {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `tokio::io::Stderr`
    |
    = note: upstream crates may add a new impl of trait `std::sealed::Sealed` for type `tokio::io::Stderr` in future versions
    = note: upstream crates may add a new impl of trait `std::io::IsTerminal` for type `tokio::io::Stderr` in future versions

tokio::io::Stderr cannot implement std::io::IsTerminal, because it's sealed and we cannot implement wasmtime_wasi::cli::IsTerminal for it if we provide a blanket implementation for std::io::IsTerminal

[alexcrichton]

FWIW a trick I've used in the past is to use T::clocks(self) in the body which forcibly ensures that recursion doesn't happen and avoids ** syntax as well. Just a style thing though, no meaningful difference

[alexcrichton]

Could the InputStream and OutputStream traits directly have a Send supertrait to avoid needing InputStream + Send for example in the builder and such?

[alexcrichton]

Would it be possible to drop Unpin since this is already in a box anyway?

[alexcrichton]

I'm a bit wary of making the implementation more generic than it already is, so would it be possible to drop these associated types and hardwire the trait objects? Or are these needed to multiplex the p2/p3 implementation?

[rvolosatovs]

Depends, if these per-proposal "View" traits are adopted by p2 implementation, then the associated types would be used for exactly this purpose. Currently, only the p3 implementation uses these traits.

I'll move this trait to wasmtime_wasi::p3::cli::WasiCliView without the associated types for now and maybe we can revisit once we have all of WASI implemented in this repo.

[alexcrichton]

Could you explain a bit more as to why this is necessary?

I forget the matrix of types/traits involved here, but I would naively expect that &mut WasiImpl<Self> is basically equivalent to WasiImpl<&mut Self>

[rvolosatovs]

I was hoping for that as well, but I've spent roughly 1h actively working on it and roughly an hour more trying things out while in meetings, but I could not figure it out without significant API changes, so I just gave up and went with the obvious solution requiring unsafe. Really, all we need here is to be able to "wrap" the value in the store.

What I'd like to get from this implementation:

• be able to select a proposal to link (wasi:cli in this case) and:
  • only implement parts relevant for that proposal (WasiCliView instead of WasiView)
  • add symbols to linker using a familiar API (wasmtime_wasi::p3::cli::add_to_linker) without having to resort to lower-level, generated wasmtime_wasi::p3::bindings::wasi::cli::* per-interface functions - which is error prone, tedious and introduces additional maintenance burden
• be able to link all of WASI, i.e. implement WasiView and call wasmtime_wasi::p3::add_to_linker

The convention is that we implement interfaces for WasiCliImpl<T>, where T: WasiCliView. The challenge then in the likes of:

wasmtime/crates/wasi/src/p3/clocks/mod.rs

Line 74 in 90ab791

type Data<'a> = WasiClocksImpl<&'a mut T>;

We need a WasiCliImpl(&mut T), where T implements WasiCliView, but I don't see a way to acquire such a T, when we start with WasiView. I can't do a |v| &mut WasiImpl(v), since that would create an intermediate reference, which would be immediately dropped. I don't see a way to thread the lifetime through in a way that supports two different traits directly here.
We also currently have a separate trait for getting the resource table and we cannot call both WasiCliView::cli and ResourceView::table inside the closure, since that would make us mutably borrow twice

There are workarounds though:

1. Have WasiCliView::cli return a tuple (&mut WasiCliCtx, &mut ResourceTable)
2. Store ResourceTable in WasiCliCtx

This morning I actually got another idea, which I'm going to try now: WasiCliCtxView<'a>, which would have to &mut borrows, &mut WasiCliCtx and &mut ResourceTable, so WasiCliView::cli would have to return that. A bit awkward to use, but potentially it could work and hopefully it'd only be required in this lower level/advanced use case API

[rvolosatovs]

It worked! No WasiCliImpl and no need for generics in the host implementations anymore

[rvolosatovs]

Semantics of wasi:cli stdio are not completely clear to me at this point, so I don't want to spend too much time designing the API for it, especially since this interface will likely change soon. There's currently no out-of-the-box way to "capture" stdout/stderr, users will have to implement their own with synchronization (e.g. using a Mutex) for now

refs WebAssembly/wasi-cli#65
refs WebAssembly/wasi-cli#64

[alexcrichton]

Nice I like how the changes worked out, thanks for pushing on it!

[alexcrichton]

There's currently no out-of-the-box way to "capture" stdout/stderr

Agreed yeah this is ok for now. For anything else along these lines mind opening an issue for that? I'll open an issue for this one specifically

[alexcrichton]

Oh while cargo vet I think was spurious the failure on Windows I think is valid which is this line. IIRC we set a few extra env vars for Windows for WASI testing