Use NonZeroU32::new_unchecked to convert wasi error

[Cyborus04]

The check in NonZeroU32::new isn't needed, as wasi::Error is NonZeroU16 internally, so wasi::Error::raw_error will never return zero

[josephlr]

Thanks for opening this.

I think it's fine to leave this as a call to unwrap. If what you say is accurate, it won't ever trigger and doesn't cost anything. It also has the advantage of avoiding unnecessary unsafe code. It's not like the single call to unwrap is going to cost anything.

It might be worth adding/chaining a comment explaining why it is OK to call unwrap() though, as we do wish to avoid panics in this library.

[newpavlov]

At the time I thought that having unwrap in the error conversion path would be fine since compiler should be able to remove the panic completely, but according to godbolt surprisingly it's not the case today. Ideally we would simply use the Into impl, but it would require bumping MSRV to 1.41. Can you please add a respective TODO comment?

Also I think it could be worth to join the unsafes into one:

unsafe {
    random_get(dest.as_mut_ptr(), dest.len())
        .map_err(|e| NonZeroU32::new_unchecked(e.raw_error() as u32).into())
}

[josephlr]

@newpavlov do you thing the unsafe code is worth the "cost" of a not-taken jump instruction? It's always going to be predicted correctly regardless. There's also not a guarantee in the wasi::Error::raw_error() docs that the function cannot return zero.

I think the better solution is for wasi::Error to implement a conversion to NonZeroU16, and for us to just use that.

[newpavlov]

do you thing the unsafe code is worth the "cost" of a not-taken jump instruction?

Considering that the conversion happens only for errors, which are already extremely rare, I am not concerned about runtime efficiency in this case at all. But I am not fond of having clearly redundant and easily removable panic paths in compiled results.

There's also not a guarantee in the wasi::Error::raw_error() docs that the function cannot return zero.

IIRC it's guaranteed by WASI docs, i.e. it specifies that zero return code always represents "success". I agree that ideally we would use a method returning NonZeroU16, but unfortunately it does not exist currently. We could use the raw random_get directly, but I don't think it's worth the trouble and it may result in additional headache when hypothetical wasi_snapshot_preview2 is released.

[josephlr]

IIRC it's guaranteed by WASI docs, i.e. it specifies that zero return code always represents "success". I agree that ideally we would use a method returning NonZeroU16, but unfortunately it does not exist currently. We could use the raw random_get directly, but I don't think it's worth the trouble and it may result in additional headache when hypothetical wasi_snapshot_preview2 is released.

Sounds reasonable, I agree that using the underlying raw call is not worth it. Approved modulo @newpavlov's suggested changes.

[Cyborus04]

Should I open a PR for Error: Into<NonZeroU16> in wasi?

Edit: specifically, is there any reason not to?

[newpavlov]

Should I open a PR for Error: Into in wasi?

I think it would be a good addition to wasi, but I think we can merge this PR with the raw_error code and update it later after release with the new impl (or method) will land.

[Cyborus04]

I had no idea unsafe blocks propagated into closures

[Cyborus04]

I think it would be a good addition to wasi, but I think we can merge this PR with the raw_error code and update it later after release with the new impl (or method) will land.

I'll go make a PR issue* for that, then make another one here for if/when that one gets merged (and published)

(*since it's auto-generated, it's not just as simple as fork->implement->PR, so I'll open an issue for it instead)