Repository navigation
significant_drop_in_scrutinee is confusing and not very useful for for loops #8987
Description
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_scrutineeThe 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
Activity
fix new clippy lints (#5160)
Gankra commentedon Jul 23, 2022
Even better, it complains about idiomatic Vec::drain usage!
Blub commentedon Jul 27, 2022
Using an existing guard in a match statement also triggers it, even if it's still used later on and held on purpose.
xFrednet commentedon Jul 27, 2022
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? 🙃Blub commentedon Jul 28, 2022
Ah sorry, my original case had the
matchinside 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.
kpreid commentedon Aug 7, 2022
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::draineither).Auto merge of #9302 - Jarcho:sig_drop_nursery, r=flip1995
xFrednet commentedon Aug 8, 2022
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 🙃
fix new clippy lints (bevyengine#5160)
BusyJay commentedon Sep 14, 2022
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 inmatchblock. But it's quite a common mistake that assuming lock only exists inif matching.nikomatsakis commentedon Sep 27, 2022
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_cellremains "locked" in this example...playground
...I think this applies equally well to for loops as well...
Basically, I think whenever a temporary is created with significant drop in a control-flow statement, that's probably worth flagging.
fix new clippy lints (bevyengine#5160)
fix new clippy lints (bevyengine#5160)
Auto merge of #12740 - lrh2000:sig-drop, r=blyxyas
fix new clippy lints (#5160)