Skip to content

Commit

Permalink
add option_as_ref_deref lint
Browse files Browse the repository at this point in the history
  • Loading branch information
basil-cow committed Jan 23, 2020
1 parent eff3bc5 commit 917fc7e
Show file tree
Hide file tree
Showing 9 changed files with 293 additions and 2 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1227,6 +1227,7 @@ Released 2018-09-13
[`ok_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#ok_expect
[`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
[`option_and_then_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_and_then_some
[`option_as_ref_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_deref
[`option_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_expect_used
[`option_map_or_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_or_none
[`option_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unit_fn
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.

[There are 348 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are 349 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)

We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:

Expand Down
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&methods::NEW_RET_NO_SELF,
&methods::OK_EXPECT,
&methods::OPTION_AND_THEN_SOME,
&methods::OPTION_AS_REF_DEREF,
&methods::OPTION_EXPECT_USED,
&methods::OPTION_MAP_OR_NONE,
&methods::OPTION_MAP_UNWRAP_OR,
Expand Down Expand Up @@ -1219,6 +1220,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::NEW_RET_NO_SELF),
LintId::of(&methods::OK_EXPECT),
LintId::of(&methods::OPTION_AND_THEN_SOME),
LintId::of(&methods::OPTION_AS_REF_DEREF),
LintId::of(&methods::OPTION_MAP_OR_NONE),
LintId::of(&methods::OR_FUN_CALL),
LintId::of(&methods::SEARCH_IS_SOME),
Expand Down Expand Up @@ -1476,6 +1478,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::FILTER_NEXT),
LintId::of(&methods::FLAT_MAP_IDENTITY),
LintId::of(&methods::OPTION_AND_THEN_SOME),
LintId::of(&methods::OPTION_AS_REF_DEREF),
LintId::of(&methods::SEARCH_IS_SOME),
LintId::of(&methods::SKIP_WHILE_NEXT),
LintId::of(&methods::SUSPICIOUS_MAP),
Expand Down
101 changes: 101 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1189,6 +1189,27 @@ declare_clippy_lint! {
"`FileType::is_file` is not recommended to test for readable file type"
}

declare_clippy_lint! {
/// **What it does:** Checks for usage of `_.as_ref().map(Deref::deref)` or it's aliases (such as String::as_str).
///
/// **Why is this bad?** Readability, this can be written more concisely as a
/// single method call.
///
/// **Known problems:** None.
///
/// **Example:**
/// ```rust,ignore
/// opt.as_ref().map(String::as_str)
/// ```
/// Can be written as
/// ```rust,ignore
/// opt.as_deref()
/// ```
pub OPTION_AS_REF_DEREF,
complexity,
"using `as_ref().map(Deref::deref)`, which is more succinctly expressed as `as_deref()`"
}

declare_lint_pass!(Methods => [
OPTION_UNWRAP_USED,
RESULT_UNWRAP_USED,
Expand Down Expand Up @@ -1238,6 +1259,7 @@ declare_lint_pass!(Methods => [
MANUAL_SATURATING_ARITHMETIC,
ZST_OFFSET,
FILETYPE_IS_FILE,
OPTION_AS_REF_DEREF,
]);

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
Expand Down Expand Up @@ -1303,6 +1325,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
check_pointer_offset(cx, expr, arg_lists[0])
},
["is_file", ..] => lint_filetype_is_file(cx, expr, arg_lists[0]),
["map", "as_ref"] => lint_option_as_ref_deref(cx, expr, arg_lists[1], arg_lists[0], false),
["map", "as_mut"] => lint_option_as_ref_deref(cx, expr, arg_lists[1], arg_lists[0], true),
_ => {},
}

Expand Down Expand Up @@ -3062,6 +3086,83 @@ fn lint_suspicious_map(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>) {
);
}

/// lint use of `_.as_ref().map(Deref::deref)` for `Option`s
fn lint_option_as_ref_deref<'a, 'tcx>(
cx: &LateContext<'a, 'tcx>,
expr: &hir::Expr<'_>,
as_ref_args: &[hir::Expr<'_>],
map_args: &[hir::Expr<'_>],
is_mut: bool,
) {
let option_ty = cx.tables.expr_ty(&as_ref_args[0]);
if !match_type(cx, option_ty, &paths::OPTION) {
return;
}

let deref_aliases: [&[&str]; 9] = [
&paths::DEREF_TRAIT_METHOD,
&paths::DEREF_MUT_TRAIT_METHOD,
&paths::CSTRING_AS_C_STR,
&paths::OS_STRING_AS_OS_STR,
&paths::PATH_BUF_AS_PATH,
&paths::STRING_AS_STR,
&paths::STRING_AS_MUT_STR,
&paths::VEC_AS_SLICE,
&paths::VEC_AS_MUT_SLICE,
];

let is_deref = match map_args[1].kind {
hir::ExprKind::Path(ref expr_qpath) => deref_aliases.iter().any(|path| match_qpath(expr_qpath, path)),
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
}
}
},

_ => false,
};

if is_deref {
let current_method = if is_mut {
".as_mut().map(DerefMut::deref_mut)"
} else {
".as_ref().map(Deref::deref)"
};
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",
current_method, hint
);
span_lint_and_sugg(
cx,
OPTION_AS_REF_DEREF,
expr.span,
&msg,
&suggestion,
hint,
Applicability::MachineApplicable
);
}
}

