significant_drop_in_scrutinee is confusing and not very useful for for loops #8987
Comments
# Objective - Nightly clippy lints should be fixed before they get stable and break CI ## Solution - fix new clippy lints - ignore `significant_drop_in_scrutinee` since it isn't relevant in our loop rust-lang/rust-clippy#8987 ```rust for line in io::stdin().lines() { ... } ``` Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>
|
Even better, it complains about idiomatic Vec::drain usage!
|
xFrednet
added
I-false-positive
|
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 |
|
Ah sorry, my original case had the |
|
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 |
Move `significant_drop_in_scrutinee` into `nursey`
The current suggestion of extending the lifetime of every sub-expression is not great and doesn't fix the error given in the lint's example, though it does make the potential deadlock easier to see, but it can also cause it's own issues by delaying the drop of the lock guard.
e.g.
```rust
match x.lock().foo {
..
}
// some stuff
let y = x.lock();
```
The suggestion would create a deadlock at the second `x.lock()` call.
This also lints even when a significant drop type isn't created as a temporary. (#9072)
I agree `@kpreid` (#8987 (comment)) that this should be back-ported before the lint hits stable.
changelog: Move `significant_drop_in_scrutinee` into `nursey`
|
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 🙃 |
# Objective - Nightly clippy lints should be fixed before they get stable and break CI ## Solution - fix new clippy lints - ignore `significant_drop_in_scrutinee` since it isn't relevant in our loop rust-lang/rust-clippy#8987 ```rust for line in io::stdin().lines() { ... } ``` Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>
|
How about only making this lint work on |
|
I believe the key thing to look for is match guards that are temporaries. For example, it is surprising to many people that 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. |
# Objective - Nightly clippy lints should be fixed before they get stable and break CI ## Solution - fix new clippy lints - ignore `significant_drop_in_scrutinee` since it isn't relevant in our loop rust-lang/rust-clippy#8987 ```rust for line in io::stdin().lines() { ... } ``` Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>
# Objective - Nightly clippy lints should be fixed before they get stable and break CI ## Solution - fix new clippy lints - ignore `significant_drop_in_scrutinee` since it isn't relevant in our loop rust-lang/rust-clippy#8987 ```rust for line in io::stdin().lines() { ... } ``` Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>
Description
The following code
produces this clippy output:
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)).
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.
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
Additional Labels
No response
The text was updated successfully, but these errors were encountered: