Skip to content

Commit ccdb3cd

Browse files
DevinR528DevinR528
committed
use Visitor api to find Mutex::lock calls
1 parent 5c66698 commit ccdb3cd

File tree

2 files changed

+96
-80
lines changed

2 files changed

+96
-80
lines changed

‎clippy_lints/src/if_let_mutex.rs

+94-78
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use crate::utils::{match_type, paths, span_lint_and_help};
2-
use if_chain::if_chain;
3-
use rustc_hir::{Arm, Expr, ExprKind, MatchSource, Stmt, StmtKind};
2+
use rustc::hir::map::Map;
3+
use rustc_hir::intravisit::{self as visit, NestedVisitorMap, Visitor};
4+
use rustc_hir::{Arm, Expr, ExprKind, MatchSource, StmtKind};
45
use rustc_lint::{LateContext, LateLintPass};
56
use rustc_session::{declare_lint_pass, declare_tool_lint};
67

@@ -40,100 +41,115 @@ declare_lint_pass!(IfLetMutex => [IF_LET_MUTEX]);
4041

4142
impl LateLintPass<'_, '_> for IfLetMutex {
4243
fn check_expr(&mut self, cx: &LateContext<'_, '_>, ex: &'_ Expr<'_>) {
43-
if_chain! {
44-
if let ExprKind::Match(ref op, ref arms, MatchSource::IfLetDesugar {
44+
let mut arm_visit = ArmVisitor {
45+
arm_mutex: false,
46+
arm_lock: false,
47+
cx,
48+
};
49+
let mut op_visit = IfLetMutexVisitor {
50+
op_mutex: false,
51+
op_lock: false,
52+
cx,
53+
};
54+
if let ExprKind::Match(
55+
ref op,
56+
ref arms,
57+
MatchSource::IfLetDesugar {
4558
contains_else_clause: true,
46-
}) = ex.kind; // if let ... {} else {}
47-
if let ExprKind::MethodCall(_, _, ref args) = op.kind;
48-
let ty = cx.tables.expr_ty(&args[0]);
49-
if match_type(cx, ty, &paths::MUTEX); // make sure receiver is Mutex
50-
if method_chain_names(op, 10).iter().any(|s| s == "lock"); // and lock is called
59+
},
60+
) = ex.kind
61+
{
62+
op_visit.visit_expr(op);
63+
if op_visit.op_mutex && op_visit.op_lock {
64+
for arm in *arms {
65+
arm_visit.visit_arm(arm);
66+
}
5167

52-
if arms.iter().any(|arm| matching_arm(arm, op, ex, cx));
53-
then {
54-
span_lint_and_help(
55-
cx,
56-
IF_LET_MUTEX,
57-
ex.span,
58-
"calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock",
59-
"move the lock call outside of the `if let ...` expression",
60-
);
68+
if arm_visit.arm_mutex && arm_visit.arm_lock {
69+
span_lint_and_help(
70+
cx,
71+
IF_LET_MUTEX,
72+
ex.span,
73+
"calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock",
74+
"move the lock call outside of the `if let ...` expression",
75+
);
76+
}
6177
}
6278
}
6379
}
6480
}
6581

66-
fn matching_arm(arm: &Arm<'_>, op: &Expr<'_>, ex: &Expr<'_>, cx: &LateContext<'_, '_>) -> bool {
67-
if let ExprKind::Block(ref block, _l) = arm.body.kind {
68-
block.stmts.iter().any(|stmt| matching_stmt(stmt, op, ex, cx))
69-
} else {
70-
false
71-
}
82+
/// Checks if `Mutex::lock` is called in the `if let _ = expr.
83+
pub struct IfLetMutexVisitor<'tcx, 'l> {
84+
pub op_mutex: bool,
85+
pub op_lock: bool,
86+
pub cx: &'tcx LateContext<'tcx, 'l>,
7287
}
7388

74-
fn matching_stmt(stmt: &Stmt<'_>, op: &Expr<'_>, ex: &Expr<'_>, cx: &LateContext<'_, '_>) -> bool {
75-
match stmt.kind {
76-
StmtKind::Local(l) => if_chain! {
77-
if let Some(ex) = l.init;
78-
if let ExprKind::MethodCall(_, _, _) = op.kind;
79-
if method_chain_names(ex, 10).iter().any(|s| s == "lock"); // and lock is called
80-
then {
81-
match_type_method_chain(cx, ex, 5)
82-
} else {
83-
false
84-
}
85-
},
86-
StmtKind::Expr(e) => if_chain! {
87-
if let ExprKind::MethodCall(_, _, _) = e.kind;
88-
if method_chain_names(e, 10).iter().any(|s| s == "lock"); // and lock is called
89-
then {
90-
match_type_method_chain(cx, ex, 5)
91-
} else {
92-
false
89+
impl<'tcx, 'l> Visitor<'tcx> for IfLetMutexVisitor<'tcx, 'l> {
90+
type Map = Map<'tcx>;
91+
92+
fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
93+
if let ExprKind::MethodCall(path, _span, args) = &expr.kind {
94+
if path.ident.to_string() == "lock" {
95+
self.op_lock = true;
9396
}
94-
},
95-
StmtKind::Semi(e) => if_chain! {
96-
if let ExprKind::MethodCall(_, _, _) = e.kind;
97-
if method_chain_names(e, 10).iter().any(|s| s == "lock"); // and lock is called
98-
then {
99-
match_type_method_chain(cx, ex, 5)
100-
} else {
101-
false
97+
let ty = self.cx.tables.expr_ty(&args[0]);
98+
if match_type(self.cx, ty, &paths::MUTEX) {
99+
self.op_mutex = true;
102100
}
103-
},
104-
_ => false,
101+
}
102+
visit::walk_expr(self, expr);
105103
}
104+
105+
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
106+
NestedVisitorMap::None
107+
}
108+
}
109+
110+
/// Checks if `Mutex::lock` is called in any of the branches.
111+
pub struct ArmVisitor<'tcx, 'l> {
112+
pub arm_mutex: bool,
113+
pub arm_lock: bool,
114+
pub cx: &'tcx LateContext<'tcx, 'l>,
106115
}
107116

108-
/// Return the names of `max_depth` number of methods called in the chain.
109-
fn method_chain_names<'tcx>(expr: &'tcx Expr<'tcx>, max_depth: usize) -> Vec<String> {
110-
let mut method_names = Vec::with_capacity(max_depth);
111-
let mut current = expr;
112-
for _ in 0..max_depth {
113-
if let ExprKind::MethodCall(path, _, args) = &current.kind {
114-
if args.iter().any(|e| e.span.from_expansion()) {
115-
break;
117+
impl<'tcx, 'l> Visitor<'tcx> for ArmVisitor<'tcx, 'l> {
118+
type Map = Map<'tcx>;
119+
120+
fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
121+
if let ExprKind::MethodCall(path, _span, args) = &expr.kind {
122+
if path.ident.to_string() == "lock" {
123+
self.arm_lock = true;
124+
}
125+
let ty = self.cx.tables.expr_ty(&args[0]);
126+
if match_type(self.cx, ty, &paths::MUTEX) {
127+
self.arm_mutex = true;
116128
}
117-
method_names.push(path.ident.to_string());
118-
current = &args[0];
119-
} else {
120-
break;
121129
}
130+
visit::walk_expr(self, expr);
122131
}
123-
method_names
124-
}
125132

126-
/// Check that lock is called on a `Mutex`.
127-
fn match_type_method_chain<'tcx>(cx: &LateContext<'_, '_>, expr: &'tcx Expr<'tcx>, max_depth: usize) -> bool {
128-
let mut current = expr;
129-
for _ in 0..max_depth {
130-
if let ExprKind::MethodCall(_, _, args) = &current.kind {
131-
let ty = cx.tables.expr_ty(&args[0]);
132-
if match_type(cx, ty, &paths::MUTEX) {
133-
return true;
133+
fn visit_arm(&mut self, arm: &'tcx Arm<'_>) {
134+
if let ExprKind::Block(ref block, _l) = arm.body.kind {
135+
for stmt in block.stmts {
136+
match stmt.kind {
137+
StmtKind::Local(loc) => {
138+
if let Some(expr) = loc.init {
139+
self.visit_expr(expr)
140+
}
141+
},
142+
StmtKind::Expr(expr) => self.visit_expr(expr),
143+
StmtKind::Semi(expr) => self.visit_expr(expr),
144+
// we don't care about `Item`
145+
_ => {},
146+
}
134147
}
135-
current = &args[0];
136-
}
148+
};
149+
visit::walk_arm(self, arm);
150+
}
151+
152+
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
153+
NestedVisitorMap::None
137154
}
138-
false
139155
}

‎tests/ui/if_let_mutex.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@
33
use std::sync::Mutex;
44

55
fn do_stuff<T>(_: T) {}
6-
fn foo() {
7-
let m = Mutex::new(1u8);
86

7+
fn if_let() {
8+
let m = Mutex::new(1u8);
99
if let Err(locked) = m.lock() {
1010
do_stuff(locked);
1111
} else {

0 commit comments

Comments
 (0)