diff --git a/CHANGELOG.md b/CHANGELOG.md index f7e8a4534de..78020c2dac6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/README.md b/README.md index d752e5b4cc1..8f65ff94ca8 100644 --- a/README.md +++ b/README.md @@ -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: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 962ff38eb6c..0c7717c4f19 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -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, @@ -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), @@ -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), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 6a5d6d64cf4..ae7b1e3485f 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1189,6 +1189,27 @@ "`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, @@ -1238,10 +1259,11 @@ MANUAL_SATURATING_ARITHMETIC, ZST_OFFSET, FILETYPE_IS_FILE, + OPTION_AS_REF_DEREF, ]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { - #[allow(clippy::cognitive_complexity)] + #[allow(clippy::cognitive_complexity, clippy::too_many_lines)] fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr<'_>) { if in_macro(expr.span) { return; @@ -1303,6 +1325,8 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr<'_>) 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), _ => {}, } @@ -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::>(); + 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` type, return its error type (`E`). fn get_error_type<'a>(cx: &LateContext<'_, '_>, ty: Ty<'a>) -> Option> { match ty.kind { diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 4f8fe02dfd2..7980a02b3ba 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -18,8 +18,10 @@ 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"]; @@ -63,10 +65,12 @@ 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"]; @@ -105,6 +109,8 @@ 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"]; @@ -114,6 +120,8 @@ 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"]; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 6858c3e15c8..5dbcb4483f2 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ 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", @@ -1470,6 +1470,13 @@ 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", diff --git a/tests/ui/option_as_ref_deref.fixed b/tests/ui/option_as_ref_deref.fixed new file mode 100644 index 00000000000..973e5b308a2 --- /dev/null +++ b/tests/ui/option_as_ref_deref.fixed @@ -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 +} diff --git a/tests/ui/option_as_ref_deref.rs b/tests/ui/option_as_ref_deref.rs new file mode 100644 index 00000000000..baad85ab908 --- /dev/null +++ b/tests/ui/option_as_ref_deref.rs @@ -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 +} diff --git a/tests/ui/option_as_ref_deref.stderr b/tests/ui/option_as_ref_deref.stderr new file mode 100644 index 00000000000..09a0fa058e6 --- /dev/null +++ b/tests/ui/option_as_ref_deref.stderr @@ -0,0 +1,92 @@ +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 + --> $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 + --> $DIR/option_as_ref_deref.rs:16:13 + | +LL | let _ = opt.clone() + | _____________^ +LL | | .as_ref().map( +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 + --> $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 + --> $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 + --> $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 + --> $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 + --> $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 + --> $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 + --> $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 + --> $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 + --> $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 + --> $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 + --> $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 + --> $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 +