From 11ef04728c90aa6105b20c681e3c12f231ba4f12 Mon Sep 17 00:00:00 2001 From: Jade Date: Thu, 29 Jul 2021 23:56:47 -0700 Subject: [PATCH 1/4] Add unwrap_or_else_default lint This will catch `unwrap_or_else(Default::default)` on Result and Option and suggest `unwrap_or_default()` instead. --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 + clippy_lints/src/methods/mod.rs | 32 ++++++++- clippy_lints/src/methods/or_fun_call.rs | 21 ++++-- .../src/methods/unwrap_or_else_default.rs | 50 +++++++++++++ clippy_utils/src/source.rs | 2 +- clippy_utils/src/ty.rs | 18 ++++- tests/ui/or_fun_call.fixed | 19 +++++ tests/ui/or_fun_call.rs | 19 +++++ tests/ui/or_fun_call.stderr | 58 +++++++++------ tests/ui/unwrap_or_else_default.fixed | 71 +++++++++++++++++++ tests/ui/unwrap_or_else_default.rs | 71 +++++++++++++++++++ tests/ui/unwrap_or_else_default.stderr | 22 ++++++ 13 files changed, 356 insertions(+), 31 deletions(-) create mode 100644 clippy_lints/src/methods/unwrap_or_else_default.rs create mode 100644 tests/ui/unwrap_or_else_default.fixed create mode 100644 tests/ui/unwrap_or_else_default.rs create mode 100644 tests/ui/unwrap_or_else_default.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index ac745793dda5..2b89170073be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2999,6 +2999,7 @@ Released 2018-09-13 [`unused_unit`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_unit [`unusual_byte_groupings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unusual_byte_groupings [`unwrap_in_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_in_result +[`unwrap_or_else_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_or_else_default [`unwrap_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_used [`upper_case_acronyms`]: https://rust-lang.github.io/rust-clippy/master/index.html#upper_case_acronyms [`use_debug`]: https://rust-lang.github.io/rust-clippy/master/index.html#use_debug diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f49b382c5ea3..dbdb4251b3be 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -797,6 +797,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: methods::UNNECESSARY_FILTER_MAP, methods::UNNECESSARY_FOLD, methods::UNNECESSARY_LAZY_EVALUATIONS, + methods::UNWRAP_OR_ELSE_DEFAULT, methods::UNWRAP_USED, methods::USELESS_ASREF, methods::WRONG_SELF_CONVENTION, @@ -1341,6 +1342,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(methods::UNNECESSARY_FILTER_MAP), LintId::of(methods::UNNECESSARY_FOLD), LintId::of(methods::UNNECESSARY_LAZY_EVALUATIONS), + LintId::of(methods::UNWRAP_OR_ELSE_DEFAULT), LintId::of(methods::USELESS_ASREF), LintId::of(methods::WRONG_SELF_CONVENTION), LintId::of(methods::ZST_OFFSET), @@ -1535,6 +1537,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(methods::STRING_EXTEND_CHARS), LintId::of(methods::UNNECESSARY_FOLD), LintId::of(methods::UNNECESSARY_LAZY_EVALUATIONS), + LintId::of(methods::UNWRAP_OR_ELSE_DEFAULT), LintId::of(methods::WRONG_SELF_CONVENTION), LintId::of(misc::TOPLEVEL_REF_ARG), LintId::of(misc::ZERO_PTR), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 5aa29424349f..12f7987fd3ad 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -56,6 +56,7 @@ mod uninit_assumed_init; mod unnecessary_filter_map; mod unnecessary_fold; mod unnecessary_lazy_eval; +mod unwrap_or_else_default; mod unwrap_used; mod useless_asref; mod utils; @@ -310,6 +311,31 @@ declare_clippy_lint! { "using `ok().expect()`, which gives worse error messages than calling `expect` directly on the Result" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usages of `_.unwrap_or_else(Default::default)` on Option and + /// Result values. + /// + /// ### Why is this bad? + /// Readability, these can be written as `option.unwrap_or_default` or + /// `result.unwrap_or_default`. + /// + /// ### Examples + /// ```rust + /// # let x = Some(1); + /// + /// // Bad + /// x.unwrap_or_else(Default::default); + /// x.unwrap_or_else(u32::default); + /// + /// // Good + /// x.unwrap_or_default(); + /// ``` + pub UNWRAP_OR_ELSE_DEFAULT, + style, + "using `.unwrap_or_else(Default::default)`, which is more succinctly expressed as `.unwrap_or_default()`" +} + declare_clippy_lint! { /// ### What it does /// Checks for usage of `option.map(_).unwrap_or(_)` or `option.map(_).unwrap_or_else(_)` or @@ -1766,6 +1792,7 @@ impl_lint_pass!(Methods => [ SHOULD_IMPLEMENT_TRAIT, WRONG_SELF_CONVENTION, OK_EXPECT, + UNWRAP_OR_ELSE_DEFAULT, MAP_UNWRAP_OR, RESULT_MAP_OR_INTO_OPTION, OPTION_MAP_OR_NONE, @@ -2172,7 +2199,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio }, ("unwrap_or_else", [u_arg]) => match method_call!(recv) { Some(("map", [recv, map_arg], _)) if map_unwrap_or::check(cx, expr, recv, map_arg, u_arg, msrv) => {}, - _ => unnecessary_lazy_eval::check(cx, expr, recv, u_arg, "unwrap_or"), + _ => { + unwrap_or_else_default::check(cx, expr, recv, u_arg); + unnecessary_lazy_eval::check(cx, expr, recv, u_arg, "unwrap_or"); + }, }, _ => {}, } diff --git a/clippy_lints/src/methods/or_fun_call.rs b/clippy_lints/src/methods/or_fun_call.rs index ef615b0aa40a..378b07241704 100644 --- a/clippy_lints/src/methods/or_fun_call.rs +++ b/clippy_lints/src/methods/or_fun_call.rs @@ -1,7 +1,8 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::eager_or_lazy::is_lazyness_candidate; use clippy_utils::source::{snippet, snippet_with_applicability, snippet_with_macro_callsite}; -use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, match_type}; +use clippy_utils::ty::{implements_trait, qpath_target_trait}; +use clippy_utils::ty::{is_type_diagnostic_item, match_type}; use clippy_utils::{contains_return, last_path_segment, paths}; use if_chain::if_chain; use rustc_errors::Applicability; @@ -34,15 +35,25 @@ pub(super) fn check<'tcx>( or_has_args: bool, span: Span, ) -> bool { + let is_default_default = |qpath, default_trait_id| { + qpath_target_trait(cx, qpath, fun.hir_id).map_or(false, |target_trait| target_trait == default_trait_id) + }; + + let implements_default = |arg, default_trait_id| { + let arg_ty = cx.typeck_results().expr_ty(arg); + implements_trait(cx, arg_ty, default_trait_id, &[]) + }; + if_chain! { if !or_has_args; if name == "unwrap_or"; if let hir::ExprKind::Path(ref qpath) = fun.kind; - let path = last_path_segment(qpath).ident.name; - if matches!(path, kw::Default | sym::new); - let arg_ty = cx.typeck_results().expr_ty(arg); if let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default); - if implements_trait(cx, arg_ty, default_trait_id, &[]); + let path = last_path_segment(qpath).ident.name; + // needs to target Default::default in particular or be *::new and have a Default impl + // available + if (matches!(path, kw::Default) && is_default_default(qpath, default_trait_id)) + || (matches!(path, sym::new) && implements_default(arg, default_trait_id)); then { let mut applicability = Applicability::MachineApplicable; diff --git a/clippy_lints/src/methods/unwrap_or_else_default.rs b/clippy_lints/src/methods/unwrap_or_else_default.rs new file mode 100644 index 000000000000..f99ae6cae93b --- /dev/null +++ b/clippy_lints/src/methods/unwrap_or_else_default.rs @@ -0,0 +1,50 @@ +//! Lint for `some_result_or_option.unwrap_or_else(Default::default)` + +use super::UNWRAP_OR_ELSE_DEFAULT; +use clippy_utils::{ + diagnostics::span_lint_and_sugg, + source::snippet_with_applicability, + ty::{is_type_diagnostic_item, qpath_target_trait}, +}; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_lint::LateContext; +use rustc_span::sym; + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'_>, + recv: &'tcx hir::Expr<'_>, + u_arg: &'tcx hir::Expr<'_>, +) { + // something.unwrap_or_else(Default::default) + // ^^^^^^^^^- recv ^^^^^^^^^^^^^^^^- u_arg + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- expr + let recv_ty = cx.typeck_results().expr_ty(recv); + let is_option = is_type_diagnostic_item(cx, recv_ty, sym::option_type); + let is_result = is_type_diagnostic_item(cx, recv_ty, sym::result_type); + + if_chain! { + if is_option || is_result; + if let hir::ExprKind::Path(ref qpath) = u_arg.kind; + if let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default); + if let Some(target_trait) = qpath_target_trait(cx, qpath, u_arg.hir_id); + if target_trait == default_trait_id; + then { + let mut applicability = Applicability::MachineApplicable; + + span_lint_and_sugg( + cx, + UNWRAP_OR_ELSE_DEFAULT, + expr.span, + "use of `.unwrap_or_else(..)` to construct default value", + "try", + format!( + "{}.unwrap_or_default()", + snippet_with_applicability(cx, recv.span, "..", &mut applicability) + ), + applicability, + ); + } + } +} diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs index 4d49b43bde9e..789079510c5e 100644 --- a/clippy_utils/src/source.rs +++ b/clippy_utils/src/source.rs @@ -168,7 +168,7 @@ pub fn snippet<'a, T: LintContext>(cx: &T, span: Span, default: &'a str) -> Cow< snippet_opt(cx, span).map_or_else(|| Cow::Borrowed(default), From::from) } -/// Same as `snippet`, but it adapts the applicability level by following rules: +/// Same as [`snippet`], but it adapts the applicability level by following rules: /// /// - Applicability level `Unspecified` will never be changed. /// - If the span is inside a macro, change the applicability level to `MaybeIncorrect`. diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index e914dc1c222f..bc31bea8b9f3 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -2,6 +2,7 @@ #![allow(clippy::module_name_repetitions)] +use hir::{HirId, QPath}; use rustc_ast::ast::Mutability; use rustc_data_structures::fx::FxHashMap; use rustc_hir as hir; @@ -114,7 +115,7 @@ pub fn has_iter_method(cx: &LateContext<'_>, probably_ref_ty: Ty<'_>) -> Option< /// Checks whether a type implements a trait. /// The function returns false in case the type contains an inference variable. -/// See also `get_trait_def_id`. +/// See also [`get_trait_def_id`]. pub fn implements_trait<'tcx>( cx: &LateContext<'tcx>, ty: Ty<'tcx>, @@ -136,6 +137,21 @@ pub fn implements_trait<'tcx>( }) } +/// Gets the trait that a path targets. For example `::a` would return the +/// [`DefId`] for `Trait`. +/// +/// `cx` must be in a body. +pub fn qpath_target_trait<'tcx>(cx: &LateContext<'tcx>, qpath: &QPath<'_>, expr_id: HirId) -> Option { + let method_res = cx.typeck_results().qpath_res(qpath, expr_id); + let method_id = match method_res { + hir::def::Res::Def(_kind, id) => Some(id), + _ => None, + }; + let method_id = method_id?; + + cx.tcx.trait_of_item(method_id) +} + /// Checks whether this type implements `Drop`. pub fn has_drop<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { match ty.ty_adt_def() { diff --git a/tests/ui/or_fun_call.fixed b/tests/ui/or_fun_call.fixed index 4390ff7dc304..c2f94d0e8575 100644 --- a/tests/ui/or_fun_call.fixed +++ b/tests/ui/or_fun_call.fixed @@ -18,6 +18,19 @@ fn or_fun_call() { } } + struct FakeDefault; + impl FakeDefault { + fn default() -> Self { + FakeDefault + } + } + + impl Default for FakeDefault { + fn default() -> Self { + FakeDefault + } + } + enum Enum { A(i32), } @@ -53,6 +66,12 @@ fn or_fun_call() { let with_default_type = Some(1); with_default_type.unwrap_or_default(); + let self_default = None::; + self_default.unwrap_or_else(::default); + + let real_default = None::; + real_default.unwrap_or_default(); + let with_vec = Some(vec![1]); with_vec.unwrap_or_default(); diff --git a/tests/ui/or_fun_call.rs b/tests/ui/or_fun_call.rs index 75908c974cc9..afaf92961b02 100644 --- a/tests/ui/or_fun_call.rs +++ b/tests/ui/or_fun_call.rs @@ -18,6 +18,19 @@ fn or_fun_call() { } } + struct FakeDefault; + impl FakeDefault { + fn default() -> Self { + FakeDefault + } + } + + impl Default for FakeDefault { + fn default() -> Self { + FakeDefault + } + } + enum Enum { A(i32), } @@ -53,6 +66,12 @@ fn or_fun_call() { let with_default_type = Some(1); with_default_type.unwrap_or(u64::default()); + let self_default = None::; + self_default.unwrap_or(::default()); + + let real_default = None::; + real_default.unwrap_or(::default()); + let with_vec = Some(vec![1]); with_vec.unwrap_or(vec![]); diff --git a/tests/ui/or_fun_call.stderr b/tests/ui/or_fun_call.stderr index 9905029ce91f..b2bcbd38c2df 100644 --- a/tests/ui/or_fun_call.stderr +++ b/tests/ui/or_fun_call.stderr @@ -1,5 +1,5 @@ error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:33:19 + --> $DIR/or_fun_call.rs:46:19 | LL | with_const_fn.unwrap_or(Duration::from_secs(5)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| Duration::from_secs(5))` @@ -7,130 +7,142 @@ LL | with_const_fn.unwrap_or(Duration::from_secs(5)); = note: `-D clippy::or-fun-call` implied by `-D warnings` error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:36:22 + --> $DIR/or_fun_call.rs:49:22 | LL | with_constructor.unwrap_or(make()); | ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(make)` error: use of `unwrap_or` followed by a call to `new` - --> $DIR/or_fun_call.rs:39:5 + --> $DIR/or_fun_call.rs:52:5 | LL | with_new.unwrap_or(Vec::new()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_new.unwrap_or_default()` error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:42:21 + --> $DIR/or_fun_call.rs:55:21 | LL | with_const_args.unwrap_or(Vec::with_capacity(12)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| Vec::with_capacity(12))` error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:45:14 + --> $DIR/or_fun_call.rs:58:14 | LL | with_err.unwrap_or(make()); | ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| make())` error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:48:19 + --> $DIR/or_fun_call.rs:61:19 | LL | with_err_args.unwrap_or(Vec::with_capacity(12)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| Vec::with_capacity(12))` error: use of `unwrap_or` followed by a call to `default` - --> $DIR/or_fun_call.rs:51:5 + --> $DIR/or_fun_call.rs:64:5 | LL | with_default_trait.unwrap_or(Default::default()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_default_trait.unwrap_or_default()` error: use of `unwrap_or` followed by a call to `default` - --> $DIR/or_fun_call.rs:54:5 + --> $DIR/or_fun_call.rs:67:5 | LL | with_default_type.unwrap_or(u64::default()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_default_type.unwrap_or_default()` +error: use of `unwrap_or` followed by a function call + --> $DIR/or_fun_call.rs:70:18 + | +LL | self_default.unwrap_or(::default()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(::default)` + +error: use of `unwrap_or` followed by a call to `default` + --> $DIR/or_fun_call.rs:73:5 + | +LL | real_default.unwrap_or(::default()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `real_default.unwrap_or_default()` + error: use of `unwrap_or` followed by a call to `new` - --> $DIR/or_fun_call.rs:57:5 + --> $DIR/or_fun_call.rs:76:5 | LL | with_vec.unwrap_or(vec![]); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_vec.unwrap_or_default()` error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:60:21 + --> $DIR/or_fun_call.rs:79:21 | LL | without_default.unwrap_or(Foo::new()); | ^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(Foo::new)` error: use of `or_insert` followed by a function call - --> $DIR/or_fun_call.rs:63:19 + --> $DIR/or_fun_call.rs:82:19 | LL | map.entry(42).or_insert(String::new()); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(String::new)` error: use of `or_insert` followed by a function call - --> $DIR/or_fun_call.rs:66:23 + --> $DIR/or_fun_call.rs:85:23 | LL | map_vec.entry(42).or_insert(vec![]); | ^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(Vec::new)` error: use of `or_insert` followed by a function call - --> $DIR/or_fun_call.rs:69:21 + --> $DIR/or_fun_call.rs:88:21 | LL | btree.entry(42).or_insert(String::new()); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(String::new)` error: use of `or_insert` followed by a function call - --> $DIR/or_fun_call.rs:72:25 + --> $DIR/or_fun_call.rs:91:25 | LL | btree_vec.entry(42).or_insert(vec![]); | ^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(Vec::new)` error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:75:21 + --> $DIR/or_fun_call.rs:94:21 | LL | let _ = stringy.unwrap_or("".to_owned()); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| "".to_owned())` error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:83:21 + --> $DIR/or_fun_call.rs:102:21 | LL | let _ = Some(1).unwrap_or(map[&1]); | ^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| map[&1])` error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:85:21 + --> $DIR/or_fun_call.rs:104:21 | LL | let _ = Some(1).unwrap_or(map[&1]); | ^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| map[&1])` error: use of `or` followed by a function call - --> $DIR/or_fun_call.rs:109:35 + --> $DIR/or_fun_call.rs:128:35 | LL | let _ = Some("a".to_string()).or(Some("b".to_string())); | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_else(|| Some("b".to_string()))` error: use of `or` followed by a function call - --> $DIR/or_fun_call.rs:113:10 + --> $DIR/or_fun_call.rs:132:10 | LL | .or(Some(Bar(b, Duration::from_secs(2)))); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_else(|| Some(Bar(b, Duration::from_secs(2))))` error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:141:14 + --> $DIR/or_fun_call.rs:160:14 | LL | None.unwrap_or(s.as_mut_vec()); | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| s.as_mut_vec())` error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:146:14 + --> $DIR/or_fun_call.rs:165:14 | LL | None.unwrap_or(unsafe { s.as_mut_vec() }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| unsafe { s.as_mut_vec() })` error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:148:14 + --> $DIR/or_fun_call.rs:167:14 | LL | None.unwrap_or( unsafe { s.as_mut_vec() } ); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| unsafe { s.as_mut_vec() })` -error: aborting due to 22 previous errors +error: aborting due to 24 previous errors diff --git a/tests/ui/unwrap_or_else_default.fixed b/tests/ui/unwrap_or_else_default.fixed new file mode 100644 index 000000000000..7ac3f426c977 --- /dev/null +++ b/tests/ui/unwrap_or_else_default.fixed @@ -0,0 +1,71 @@ +// run-rustfix + +#![warn(clippy::unwrap_or_else_default)] +#![allow(dead_code)] +#![allow(clippy::unnecessary_wraps)] + +/// Checks implementation of the `UNWRAP_OR_ELSE_DEFAULT` lint. +fn unwrap_or_else_default() { + struct Foo; + + impl Foo { + fn new() -> Foo { + Foo + } + + // fake default, we should not trigger on this + fn default() -> Foo { + Foo + } + } + + struct HasDefaultAndDuplicate; + + impl HasDefaultAndDuplicate { + fn default() -> Self { + HasDefaultAndDuplicate + } + } + + impl Default for HasDefaultAndDuplicate { + fn default() -> Self { + HasDefaultAndDuplicate + } + } + + enum Enum { + A(), + } + + fn make(_: V) -> T { + unimplemented!(); + } + + let with_enum = Some(Enum::A()); + with_enum.unwrap_or_else(Enum::A); + + let with_new = Some(vec![1]); + with_new.unwrap_or_else(Vec::new); + + let with_err: Result<_, ()> = Ok(vec![1]); + with_err.unwrap_or_else(make); + + // should not be changed + let with_fake_default = None::; + with_fake_default.unwrap_or_else(Foo::default); + + // should not be changed + let with_fake_default2 = None::; + with_fake_default2.unwrap_or_else(::default); + + let with_real_default = None::; + with_real_default.unwrap_or_default(); + + let with_default_trait = Some(1); + with_default_trait.unwrap_or_default(); + + let with_default_type = Some(1); + with_default_type.unwrap_or_default(); +} + +fn main() {} diff --git a/tests/ui/unwrap_or_else_default.rs b/tests/ui/unwrap_or_else_default.rs new file mode 100644 index 000000000000..82b727a039ed --- /dev/null +++ b/tests/ui/unwrap_or_else_default.rs @@ -0,0 +1,71 @@ +// run-rustfix + +#![warn(clippy::unwrap_or_else_default)] +#![allow(dead_code)] +#![allow(clippy::unnecessary_wraps)] + +/// Checks implementation of the `UNWRAP_OR_ELSE_DEFAULT` lint. +fn unwrap_or_else_default() { + struct Foo; + + impl Foo { + fn new() -> Foo { + Foo + } + + // fake default, we should not trigger on this + fn default() -> Foo { + Foo + } + } + + struct HasDefaultAndDuplicate; + + impl HasDefaultAndDuplicate { + fn default() -> Self { + HasDefaultAndDuplicate + } + } + + impl Default for HasDefaultAndDuplicate { + fn default() -> Self { + HasDefaultAndDuplicate + } + } + + enum Enum { + A(), + } + + fn make(_: V) -> T { + unimplemented!(); + } + + let with_enum = Some(Enum::A()); + with_enum.unwrap_or_else(Enum::A); + + let with_new = Some(vec![1]); + with_new.unwrap_or_else(Vec::new); + + let with_err: Result<_, ()> = Ok(vec![1]); + with_err.unwrap_or_else(make); + + // should not be changed + let with_fake_default = None::; + with_fake_default.unwrap_or_else(Foo::default); + + // should not be changed + let with_fake_default2 = None::; + with_fake_default2.unwrap_or_else(::default); + + let with_real_default = None::; + with_real_default.unwrap_or_else(::default); + + let with_default_trait = Some(1); + with_default_trait.unwrap_or_else(Default::default); + + let with_default_type = Some(1); + with_default_type.unwrap_or_else(u64::default); +} + +fn main() {} diff --git a/tests/ui/unwrap_or_else_default.stderr b/tests/ui/unwrap_or_else_default.stderr new file mode 100644 index 000000000000..feb215b09f66 --- /dev/null +++ b/tests/ui/unwrap_or_else_default.stderr @@ -0,0 +1,22 @@ +error: use of `.unwrap_or_else(..)` to construct default value + --> $DIR/unwrap_or_else_default.rs:62:5 + | +LL | with_real_default.unwrap_or_else(::default); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `with_real_default.unwrap_or_default()` + | + = note: `-D clippy::unwrap-or-else-default` implied by `-D warnings` + +error: use of `.unwrap_or_else(..)` to construct default value + --> $DIR/unwrap_or_else_default.rs:65:5 + | +LL | with_default_trait.unwrap_or_else(Default::default); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `with_default_trait.unwrap_or_default()` + +error: use of `.unwrap_or_else(..)` to construct default value + --> $DIR/unwrap_or_else_default.rs:68:5 + | +LL | with_default_type.unwrap_or_else(u64::default); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `with_default_type.unwrap_or_default()` + +error: aborting due to 3 previous errors + From c78cc7ac1f1e89cd2e12d29374e1c5e6110d9217 Mon Sep 17 00:00:00 2001 From: Jade Date: Thu, 5 Aug 2021 20:08:52 -0700 Subject: [PATCH 2/4] Add is_trait_item, refactor or_fun_call and unwrap_or_else_default --- clippy_lints/src/methods/mod.rs | 8 ++++---- clippy_lints/src/methods/or_fun_call.rs | 9 ++++----- .../src/methods/unwrap_or_else_default.rs | 9 ++------- clippy_utils/src/lib.rs | 19 +++++++++++++++++++ clippy_utils/src/ty.rs | 16 ---------------- 5 files changed, 29 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 12f7987fd3ad..bf74cad039e1 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -313,12 +313,12 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for usages of `_.unwrap_or_else(Default::default)` on Option and - /// Result values. + /// Checks for usages of `_.unwrap_or_else(Default::default)` on `Option` and + /// `Result` values. /// /// ### Why is this bad? - /// Readability, these can be written as `option.unwrap_or_default` or - /// `result.unwrap_or_default`. + /// Readability, these can be written as `_.unwrap_or_default`, which is + /// simpler and more concise. /// /// ### Examples /// ```rust diff --git a/clippy_lints/src/methods/or_fun_call.rs b/clippy_lints/src/methods/or_fun_call.rs index 378b07241704..c1d22e5d72c1 100644 --- a/clippy_lints/src/methods/or_fun_call.rs +++ b/clippy_lints/src/methods/or_fun_call.rs @@ -1,7 +1,8 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::eager_or_lazy::is_lazyness_candidate; +use clippy_utils::is_trait_item; use clippy_utils::source::{snippet, snippet_with_applicability, snippet_with_macro_callsite}; -use clippy_utils::ty::{implements_trait, qpath_target_trait}; +use clippy_utils::ty::implements_trait; use clippy_utils::ty::{is_type_diagnostic_item, match_type}; use clippy_utils::{contains_return, last_path_segment, paths}; use if_chain::if_chain; @@ -35,9 +36,7 @@ pub(super) fn check<'tcx>( or_has_args: bool, span: Span, ) -> bool { - let is_default_default = |qpath, default_trait_id| { - qpath_target_trait(cx, qpath, fun.hir_id).map_or(false, |target_trait| target_trait == default_trait_id) - }; + let is_default_default = || is_trait_item(cx, fun, sym::Default); let implements_default = |arg, default_trait_id| { let arg_ty = cx.typeck_results().expr_ty(arg); @@ -52,7 +51,7 @@ pub(super) fn check<'tcx>( let path = last_path_segment(qpath).ident.name; // needs to target Default::default in particular or be *::new and have a Default impl // available - if (matches!(path, kw::Default) && is_default_default(qpath, default_trait_id)) + if (matches!(path, kw::Default) && is_default_default()) || (matches!(path, sym::new) && implements_default(arg, default_trait_id)); then { diff --git a/clippy_lints/src/methods/unwrap_or_else_default.rs b/clippy_lints/src/methods/unwrap_or_else_default.rs index f99ae6cae93b..677aa80e1b76 100644 --- a/clippy_lints/src/methods/unwrap_or_else_default.rs +++ b/clippy_lints/src/methods/unwrap_or_else_default.rs @@ -2,9 +2,7 @@ use super::UNWRAP_OR_ELSE_DEFAULT; use clippy_utils::{ - diagnostics::span_lint_and_sugg, - source::snippet_with_applicability, - ty::{is_type_diagnostic_item, qpath_target_trait}, + diagnostics::span_lint_and_sugg, is_trait_item, source::snippet_with_applicability, ty::is_type_diagnostic_item, }; use rustc_errors::Applicability; use rustc_hir as hir; @@ -26,10 +24,7 @@ pub(super) fn check<'tcx>( if_chain! { if is_option || is_result; - if let hir::ExprKind::Path(ref qpath) = u_arg.kind; - if let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default); - if let Some(target_trait) = qpath_target_trait(cx, qpath, u_arg.hir_id); - if target_trait == default_trait_id; + if is_trait_item(cx, u_arg, sym::Default); then { let mut applicability = Applicability::MachineApplicable; diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 59f878f8b20a..da9560f8ccfd 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -326,6 +326,25 @@ pub fn is_trait_method(cx: &LateContext<'_>, expr: &Expr<'_>, diag_item: Symbol) .map_or(false, |did| is_diag_trait_item(cx, did, diag_item)) } +/// Checks if the given expression is a path referring an item on the trait +/// that is marked with the given diagnostic item. +/// +/// For checking method call expressions instead of path expressions, use +/// [`is_trait_method`]. +/// +/// For example, to find if an expression like `u64::default` refers to an item +/// of the trait `Default`, which is marked `#[rustc_diagnostic_item = "Default"]`, +/// a `diag_item` of `sym::Default` should be used. +pub fn is_trait_item(cx: &LateContext<'_>, expr: &Expr<'_>, diag_item: Symbol) -> bool { + if let hir::ExprKind::Path(ref qpath) = expr.kind { + cx.qpath_res(qpath, expr.hir_id) + .opt_def_id() + .map_or(false, |def_id| is_diag_trait_item(cx, def_id, diag_item)) + } else { + false + } +} + pub fn last_path_segment<'tcx>(path: &QPath<'tcx>) -> &'tcx PathSegment<'tcx> { match *path { QPath::Resolved(_, path) => path.segments.last().expect("A path must have at least one segment"), diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index bc31bea8b9f3..536d0f006e30 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -2,7 +2,6 @@ #![allow(clippy::module_name_repetitions)] -use hir::{HirId, QPath}; use rustc_ast::ast::Mutability; use rustc_data_structures::fx::FxHashMap; use rustc_hir as hir; @@ -137,21 +136,6 @@ pub fn implements_trait<'tcx>( }) } -/// Gets the trait that a path targets. For example `::a` would return the -/// [`DefId`] for `Trait`. -/// -/// `cx` must be in a body. -pub fn qpath_target_trait<'tcx>(cx: &LateContext<'tcx>, qpath: &QPath<'_>, expr_id: HirId) -> Option { - let method_res = cx.typeck_results().qpath_res(qpath, expr_id); - let method_id = match method_res { - hir::def::Res::Def(_kind, id) => Some(id), - _ => None, - }; - let method_id = method_id?; - - cx.tcx.trait_of_item(method_id) -} - /// Checks whether this type implements `Drop`. pub fn has_drop<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { match ty.ty_adt_def() { From 23d398b1848571709a0cbb8a08436f6de56d43a8 Mon Sep 17 00:00:00 2001 From: Jade Date: Thu, 5 Aug 2021 19:46:26 -0700 Subject: [PATCH 3/4] tree-wide: Fix all the rustdoc warnings --- clippy_lints/src/needless_continue.rs | 2 +- clippy_lints/src/needless_for_each.rs | 2 +- clippy_utils/src/diagnostics.rs | 6 +++--- clippy_utils/src/higher.rs | 6 +++--- clippy_utils/src/ty.rs | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs index 5088b8bb0d36..5a50cc48d61b 100644 --- a/clippy_lints/src/needless_continue.rs +++ b/clippy_lints/src/needless_continue.rs @@ -422,7 +422,7 @@ fn check_and_warn<'a>(cx: &EarlyContext<'_>, expr: &'a ast::Expr) { /// /// is transformed to /// -/// ```ignore +/// ```text /// { /// let x = 5; /// ``` diff --git a/clippy_lints/src/needless_for_each.rs b/clippy_lints/src/needless_for_each.rs index d9aa42fe8eeb..9a6ddc72ce56 100644 --- a/clippy_lints/src/needless_for_each.rs +++ b/clippy_lints/src/needless_for_each.rs @@ -122,7 +122,7 @@ impl LateLintPass<'_> for NeedlessForEach { /// 2. Detect use of `return` in `Loop` in the closure body. /// /// NOTE: The functionality of this type is similar to -/// [`crate::utilts::visitors::find_all_ret_expressions`], but we can't use +/// [`clippy_utils::visitors::find_all_ret_expressions`], but we can't use /// `find_all_ret_expressions` instead of this type. The reasons are: /// 1. `find_all_ret_expressions` passes the argument of `ExprKind::Ret` to a callback, but what we /// need here is `ExprKind::Ret` itself. diff --git a/clippy_utils/src/diagnostics.rs b/clippy_utils/src/diagnostics.rs index 7c94474cb35d..71cfa196fc33 100644 --- a/clippy_utils/src/diagnostics.rs +++ b/clippy_utils/src/diagnostics.rs @@ -65,7 +65,7 @@ pub fn span_lint(cx: &T, lint: &'static Lint, sp: impl Into $DIR/zero_div_zero.rs:6:25 /// | @@ -103,7 +103,7 @@ pub fn span_lint_and_help<'a, T: LintContext>( /// /// # Example /// -/// ```ignore +/// ```text /// error: calls to `std::mem::forget` with a reference instead of an owned value. Forgetting a reference does nothing. /// --> $DIR/drop_forget_ref.rs:10:5 /// | @@ -189,7 +189,7 @@ pub fn span_lint_hir_and_then( /// /// # Example /// -/// ```ignore +/// ```text /// error: This `.fold` can be more succinctly expressed as `.any` /// --> $DIR/methods.rs:390:13 /// | diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index f32f1109b08e..884180f0586e 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -195,8 +195,8 @@ pub fn vec_macro<'e>(cx: &LateContext<'_>, expr: &'e hir::Expr<'_>) -> Option(e: &'tcx Expr<'tcx>) -> Option>> { /// Try to match the AST for a pattern that contains a match, for example when two args are /// compared @@ -283,7 +283,7 @@ pub struct FormatArgsExpn<'tcx> { /// String literal expressions which represent the format string split by "{}" pub format_string_parts: &'tcx [Expr<'tcx>], - /// Symbols corresponding to [`format_string_parts`] + /// Symbols corresponding to [`Self::format_string_parts`] pub format_string_symbols: Vec, /// Expressions like `ArgumentV1::new(arg0, Debug::fmt)` pub args: &'tcx [Expr<'tcx>], diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 536d0f006e30..4f9aaf396b80 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -114,7 +114,7 @@ pub fn has_iter_method(cx: &LateContext<'_>, probably_ref_ty: Ty<'_>) -> Option< /// Checks whether a type implements a trait. /// The function returns false in case the type contains an inference variable. -/// See also [`get_trait_def_id`]. +/// See also [`get_trait_def_id`](super::get_trait_def_id). pub fn implements_trait<'tcx>( cx: &LateContext<'tcx>, ty: Ty<'tcx>, From 295df88986408c504eaa9982208dca0b505f1de1 Mon Sep 17 00:00:00 2001 From: Jade Date: Wed, 11 Aug 2021 18:28:42 -0700 Subject: [PATCH 4/4] Reword is_trait_item description --- clippy_utils/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index da9560f8ccfd..075f1890c173 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -332,9 +332,9 @@ pub fn is_trait_method(cx: &LateContext<'_>, expr: &Expr<'_>, diag_item: Symbol) /// For checking method call expressions instead of path expressions, use /// [`is_trait_method`]. /// -/// For example, to find if an expression like `u64::default` refers to an item -/// of the trait `Default`, which is marked `#[rustc_diagnostic_item = "Default"]`, -/// a `diag_item` of `sym::Default` should be used. +/// For example, this can be used to find if an expression like `u64::default` +/// refers to an item of the trait `Default`, which is associated with the +/// `diag_item` of `sym::Default`. pub fn is_trait_item(cx: &LateContext<'_>, expr: &Expr<'_>, diag_item: Symbol) -> bool { if let hir::ExprKind::Path(ref qpath) = expr.kind { cx.qpath_res(qpath, expr.hir_id)