Replace a panic with an error branch

[jrvanwhy]

This enables Clippy's expect_used lint, which errors on calls to Option::expect and Result::{expect, expect_err}. The intent of enabling this lint is to reduce the number of panics emitted in zerocopy's code (issue #202).

[jrvanwhy]

Question: is expect_used worth the false positives? It triggered three times:

1. True positive on AsBytes::write_to_suffix on a dead branch. I think this expect would have been optimized away but this change makes it not emit an expect call in the first place.
2. False positive on Ref::new_slice, which is documented as panicing.
3. False positive on FromZeroes::new_box_slice_zeroed, which is documented as panicing.

[jrvanwhy]

Question: This branch is already out-of-date and needs changes, do you prefer that I do a merge or that I do a rebase?

[joshlf]

Question: This branch is already out-of-date and needs changes, do you prefer that I do a merge or that I do a rebase?

Rebase please, thanks!

[joshlf]

Maybe we could also enable clippy::unwrap_used in this PR and get more coverage?

Also, we'll probably want some way of disabling these in testing code, since we do want to be able to unwrap and expect in tests. Maybe we can just put these behind a cfg_attr(not(test), ...)?

[jrvanwhy]

Maybe we could also enable clippy::unwrap_used in this PR and get more coverage?

clippy::unwrap_used is already enabled in zerocopy :-)

Also, we'll probably want some way of disabling these in testing code, since we do want to be able to unwrap and expect in tests. Maybe we can just put these behind a cfg_attr(not(test), ...)?

Good point. There is already a cfg_attr block that allows clippy::unwrap_used and a few other lints in the test and kani environments. I added clippy::expect_used to that block to accept it in test environments.

[jrvanwhy]

Updated statistics for clippy::expect_used:

We have one function that is documented as panicing as part of its public API (FromZeros::new_box_slice_zeroed).

We have one function that called expect which we can replace with a lighter-weight branch (IntoBytes::write_to_suffix).

We have four functions (Ref::into_ref, Ref::into_mut, Ref::deref, Ref::deref_mut) with "should never be hit" panics that we cannot replace easily. I don't think we can rely on the optimizer to reliably remove these panics, as their unreachability depends on invariants of Ref. The only thing we could do to remove these panics without changing the public API is to replace the panic with something else undesirable, such as loop {} or unreachable_unchecked!.

One useful warning versus five useless warnings is not a great ratio. I'll let @joshlf decide whether you think that clippy::expect_used is worth the noise.

[joshlf]

Updated statistics for clippy::expect_used:

We have one function that is documented as panicing as part of its public API (FromZeros::new_box_slice_zeroed).

We have one function that called expect which we can replace with a lighter-weight branch (IntoBytes::write_to_suffix).

We have four functions (Ref::into_ref, Ref::into_mut, Ref::deref, Ref::deref_mut) with "should never be hit" panics that we cannot replace easily. I don't think we can rely on the optimizer to reliably remove these panics, as their unreachability depends on invariants of Ref. The only thing we could do to remove these panics without changing the public API is to replace the panic with something else undesirable, such as loop {} or unreachable_unchecked!.

IIUC these will be resolved by #368, at least for byte slice types (smart pointer types like core::cell::Ref will still need to have panic paths).

One useful warning versus five useless warnings is not a great ratio. I'll let @joshlf decide whether you think that clippy::expect_used is worth the noise.

I'm leaning no, which is a shame because you've put a lot of work into this PR, but I agree that it's a lot of noise for not much benefit.

That said, maybe you could land the refactor of write_to_suffix (either just simplifying this PR to contain only that change or in a new PR)? Even without the clippy lint, that's a useful change.

[jrvanwhy]

One useful warning versus five useless warnings is not a great ratio. I'll let @joshlf decide whether you think that clippy::expect_used is worth the noise.

I'm leaning no, which is a shame because you've put a lot of work into this PR, but I agree that it's a lot of noise for not much benefit.

Not really -- most of the work on this PR has been on further refining what exactly it means to "prevent panics" in zerocopy, and what sort of tradeoffs we want to make in achieving that goal. I decided to stick with one-Clippy-lint-per-PR so that "we don't want lint X" is an easy decision to make.

That said, maybe you could land the refactor of write_to_suffix (either just simplifying this PR to contain only that change or in a new PR)? Even without the clippy lint, that's a useful change.

I followed the "simplify this PR" route so it only lands that one change, and renamed the PR accordingly.

[joshlf]

Sorry this has taken so long to get to.

Also, see #1125 and feel free to chime in there.

[joshlf]

Suggested change

	// returning None is generally lighter-weight than panicing.
	// returning None is generally lighter-weight than panicking.

[jrvanwhy]

Fixed.