-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of #4680 - hellow554:debug_assert_mut_call, r=flip1995
Add lint for debug_assert_with_mut_call closes #1526 **What does not work:** * detecting a mut call in the format string itself, e.g. `debug_assert!(false, "{}", vec![1].pop())` * detecting `*mut T` usage (pointer) --- changelog: add new lint `debug_assert_with_mut_call`
- Loading branch information
Showing
7 changed files
with
465 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,155 @@ | ||
| use crate::utils::{is_direct_expn_of, span_lint}; | ||
| use if_chain::if_chain; | ||
| use matches::matches; | ||
| 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; | ||
|
|
||
| declare_clippy_lint! { | ||
| /// **What it does:** Checks for function/method calls with a mutable | ||
| /// parameter in `debug_assert!`, `debug_assert_eq!` and `debug_assert_ne!` macros. | ||
| /// | ||
| /// **Why is this bad?** In release builds `debug_assert!` macros are optimized out by the | ||
| /// compiler. | ||
| /// Therefore mutating something in a `debug_assert!` macro results in different behaviour | ||
| /// between a release and debug build. | ||
| /// | ||
| /// **Known problems:** None | ||
| /// | ||
| /// **Example:** | ||
| /// ```rust,ignore | ||
| /// debug_assert_eq!(vec![3].pop(), Some(3)); | ||
| /// // or | ||
| /// fn take_a_mut_parameter(_: &mut u32) -> bool { unimplemented!() } | ||
| /// debug_assert!(take_a_mut_parameter(&mut 5)); | ||
| /// ``` | ||
| pub DEBUG_ASSERT_WITH_MUT_CALL, | ||
| correctness, | ||
| "mutable arguments in `debug_assert{,_ne,_eq}!`" | ||
| } | ||
|
|
||
| declare_lint_pass!(DebugAssertWithMutCall => [DEBUG_ASSERT_WITH_MUT_CALL]); | ||
|
|
||
| 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), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| //HACK(hellow554): remove this when #4694 is implemented | ||
| #[allow(clippy::cognitive_complexity)] | ||
| 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; | ||
| if let StmtKind::Semi(ref matchexpr) = block.stmts[0].kind; | ||
| then { | ||
| if_chain! { | ||
| if let ExprKind::Match(ref ifclause, _, _) = matchexpr.kind; | ||
| if let ExprKind::DropTemps(ref droptmp) = ifclause.kind; | ||
| if let ExprKind::Unary(UnOp::UnNot, ref condition) = droptmp.kind; | ||
| then { | ||
| // debug_assert | ||
| let mut visitor = MutArgVisitor::new(cx); | ||
| visitor.visit_expr(condition); | ||
| return visitor.expr_span(); | ||
| } else { | ||
| // debug_assert_{eq,ne} | ||
| if_chain! { | ||
| if let ExprKind::Block(ref matchblock, _) = matchexpr.kind; | ||
| if let Some(ref matchheader) = matchblock.expr; | ||
| if let ExprKind::Match(ref headerexpr, _, _) = matchheader.kind; | ||
| if let ExprKind::Tup(ref conditions) = headerexpr.kind; | ||
| if conditions.len() == 2; | ||
| then { | ||
| if let ExprKind::AddrOf(_, ref lhs) = conditions[0].kind { | ||
| let mut visitor = MutArgVisitor::new(cx); | ||
| visitor.visit_expr(lhs); | ||
| if let Some(span) = visitor.expr_span() { | ||
| return Some(span); | ||
| } | ||
| } | ||
| if let ExprKind::AddrOf(_, ref rhs) = conditions[1].kind { | ||
| let mut visitor = MutArgVisitor::new(cx); | ||
| visitor.visit_expr(rhs); | ||
| if let Some(span) = visitor.expr_span() { | ||
| return Some(span); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| None | ||
| } | ||
|
|
||
| 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 expr_span(&self) -> Option<Span> { | ||
| if self.found { | ||
| self.expr_span | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 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::OnlyBodies(&self.cx.tcx.hir()) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| #![feature(custom_inner_attributes)] | ||
| #![rustfmt::skip] | ||
| #![allow(clippy::trivially_copy_pass_by_ref, clippy::cognitive_complexity, clippy::redundant_closure_call)] | ||
|
|
||
| struct S; | ||
|
|
||
| impl S { | ||
| fn bool_self_ref(&self) -> bool { false } | ||
| fn bool_self_mut(&mut self) -> bool { false } | ||
| fn bool_self_ref_arg_ref(&self, _: &u32) -> bool { false } | ||
| fn bool_self_ref_arg_mut(&self, _: &mut u32) -> bool { false } | ||
| fn bool_self_mut_arg_ref(&mut self, _: &u32) -> bool { false } | ||
| fn bool_self_mut_arg_mut(&mut self, _: &mut u32) -> bool { false } | ||
|
|
||
| fn u32_self_ref(&self) -> u32 { 0 } | ||
| fn u32_self_mut(&mut self) -> u32 { 0 } | ||
| fn u32_self_ref_arg_ref(&self, _: &u32) -> u32 { 0 } | ||
| fn u32_self_ref_arg_mut(&self, _: &mut u32) -> u32 { 0 } | ||
| fn u32_self_mut_arg_ref(&mut self, _: &u32) -> u32 { 0 } | ||
| fn u32_self_mut_arg_mut(&mut self, _: &mut u32) -> u32 { 0 } | ||
| } | ||
|
|
||
| fn bool_ref(_: &u32) -> bool { false } | ||
| fn bool_mut(_: &mut u32) -> bool { false } | ||
| fn u32_ref(_: &u32) -> u32 { 0 } | ||
| fn u32_mut(_: &mut u32) -> u32 { 0 } | ||
|
|
||
| fn func_non_mutable() { | ||
| debug_assert!(bool_ref(&3)); | ||
| debug_assert!(!bool_ref(&3)); | ||
|
|
||
| debug_assert_eq!(0, u32_ref(&3)); | ||
| debug_assert_eq!(u32_ref(&3), 0); | ||
|
|
||
| debug_assert_ne!(1, u32_ref(&3)); | ||
| debug_assert_ne!(u32_ref(&3), 1); | ||
| } | ||
|
|
||
| fn func_mutable() { | ||
| debug_assert!(bool_mut(&mut 3)); | ||
| debug_assert!(!bool_mut(&mut 3)); | ||
|
|
||
| debug_assert_eq!(0, u32_mut(&mut 3)); | ||
| debug_assert_eq!(u32_mut(&mut 3), 0); | ||
|
|
||
| debug_assert_ne!(1, u32_mut(&mut 3)); | ||
| debug_assert_ne!(u32_mut(&mut 3), 1); | ||
| } | ||
|
|
||
| fn method_non_mutable() { | ||
| debug_assert!(S.bool_self_ref()); | ||
| debug_assert!(S.bool_self_ref_arg_ref(&3)); | ||
|
|
||
| debug_assert_eq!(S.u32_self_ref(), 0); | ||
| debug_assert_eq!(S.u32_self_ref_arg_ref(&3), 0); | ||
|
|
||
| debug_assert_ne!(S.u32_self_ref(), 1); | ||
| debug_assert_ne!(S.u32_self_ref_arg_ref(&3), 1); | ||
| } | ||
|
|
||
| fn method_mutable() { | ||
| debug_assert!(S.bool_self_mut()); | ||
| debug_assert!(!S.bool_self_mut()); | ||
| debug_assert!(S.bool_self_ref_arg_mut(&mut 3)); | ||
| debug_assert!(S.bool_self_mut_arg_ref(&3)); | ||
| debug_assert!(S.bool_self_mut_arg_mut(&mut 3)); | ||
|
|
||
| debug_assert_eq!(S.u32_self_mut(), 0); | ||
| debug_assert_eq!(S.u32_self_mut_arg_ref(&3), 0); | ||
| debug_assert_eq!(S.u32_self_ref_arg_mut(&mut 3), 0); | ||
| debug_assert_eq!(S.u32_self_mut_arg_mut(&mut 3), 0); | ||
|
|
||
| debug_assert_ne!(S.u32_self_mut(), 1); | ||
| debug_assert_ne!(S.u32_self_mut_arg_ref(&3), 1); | ||
| debug_assert_ne!(S.u32_self_ref_arg_mut(&mut 3), 1); | ||
| debug_assert_ne!(S.u32_self_mut_arg_mut(&mut 3), 1); | ||
| } | ||
|
|
||
| fn misc() { | ||
| // with variable | ||
| let mut v: Vec<u32> = vec![1, 2, 3, 4]; | ||
| debug_assert_eq!(v.get(0), Some(&1)); | ||
| debug_assert_ne!(v[0], 2); | ||
| debug_assert_eq!(v.pop(), Some(1)); | ||
| debug_assert_ne!(Some(3), v.pop()); | ||
|
|
||
| let a = &mut 3; | ||
| debug_assert!(bool_mut(a)); | ||
|
|
||
| // nested | ||
| debug_assert!(!(bool_ref(&u32_mut(&mut 3)))); | ||
|
|
||
| // chained | ||
| debug_assert_eq!(v.pop().unwrap(), 3); | ||
|
|
||
| // format args | ||
| debug_assert!(bool_ref(&3), "w/o format"); | ||
| debug_assert!(bool_mut(&mut 3), "w/o format"); | ||
| debug_assert!(bool_ref(&3), "{} format", "w/"); | ||
| debug_assert!(bool_mut(&mut 3), "{} format", "w/"); | ||
|
|
||
| // sub block | ||
| let mut x = 42_u32; | ||
| debug_assert!({ | ||
| bool_mut(&mut x); | ||
| x > 10 | ||
| }); | ||
|
|
||
| // closures | ||
| debug_assert!((|| { | ||
| let mut x = 42; | ||
| bool_mut(&mut x); | ||
| x > 10 | ||
| })()); | ||
| } | ||
|
|
||
| fn main() { | ||
| func_non_mutable(); | ||
| func_mutable(); | ||
| method_non_mutable(); | ||
| method_mutable(); | ||
|
|
||
| misc(); | ||
| } |
Oops, something went wrong.