From 4f14826e091e08f1ee4ff06d3ca6d5015045cfb5 Mon Sep 17 00:00:00 2001 From: xiongmao86 Date: Mon, 6 Apr 2020 21:48:38 +0800 Subject: [PATCH] Lint on opt.as_ref().map(|x| &**x). --- clippy_lints/src/loops.rs | 4 +-- clippy_lints/src/methods/mod.rs | 49 ++++++++++++++++++++--------- tests/ui/option_as_ref_deref.fixed | 3 ++ tests/ui/option_as_ref_deref.rs | 3 ++ tests/ui/option_as_ref_deref.stderr | 14 ++++++++- 5 files changed, 55 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 43217b6cc64e..7cc21e4bdd23 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -654,7 +654,7 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult fn never_loop_block(block: &Block<'_>, main_loop_id: HirId) -> NeverLoopResult { let stmts = block.stmts.iter().map(stmt_to_expr); - let expr = once(block.expr.as_ref().map(|p| &**p)); + let expr = once(block.expr.as_deref()); let mut iter = stmts.chain(expr).filter_map(|e| e); never_loop_expr_seq(&mut iter, main_loop_id) } @@ -662,7 +662,7 @@ fn never_loop_block(block: &Block<'_>, main_loop_id: HirId) -> NeverLoopResult { fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<&'tcx Expr<'tcx>> { match stmt.kind { StmtKind::Semi(ref e, ..) | StmtKind::Expr(ref e, ..) => Some(e), - StmtKind::Local(ref local) => local.init.as_ref().map(|p| &**p), + StmtKind::Local(ref local) => local.init.as_deref(), _ => None, } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 527508af8a31..6f85d1d69596 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3159,6 +3159,8 @@ fn lint_option_as_ref_deref<'a, 'tcx>( map_args: &[hir::Expr<'_>], is_mut: bool, ) { + let same_mutability = |m| (is_mut && m == &hir::Mutability::Mut) || (!is_mut && m == &hir::Mutability::Not); + let option_ty = cx.tables.expr_ty(&as_ref_args[0]); if !match_type(cx, option_ty, &paths::OPTION) { return; @@ -3181,23 +3183,40 @@ fn lint_option_as_ref_deref<'a, 'tcx>( hir::ExprKind::Closure(_, _, body_id, _, _) => { let closure_body = cx.tcx.hir().body(body_id); let closure_expr = remove_blocks(&closure_body.value); - if_chain! { - if let hir::ExprKind::MethodCall(_, _, args) = &closure_expr.kind; - if args.len() == 1; - if let hir::ExprKind::Path(qpath) = &args[0].kind; - if let hir::def::Res::Local(local_id) = cx.tables.qpath_res(qpath, args[0].hir_id); - if closure_body.params[0].pat.hir_id == local_id; - let adj = cx.tables.expr_adjustments(&args[0]).iter().map(|x| &x.kind).collect::>(); - if let [ty::adjustment::Adjust::Deref(None), ty::adjustment::Adjust::Borrow(_)] = *adj; - then { - let method_did = cx.tables.type_dependent_def_id(closure_expr.hir_id).unwrap(); - deref_aliases.iter().any(|path| match_def_path(cx, method_did, path)) - } else { - false - } + + match &closure_expr.kind { + hir::ExprKind::MethodCall(_, _, args) => { + if_chain! { + if args.len() == 1; + if let hir::ExprKind::Path(qpath) = &args[0].kind; + if let hir::def::Res::Local(local_id) = cx.tables.qpath_res(qpath, args[0].hir_id); + if closure_body.params[0].pat.hir_id == local_id; + let adj = cx.tables.expr_adjustments(&args[0]).iter().map(|x| &x.kind).collect::>(); + if let [ty::adjustment::Adjust::Deref(None), ty::adjustment::Adjust::Borrow(_)] = *adj; + then { + let method_did = cx.tables.type_dependent_def_id(closure_expr.hir_id).unwrap(); + deref_aliases.iter().any(|path| match_def_path(cx, method_did, path)) + } else { + false + } + } + }, + hir::ExprKind::AddrOf(hir::BorrowKind::Ref, m, ref inner) if same_mutability(m) => { + if_chain! { + if let hir::ExprKind::Unary(hir::UnOp::UnDeref, ref inner1) = inner.kind; + if let hir::ExprKind::Unary(hir::UnOp::UnDeref, ref inner2) = inner1.kind; + if let hir::ExprKind::Path(ref qpath) = inner2.kind; + if let hir::def::Res::Local(local_id) = cx.tables.qpath_res(qpath, inner2.hir_id); + then { + closure_body.params[0].pat.hir_id == local_id + } else { + false + } + } + }, + _ => false, } }, - _ => false, }; diff --git a/tests/ui/option_as_ref_deref.fixed b/tests/ui/option_as_ref_deref.fixed index 973e5b308a2c..076692e64451 100644 --- a/tests/ui/option_as_ref_deref.fixed +++ b/tests/ui/option_as_ref_deref.fixed @@ -35,4 +35,7 @@ fn main() { let _ = Some(1_usize).as_ref().map(|x| vc[*x].as_str()); // should not be linted let _: Option<&str> = Some(&String::new()).as_ref().map(|x| x.as_str()); // should not be linted + + let _ = opt.as_deref(); + let _ = opt.as_deref_mut(); } diff --git a/tests/ui/option_as_ref_deref.rs b/tests/ui/option_as_ref_deref.rs index baad85ab9083..3bf5f715f833 100644 --- a/tests/ui/option_as_ref_deref.rs +++ b/tests/ui/option_as_ref_deref.rs @@ -38,4 +38,7 @@ fn main() { let _ = Some(1_usize).as_ref().map(|x| vc[*x].as_str()); // should not be linted let _: Option<&str> = Some(&String::new()).as_ref().map(|x| x.as_str()); // should not be linted + + let _ = opt.as_ref().map(|x| &**x); + let _ = opt.as_mut().map(|x| &mut **x); } diff --git a/tests/ui/option_as_ref_deref.stderr b/tests/ui/option_as_ref_deref.stderr index 09a0fa058e62..6c2bf8517060 100644 --- a/tests/ui/option_as_ref_deref.stderr +++ b/tests/ui/option_as_ref_deref.stderr @@ -88,5 +88,17 @@ error: called `.as_mut().map(DerefMut::deref_mut)` (or with one of deref aliases LL | let _ = opt.clone().as_mut().map(|x| x.deref_mut()).map(|x| x.len()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref_mut instead: `opt.clone().as_deref_mut()` -error: aborting due to 14 previous errors +error: called `.as_ref().map(Deref::deref)` (or with one of deref aliases) on an Option value. This can be done more directly by calling `opt.as_deref()` instead + --> $DIR/option_as_ref_deref.rs:42:13 + | +LL | let _ = opt.as_ref().map(|x| &**x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref instead: `opt.as_deref()` + +error: called `.as_mut().map(DerefMut::deref_mut)` (or with one of deref aliases) on an Option value. This can be done more directly by calling `opt.as_deref_mut()` instead + --> $DIR/option_as_ref_deref.rs:43:13 + | +LL | let _ = opt.as_mut().map(|x| &mut **x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref_mut instead: `opt.as_deref_mut()` + +error: aborting due to 16 previous errors