From 4f14826e091e08f1ee4ff06d3ca6d5015045cfb5 Mon Sep 17 00:00:00 2001 From: xiongmao86 Date: Mon, 6 Apr 2020 21:48:38 +0800 Subject: [PATCH 1/2] 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 43217b6cc..7cc21e4bd 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 527508af8..6f85d1d69 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 973e5b308..076692e64 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 baad85ab9..3bf5f715f 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 09a0fa058..6c2bf8517 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 From d7056f8ffba7ad87622b7fdcc429f33abc5c62a7 Mon Sep 17 00:00:00 2001 From: xiongmao86 Date: Tue, 7 Apr 2020 21:25:07 +0800 Subject: [PATCH 2/2] Refine lint message. --- clippy_lints/src/methods/mod.rs | 8 ++++---- tests/ui/option_as_ref_deref.stderr | 32 ++++++++++++++--------------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 6f85d1d69..0f331aad0 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3222,17 +3222,17 @@ fn lint_option_as_ref_deref<'a, 'tcx>( if is_deref { let current_method = if is_mut { - ".as_mut().map(DerefMut::deref_mut)" + format!(".as_mut().map({})", snippet(cx, map_args[1].span, "..")) } else { - ".as_ref().map(Deref::deref)" + format!(".as_ref().map({})", snippet(cx, map_args[1].span, "..")) }; let method_hint = if is_mut { "as_deref_mut" } else { "as_deref" }; let hint = format!("{}.{}()", snippet(cx, as_ref_args[0].span, ".."), method_hint); let suggestion = format!("try using {} instead", method_hint); let msg = format!( - "called `{0}` (or with one of deref aliases) on an Option value. \ - This can be done more directly by calling `{1}` instead", + "called `{0}` on an Option value. This can be done more directly \ + by calling `{1}` instead", current_method, hint ); span_lint_and_sugg( diff --git a/tests/ui/option_as_ref_deref.stderr b/tests/ui/option_as_ref_deref.stderr index 6c2bf8517..a106582a6 100644 --- a/tests/ui/option_as_ref_deref.stderr +++ b/tests/ui/option_as_ref_deref.stderr @@ -1,4 +1,4 @@ -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.clone().as_deref()` instead +error: called `.as_ref().map(Deref::deref)` on an Option value. This can be done more directly by calling `opt.clone().as_deref()` instead --> $DIR/option_as_ref_deref.rs:13:13 | LL | let _ = opt.clone().as_ref().map(Deref::deref).map(str::len); @@ -6,7 +6,7 @@ LL | let _ = opt.clone().as_ref().map(Deref::deref).map(str::len); | = note: `-D clippy::option-as-ref-deref` implied by `-D warnings` -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.clone().as_deref()` instead +error: called `.as_ref().map(Deref::deref)` on an Option value. This can be done more directly by calling `opt.clone().as_deref()` instead --> $DIR/option_as_ref_deref.rs:16:13 | LL | let _ = opt.clone() @@ -16,85 +16,85 @@ LL | | Deref::deref LL | | ) | |_________^ help: try using as_deref instead: `opt.clone().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 +error: called `.as_mut().map(DerefMut::deref_mut)` on an Option value. This can be done more directly by calling `opt.as_deref_mut()` instead --> $DIR/option_as_ref_deref.rs:22:13 | LL | let _ = opt.as_mut().map(DerefMut::deref_mut); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref_mut instead: `opt.as_deref_mut()` -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 +error: called `.as_ref().map(String::as_str)` on an Option value. This can be done more directly by calling `opt.as_deref()` instead --> $DIR/option_as_ref_deref.rs:24:13 | LL | let _ = opt.as_ref().map(String::as_str); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref instead: `opt.as_deref()` -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 +error: called `.as_ref().map(|x| x.as_str())` on an Option value. This can be done more directly by calling `opt.as_deref()` instead --> $DIR/option_as_ref_deref.rs:25:13 | LL | let _ = opt.as_ref().map(|x| x.as_str()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 +error: called `.as_mut().map(String::as_mut_str)` on an Option value. This can be done more directly by calling `opt.as_deref_mut()` instead --> $DIR/option_as_ref_deref.rs:26:13 | LL | let _ = opt.as_mut().map(String::as_mut_str); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref_mut instead: `opt.as_deref_mut()` -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 +error: called `.as_mut().map(|x| x.as_mut_str())` on an Option value. This can be done more directly by calling `opt.as_deref_mut()` instead --> $DIR/option_as_ref_deref.rs:27:13 | LL | let _ = opt.as_mut().map(|x| x.as_mut_str()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref_mut instead: `opt.as_deref_mut()` -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 `Some(CString::new(vec![]).unwrap()).as_deref()` instead +error: called `.as_ref().map(CString::as_c_str)` on an Option value. This can be done more directly by calling `Some(CString::new(vec![]).unwrap()).as_deref()` instead --> $DIR/option_as_ref_deref.rs:28:13 | LL | let _ = Some(CString::new(vec![]).unwrap()).as_ref().map(CString::as_c_str); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref instead: `Some(CString::new(vec![]).unwrap()).as_deref()` -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 `Some(OsString::new()).as_deref()` instead +error: called `.as_ref().map(OsString::as_os_str)` on an Option value. This can be done more directly by calling `Some(OsString::new()).as_deref()` instead --> $DIR/option_as_ref_deref.rs:29:13 | LL | let _ = Some(OsString::new()).as_ref().map(OsString::as_os_str); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref instead: `Some(OsString::new()).as_deref()` -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 `Some(PathBuf::new()).as_deref()` instead +error: called `.as_ref().map(PathBuf::as_path)` on an Option value. This can be done more directly by calling `Some(PathBuf::new()).as_deref()` instead --> $DIR/option_as_ref_deref.rs:30:13 | LL | let _ = Some(PathBuf::new()).as_ref().map(PathBuf::as_path); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref instead: `Some(PathBuf::new()).as_deref()` -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 `Some(Vec::<()>::new()).as_deref()` instead +error: called `.as_ref().map(Vec::as_slice)` on an Option value. This can be done more directly by calling `Some(Vec::<()>::new()).as_deref()` instead --> $DIR/option_as_ref_deref.rs:31:13 | LL | let _ = Some(Vec::<()>::new()).as_ref().map(Vec::as_slice); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref instead: `Some(Vec::<()>::new()).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 `Some(Vec::<()>::new()).as_deref_mut()` instead +error: called `.as_mut().map(Vec::as_mut_slice)` on an Option value. This can be done more directly by calling `Some(Vec::<()>::new()).as_deref_mut()` instead --> $DIR/option_as_ref_deref.rs:32:13 | LL | let _ = Some(Vec::<()>::new()).as_mut().map(Vec::as_mut_slice); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref_mut instead: `Some(Vec::<()>::new()).as_deref_mut()` -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 +error: called `.as_ref().map(|x| x.deref())` on an Option value. This can be done more directly by calling `opt.as_deref()` instead --> $DIR/option_as_ref_deref.rs:34:13 | LL | let _ = opt.as_ref().map(|x| x.deref()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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.clone().as_deref_mut()` instead +error: called `.as_mut().map(|x| x.deref_mut())` on an Option value. This can be done more directly by calling `opt.clone().as_deref_mut()` instead --> $DIR/option_as_ref_deref.rs:35:13 | 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: 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 +error: called `.as_ref().map(|x| &**x)` 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 +error: called `.as_mut().map(|x| &mut **x)` 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);