lint(clippy): add unwrap_used lint
#4732
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4732 +/- ##
==========================================
+ Coverage 78.91% 78.95% +0.04%
==========================================
Files 306 306
Lines 37549 37769 +220
==========================================
+ Hits 29630 29819 +189
- Misses 7919 7950 +31 |
|
I'm really not sure about this, it seems like a very large change. We'll have to review every Did you find any bugs that justify this change? If we decide to merge this PR, I'd like to split up the review into different PRs, like we did the consensus rule documentation work. Then we can use the feedback from the first PR to make the other PRs easier to review. We can also schedule the lint PRs so they don't conflict with other PRs we're actively working on. I'd also like to split up any other tickets that are this big before we start working on them. |
When will the Rust version with that lint option be released as stable? |
| @@ -50,10 +50,10 @@ fn prp_d(K: [u8; 32], d: [u8; 11]) -> [u8; 11] { | |||
| let ff = FF1::<Aes256>::new(&K, radix).expect("valid radix"); | |||
|
|
|||
| ff.encrypt(tweak, &BinaryNumeralString::from_bytes_le(&d)) | |||
| .unwrap() | |||
| .expect("bytes are valid") | |||
There was a problem hiding this comment.
Please check the callers to make sure they always provide valid data.
| @@ -298,7 +298,7 @@ impl Eq for SpendValidatingKey {} | |||
|
|
|||
| impl From<[u8; 32]> for SpendValidatingKey { | |||
| fn from(bytes: [u8; 32]) -> Self { | |||
| Self(redpallas::VerificationKey::try_from(bytes).unwrap()) | |||
| Self(redpallas::VerificationKey::try_from(bytes).expect("bytes are valid")) | |||
There was a problem hiding this comment.
How do we know that the supplied data is always valid?
This might need to be a TryFrom impl instead, please check with the original author of this code.
(I think Deirdre is busy with FROST right now, so we might need to open a ticket and schedule this fix later.)
There was a problem hiding this comment.
There is no difference between unwrap() or expect(), both will panic if bytes are not valid. The idea of this lint is just to use a message instead of just panicking but i don't think we should check every unwrap to see if it should panic or not while we are here.
| @@ -45,7 +45,7 @@ pub fn extract_p_bottom(maybe_point: Option<pallas::Point>) -> Option<pallas::Ba | |||
| /// <https://zips.z.cash/protocol/nu5.pdf#concretegrouphashpallasandvesta> | |||
| #[allow(non_snake_case)] | |||
| pub fn pallas_group_hash(D: &[u8], M: &[u8]) -> pallas::Point { | |||
| let domain_separator = std::str::from_utf8(D).unwrap(); | |||
| let domain_separator = std::str::from_utf8(D).expect("slice is valid utf-8"); | |||
There was a problem hiding this comment.
Can we be more specific here?
The domain separators are constant strings, which are guaranteed to be valid UTF-8 by the protocol. The message is also a slice, and it can be arbitrary data.
| let domain_separator = std::str::from_utf8(D).expect("slice is valid utf-8"); | |
| let domain_separator = std::str::from_utf8(D).expect("domain separator is valid utf-8"); |
There was a problem hiding this comment.
I had a quick look at some of the changes.
I think this lint is valuable, because it has found a bug in some of our unused cryptographic code. But to find other similar bugs in this PR, we'll need a detailed cryptographic review.
Most of our cryptographers are currently busy with FROST, so we might need to schedule the cryptographic parts of this PR later.
I don't think this PR is created to do this but just to add a message with the panic.
Not really, this PR is just to avoid the use of unwrap in production code.
This will be a pain , i don't think it will worth it. What we need to decide is if we want to add it or not. There are a lot of changes in this PR for sure but i dont think they are hard to review.
This PR is the last from a list (#2297), the easier thing to do, if we decide that we want it, is to give it high priority and merge it as soon as possible.
It is too late for this, i understand your suggestion but i think we should try to push it forward, again, only if we want the lint to be included. |
|
I think we might want to close this one as i don't see us implementing it in the short to medium run. |
|
I agree that we should close this PR. |
Motivation
In #2297 we want to introduce some panic lints to Zebra. This PR adds the
unwrap_usedlint to the codebase, changeunwrap()toexpect()in production code and allow the lint in test code.If merged, this will finally close #2297
Solution
There is an lint config option to allow this lint in test code. Unfortunately this is not available for stable rustc (rust-lang/rust-clippy#9062) so this was added manually where needed.
In production code all
unwrap()was changed toexpect().Review
It is going to be a pain to review by the amount of files changed. I have to review it myself as i made the changes to make it work but some error messages might still be unclear or wrong.
This PR is a draft until we are sure we will want to add this now.
Reviewer Checklist