Description
Summary
I've been looking into a problem I've been having with rust-fuzz/arbitrary where an error was not being flagged when it seemingly should have been, and noticed what look likes a false negative in clippy in the process.
https://docs.rs/arbitrary/1.4.1/src/arbitrary/unstructured.rs.html#295-302
Lint Name
unnecessary_wraps
Reproducer
Based on the latest version of the crate I had this code:
pub fn int_in_range<T>(&mut self, range: ops::RangeInclusive<T>) -> Result<T>
where
T: Int,
{
let (result, bytes_consumed) = Self::int_in_range_impl(range, self.data.iter().cloned());
self.data = &self.data[bytes_consumed..];
Ok(result)
}
For context, in the code as published, Self::int_in_range_impl
returns Result
, so the equivalent to the above snippet includes an extra question mark to unwrap it. However, Self::int_in_range_impl
is correctly flagged as unnecessary_wraps
. When I fixed it to unconditionally return (T, usize)
and updated callers, I got the above snippet. (Example on playground here)
I expected to see this happen: a unneccessary_wraps
warning from clippy, because int_in_range
seemingly only ever returns Ok
.
Instead, this happened: no warning
Version
rustc 1.85.0 (4d91de4e4 2025-02-17)
binary: rustc
commit-hash: 4d91de4e48198da2e33413efdcd9cd2cc0c46688
commit-date: 2025-02-17
host: x86_64-pc-windows-msvc
release: 1.85.0
LLVM version: 19.1.7
Activity
ejmount commentedon Apr 10, 2025
After having a closer look at the implementation of
unnecessary_wraps
I see it excludes all public items ifavoid-breaking-exported-api
is set, which it is by default, and that is whyint_in_range
is not being flagged.While I understand the logic going on now, IMO this could be highlighted better in the documentation - while it is my fault for missing in the description that it "checks for private functions...", that sentence isn't completely accurate (my example is flagged if
avoid-breaking-exported-api
is set to false) and I've had issues in the past whereunnecessary_wraps
has been triggered for functions that are being passed as values (and so need specific signatures) so it checking in this case took me by surprise.Additionally, "public" and "can cause breakage for other crates" are not necessarily the same thing - even understanding the latter, I would want it to fire on functions that are public but not accessible from the crate root, but I don't think the documentation implies it currently does.
samueltardieu commentedon Apr 11, 2025
I've edited the labels to make it clearer that this is a documentation issue. Don't hesitate to submit a pull request if you have ideas on how to best formulate this.