Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ehance opt_as_ref_deref lint. #5425

Merged
merged 2 commits into from
Apr 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions clippy_lints/src/loops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -654,15 +654,15 @@ 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)
}

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,
}
}
Expand Down
57 changes: 38 additions & 19 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -3181,39 +3183,56 @@ 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::<Box<[_]>>();
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::<Box<[_]>>();
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,
};

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(
Expand Down
3 changes: 3 additions & 0 deletions tests/ui/option_as_ref_deref.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
3 changes: 3 additions & 0 deletions tests/ui/option_as_ref_deref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
42 changes: 27 additions & 15 deletions tests/ui/option_as_ref_deref.stderr
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
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);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref instead: `opt.clone().as_deref()`
|
= 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()
Expand All @@ -16,77 +16,89 @@ 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: aborting due to 14 previous errors
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(|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);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref_mut instead: `opt.as_deref_mut()`

error: aborting due to 16 previous errors