Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5922,6 +5922,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
4 changes: 2 additions & 2 deletions clippy_config/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ msrv_aliases! {
1,83,0 { CONST_EXTERN_FN, CONST_FLOAT_BITS_CONV, CONST_FLOAT_CLASSIFY }
1,82,0 { IS_NONE_OR, REPEAT_N }
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 @@ -40,7 +40,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 @@ -452,6 +452,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
@@ -1,17 +1,23 @@
use clippy_config::msrvs::{self, Msrv};
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::ty::get_type_diagnostic_name;
use clippy_utils::{path_to_local_id, peel_blocks};
use rustc_errors::Applicability;
use rustc_hir as hir;
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,16 +26,16 @@ 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) {
return;
}
let target = {
let target_ty = cx.typeck_results().expr_ty(as_ref_recv);
match get_type_diagnostic_name(cx, target_ty) {
Some(sym::Option) if msrv.meets(msrvs::OPTION_AS_DEREF) => Target::Option,
Some(sym::Result) if msrv.meets(msrvs::RESULT_AS_DEREF) => Target::Result,
_ => return,
}
};

let deref_aliases: [Symbol; 7] = [
sym::cstring_as_c_str,
Expand Down Expand Up @@ -103,10 +109,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 Down Expand Up @@ -79,7 +80,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 @@ -1762,7 +1762,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 @@ -1786,6 +1787,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.84.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 @@ -4339,6 +4367,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 @@ -4932,8 +4961,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)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you add error markers (//~^) to lines which are supposed to shown an error? This will then be checked automatically for non-regressions, instead of having to visually inspect changes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. Sorry for the delay. Can I generate those comments with uibless, or do I have to write them manually? I haven't use them, because I have just copied tests/ui/option_as_ref_deref.rs file and tweaked it, and it already had no such comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those would have to be added manually. The tests haven't all been updated to use them, but they are preferred for anything new. You can add //~ lint_name on the end of any line that should lint. e.g.

let _ = res.clone().as_ref().map(Deref::deref).map(str::len) //~ result_as_ref_deref

Using //~^ line_name will let you add it to the following line, but that should only be used for longer lines.

#![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