Skip to content

Commit

Permalink
Add clippy::result_as_ref_deref lint
Browse files Browse the repository at this point in the history
  • Loading branch information
aleksanderkrauze committed Sep 28, 2024
1 parent 897f0e4 commit e1a8ff9
Show file tree
Hide file tree
Showing 8 changed files with 304 additions and 17 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions clippy_config/src/msrvs.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use rustc_ast::Attribute;
use rustc_attr::parse_version;
use rustc_session::{RustcVersion, Session};
use rustc_span::{Symbol, sym};
use rustc_span::{sym, Symbol};
use serde::Deserialize;
use std::fmt;

Expand All @@ -20,7 +20,7 @@ msrv_aliases! {
1,83,0 { CONST_EXTERN_FN }
1,83,0 { CONST_FLOAT_BITS_CONV }
1,81,0 { LINT_REASONS_STABILIZATION }
1,80,0 { BOX_INTO_ITER}
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 }
Expand All @@ -39,7 +39,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 }
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,15 @@ use rustc_lint::LateContext;
use rustc_middle::ty;
use rustc_span::{Symbol, 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<'_>,
Expand All @@ -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;
}

Expand Down Expand Up @@ -103,10 +118,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,
Expand Down
37 changes: 33 additions & 4 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -1759,7 +1759,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
Expand All @@ -1783,6 +1784,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
Expand Down Expand Up @@ -4252,6 +4280,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,
Expand Down Expand Up @@ -4821,8 +4850,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);
},
Expand Down
53 changes: 53 additions & 0 deletions tests/ui/result_as_ref_deref.fixed
Original file line number Diff line number Diff line change
@@ -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();
}
58 changes: 58 additions & 0 deletions tests/ui/result_as_ref_deref.rs
Original file line number Diff line number Diff line change
@@ -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);
}
Loading

0 comments on commit e1a8ff9

Please sign in to comment.