/// Given a `Result<T, E>` type, return its error type (`E`).
fn get_error_type<'a>(cx: &LateContext<'_, '_>, ty: Ty<'a>) -> Option<Ty<'a>> {
match ty.kind {
Expand Down
8 changes: 8 additions & 0 deletions clippy_lints/src/utils/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"];
pub const CMP_MIN: [&str; 3] = ["core", "cmp", "min"];
pub const COW: [&str; 3] = ["alloc", "borrow", "Cow"];
pub const CSTRING: [&str; 4] = ["std", "ffi", "c_str", "CString"];
pub const CSTRING_AS_C_STR: [&str; 5] = ["std", "ffi", "c_str", "CString", "as_c_str"];
pub const DEFAULT_TRAIT: [&str; 3] = ["core", "default", "Default"];
pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"];
pub const DEREF_MUT_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "DerefMut", "deref_mut"];
pub const DEREF_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "Deref", "deref"];
pub const DISPLAY_FMT_METHOD: [&str; 4] = ["core", "fmt", "Display", "fmt"];
pub const DOUBLE_ENDED_ITERATOR: [&str; 4] = ["core", "iter", "traits", "DoubleEndedIterator"];
Expand Down Expand Up @@ -63,10 +65,12 @@ pub const OPTION_NONE: [&str; 4] = ["core", "option", "Option", "None"];
pub const OPTION_SOME: [&str; 4] = ["core", "option", "Option", "Some"];
pub const ORD: [&str; 3] = ["core", "cmp", "Ord"];
pub const OS_STRING: [&str; 4] = ["std", "ffi", "os_str", "OsString"];
pub const OS_STRING_AS_OS_STR: [&str; 5] = ["std", "ffi", "os_str", "OsString", "as_os_str"];
pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"];
pub const PARTIAL_ORD: [&str; 3] = ["core", "cmp", "PartialOrd"];
pub const PATH: [&str; 3] = ["std", "path", "Path"];
pub const PATH_BUF: [&str; 3] = ["std", "path", "PathBuf"];
pub const PATH_BUF_AS_PATH: [&str; 4] = ["std", "path", "PathBuf", "as_path"];
pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"];
pub const PTR_NULL: [&str; 2] = ["ptr", "null"];
pub const PTR_NULL_MUT: [&str; 2] = ["ptr", "null_mut"];
Expand Down Expand Up @@ -105,6 +109,8 @@ pub const STD_CONVERT_IDENTITY: [&str; 3] = ["std", "convert", "identity"];
pub const STD_MEM_TRANSMUTE: [&str; 3] = ["std", "mem", "transmute"];
pub const STD_PTR_NULL: [&str; 3] = ["std", "ptr", "null"];
pub const STRING: [&str; 3] = ["alloc", "string", "String"];
pub const STRING_AS_MUT_STR: [&str; 4] = ["alloc", "string", "String", "as_mut_str"];
pub const STRING_AS_STR: [&str; 4] = ["alloc", "string", "String", "as_str"];
pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"];
pub const TO_OWNED: [&str; 3] = ["alloc", "borrow", "ToOwned"];
pub const TO_OWNED_METHOD: [&str; 4] = ["alloc", "borrow", "ToOwned", "to_owned"];
Expand All @@ -114,6 +120,8 @@ pub const TRANSMUTE: [&str; 4] = ["core", "intrinsics", "", "transmute"];
pub const TRY_FROM_ERROR: [&str; 4] = ["std", "ops", "Try", "from_error"];
pub const TRY_INTO_RESULT: [&str; 4] = ["std", "ops", "Try", "into_result"];
pub const VEC: [&str; 3] = ["alloc", "vec", "Vec"];
pub const VEC_AS_MUT_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_mut_slice"];
pub const VEC_AS_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_slice"];
pub const VEC_DEQUE: [&str; 4] = ["alloc", "collections", "vec_deque", "VecDeque"];
pub const VEC_FROM_ELEM: [&str; 3] = ["alloc", "vec", "from_elem"];
pub const WEAK_ARC: [&str; 3] = ["alloc", "sync", "Weak"];
Expand Down
9 changes: 8 additions & 1 deletion src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub use lint::Lint;
pub use lint::LINT_LEVELS;

