Auto merge of #10368 - c410-f3r:lock-1, r=xFrednet

[significant_drop_tightening] Evaluate the return expression of a block

For whatever reason, the return expression of a block is not contained inside the slice of statements and because of that the lint wasn't evaluating things that could potentially block the release of a lock.

```rust
pub fn example() -> i32 {
    let mutex = Mutex::new(1);
    let lock = mutex.lock().unwrap();
    let _ = *lock;
    let _ = *lock;
   do_heavy_computation_that_takes_time_and_returns_i32()
}
```

---

changelog: none
<!-- changelog_checked -->
This commit is contained in:
bors 2023-02-18 13:23:35 +00:00
commit 85d4b5ac48
4 changed files with 95 additions and 14 deletions

View file

@ -61,10 +61,26 @@ pub struct SignificantDropTightening<'tcx> {
} }
impl<'tcx> SignificantDropTightening<'tcx> { impl<'tcx> SignificantDropTightening<'tcx> {
/// Unifies the statements of a block with its return expression.
fn all_block_stmts<'ret, 'rslt, 'stmts>(
block_stmts: &'stmts [hir::Stmt<'tcx>],
dummy_ret_stmt: Option<&'ret hir::Stmt<'tcx>>,
) -> impl Iterator<Item = &'rslt hir::Stmt<'tcx>>
where
'ret: 'rslt,
'stmts: 'rslt,
{
block_stmts.iter().chain(dummy_ret_stmt)
}
/// Searches for at least one statement that could slow down the release of a significant drop. /// Searches for at least one statement that could slow down the release of a significant drop.
fn at_least_one_stmt_is_expensive(stmts: &[hir::Stmt<'_>]) -> bool { fn at_least_one_stmt_is_expensive<'stmt>(stmts: impl Iterator<Item = &'stmt hir::Stmt<'tcx>>) -> bool
where
'tcx: 'stmt,
{
for stmt in stmts { for stmt in stmts {
match stmt.kind { match stmt.kind {
hir::StmtKind::Expr(expr) if let hir::ExprKind::Path(_) = expr.kind => {}
hir::StmtKind::Local(local) if let Some(expr) = local.init hir::StmtKind::Local(local) if let Some(expr) = local.init
&& let hir::ExprKind::Path(_) = expr.kind => {}, && let hir::ExprKind::Path(_) = expr.kind => {},
_ => return true _ => return true
@ -99,7 +115,7 @@ impl<'tcx> SignificantDropTightening<'tcx> {
expr: &'tcx hir::Expr<'_>, expr: &'tcx hir::Expr<'_>,
idx: usize, idx: usize,
sdap: &mut SigDropAuxParams, sdap: &mut SigDropAuxParams,
stmt: &'tcx hir::Stmt<'_>, stmt: &hir::Stmt<'_>,
cb: impl Fn(&mut SigDropAuxParams), cb: impl Fn(&mut SigDropAuxParams),
) { ) {
let mut sig_drop_finder = SigDropFinder::new(cx, &mut self.seen_types); let mut sig_drop_finder = SigDropFinder::new(cx, &mut self.seen_types);
@ -117,7 +133,7 @@ impl<'tcx> SignificantDropTightening<'tcx> {
} }
} }
/// Shows a generic overall message as well as specialized messages depending on the usage. /// Shows generic overall messages as well as specialized messages depending on the usage.
fn set_suggestions(cx: &LateContext<'tcx>, block_span: Span, diag: &mut Diagnostic, sdap: &SigDropAuxParams) { fn set_suggestions(cx: &LateContext<'tcx>, block_span: Span, diag: &mut Diagnostic, sdap: &SigDropAuxParams) {
match sdap.number_of_stmts { match sdap.number_of_stmts {
0 | 1 => {}, 0 | 1 => {},
@ -172,8 +188,13 @@ impl<'tcx> SignificantDropTightening<'tcx> {
impl<'tcx> LateLintPass<'tcx> for SignificantDropTightening<'tcx> { impl<'tcx> LateLintPass<'tcx> for SignificantDropTightening<'tcx> {
fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) { fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) {
let dummy_ret_stmt = block.expr.map(|expr| hir::Stmt {
hir_id: hir::HirId::INVALID,
kind: hir::StmtKind::Expr(expr),
span: DUMMY_SP,
});
let mut sdap = SigDropAuxParams::default(); let mut sdap = SigDropAuxParams::default();
for (idx, stmt) in block.stmts.iter().enumerate() { for (idx, stmt) in Self::all_block_stmts(block.stmts, dummy_ret_stmt.as_ref()).enumerate() {
match stmt.kind { match stmt.kind {
hir::StmtKind::Expr(expr) => self.modify_sdap_if_sig_drop_exists( hir::StmtKind::Expr(expr) => self.modify_sdap_if_sig_drop_exists(
cx, cx,
@ -213,11 +234,9 @@ impl<'tcx> LateLintPass<'tcx> for SignificantDropTightening<'tcx> {
_ => {} _ => {}
}; };
} }
let stmts_after_last_use = sdap
.last_use_stmt_idx let idx = sdap.last_use_stmt_idx.wrapping_add(1);
.checked_add(1) let stmts_after_last_use = Self::all_block_stmts(block.stmts, dummy_ret_stmt.as_ref()).skip(idx);
.and_then(|idx| block.stmts.get(idx..))
.unwrap_or_default();
if sdap.number_of_stmts > 1 && Self::at_least_one_stmt_is_expensive(stmts_after_last_use) { if sdap.number_of_stmts > 1 && Self::at_least_one_stmt_is_expensive(stmts_after_last_use) {
span_lint_and_then( span_lint_and_then(
cx, cx,

View file

@ -4,6 +4,26 @@
use std::sync::Mutex; use std::sync::Mutex;
pub fn complex_return_triggers_the_lint() -> i32 {
fn foo() -> i32 {
1
}
let mutex = Mutex::new(1);
let lock = mutex.lock().unwrap();
let _ = *lock;
let _ = *lock;
drop(lock);
foo()
}
pub fn path_return_can_be_ignored() -> i32 {
let mutex = Mutex::new(1);
let lock = mutex.lock().unwrap();
let rslt = *lock;
let _ = *lock;
rslt
}
pub fn post_bindings_can_be_ignored() { pub fn post_bindings_can_be_ignored() {
let mutex = Mutex::new(1); let mutex = Mutex::new(1);
let lock = mutex.lock().unwrap(); let lock = mutex.lock().unwrap();

View file

@ -4,6 +4,25 @@
use std::sync::Mutex; use std::sync::Mutex;
pub fn complex_return_triggers_the_lint() -> i32 {
fn foo() -> i32 {
1
}
let mutex = Mutex::new(1);
let lock = mutex.lock().unwrap();
let _ = *lock;
let _ = *lock;
foo()
}
pub fn path_return_can_be_ignored() -> i32 {
let mutex = Mutex::new(1);
let lock = mutex.lock().unwrap();
let rslt = *lock;
let _ = *lock;
rslt
}
pub fn post_bindings_can_be_ignored() { pub fn post_bindings_can_be_ignored() {
let mutex = Mutex::new(1); let mutex = Mutex::new(1);
let lock = mutex.lock().unwrap(); let lock = mutex.lock().unwrap();

View file

@ -1,5 +1,29 @@
error: temporary with significant `Drop` can be early dropped error: temporary with significant `Drop` can be early dropped
--> $DIR/significant_drop_tightening.rs:25:13 --> $DIR/significant_drop_tightening.rs:12:9
|
LL | pub fn complex_return_triggers_the_lint() -> i32 {
| __________________________________________________-
LL | | fn foo() -> i32 {
LL | | 1
LL | | }
LL | | let mutex = Mutex::new(1);
LL | | let lock = mutex.lock().unwrap();
| | ^^^^
... |
LL | | foo()
LL | | }
| |_- temporary `lock` is currently being dropped at the end of its contained scope
|
= note: this might lead to unnecessary resource contention
= note: `-D clippy::significant-drop-tightening` implied by `-D warnings`
help: drop the temporary after the end of its last usage
|
LL ~ let _ = *lock;
LL + drop(lock);
|
error: temporary with significant `Drop` can be early dropped
--> $DIR/significant_drop_tightening.rs:44:13
| |
LL | / { LL | / {
LL | | let mutex = Mutex::new(1i32); LL | | let mutex = Mutex::new(1i32);
@ -12,7 +36,6 @@ LL | | }
| |_____- temporary `lock` is currently being dropped at the end of its contained scope | |_____- temporary `lock` is currently being dropped at the end of its contained scope
| |
= note: this might lead to unnecessary resource contention = note: this might lead to unnecessary resource contention
= note: `-D clippy::significant-drop-tightening` implied by `-D warnings`
help: drop the temporary after the end of its last usage help: drop the temporary after the end of its last usage
| |
LL ~ let rslt1 = lock.is_positive(); LL ~ let rslt1 = lock.is_positive();
@ -20,7 +43,7 @@ LL + drop(lock);
| |
error: temporary with significant `Drop` can be early dropped error: temporary with significant `Drop` can be early dropped
--> $DIR/significant_drop_tightening.rs:46:13 --> $DIR/significant_drop_tightening.rs:65:13
| |
LL | / { LL | / {
LL | | let mutex = Mutex::new(1i32); LL | | let mutex = Mutex::new(1i32);
@ -44,7 +67,7 @@ LL +
| |
error: temporary with significant `Drop` can be early dropped error: temporary with significant `Drop` can be early dropped
--> $DIR/significant_drop_tightening.rs:52:17 --> $DIR/significant_drop_tightening.rs:71:17
| |
LL | / { LL | / {
LL | | let mutex = Mutex::new(vec![1i32]); LL | | let mutex = Mutex::new(vec![1i32]);
@ -67,5 +90,5 @@ LL - lock.clear();
LL + LL +
| |
error: aborting due to 3 previous errors error: aborting due to 4 previous errors