use Visitor to walk through expressions

Author: hellow554

clippy_lints/src/mutable_debug_assertion.rs

@@ -1,8 +1,8 @@
 use crate::utils::{is_direct_expn_of, span_lint};
-use core::ops::Deref;
 use if_chain::if_chain;
 use matches::matches;
-use rustc::hir::{Expr, ExprKind, Guard, Mutability, StmtKind, UnOp};
+use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
+use rustc::hir::{Expr, ExprKind, Mutability, StmtKind, UnOp};
 use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use rustc::{declare_lint_pass, declare_tool_lint, ty};
 use syntax_pos::Span;
@@ -32,72 +32,28 @@ declare_clippy_lint! {
 
 declare_lint_pass!(DebugAssertWithMutCall => [DEBUG_ASSERT_WITH_MUT_CALL]);
 
-fn check_for_mutable_call(cx: &LateContext<'_, '_>, span: Span, e: &Expr) -> Option<Span> {
-    match &e.kind {
-        ExprKind::AddrOf(Mutability::MutMutable, _) => Some(span),
-        ExprKind::Path(_) => cx.tables.adjustments().get(e.hir_id).and_then(|adj| {
-            if adj
-                .iter()
-                .any(|a| matches!(a.target.kind, ty::Ref(_, _, Mutability::MutMutable)))
-            {
-                Some(span)
-            } else {
-                None
+const DEBUG_MACRO_NAMES: [&str; 3] = ["debug_assert", "debug_assert_eq", "debug_assert_ne"];
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for DebugAssertWithMutCall {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) {
+        for dmn in &DEBUG_MACRO_NAMES {
+            if is_direct_expn_of(e.span, dmn).is_some() {
+                if let Some(span) = extract_call(cx, e) {
+                    span_lint(
+                        cx,
+                        DEBUG_ASSERT_WITH_MUT_CALL,
+                        span,
+                        &format!("do not call a function with mutable arguments inside of `{}!`", dmn),
+                    );
+                }
             }
-        }),
-        ExprKind::AddrOf(Mutability::MutImmutable, expr)
-        | ExprKind::Assign(_, expr)
-        | ExprKind::AssignOp(_, _, expr)
-        | ExprKind::Box(expr)
-        | ExprKind::Break(_, Some(expr))
-        | ExprKind::Cast(expr, _)
-        | ExprKind::DropTemps(expr)
-        | ExprKind::Field(expr, _)
-        | ExprKind::Repeat(expr, _)
-        | ExprKind::Ret(Some(expr))
-        | ExprKind::Type(expr, _)
-        | ExprKind::Unary(_, expr)
-        | ExprKind::Yield(expr, _) => check_for_mutable_call(cx, expr.span, expr),
-        ExprKind::Binary(_, lhs, rhs) | ExprKind::Index(lhs, rhs) => {
-            check_for_mutable_call(cx, lhs.span, lhs).or_else(|| check_for_mutable_call(cx, rhs.span, rhs))
-        },
-        ExprKind::Array(args) | ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args) | ExprKind::Tup(args) => {
-            (*args).iter().find_map(|a| check_for_mutable_call(cx, span, a))
-        },
-        ExprKind::Match(header, arm, _) => check_for_mutable_call(cx, header.span, header).or_else(|| {
-            (*arm).iter().find_map(|a| {
-                check_for_mutable_call(cx, a.body.span, &a.body).or_else(|| {
-                    a.guard.as_ref().and_then(|g| {
-                        let Guard::If(e) = g;
-                        check_for_mutable_call(cx, e.span, &e)
-                    })
-                })
-            })
-        }),
-        ExprKind::Block(block, _) | ExprKind::Loop(block, _, _) => block
-            .stmts
-            .iter()
-            .filter_map(|s| match &s.kind {
-                StmtKind::Local(l) => l.init.as_ref().map(|x| x.deref()),
-                StmtKind::Expr(e) => Some(e),
-                StmtKind::Semi(e) => Some(e),
-                StmtKind::Item(_) => None,
-            })
-            .chain(block.expr.as_ref().map(|x| x.deref()))
-            .find_map(|a| check_for_mutable_call(cx, a.span, a)),
-        ExprKind::Err
-        | ExprKind::Lit(_)
-        | ExprKind::Continue(_)
-        | ExprKind::InlineAsm(_, _, _)
-        | ExprKind::Struct(_, _, _)
-        | ExprKind::Closure(_, _, _, _, _)
-        | ExprKind::Break(_, None)
-        | ExprKind::Ret(None) => None,
+        }
     }
 }
 
+//HACK(hellow554): remove this when #4694 is implemented
 #[allow(clippy::cognitive_complexity)]
-fn extract_call(cx: &LateContext<'_, '_>, e: &Expr) -> Option<Span> {
+fn extract_call<'a, 'tcx>(cx: &'a LateContext<'a, 'tcx>, e: &'tcx Expr) -> Option<Span> {
     if_chain! {
         if let ExprKind::Block(ref block, _) = e.kind;
         if block.stmts.len() == 1;
@@ -109,7 +65,7 @@ fn extract_call(cx: &LateContext<'_, '_>, e: &Expr) -> Option<Span> {
                 if let ExprKind::Unary(UnOp::UnNot, ref condition) = droptmp.kind;
                 then {
                     // debug_assert
-                    return check_for_mutable_call(cx, condition.span, condition)
+                    return MutArgVisitor::go(cx, condition);
                 } else {
                     // debug_assert_{eq,ne}
                     if_chain! {
@@ -120,12 +76,12 @@ fn extract_call(cx: &LateContext<'_, '_>, e: &Expr) -> Option<Span> {
                         if conditions.len() == 2;
                         then {
                             if let ExprKind::AddrOf(_, ref lhs) = conditions[0].kind {
-                                if let Some(span) = check_for_mutable_call(cx, lhs.span, lhs) {
+                                if let Some(span) = MutArgVisitor::go(cx, lhs) {
                                     return Some(span);
                                 }
                             }
                             if let ExprKind::AddrOf(_, ref rhs) = conditions[1].kind {
-                                if let Some(span) = check_for_mutable_call(cx, rhs.span, rhs) {
+                                if let Some(span) = MutArgVisitor::go(cx, rhs) {
                                     return Some(span);
                                 }
                             }
@@ -138,20 +94,62 @@ fn extract_call(cx: &LateContext<'_, '_>, e: &Expr) -> Option<Span> {
 
     None
 }
-impl<'a, 'tcx> LateLintPass<'a, 'tcx> for DebugAssertWithMutCall {
-    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) {
-        if ["debug_assert", "debug_assert_eq", "debug_assert_ne"]
-            .iter()
-            .any(|name| is_direct_expn_of(e.span, name).is_some())
-        {
-            if let Some(span) = extract_call(cx, e) {
-                span_lint(
-                    cx,
-                    DEBUG_ASSERT_WITH_MUT_CALL,
-                    span,
-                    "do not call functions with mutable arguments inside of a `debug_assert!`",
-                );
-            }
+
+struct MutArgVisitor<'a, 'tcx> {
+    cx: &'a LateContext<'a, 'tcx>,
+    expr_span: Option<Span>,
+    found: bool,
+}
+
+impl<'a, 'tcx> MutArgVisitor<'a, 'tcx> {
+    fn new(cx: &'a LateContext<'a, 'tcx>) -> Self {
+        Self {
+            cx,
+            expr_span: None,
+            found: false,
+        }
+    }
+
+    fn get_span(&self) -> Option<Span> {
+        if self.found {
+            self.expr_span
+        } else {
+            None
+        }
+    }
+
+    fn go(cx: &'a LateContext<'a, 'tcx>, start: &'tcx Expr) -> Option<Span> {
+        let mut s = Self::new(cx);
+        s.visit_expr(start);
+        s.get_span()
+    }
+}
+
+impl<'a, 'tcx> Visitor<'tcx> for MutArgVisitor<'a, 'tcx> {
+    fn visit_expr(&mut self, expr: &'tcx Expr) {
+        match expr.kind {
+            ExprKind::AddrOf(Mutability::MutMutable, _) => {
+                self.found = true;
+                return;
+            },
+            ExprKind::Path(_) => {
+                if let Some(adj) = self.cx.tables.adjustments().get(expr.hir_id) {
+                    if adj
+                        .iter()
+                        .any(|a| matches!(a.target.kind, ty::Ref(_, _, Mutability::MutMutable)))
+                    {
+                        self.found = true;
+                        return;
+                    }
+                }
+            },
+            _ if !self.found => self.expr_span = Some(expr.span),
+            _ => return,
         }
+        walk_expr(self, expr)
+    }
+
+    fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
+        NestedVisitorMap::All(&self.cx.tcx.hir())
     }
 }

tests/ui/debug_assert_with_mut_call.rs

@@ -1,6 +1,6 @@
 #![feature(custom_inner_attributes)]
 #![rustfmt::skip]
-#![allow(clippy::trivially_copy_pass_by_ref, clippy::cognitive_complexity)]
+#![allow(clippy::trivially_copy_pass_by_ref, clippy::cognitive_complexity, clippy::redundant_closure_call)]
 
 struct S;
 
@@ -105,6 +105,13 @@ fn misc() {
         bool_mut(&mut x);
         x > 10
     });
+
+    // closures
+    debug_assert!((|| {
+        let mut x = 42;
+        bool_mut(&mut x);
+        x > 10
+    })());
 }
 
 fn main() {