user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Auto merge of #12740 - lrh2000:sig-drop, r=blyxyas

Browse source 
`significant_drop_in_scrutinee`: Trigger lint only if lifetime allows early significant drop

I want to argue that the following code snippet should not trigger `significant_drop_in_scrutinee` (https://github.com/rust-lang/rust-clippy/issues/8987). The iterator holds a reference to the locked data, so it is expected that the mutex guard must be alive until the entire loop is finished.
```rust
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);
    }
}
```

However, the lint should be triggered when we clone the vector. In this case, the iterator does not hold any reference to the locked data.
```diff
-     for number in mutex_vec.lock().unwrap().iter() {
+     for number in mutex_vec.lock().unwrap().clone().iter() {
```

Unfortunately, it seems that regions on the types of local variables are mostly erased (`ReErased`) in the late lint pass. So it is hard to tell if the final expression has a lifetime relevant to the value with a significant drop.

In this PR, I try to make a best-effort guess based on the function signatures. To avoid false positives, no lint is issued if the result is uncertain. I'm not sure if this is acceptable or not, so any comments are welcome.

Fixes https://github.com/rust-lang/rust-clippy/issues/8987

changelog: [`significant_drop_in_scrutinee`]: Trigger lint only if lifetime allows early significant drop.

r? `@flip1995`
This commit is contained in:
bors 2024-05-25 13:11:21 +00:00
commit 5aae5f6ae6
3 changed files with 434 additions and 304 deletions
Download patch file Download diff file Expand all files Collapse all files

98
tests/ui/significant_drop_in_scrutinee.rs
View file

@ -274,11 +274,20 @@ fn should_trigger_lint_for_tuple_in_scrutinee() {
},
(_, _, _) => {},
};
}
}
// Should not trigger lint since `String::as_str` returns a reference (i.e., `&str`)
// to the locked data (i.e., the `String`) and it is not surprising that matching such
// a reference needs to keep the data locked until the end of the match block.
fn should_not_trigger_lint_for_string_as_str() {
let mutex1 = Mutex::new(StateWithField { s: "one".to_owned() });
{
let mutex2 = Mutex::new(StateWithField { s: "two".to_owned() });
let mutex3 = Mutex::new(StateWithField { s: "three".to_owned() });
match mutex3.lock().unwrap().s.as_str() {
//~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until
//~| NOTE: this might lead to deadlocks or other unexpected behavior
"three" => {
println!("started");
mutex1.lock().unwrap().s.len();
@ -289,8 +298,6 @@ fn should_trigger_lint_for_tuple_in_scrutinee() {
};
match (true, mutex3.lock().unwrap().s.as_str()) {
//~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until
//~| NOTE: this might lead to deadlocks or other unexpected behavior
(_, "three") => {
println!("started");
mutex1.lock().unwrap().s.len();
@ -514,16 +521,15 @@ impl StateStringWithBoxedMutexGuard {
}
}
fn should_trigger_lint_for_boxed_mutex_guard_holding_string() {
fn should_not_trigger_lint_for_string_ref() {
let s = StateStringWithBoxedMutexGuard::new();
let matcher = String::from("A String");
// Should trigger lint because a temporary Box holding a type with a significant drop in a match
// scrutinee may have a potentially surprising lifetime.
// Should not trigger lint because the second `deref` returns a string reference (`&String`).
// So it is not surprising that matching the reference implies that the lock needs to be held
// until the end of the block.
match s.lock().deref().deref() {
//~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the
//~| NOTE: this might lead to deadlocks or other unexpected behavior
matcher => println!("Value is {}", s.lock().deref()),
_ => println!("Value was not a match"),
};
@ -639,13 +645,12 @@ fn should_trigger_lint_for_non_ref_move_and_clone_suggestion() {
};
}
fn should_trigger_lint_for_read_write_lock_for_loop() {
// For-in loops desugar to match expressions and are prone to the type of deadlock this lint is
// designed to look for.
fn should_not_trigger_lint_for_read_write_lock_for_loop() {
let rwlock = RwLock::<Vec<String>>::new(vec!["1".to_string()]);
// Should not trigger lint. Since we're iterating over the data, it's obvious that the lock
// has to be held until the iteration finishes.
// https://github.com/rust-lang/rust-clippy/issues/8987
for s in rwlock.read().unwrap().iter() {
//~^ ERROR: temporary with significant `Drop` in `for` loop condition will live until
//~| NOTE: this might lead to deadlocks or other unexpected behavior
println!("{}", s);
}
}
@ -731,4 +736,69 @@ fn should_not_trigger_for_significant_drop_ref() {
};
}
struct Foo<'a>(&'a Vec<u32>);
impl<'a> Foo<'a> {
fn copy_old_lifetime(&self) -> &'a Vec<u32> {
self.0
}
fn reborrow_new_lifetime(&self) -> &Vec<u32> {
self.0
}
}
fn should_trigger_lint_if_and_only_if_lifetime_is_irrelevant() {
let vec = Vec::new();
let mutex = Mutex::new(Foo(&vec));
// Should trigger lint even if `copy_old_lifetime()` has a lifetime, as the lifetime of
// `&vec` is unrelated to the temporary with significant drop (i.e., the `MutexGuard`).
for val in mutex.lock().unwrap().copy_old_lifetime() {
//~^ ERROR: temporary with significant `Drop` in `for` loop condition will live until the
//~| NOTE: this might lead to deadlocks or other unexpected behavior
println!("{}", val);
}
// Should not trigger lint because `reborrow_new_lifetime()` has a lifetime and the
// lifetime is related to the temporary with significant drop (i.e., the `MutexGuard`).
for val in mutex.lock().unwrap().reborrow_new_lifetime() {
println!("{}", val);
}
}
fn should_not_trigger_lint_for_complex_lifetime() {
let mutex = Mutex::new(vec!["hello".to_owned()]);
let string = "world".to_owned();
// Should not trigger lint due to the relevant lifetime.
for c in mutex.lock().unwrap().first().unwrap().chars() {
println!("{}", c);
}
// Should trigger lint due to the irrelevant lifetime.
//
// FIXME: The lifetime is too complex to analyze. In order to avoid false positives, we do not
// trigger lint.
for c in mutex.lock().unwrap().first().map(|_| &string).unwrap().chars() {
println!("{}", c);
}
}
fn should_not_trigger_lint_with_explicit_drop() {
let mutex = Mutex::new(vec![1]);
// Should not trigger lint since the drop explicitly happens.
for val in [drop(mutex.lock().unwrap()), ()] {
println!("{:?}", val);
}
// Should trigger lint if there is no explicit drop.
for val in [mutex.lock().unwrap()[0], 2] {
//~^ ERROR: temporary with significant `Drop` in `for` loop condition will live until the
//~| NOTE: this might lead to deadlocks or other unexpected behavior
println!("{:?}", val);
}
}
fn main() {}

220
tests/ui/significant_drop_in_scrutinee.stderr
View file

@ -160,42 +160,10 @@ LL ~ match (mutex1.lock().unwrap().s.len(), true, value) {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:279:15
|
LL | match mutex3.lock().unwrap().s.as_str() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | mutex1.lock().unwrap().s.len();
| ---------------------- another value with significant `Drop` created here
LL | mutex2.lock().unwrap().s.len();
| ---------------------- another value with significant `Drop` created here
...
LL | };
| - temporary lives until here
|
= note: this might lead to deadlocks or other unexpected behavior
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:291:22
|
LL | match (true, mutex3.lock().unwrap().s.as_str()) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | mutex1.lock().unwrap().s.len();
| ---------------------- another value with significant `Drop` created here
LL | mutex2.lock().unwrap().s.len();
| ---------------------- another value with significant `Drop` created here
...
LL | };
| - temporary lives until here
|
= note: this might lead to deadlocks or other unexpected behavior
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:312:11
--> tests/ui/significant_drop_in_scrutinee.rs:319:11
|
LL | match mutex.lock().unwrap().s.len() > 1 {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | mutex.lock().unwrap().s.len();
| --------------------- another value with significant `Drop` created here
@ -206,15 +174,15 @@ LL | };
= note: this might lead to deadlocks or other unexpected behavior
help: try moving the temporary above the match
|
LL ~ let value = mutex.lock().unwrap().s.len() > 1;
LL ~ match value {
LL ~ let value = mutex.lock().unwrap().s.len();
LL ~ match value > 1 {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:321:11
--> tests/ui/significant_drop_in_scrutinee.rs:328:15
|
LL | match 1 < mutex.lock().unwrap().s.len() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | mutex.lock().unwrap().s.len();
| --------------------- another value with significant `Drop` created here
@ -225,15 +193,15 @@ LL | };
= note: this might lead to deadlocks or other unexpected behavior
help: try moving the temporary above the match
|
LL ~ let value = 1 < mutex.lock().unwrap().s.len();
LL ~ match value {
LL ~ let value = mutex.lock().unwrap().s.len();
LL ~ match 1 < value {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:341:11
--> tests/ui/significant_drop_in_scrutinee.rs:348:11
|
LL | match mutex1.lock().unwrap().s.len() < mutex2.lock().unwrap().s.len() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | mutex1.lock().unwrap().s.len(),
| ---------------------- another value with significant `Drop` created here
@ -246,15 +214,36 @@ LL | };
= note: this might lead to deadlocks or other unexpected behavior
help: try moving the temporary above the match
|
LL ~ let value = mutex1.lock().unwrap().s.len() < mutex2.lock().unwrap().s.len();
LL ~ match value {
LL ~ let value = mutex1.lock().unwrap().s.len();
LL ~ match value < mutex2.lock().unwrap().s.len() {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:354:11
--> tests/ui/significant_drop_in_scrutinee.rs:348:44
|
LL | match mutex1.lock().unwrap().s.len() < mutex2.lock().unwrap().s.len() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | mutex1.lock().unwrap().s.len(),
| ---------------------- another value with significant `Drop` created here
LL | mutex2.lock().unwrap().s.len()
| ---------------------- another value with significant `Drop` created here
...
LL | };
| - temporary lives until here
|
= note: this might lead to deadlocks or other unexpected behavior
help: try moving the temporary above the match
|
LL ~ let value = mutex2.lock().unwrap().s.len();
LL ~ match mutex1.lock().unwrap().s.len() < value {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:361:11
|
LL | match mutex1.lock().unwrap().s.len() >= mutex2.lock().unwrap().s.len() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | mutex1.lock().unwrap().s.len(),
| ---------------------- another value with significant `Drop` created here
@ -267,15 +256,36 @@ LL | };
= note: this might lead to deadlocks or other unexpected behavior
help: try moving the temporary above the match
|
LL ~ let value = mutex1.lock().unwrap().s.len() >= mutex2.lock().unwrap().s.len();
LL ~ match value {
LL ~ let value = mutex1.lock().unwrap().s.len();
LL ~ match value >= mutex2.lock().unwrap().s.len() {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:391:11
--> tests/ui/significant_drop_in_scrutinee.rs:361:45
|
LL | match mutex1.lock().unwrap().s.len() >= mutex2.lock().unwrap().s.len() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | mutex1.lock().unwrap().s.len(),
| ---------------------- another value with significant `Drop` created here
LL | mutex2.lock().unwrap().s.len()
| ---------------------- another value with significant `Drop` created here
...
LL | };
| - temporary lives until here
|
= note: this might lead to deadlocks or other unexpected behavior
help: try moving the temporary above the match
|
LL ~ let value = mutex2.lock().unwrap().s.len();
LL ~ match mutex1.lock().unwrap().s.len() >= value {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:398:11
|
LL | match get_mutex_guard().s.len() > 1 {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | mutex1.lock().unwrap().s.len();
| ---------------------- another value with significant `Drop` created here
@ -286,12 +296,12 @@ LL | };
= note: this might lead to deadlocks or other unexpected behavior
help: try moving the temporary above the match
|
LL ~ let value = get_mutex_guard().s.len() > 1;
LL ~ match value {
LL ~ let value = get_mutex_guard().s.len();
LL ~ match value > 1 {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:410:11
--> tests/ui/significant_drop_in_scrutinee.rs:417:11
|
LL | match match i {
| ___________^
@ -299,9 +309,9 @@ LL | |
LL | |
LL | | 100 => mutex1.lock().unwrap(),
... |
LL | | .s
LL | | .len()
LL | | > 1
| |___________^
| |__________^
...
LL | mutex1.lock().unwrap().s.len();
| ---------------------- another value with significant `Drop` created here
@ -319,13 +329,12 @@ LL + 100 => mutex1.lock().unwrap(),
LL + _ => mutex2.lock().unwrap(),
LL + }
LL + .s
LL + .len()
LL + > 1;
LL + .len();
LL ~ match value
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:438:11
--> tests/ui/significant_drop_in_scrutinee.rs:445:11
|
LL | match if i > 1 {
| ___________^
@ -333,9 +342,9 @@ LL | |
LL | |
LL | | mutex1.lock().unwrap()
... |
LL | | .s
LL | | .len()
LL | | > 1
| |___________^
| |__________^
...
LL | mutex1.lock().unwrap().s.len();
| ---------------------- another value with significant `Drop` created here
@ -354,13 +363,12 @@ LL + } else {
LL + mutex2.lock().unwrap()
LL + }
LL + .s
LL + .len()
LL + > 1;
LL + .len();
LL ~ match value
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:494:11
--> tests/ui/significant_drop_in_scrutinee.rs:501:11
|
LL | match s.lock().deref().deref() {
| ^^^^^^^^^^^^^^^^^^^^^^^^
@ -374,25 +382,11 @@ LL | };
help: try moving the temporary above the match and create a copy
|
LL ~ let value = *s.lock().deref().deref();
LL ~ match value {
LL ~ match (&value) {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:524:11
|
LL | match s.lock().deref().deref() {
| ^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | matcher => println!("Value is {}", s.lock().deref()),
| -------- another value with significant `Drop` created here
LL | _ => println!("Value was not a match"),
LL | };
| - temporary lives until here
|
= note: this might lead to deadlocks or other unexpected behavior
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:545:11
--> tests/ui/significant_drop_in_scrutinee.rs:551:11
|
LL | match mutex.lock().unwrap().i = i {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
@ -411,10 +405,10 @@ LL ~ match () {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:553:11
--> tests/ui/significant_drop_in_scrutinee.rs:559:15
|
LL | match i = mutex.lock().unwrap().i {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^
...
LL | println!("{}", mutex.lock().unwrap().i);
| --------------------- another value with significant `Drop` created here
@ -425,12 +419,12 @@ LL | };
= note: this might lead to deadlocks or other unexpected behavior
help: try moving the temporary above the match
|
LL ~ i = mutex.lock().unwrap().i;
LL ~ match () {
LL ~ let value = mutex.lock().unwrap().i;
LL ~ match i = value {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:561:11
--> tests/ui/significant_drop_in_scrutinee.rs:567:11
|
LL | match mutex.lock().unwrap().i += 1 {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@ -449,10 +443,10 @@ LL ~ match () {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:569:11
--> tests/ui/significant_drop_in_scrutinee.rs:575:16
|
LL | match i += mutex.lock().unwrap().i {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^
...
LL | println!("{}", mutex.lock().unwrap().i);
| --------------------- another value with significant `Drop` created here
@ -463,12 +457,12 @@ LL | };
= note: this might lead to deadlocks or other unexpected behavior
help: try moving the temporary above the match
|
LL ~ i += mutex.lock().unwrap().i;
LL ~ match () {
LL ~ let value = mutex.lock().unwrap().i;
LL ~ match i += value {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:634:11
--> tests/ui/significant_drop_in_scrutinee.rs:640:11
|
LL | match rwlock.read().unwrap().to_number() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@ -477,20 +471,14 @@ LL | };
| - temporary lives until here
|
= note: this might lead to deadlocks or other unexpected behavior
error: temporary with significant `Drop` in `for` loop condition will live until the end of the `for` expression
--> tests/ui/significant_drop_in_scrutinee.rs:646:14
help: try moving the temporary above the match
|
LL | for s in rwlock.read().unwrap().iter() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | }
| - temporary lives until here
LL ~ let value = rwlock.read().unwrap().to_number();
LL ~ match value {
|
= note: this might lead to deadlocks or other unexpected behavior
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:663:11
--> tests/ui/significant_drop_in_scrutinee.rs:668:11
|
LL | match mutex.lock().unwrap().foo() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
@ -506,7 +494,7 @@ LL ~ match value {
|
error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression
--> tests/ui/significant_drop_in_scrutinee.rs:726:11
--> tests/ui/significant_drop_in_scrutinee.rs:731:11
|
LL | match guard.take().len() {
| ^^^^^^^^^^^^^^^^^^
@ -521,5 +509,37 @@ LL ~ let value = guard.take().len();
LL ~ match value {
|
error: temporary with significant `Drop` in `for` loop condition will live until the end of the `for` expression
--> tests/ui/significant_drop_in_scrutinee.rs:757:16
|
LL | for val in mutex.lock().unwrap().copy_old_lifetime() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | }
| - temporary lives until here
|
= note: this might lead to deadlocks or other unexpected behavior
help: try moving the temporary above the match
|
LL ~ let value = mutex.lock().unwrap().copy_old_lifetime();
LL ~ for val in value {
|
error: temporary with significant `Drop` in `for` loop condition will live until the end of the `for` expression
--> tests/ui/significant_drop_in_scrutinee.rs:797:17
|
LL | for val in [mutex.lock().unwrap()[0], 2] {
| ^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | }
| - temporary lives until here
|
= note: this might lead to deadlocks or other unexpected behavior
help: try moving the temporary above the match
|
LL ~ let value = mutex.lock().unwrap()[0];
LL ~ for val in [value, 2] {
|
error: aborting due to 27 previous errors