Auto merge of rust-lang#4945 - Areredify:as_deref, r=flip1995

add `option_as_ref_deref` lint

changelog: add a new lint that lints `option.as_ref().map(Deref::deref)` (and similar calls), which could be expressed more succinctly as `option.as_deref[_mut]()`. Closes rust-lang#4918.

Author: bors

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

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:
 

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),

clippy_lints/src/methods/mod.rs

@@ -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,
@@ -1238,10 +1259,11 @@ declare_lint_pass!(Methods => [
     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 @@ 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),
             _ => {},
         }
 
@@ -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 {

clippy_lints/src/utils/paths.rs

@@ -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"];
@@ -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"];
@@ -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"];
@@ -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"];

src/lintlist/mod.rs

@@ -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",
@@ -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",

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
+}

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
+}