rust-clippy
rust-clippy copied to clipboard
rust-lang
significant_drop_in_scrutinee is confusing and not very useful for for loops
Description
The following code
use std::sync::Mutex;
fn main() {
let mutex_vec = Mutex::new(vec![1, 2, 3]);
for number in mutex_vec.lock().unwrap().iter() {
dbg!(number);
}
}
produces this clippy output:
warning: temporary with significant drop in for loop
--> src/main.rs:5:19
|
5 | for number in mutex_vec.lock().unwrap().iter() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(clippy::significant_drop_in_scrutinee)]` on by default
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_in_scrutinee
The first confusing thing is the name. Scrutinee only makes sense for match, it's not used in the context of for loops. If one visits the lint page, it doesn't talk about for loops at all, and what the ideal code would look like for for loops.
The naive thing to do here would be to bind the guard before the for loop (ignoring that it still triggers the lint (#8963)).
use std::sync::Mutex;
fn main() {
let mutex_vec = Mutex::new(vec![1, 2, 3]);
let guard = mutex_vec.lock().unwrap();
for number in guard.iter() {
dbg!(number);
}
}
However, that actually increases the scope of the guard, and now it will be locked for the entire lifetime of the guard binding. The proper thing to do would be to drop it after the for loop.
use std::sync::Mutex;
fn main() {
let mutex_vec = Mutex::new(vec![1, 2, 3]);
let guard = mutex_vec.lock().unwrap();
for number in guard.iter() {
dbg!(number);
}
drop(guard);
}
However, the lint page never talks about this, and this could easily be a trap for beginners/people not paying attention. And is it really better than just locking in the for loop expression? I personally think it's pretty clear that the guard lasts for the whole loop. Abiding this lint just adds a new opportunity for error (dropping the guard late).
Version
rustc 1.63.0-nightly (ec55c6130 2022-06-10)
binary: rustc
commit-hash: ec55c61305eaf385fc1b93ac9a78284b4d887fe5
commit-date: 2022-06-10
host: x86_64-unknown-linux-gnu
release: 1.63.0-nightly
LLVM version: 14.0.5
Additional Labels
No response
Using an existing guard in a match statement also triggers it, even if it's still used later on and held on purpose.
fn main() {
let data = std::sync::Mutex::new(vec![1u32, 2, 3]);
let mut guard = data.lock().unwrap();
match guard.pop() { // clippy complains about `guard.pop` here!
None => println!("nope"),
Some(x) => println!("{x}"),
}
// we're actually still using the guard later: (doesn't matter to clippy - warns with and without)
guard.push(99);
println!("{}", guard.len());
}
Hey @Blub, thank you for the report. This seems like a second issue, then it linting in a for-loop. Would you mind creating a new issue for it? :upside_down_face:
Ah sorry, my original case had the match inside a loop and I kind of missed that it diverged away from this issue when minimizing the reproducer.
In fact, it now looks like #9072 actually.
Can this lint be moved to nursery or something (and backported, since it's already hit 1.63 beta) soon?
It's quite noisy (9 occurrences in my code), and I think it would be bad for the Rust ecosystem if this hits stable as-is and people started moving things to variables just to silence this lint (as noted in the first comment, such changes would increase the scope of lock guards, and there really shouldn't be a lint on Vec::drain either).
The lint was moved to nursery on the current master in #9302, as said in the PR, we will try to do a backport for it :upside_down_face:
How about only making this lint work on if let/else? In most cases, it's easy to understand the lock will exist during the loop or in match block. But it's quite a common mistake that assuming lock only exists in if matching.
if let Some(res) = lock_on_option.lock().as_ref() {
// ...
} else {
// ... lock still alive in else branch!
}
I believe the key thing to look for is match guards that are temporaries. For example, it is surprising to many people that some_ref_cell remains "locked" in this example...
match shared_vec.borrow_mut().pop() {
Some(_) => {}
None => shared_vec.borrow_mut().push(String::new()),
}
...I think this applies equally well to for loops as well...
for x in shared_vec.borrow_mut().pop() {
}
Basically, I think whenever a temporary is created with significant drop in a control-flow statement, that's probably worth flagging.
