diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e5d1688e4a7..8b9189c90932 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5880,6 +5880,7 @@ Released 2018-09-13 [`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts [`reserve_after_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#reserve_after_initialization [`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs +[`result_as_ref_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_as_ref_deref [`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used [`result_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_filter_map [`result_large_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err diff --git a/clippy_config/src/msrvs.rs b/clippy_config/src/msrvs.rs index a70effd63769..632786e27c8a 100644 --- a/clippy_config/src/msrvs.rs +++ b/clippy_config/src/msrvs.rs @@ -17,8 +17,8 @@ macro_rules! msrv_aliases { // names may refer to stabilized feature flags or library items msrv_aliases! { - 1,81,0 { LINT_REASONS_STABILIZATION } - 1,80,0 { BOX_INTO_ITER} + 1,81,0 { LINT_REASONS_STABILIZATION } + 1,80,0 { BOX_INTO_ITER } 1,77,0 { C_STR_LITERALS } 1,76,0 { PTR_FROM_REF, OPTION_RESULT_INSPECT } 1,73,0 { MANUAL_DIV_CEIL } @@ -37,7 +37,7 @@ msrv_aliases! { 1,52,0 { STR_SPLIT_ONCE, REM_EUCLID_CONST } 1,51,0 { BORROW_AS_PTR, SEEK_FROM_CURRENT, UNSIGNED_ABS } 1,50,0 { BOOL_THEN, CLAMP } - 1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST } + 1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST, RESULT_AS_DEREF } 1,46,0 { CONST_IF_MATCH } 1,45,0 { STR_STRIP_PREFIX } 1,43,0 { LOG2_10, LOG10_2, NUMERIC_ASSOCIATED_CONSTANTS } diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 16c64830e70d..84ee755c2d37 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -444,6 +444,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::methods::READ_LINE_WITHOUT_TRIM_INFO, crate::methods::REDUNDANT_AS_STR_INFO, crate::methods::REPEAT_ONCE_INFO, + crate::methods::RESULT_AS_REF_DEREF_INFO, crate::methods::RESULT_FILTER_MAP_INFO, crate::methods::RESULT_MAP_OR_INTO_OPTION_INFO, crate::methods::SEARCH_IS_SOME_INFO, diff --git a/clippy_lints/src/methods/option_as_ref_deref.rs b/clippy_lints/src/methods/manual_as_ref_deref.rs similarity index 78% rename from clippy_lints/src/methods/option_as_ref_deref.rs rename to clippy_lints/src/methods/manual_as_ref_deref.rs index cb57689b0c41..f77bcae1e273 100644 --- a/clippy_lints/src/methods/option_as_ref_deref.rs +++ b/clippy_lints/src/methods/manual_as_ref_deref.rs @@ -9,9 +9,15 @@ use rustc_lint::LateContext; use rustc_middle::ty; use rustc_span::sym; -use super::OPTION_AS_REF_DEREF; +use super::{OPTION_AS_REF_DEREF, RESULT_AS_REF_DEREF}; -/// lint use of `_.as_ref().map(Deref::deref)` for `Option`s +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Target { + Option, + Result, +} + +/// lint use of `_.as_ref().map(Deref::deref)` for `Option`s and `Result`s pub(super) fn check( cx: &LateContext<'_>, expr: &hir::Expr<'_>, @@ -20,14 +26,23 @@ pub(super) fn check( is_mut: bool, msrv: &Msrv, ) { - if !msrv.meets(msrvs::OPTION_AS_DEREF) { - return; - } - let same_mutability = |m| (is_mut && m == &hir::Mutability::Mut) || (!is_mut && m == &hir::Mutability::Not); - let option_ty = cx.typeck_results().expr_ty(as_ref_recv); - if !is_type_diagnostic_item(cx, option_ty, sym::Option) { + let target = 'target: { + let target_ty = cx.typeck_results().expr_ty(as_ref_recv); + if is_type_diagnostic_item(cx, target_ty, sym::Option) { + break 'target Target::Option; + } + if is_type_diagnostic_item(cx, target_ty, sym::Result) { + break 'target Target::Result; + } + return; + }; + + if target == Target::Option && !msrv.meets(msrvs::OPTION_AS_DEREF) { + return; + } + if target == Target::Result && !msrv.meets(msrvs::RESULT_AS_DEREF) { return; } @@ -99,10 +114,20 @@ pub(super) fn check( let hint = format!("{}.{method_hint}()", snippet(cx, as_ref_recv.span, "..")); let suggestion = format!("consider using {method_hint}"); - let msg = format!("called `{current_method}` on an `Option` value"); + let target_name_with_article = match target { + Target::Option => "an `Option`", + Target::Result => "a `Result`", + }; + let msg = format!("called `{current_method}` on {target_name_with_article} value"); + + let lint = match target { + Target::Option => OPTION_AS_REF_DEREF, + Target::Result => RESULT_AS_REF_DEREF, + }; + span_lint_and_sugg( cx, - OPTION_AS_REF_DEREF, + lint, expr.span, msg, suggestion, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 9b7cba9dafba..67956761f805 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -52,6 +52,7 @@ mod iter_skip_zero; mod iter_with_drain; mod iterator_step_by_zero; mod join_absolute_paths; +mod manual_as_ref_deref; mod manual_c_str_literals; mod manual_inspect; mod manual_is_variant_and; @@ -76,7 +77,6 @@ mod obfuscated_if_else; mod ok_expect; mod open_options; mod option_as_ref_cloned; -mod option_as_ref_deref; mod option_map_or_err_ok; mod option_map_or_none; mod option_map_unwrap_or; @@ -1758,7 +1758,8 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for usage of `_.as_ref().map(Deref::deref)` or its aliases (such as String::as_str). + /// Checks for usage of `_.as_ref().map(Deref::deref)` or its aliases + /// (such as `String::as_str`) on `Option`. /// /// ### Why is this bad? /// Readability, this can be written more concisely as @@ -1782,6 +1783,33 @@ declare_clippy_lint! { "using `as_ref().map(Deref::deref)`, which is more succinctly expressed as `as_deref()`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `_.as_ref().map(Deref::deref)` or its aliases + /// (such as `String::as_str`) on `Result`. + /// + /// ### Why is this bad? + /// Readability, this can be written more concisely as + /// `_.as_deref()`. + /// + /// ### Example + /// ```no_run + /// # let res = Ok::<_, ()>("".to_string()); + /// res.as_ref().map(String::as_str) + /// # ; + /// ``` + /// Use instead: + /// ```no_run + /// # let res = OK::<_, ()>("".to_string()); + /// res.as_deref() + /// # ; + /// ``` + #[clippy::version = "1.83.0"] + pub RESULT_AS_REF_DEREF, + complexity, + "using `as_ref().map(Deref::deref)`, which is more succinctly expressed as `as_deref()`" +} + declare_clippy_lint! { /// ### What it does /// Checks for usage of `iter().next()` on a Slice or an Array @@ -4223,6 +4251,7 @@ impl_lint_pass!(Methods => [ ZST_OFFSET, FILETYPE_IS_FILE, OPTION_AS_REF_DEREF, + RESULT_AS_REF_DEREF, UNNECESSARY_LAZY_EVALUATIONS, MAP_COLLECT_RESULT_UNIT, FROM_ITER_INSTEAD_OF_COLLECT, @@ -4791,8 +4820,8 @@ impl Methods { } if let Some((name, recv2, args, span2, _)) = method_call(recv) { match (name, args) { - ("as_mut", []) => option_as_ref_deref::check(cx, expr, recv2, m_arg, true, &self.msrv), - ("as_ref", []) => option_as_ref_deref::check(cx, expr, recv2, m_arg, false, &self.msrv), + ("as_mut", []) => manual_as_ref_deref::check(cx, expr, recv2, m_arg, true, &self.msrv), + ("as_ref", []) => manual_as_ref_deref::check(cx, expr, recv2, m_arg, false, &self.msrv), ("filter", [f_arg]) => { filter_map::check(cx, expr, recv2, f_arg, span2, recv, m_arg, span, false); }, diff --git a/tests/ui/result_as_ref_deref.fixed b/tests/ui/result_as_ref_deref.fixed new file mode 100644 index 000000000000..72f2a177c19a --- /dev/null +++ b/tests/ui/result_as_ref_deref.fixed @@ -0,0 +1,53 @@ +#![allow(unused, clippy::redundant_clone, clippy::useless_vec)] +#![warn(clippy::result_as_ref_deref)] + +use std::ffi::{CString, OsString}; +use std::ops::{Deref, DerefMut}; +use std::path::PathBuf; + +fn main() { + let mut res = Ok::<_, ()>(String::from("123")); + + let _ = res.clone().as_deref().map(str::len); + + #[rustfmt::skip] + let _ = res.clone().as_deref() + .map(str::len); + + let _ = res.as_deref_mut(); + + let _ = res.as_deref(); + let _ = res.as_deref(); + let _ = res.as_deref_mut(); + let _ = res.as_deref_mut(); + let _ = Ok::<_, ()>(CString::new(vec![]).unwrap()).as_deref(); + let _ = Ok::<_, ()>(OsString::new()).as_deref(); + let _ = Ok::<_, ()>(PathBuf::new()).as_deref(); + let _ = Ok::<_, ()>(Vec::<()>::new()).as_deref(); + let _ = Ok::<_, ()>(Vec::<()>::new()).as_deref_mut(); + + let _ = res.as_deref(); + let _ = res.clone().as_deref_mut().map(|x| x.len()); + + let vc = vec![String::new()]; + let _ = Ok::<_, ()>(1_usize).as_ref().map(|x| vc[*x].as_str()); // should not be linted + + let _: Result<&str, &()> = Ok(&String::new()).as_ref().map(|x| x.as_str()); // should not be linted + + let _ = res.as_deref(); + let _ = res.as_deref_mut(); + + let _ = res.as_deref(); +} + +#[clippy::msrv = "1.46"] +fn msrv_1_46() { + let res = Ok::<_, ()>(String::from("123")); + let _ = res.as_ref().map(String::as_str); +} + +#[clippy::msrv = "1.47"] +fn msrv_1_47() { + let res = Ok::<_, ()>(String::from("123")); + let _ = res.as_deref(); +} diff --git a/tests/ui/result_as_ref_deref.rs b/tests/ui/result_as_ref_deref.rs new file mode 100644 index 000000000000..e9f5836c57cc --- /dev/null +++ b/tests/ui/result_as_ref_deref.rs @@ -0,0 +1,58 @@ +#![allow(unused, clippy::redundant_clone, clippy::useless_vec)] +#![warn(clippy::result_as_ref_deref)] + +use std::ffi::{CString, OsString}; +use std::ops::{Deref, DerefMut}; +use std::path::PathBuf; + +fn main() { + let mut res = Ok::<_, ()>(String::from("123")); + + let _ = res.clone().as_ref().map(Deref::deref).map(str::len); + + #[rustfmt::skip] + let _ = res.clone() + .as_ref().map( + Deref::deref + ) + .map(str::len); + + let _ = res.as_mut().map(DerefMut::deref_mut); + + let _ = res.as_ref().map(String::as_str); + let _ = res.as_ref().map(|x| x.as_str()); + let _ = res.as_mut().map(String::as_mut_str); + let _ = res.as_mut().map(|x| x.as_mut_str()); + let _ = Ok::<_, ()>(CString::new(vec![]).unwrap()) + .as_ref() + .map(CString::as_c_str); + let _ = Ok::<_, ()>(OsString::new()).as_ref().map(OsString::as_os_str); + let _ = Ok::<_, ()>(PathBuf::new()).as_ref().map(PathBuf::as_path); + let _ = Ok::<_, ()>(Vec::<()>::new()).as_ref().map(Vec::as_slice); + let _ = Ok::<_, ()>(Vec::<()>::new()).as_mut().map(Vec::as_mut_slice); + + let _ = res.as_ref().map(|x| x.deref()); + let _ = res.clone().as_mut().map(|x| x.deref_mut()).map(|x| x.len()); + + let vc = vec![String::new()]; + let _ = Ok::<_, ()>(1_usize).as_ref().map(|x| vc[*x].as_str()); // should not be linted + + let _: Result<&str, &()> = Ok(&String::new()).as_ref().map(|x| x.as_str()); // should not be linted + + let _ = res.as_ref().map(|x| &**x); + let _ = res.as_mut().map(|x| &mut **x); + + let _ = res.as_ref().map(std::ops::Deref::deref); +} + +#[clippy::msrv = "1.46"] +fn msrv_1_46() { + let res = Ok::<_, ()>(String::from("123")); + let _ = res.as_ref().map(String::as_str); +} + +#[clippy::msrv = "1.47"] +fn msrv_1_47() { + let res = Ok::<_, ()>(String::from("123")); + let _ = res.as_ref().map(String::as_str); +} diff --git a/tests/ui/result_as_ref_deref.stderr b/tests/ui/result_as_ref_deref.stderr new file mode 100644 index 000000000000..40fe72720430 --- /dev/null +++ b/tests/ui/result_as_ref_deref.stderr @@ -0,0 +1,120 @@ +error: called `.as_ref().map(Deref::deref)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:11:13 + | +LL | let _ = res.clone().as_ref().map(Deref::deref).map(str::len); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref: `res.clone().as_deref()` + | + = note: `-D clippy::result-as-ref-deref` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::result_as_ref_deref)]` + +error: called `.as_ref().map(Deref::deref)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:14:13 + | +LL | let _ = res.clone() + | _____________^ +LL | | .as_ref().map( +LL | | Deref::deref +LL | | ) + | |_________^ help: consider using as_deref: `res.clone().as_deref()` + +error: called `.as_mut().map(DerefMut::deref_mut)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:20:13 + | +LL | let _ = res.as_mut().map(DerefMut::deref_mut); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref_mut: `res.as_deref_mut()` + +error: called `.as_ref().map(String::as_str)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:22:13 + | +LL | let _ = res.as_ref().map(String::as_str); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref: `res.as_deref()` + +error: called `.as_ref().map(|x| x.as_str())` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:23:13 + | +LL | let _ = res.as_ref().map(|x| x.as_str()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref: `res.as_deref()` + +error: called `.as_mut().map(String::as_mut_str)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:24:13 + | +LL | let _ = res.as_mut().map(String::as_mut_str); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref_mut: `res.as_deref_mut()` + +error: called `.as_mut().map(|x| x.as_mut_str())` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:25:13 + | +LL | let _ = res.as_mut().map(|x| x.as_mut_str()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref_mut: `res.as_deref_mut()` + +error: called `.as_ref().map(CString::as_c_str)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:26:13 + | +LL | let _ = Ok::<_, ()>(CString::new(vec![]).unwrap()) + | _____________^ +LL | | .as_ref() +LL | | .map(CString::as_c_str); + | |_______________________________^ help: consider using as_deref: `Ok::<_, ()>(CString::new(vec![]).unwrap()).as_deref()` + +error: called `.as_ref().map(OsString::as_os_str)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:29:13 + | +LL | let _ = Ok::<_, ()>(OsString::new()).as_ref().map(OsString::as_os_str); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref: `Ok::<_, ()>(OsString::new()).as_deref()` + +error: called `.as_ref().map(PathBuf::as_path)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:30:13 + | +LL | let _ = Ok::<_, ()>(PathBuf::new()).as_ref().map(PathBuf::as_path); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref: `Ok::<_, ()>(PathBuf::new()).as_deref()` + +error: called `.as_ref().map(Vec::as_slice)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:31:13 + | +LL | let _ = Ok::<_, ()>(Vec::<()>::new()).as_ref().map(Vec::as_slice); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref: `Ok::<_, ()>(Vec::<()>::new()).as_deref()` + +error: called `.as_mut().map(Vec::as_mut_slice)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:32:13 + | +LL | let _ = Ok::<_, ()>(Vec::<()>::new()).as_mut().map(Vec::as_mut_slice); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref_mut: `Ok::<_, ()>(Vec::<()>::new()).as_deref_mut()` + +error: called `.as_ref().map(|x| x.deref())` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:34:13 + | +LL | let _ = res.as_ref().map(|x| x.deref()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref: `res.as_deref()` + +error: called `.as_mut().map(|x| x.deref_mut())` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:35:13 + | +LL | let _ = res.clone().as_mut().map(|x| x.deref_mut()).map(|x| x.len()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref_mut: `res.clone().as_deref_mut()` + +error: called `.as_ref().map(|x| &**x)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:42:13 + | +LL | let _ = res.as_ref().map(|x| &**x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref: `res.as_deref()` + +error: called `.as_mut().map(|x| &mut **x)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:43:13 + | +LL | let _ = res.as_mut().map(|x| &mut **x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref_mut: `res.as_deref_mut()` + +error: called `.as_ref().map(std::ops::Deref::deref)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:45:13 + | +LL | let _ = res.as_ref().map(std::ops::Deref::deref); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref: `res.as_deref()` + +error: called `.as_ref().map(String::as_str)` on a `Result` value + --> tests/ui/result_as_ref_deref.rs:57:13 + | +LL | let _ = res.as_ref().map(String::as_str); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using as_deref: `res.as_deref()` + +error: aborting due to 18 previous errors +