// begin lint list, do not remove this comment, it’s used in `update_lints`
pub const ALL_LINTS: [Lint; 348] = [
pub const ALL_LINTS: [Lint; 349] = [
Lint {
name: "absurd_extreme_comparisons",
group: "correctness",
Expand Down Expand Up @@ -1470,6 +1470,13 @@ pub const ALL_LINTS: [Lint; 348] = [
deprecation: None,
module: "methods",
},
Lint {
name: "option_as_ref_deref",
group: "complexity",
desc: "using `as_ref().map(Deref::deref)`, which is more succinctly expressed as `as_deref()`",
deprecation: None,
module: "methods",
},
Lint {
name: "option_expect_used",
group: "restriction",
Expand Down
38 changes: 38 additions & 0 deletions tests/ui/option_as_ref_deref.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// run-rustfix

#![allow(unused_imports, clippy::redundant_clone)]
#![warn(clippy::option_as_ref_deref)]

use std::ffi::{CString, OsString};
use std::ops::{Deref, DerefMut};
use std::path::PathBuf;

fn main() {
let mut opt = Some(String::from("123"));

let _ = opt.clone().as_deref().map(str::len);

#[rustfmt::skip]
let _ = opt.clone().as_deref()
.map(str::len);

let _ = opt.as_deref_mut();

let _ = opt.as_deref();
let _ = opt.as_deref();
let _ = opt.as_deref_mut();
let _ = opt.as_deref_mut();
let _ = Some(CString::new(vec![]).unwrap()).as_deref();
let _ = Some(OsString::new()).as_deref();
let _ = Some(PathBuf::new()).as_deref();
let _ = Some(Vec::<()>::new()).as_deref();
let _ = Some(Vec::<()>::new()).as_deref_mut();

let _ = opt.as_deref();
let _ = opt.clone().as_deref_mut().map(|x| x.len());

let vc = vec![String::new()];
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
}
41 changes: 41 additions & 0 deletions tests/ui/option_as_ref_deref.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// run-rustfix

#![allow(unused_imports, clippy::redundant_clone)]
#![warn(clippy::option_as_ref_deref)]

use std::ffi::{CString, OsString};
use std::ops::{Deref, DerefMut};
use std::path::PathBuf;

fn main() {
let mut opt = Some(String::from("123"));

let _ = opt.clone().as_ref().map(Deref::deref).map(str::len);

#[rustfmt::skip]
let _ = opt.clone()
.as_ref().map(
Deref::deref
)
.map(str::len);

let _ = opt.as_mut().map(DerefMut::deref_mut);

let _ = opt.as_ref().map(String::as_str);
let _ = opt.as_ref().map(|x| x.as_str());
let _ = opt.as_mut().map(String::as_mut_str);
let _ = opt.as_mut().map(|x| x.as_mut_str());
let _ = Some(CString::new(vec![]).unwrap()).as_ref().map(CString::as_c_str);
let _ = Some(OsString::new()).as_ref().map(OsString::as_os_str);
let _ = Some(PathBuf::new()).as_ref().map(PathBuf::as_path);
let _ = Some(Vec::<()>::new()).as_ref().map(Vec::as_slice);
let _ = Some(Vec::<()>::new()).as_mut().map(Vec::as_mut_slice);

let _ = opt.as_ref().map(|x| x.deref());
let _ = opt.clone().as_mut().map(|x| x.deref_mut()).map(|x| x.len());

let vc = vec![String::new()];
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
}
Loading

0 comments on commit 917fc7e

Please sign in to comment.