Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace a panic with an error branch #544

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

jrvanwhy
Copy link
Contributor

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
Copy link
Contributor Author

jrvanwhy commented Oct 24, 2023

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
Copy link
Contributor Author

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
Copy link
Member

joshlf commented Oct 24, 2023

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
Copy link
Member

joshlf commented Oct 25, 2023

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
Copy link
Contributor Author

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.

src/lib.rs Show resolved Hide resolved
@jrvanwhy jrvanwhy force-pushed the clippy-expect_used branch 2 times, most recently from 23eea59 to 97d850e Compare March 26, 2024 19:25
@jrvanwhy
Copy link
Contributor Author

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
Copy link
Member

joshlf commented Mar 27, 2024

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 jrvanwhy force-pushed the clippy-expect_used branch 2 times, most recently from aab9374 to d13395c Compare March 29, 2024 18:01
@jrvanwhy jrvanwhy changed the title Enable clippy::expect_used, address warnings Replace a panic with an error branch Mar 29, 2024
@jrvanwhy
Copy link
Contributor Author

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.

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this has taken so long to get to.

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

src/lib.rs Outdated
.copy_from_slice(self.as_bytes());
// get_mut() should never return None here. We use ? rather than
// .unwrap() because in the event the branch is not optimized away,
// returning None is generally lighter-weight than panicing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

This panic is unreachable. Panicxing is heavyweight, so we're trying to
reduce panics (issue google#202). This PR replaces the panic with a branch
that should be lighter-weight (if it is not optimized away entirely).
@joshlf joshlf enabled auto-merge April 24, 2024 18:00
@joshlf joshlf added this pull request to the merge queue Apr 24, 2024
Merged via the queue into google:main with commit 54dc8da Apr 24, 2024
210 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants