From a9f1bb43ef6ba4a4774ccbd6211814bb35d7724f Mon Sep 17 00:00:00 2001 From: Devin R Date: Wed, 15 Apr 2020 17:08:26 -0400 Subject: [PATCH] test for mutex eq, add another test case --- clippy_lints/src/if_let_mutex.rs | 50 +++++++++++++++----------------- tests/ui/if_let_mutex.rs | 13 ++++++++- tests/ui/if_let_mutex.stderr | 17 +++++++++-- 3 files changed, 51 insertions(+), 29 deletions(-) diff --git a/clippy_lints/src/if_let_mutex.rs b/clippy_lints/src/if_let_mutex.rs index b98f190eb36c..21749f8ad094 100644 --- a/clippy_lints/src/if_let_mutex.rs +++ b/clippy_lints/src/if_let_mutex.rs @@ -1,7 +1,7 @@ +use crate::utils::{match_type, paths, span_lint_and_help, SpanlessEq}; use if_chain::if_chain; -use crate::utils::{match_type, paths, span_lint_and_help}; use rustc_hir::intravisit::{self as visit, NestedVisitorMap, Visitor}; -use rustc_hir::{Arm, Expr, ExprKind, MatchSource, StmtKind}; +use rustc_hir::{Expr, ExprKind, MatchSource}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::map::Map; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -44,10 +44,12 @@ impl LateLintPass<'_, '_> for IfLetMutex { fn check_expr(&mut self, cx: &LateContext<'_, '_>, ex: &'_ Expr<'_>) { let mut arm_visit = ArmVisitor { mutex_lock_called: false, + found_mutex: None, cx, }; let mut op_visit = OppVisitor { mutex_lock_called: false, + found_mutex: None, cx, }; if let ExprKind::Match( @@ -64,7 +66,7 @@ impl LateLintPass<'_, '_> for IfLetMutex { arm_visit.visit_arm(arm); } - if arm_visit.mutex_lock_called { + if arm_visit.mutex_lock_called && arm_visit.same_mutex(cx, op_visit.found_mutex.unwrap()) { span_lint_and_help( cx, IF_LET_MUTEX, @@ -80,8 +82,9 @@ impl LateLintPass<'_, '_> for IfLetMutex { /// Checks if `Mutex::lock` is called in the `if let _ = expr. pub struct OppVisitor<'tcx, 'l> { - pub mutex_lock_called: bool, - pub cx: &'tcx LateContext<'tcx, 'l>, + mutex_lock_called: bool, + found_mutex: Option<&'tcx Expr<'tcx>>, + cx: &'tcx LateContext<'tcx, 'l>, } impl<'tcx, 'l> Visitor<'tcx> for OppVisitor<'tcx, 'l> { @@ -94,6 +97,7 @@ impl<'tcx, 'l> Visitor<'tcx> for OppVisitor<'tcx, 'l> { let ty = self.cx.tables.expr_ty(&args[0]); if match_type(self.cx, ty, &paths::MUTEX); then { + self.found_mutex = Some(&args[0]); self.mutex_lock_called = true; return; } @@ -108,20 +112,22 @@ impl<'tcx, 'l> Visitor<'tcx> for OppVisitor<'tcx, 'l> { /// Checks if `Mutex::lock` is called in any of the branches. pub struct ArmVisitor<'tcx, 'l> { - pub mutex_lock_called: bool, - pub cx: &'tcx LateContext<'tcx, 'l>, + mutex_lock_called: bool, + found_mutex: Option<&'tcx Expr<'tcx>>, + cx: &'tcx LateContext<'tcx, 'l>, } impl<'tcx, 'l> Visitor<'tcx> for ArmVisitor<'tcx, 'l> { type Map = Map<'tcx>; - fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { if_chain! { if let ExprKind::MethodCall(path, _span, args) = &expr.kind; if path.ident.to_string() == "lock"; let ty = self.cx.tables.expr_ty(&args[0]); if match_type(self.cx, ty, &paths::MUTEX); then { + self.found_mutex = Some(&args[0]); self.mutex_lock_called = true; return; } @@ -129,25 +135,17 @@ impl<'tcx, 'l> Visitor<'tcx> for ArmVisitor<'tcx, 'l> { visit::walk_expr(self, expr); } - fn visit_arm(&mut self, arm: &'tcx Arm<'_>) { - if let ExprKind::Block(ref block, _l) = arm.body.kind { - for stmt in block.stmts { - match stmt.kind { - StmtKind::Local(loc) => { - if let Some(expr) = loc.init { - self.visit_expr(expr) - } - }, - StmtKind::Expr(expr) | StmtKind::Semi(expr) => self.visit_expr(expr), - // we don't care about `Item` - _ => {}, - } - } - }; - visit::walk_arm(self, arm); - } - fn nested_visit_map(&mut self) -> NestedVisitorMap { NestedVisitorMap::None } } + +impl<'tcx, 'l> ArmVisitor<'tcx, 'l> { + fn same_mutex(&self, cx: &LateContext<'_, '_>, op_mutex: &Expr<'_>) -> bool { + if let Some(arm_mutex) = self.found_mutex { + SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex) + } else { + false + } + } +} diff --git a/tests/ui/if_let_mutex.rs b/tests/ui/if_let_mutex.rs index 1d7cc756dc5b..d59ec83679a9 100644 --- a/tests/ui/if_let_mutex.rs +++ b/tests/ui/if_let_mutex.rs @@ -1,11 +1,12 @@ #![warn(clippy::if_let_mutex)] +use std::ops::Deref; use std::sync::Mutex; fn do_stuff(_: T) {} fn if_let() { - let m = Mutex::new(1u8); + let m = Mutex::new(1_u8); if let Err(locked) = m.lock() { do_stuff(locked); } else { @@ -14,4 +15,14 @@ fn if_let() { }; } +fn if_let_option() { + let m = Mutex::new(Some(0_u8)); + if let Some(locked) = m.lock().unwrap().deref() { + do_stuff(locked); + } else { + let lock = m.lock().unwrap(); + do_stuff(lock); + }; +} + fn main() {} diff --git a/tests/ui/if_let_mutex.stderr b/tests/ui/if_let_mutex.stderr index 84c19ed03d58..0c24d650c27b 100644 --- a/tests/ui/if_let_mutex.stderr +++ b/tests/ui/if_let_mutex.stderr @@ -1,5 +1,5 @@ error: calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock - --> $DIR/if_let_mutex.rs:9:5 + --> $DIR/if_let_mutex.rs:10:5 | LL | / if let Err(locked) = m.lock() { LL | | do_stuff(locked); @@ -12,5 +12,18 @@ LL | | }; = note: `-D clippy::if-let-mutex` implied by `-D warnings` = help: move the lock call outside of the `if let ...` expression -error: aborting due to previous error +error: calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock + --> $DIR/if_let_mutex.rs:20:5 + | +LL | / if let Some(locked) = m.lock().unwrap().deref() { +LL | | do_stuff(locked); +LL | | } else { +LL | | let lock = m.lock().unwrap(); +LL | | do_stuff(lock); +LL | | }; + | |_____^ + | + = help: move the lock call outside of the `if let ...` expression + +error: aborting due to 2 previous errors