diff --git a/clippy_lints/src/derivable_impls.rs b/clippy_lints/src/derivable_impls.rs index 6b8a6aec92fa..992ed320ce68 100644 --- a/clippy_lints/src/derivable_impls.rs +++ b/clippy_lints/src/derivable_impls.rs @@ -105,7 +105,7 @@ fn check_struct<'tcx>( if let TyKind::Path(QPath::Resolved(_, p)) = self_ty.kind && let Some(PathSegment { args, .. }) = p.segments.last() { - let args = args.map(|a| a.args).unwrap_or(&[]); + let args = args.map(|a| a.args).unwrap_or_default(); // ty_args contains the generic parameters of the type declaration, while args contains the // arguments used at instantiation time. If both len are not equal, it means that some diff --git a/clippy_lints/src/methods/map_unwrap_or.rs b/clippy_lints/src/methods/map_unwrap_or.rs index f2e3cc5c0091..ac2f99180486 100644 --- a/clippy_lints/src/methods/map_unwrap_or.rs +++ b/clippy_lints/src/methods/map_unwrap_or.rs @@ -1,13 +1,13 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::msrvs::{self, Msrv}; -use clippy_utils::res::MaybeDef; +use clippy_utils::res::{MaybeDef as _, MaybeResPath as _}; use clippy_utils::source::snippet_with_applicability; use clippy_utils::ty::is_copy; use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; use rustc_hir::def::Res; -use rustc_hir::intravisit::{Visitor, walk_path}; -use rustc_hir::{ExprKind, HirId, Node, PatKind, Path, QPath}; +use rustc_hir::intravisit::{Visitor, walk_expr, walk_path}; +use rustc_hir::{ExprKind, HirId, LangItem, Node, PatKind, Path, QPath}; use rustc_lint::LateContext; use rustc_middle::hir::nested_filter; use rustc_span::{Span, sym}; @@ -28,116 +28,131 @@ pub(super) fn check<'tcx>( msrv: Msrv, ) { let recv_ty = cx.typeck_results().expr_ty(recv).peel_refs(); - let is_option = recv_ty.is_diag_item(cx, sym::Option); - let is_result = recv_ty.is_diag_item(cx, sym::Result); + let recv_ty_kind = match recv_ty.opt_diag_name(cx) { + Some(sym::Option) => sym::Option, + Some(sym::Result) if msrv.meets(cx, msrvs::RESULT_MAP_OR) => sym::Result, + _ => return, + }; - if is_result && !msrv.meets(cx, msrvs::RESULT_MAP_OR) { + let unwrap_arg_ty = cx.typeck_results().expr_ty(unwrap_arg); + if !is_copy(cx, unwrap_arg_ty) { + // Replacing `.map().unwrap_or()` with `.map_or(, )` can sometimes lead to + // borrowck errors, see #10579 for one such instance. + // In particular, if `a` causes a move and `f` references that moved binding, then we cannot lint: + // ``` + // let x = vec![1, 2]; + // x.get(0..1).map(|s| s.to_vec()).unwrap_or(x); + // ``` + // This compiles, but changing it to `map_or` will produce a compile error: + // ``` + // let x = vec![1, 2]; + // x.get(0..1).map_or(x, |s| s.to_vec()) + // ^ moving `x` here + // ^^^^^^^^^^^ while it is borrowed here (and later used in the closure) + // ``` + // So, we have to check that `a` is not referenced anywhere (even outside of the `.map` closure!) + // before the call to `unwrap_or`. + + let mut unwrap_visitor = UnwrapVisitor { + cx, + identifiers: FxHashSet::default(), + }; + unwrap_visitor.visit_expr(unwrap_arg); + + let mut reference_visitor = ReferenceVisitor { + cx, + identifiers: unwrap_visitor.identifiers, + unwrap_or_span: unwrap_arg.span, + }; + + let body = cx.tcx.hir_body_owned_by(cx.tcx.hir_enclosing_body_owner(expr.hir_id)); + + // Visit the body, and return if we've found a reference + if reference_visitor.visit_body(body).is_break() { + return; + } + } + + if !unwrap_arg.span.eq_ctxt(map_span) { return; } - // lint if the caller of `map()` is an `Option` - if is_option || is_result { - if !is_copy(cx, cx.typeck_results().expr_ty(unwrap_arg)) { - // Replacing `.map().unwrap_or()` with `.map_or(, )` can sometimes lead to - // borrowck errors, see #10579 for one such instance. - // In particular, if `a` causes a move and `f` references that moved binding, then we cannot lint: - // ``` - // let x = vec![1, 2]; - // x.get(0..1).map(|s| s.to_vec()).unwrap_or(x); - // ``` - // This compiles, but changing it to `map_or` will produce a compile error: - // ``` - // let x = vec![1, 2]; - // x.get(0..1).map_or(x, |s| s.to_vec()) - // ^ moving `x` here - // ^^^^^^^^^^^ while it is borrowed here (and later used in the closure) - // ``` - // So, we have to check that `a` is not referenced anywhere (even outside of the `.map` closure!) - // before the call to `unwrap_or`. + let mut applicability = Applicability::MachineApplicable; + // get snippet for unwrap_or() + let unwrap_snippet = snippet_with_applicability(cx, unwrap_arg.span, "..", &mut applicability); + // lint message - let mut unwrap_visitor = UnwrapVisitor { - cx, - identifiers: FxHashSet::default(), - }; - unwrap_visitor.visit_expr(unwrap_arg); - - let mut reference_visitor = ReferenceVisitor { - cx, - identifiers: unwrap_visitor.identifiers, - unwrap_or_span: unwrap_arg.span, - }; - - let body = cx.tcx.hir_body_owned_by(cx.tcx.hir_enclosing_body_owner(expr.hir_id)); - - // Visit the body, and return if we've found a reference - if reference_visitor.visit_body(body).is_break() { - return; - } - } - - if !unwrap_arg.span.eq_ctxt(map_span) { - return; - } - - // is_some_and is stabilised && `unwrap_or` argument is false; suggest `is_some_and` instead - let suggest_is_some_and = matches!(&unwrap_arg.kind, ExprKind::Lit(lit) - if matches!(lit.node, rustc_ast::LitKind::Bool(false))) - && msrv.meets(cx, msrvs::OPTION_RESULT_IS_VARIANT_AND); - - let mut applicability = Applicability::MachineApplicable; - // get snippet for unwrap_or() - let unwrap_snippet = snippet_with_applicability(cx, unwrap_arg.span, "..", &mut applicability); - // lint message - // comparing the snippet from source to raw text ("None") below is safe - // because we already have checked the type. - let unwrap_snippet_none = is_option && unwrap_snippet == "None"; - let arg = if unwrap_snippet_none { - "None" - } else if suggest_is_some_and { - "false" - } else { - "" - }; - let suggest = if unwrap_snippet_none { - "and_then()" - } else if suggest_is_some_and { - if is_result { - "is_ok_and()" - } else { - "is_some_and()" - } - } else { - "map_or(, )" - }; - let msg = format!( - "called `map().unwrap_or({arg})` on an `{}` value", - if is_option { "Option" } else { "Result" } - ); - - span_lint_and_then(cx, MAP_UNWRAP_OR, expr.span, msg, |diag| { - let map_arg_span = map_arg.span; - - let mut suggestion = vec![ - ( - map_span, - String::from(if unwrap_snippet_none { - "and_then" - } else if suggest_is_some_and { - if is_result { "is_ok_and" } else { "is_some_and" } - } else { - "map_or" - }), - ), - (expr.span.with_lo(unwrap_recv.span.hi()), String::new()), - ]; - - if !unwrap_snippet_none && !suggest_is_some_and { - suggestion.push((map_arg_span.with_hi(map_arg_span.lo()), format!("{unwrap_snippet}, "))); - } - - diag.multipart_suggestion(format!("use `{suggest}` instead"), suggestion, applicability); - }); + let suggest_kind = if recv_ty_kind == sym::Option + && unwrap_arg + .basic_res() + .ctor_parent(cx) + .is_lang_item(cx, LangItem::OptionNone) + { + SuggestedKind::AndThen } + // is_some_and is stabilised && `unwrap_or` argument is false; suggest `is_some_and` instead + else if matches!(&unwrap_arg.kind, ExprKind::Lit(lit) + if matches!(lit.node, rustc_ast::LitKind::Bool(false))) + && msrv.meets(cx, msrvs::OPTION_RESULT_IS_VARIANT_AND) + { + SuggestedKind::IsVariantAnd + } else { + SuggestedKind::Other + }; + + let arg = match suggest_kind { + SuggestedKind::AndThen => "None", + SuggestedKind::IsVariantAnd => "false", + SuggestedKind::Other => "", + }; + + let suggest = match (suggest_kind, recv_ty_kind) { + (SuggestedKind::AndThen, _) => "and_then()", + (SuggestedKind::IsVariantAnd, sym::Result) => "is_ok_and()", + (SuggestedKind::IsVariantAnd, sym::Option) => "is_some_and()", + _ => "map_or(, )", + }; + + let msg = format!( + "called `map().unwrap_or({arg})` on {} `{recv_ty_kind}` value", + if recv_ty_kind == sym::Option { "an" } else { "a" } + ); + + span_lint_and_then(cx, MAP_UNWRAP_OR, expr.span, msg, |diag| { + let map_arg_span = map_arg.span; + + let mut suggestion = vec![ + ( + map_span, + String::from(match (suggest_kind, recv_ty_kind) { + (SuggestedKind::AndThen, _) => "and_then", + (SuggestedKind::IsVariantAnd, sym::Result) => "is_ok_and", + (SuggestedKind::IsVariantAnd, sym::Option) => "is_some_and", + (SuggestedKind::Other, _) + if unwrap_arg_ty.peel_refs().is_array() + && cx.typeck_results().expr_ty_adjusted(unwrap_arg).peel_refs().is_slice() => + { + return; + }, + _ => "map_or", + }), + ), + (expr.span.with_lo(unwrap_recv.span.hi()), String::new()), + ]; + + if matches!(suggest_kind, SuggestedKind::Other) { + suggestion.push((map_arg_span.with_hi(map_arg_span.lo()), format!("{unwrap_snippet}, "))); + } + + diag.multipart_suggestion(format!("use `{suggest}` instead"), suggestion, applicability); + }); +} + +#[derive(Clone, Copy, PartialEq, Eq)] +enum SuggestedKind { + AndThen, + IsVariantAnd, + Other, } struct UnwrapVisitor<'a, 'tcx> { @@ -186,7 +201,7 @@ fn visit_expr(&mut self, expr: &'tcx rustc_hir::Expr<'_>) -> ControlFlow<()> { { return ControlFlow::Break(()); } - rustc_hir::intravisit::walk_expr(self, expr) + walk_expr(self, expr) } fn maybe_tcx(&mut self) -> Self::MaybeTyCtxt { diff --git a/clippy_lints/src/methods/map_unwrap_or_else.rs b/clippy_lints/src/methods/map_unwrap_or_else.rs index a2f157c0cb8a..8bb532b21635 100644 --- a/clippy_lints/src/methods/map_unwrap_or_else.rs +++ b/clippy_lints/src/methods/map_unwrap_or_else.rs @@ -1,18 +1,18 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::msrvs::{self, Msrv}; -use clippy_utils::res::MaybeDef; +use clippy_utils::res::MaybeDef as _; use clippy_utils::source::snippet; +use clippy_utils::sym; use clippy_utils::usage::mutated_variables; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::LateContext; -use rustc_span::symbol::sym; use super::MAP_UNWRAP_OR; /// lint use of `map().unwrap_or_else()` for `Option`s and `Result`s /// -/// Returns true if the lint was emitted +/// Is part of the `map_unwrap_or` lint, split into separate files for readability. pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, @@ -22,49 +22,46 @@ pub(super) fn check<'tcx>( msrv: Msrv, ) -> bool { let recv_ty = cx.typeck_results().expr_ty(recv).peel_refs(); - let is_option = recv_ty.is_diag_item(cx, sym::Option); - let is_result = recv_ty.is_diag_item(cx, sym::Result); + let recv_ty_kind = match recv_ty.opt_diag_name(cx) { + Some(sym::Option) => sym::Option, + Some(sym::Result) if msrv.meets(cx, msrvs::RESULT_MAP_OR_ELSE) => sym::Result, + _ => return false, + }; - if is_result && !msrv.meets(cx, msrvs::RESULT_MAP_OR_ELSE) { + // Don't make a suggestion that may fail to compile due to mutably borrowing + // the same variable twice. + let Some(map_mutated_vars) = mutated_variables(recv, cx) else { + return false; + }; + let Some(unwrap_mutated_vars) = mutated_variables(unwrap_arg, cx) else { + return false; + }; + if map_mutated_vars.intersection(&unwrap_mutated_vars).next().is_some() { return false; } - if is_option || is_result { - // Don't make a suggestion that may fail to compile due to mutably borrowing - // the same variable twice. - let map_mutated_vars = mutated_variables(recv, cx); - let unwrap_mutated_vars = mutated_variables(unwrap_arg, cx); - if let (Some(map_mutated_vars), Some(unwrap_mutated_vars)) = (map_mutated_vars, unwrap_mutated_vars) { - if map_mutated_vars.intersection(&unwrap_mutated_vars).next().is_some() { - return false; - } - } else { - return false; - } - - // lint message - let msg = if is_option { - "called `map().unwrap_or_else()` on an `Option` value" - } else { - "called `map().unwrap_or_else()` on a `Result` value" - }; - // get snippets for args to map() and unwrap_or_else() - let map_snippet = snippet(cx, map_arg.span, ".."); - let unwrap_snippet = snippet(cx, unwrap_arg.span, ".."); - // lint, with note if both map() and unwrap_or_else() have the same span - if map_arg.span.eq_ctxt(unwrap_arg.span) { - let var_snippet = snippet(cx, recv.span, ".."); - span_lint_and_sugg( - cx, - MAP_UNWRAP_OR, - expr.span, - msg, - "try", - format!("{var_snippet}.map_or_else({unwrap_snippet}, {map_snippet})"), - Applicability::MachineApplicable, - ); - return true; - } + // lint message + let msg = if recv_ty_kind == sym::Option { + "called `map().unwrap_or_else()` on an `Option` value" + } else { + "called `map().unwrap_or_else()` on a `Result` value" + }; + // get snippets for args to map() and unwrap_or_else() + let map_snippet = snippet(cx, map_arg.span, ".."); + let unwrap_snippet = snippet(cx, unwrap_arg.span, ".."); + // lint, with note if both map() and unwrap_or_else() have the same span + if map_arg.span.eq_ctxt(unwrap_arg.span) { + let var_snippet = snippet(cx, recv.span, ".."); + span_lint_and_sugg( + cx, + MAP_UNWRAP_OR, + expr.span, + msg, + "try", + format!("{var_snippet}.map_or_else({unwrap_snippet}, {map_snippet})"), + Applicability::MachineApplicable, + ); + return true; } false diff --git a/tests/ui/map_unwrap_or.rs b/tests/ui/map_unwrap_or.rs index fba81cb493cd..dccacd7df8af 100644 --- a/tests/ui/map_unwrap_or.rs +++ b/tests/ui/map_unwrap_or.rs @@ -156,3 +156,11 @@ struct VecInStruct { println!("{y:?}"); } } + +fn issue15752() { + struct Foo<'a>(&'a [u32]); + + let x = Some(Foo(&[1, 2, 3])); + x.map(|y| y.0).unwrap_or(&[]); + //~^ map_unwrap_or +} diff --git a/tests/ui/map_unwrap_or.stderr b/tests/ui/map_unwrap_or.stderr index df0207c420e6..bd9e0eeb0bda 100644 --- a/tests/ui/map_unwrap_or.stderr +++ b/tests/ui/map_unwrap_or.stderr @@ -232,5 +232,11 @@ LL - let _ = opt.map(|x| x > 5).unwrap_or(false); LL + let _ = opt.is_some_and(|x| x > 5); | -error: aborting due to 15 previous errors +error: called `map().unwrap_or()` on an `Option` value + --> tests/ui/map_unwrap_or.rs:164:5 + | +LL | x.map(|y| y.0).unwrap_or(&[]); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 16 previous errors diff --git a/tests/ui/map_unwrap_or_fixable.fixed b/tests/ui/map_unwrap_or_fixable.fixed index 1789c7f4d487..dca2536132d7 100644 --- a/tests/ui/map_unwrap_or_fixable.fixed +++ b/tests/ui/map_unwrap_or_fixable.fixed @@ -1,7 +1,11 @@ //@aux-build:option_helpers.rs #![warn(clippy::map_unwrap_or)] -#![allow(clippy::unnecessary_lazy_evaluations)] +#![allow( + clippy::unnecessary_lazy_evaluations, + clippy::manual_is_variant_and, + clippy::unnecessary_map_or +)] #[macro_use] extern crate option_helpers; diff --git a/tests/ui/map_unwrap_or_fixable.rs b/tests/ui/map_unwrap_or_fixable.rs index 309067edce4d..c60cb082ae3c 100644 --- a/tests/ui/map_unwrap_or_fixable.rs +++ b/tests/ui/map_unwrap_or_fixable.rs @@ -1,7 +1,11 @@ //@aux-build:option_helpers.rs #![warn(clippy::map_unwrap_or)] -#![allow(clippy::unnecessary_lazy_evaluations)] +#![allow( + clippy::unnecessary_lazy_evaluations, + clippy::manual_is_variant_and, + clippy::unnecessary_map_or +)] #[macro_use] extern crate option_helpers; diff --git a/tests/ui/map_unwrap_or_fixable.stderr b/tests/ui/map_unwrap_or_fixable.stderr index 696f516ee055..33a865d6769c 100644 --- a/tests/ui/map_unwrap_or_fixable.stderr +++ b/tests/ui/map_unwrap_or_fixable.stderr @@ -1,5 +1,5 @@ error: called `map().unwrap_or_else()` on an `Option` value - --> tests/ui/map_unwrap_or_fixable.rs:17:13 + --> tests/ui/map_unwrap_or_fixable.rs:21:13 | LL | let _ = opt.map(|x| x + 1) | _____________^ @@ -11,7 +11,7 @@ LL | | .unwrap_or_else(|| 0); = help: to override `-D warnings` add `#[allow(clippy::map_unwrap_or)]` error: called `map().unwrap_or_else()` on a `Result` value - --> tests/ui/map_unwrap_or_fixable.rs:48:13 + --> tests/ui/map_unwrap_or_fixable.rs:52:13 | LL | let _ = res.map(|x| x + 1) | _____________^ @@ -20,7 +20,7 @@ LL | | .unwrap_or_else(|_e| 0); | |_______________________________^ help: try: `res.map_or_else(|_e| 0, |x| x + 1)` error: called `map().unwrap_or()` on an `Option` value - --> tests/ui/map_unwrap_or_fixable.rs:65:20 + --> tests/ui/map_unwrap_or_fixable.rs:69:20 | LL | println!("{}", o.map(|y| y + 1).unwrap_or(3)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -32,13 +32,13 @@ LL + println!("{}", o.map_or(3, |y| y + 1)); | error: called `map().unwrap_or_else()` on an `Option` value - --> tests/ui/map_unwrap_or_fixable.rs:67:20 + --> tests/ui/map_unwrap_or_fixable.rs:71:20 | LL | println!("{}", o.map(|y| y + 1).unwrap_or_else(|| 3)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `o.map_or_else(|| 3, |y| y + 1)` -error: called `map().unwrap_or()` on an `Result` value - --> tests/ui/map_unwrap_or_fixable.rs:69:20 +error: called `map().unwrap_or()` on a `Result` value + --> tests/ui/map_unwrap_or_fixable.rs:73:20 | LL | println!("{}", r.map(|y| y + 1).unwrap_or(3)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -50,13 +50,13 @@ LL + println!("{}", r.map_or(3, |y| y + 1)); | error: called `map().unwrap_or_else()` on a `Result` value - --> tests/ui/map_unwrap_or_fixable.rs:71:20 + --> tests/ui/map_unwrap_or_fixable.rs:75:20 | LL | println!("{}", r.map(|y| y + 1).unwrap_or_else(|()| 3)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `r.map_or_else(|()| 3, |y| y + 1)` -error: called `map().unwrap_or(false)` on an `Result` value - --> tests/ui/map_unwrap_or_fixable.rs:74:20 +error: called `map().unwrap_or(false)` on a `Result` value + --> tests/ui/map_unwrap_or_fixable.rs:78:20 | LL | println!("{}", r.map(|y| y == 1).unwrap_or(false)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -68,18 +68,6 @@ LL + println!("{}", r.is_ok_and(|y| y == 1)); | error: called `map().unwrap_or()` on an `Option` value - --> tests/ui/map_unwrap_or_fixable.rs:80:20 - | -LL | println!("{}", x.map(|y| y + 1).unwrap_or(3)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -help: use `map_or(, )` instead - | -LL - println!("{}", x.map(|y| y + 1).unwrap_or(3)); -LL + println!("{}", x.map_or(3, |y| y + 1)); - | - -error: called `map().unwrap_or()` on an `Result` value --> tests/ui/map_unwrap_or_fixable.rs:84:20 | LL | println!("{}", x.map(|y| y + 1).unwrap_or(3)); @@ -91,14 +79,26 @@ LL - println!("{}", x.map(|y| y + 1).unwrap_or(3)); LL + println!("{}", x.map_or(3, |y| y + 1)); | -error: called `map().unwrap_or_else()` on an `Option` value +error: called `map().unwrap_or()` on a `Result` value --> tests/ui/map_unwrap_or_fixable.rs:88:20 | +LL | println!("{}", x.map(|y| y + 1).unwrap_or(3)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `map_or(, )` instead + | +LL - println!("{}", x.map(|y| y + 1).unwrap_or(3)); +LL + println!("{}", x.map_or(3, |y| y + 1)); + | + +error: called `map().unwrap_or_else()` on an `Option` value + --> tests/ui/map_unwrap_or_fixable.rs:92:20 + | LL | println!("{}", x.map(|y| y + 1).unwrap_or_else(|| 3)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.map_or_else(|| 3, |y| y + 1)` error: called `map().unwrap_or_else()` on a `Result` value - --> tests/ui/map_unwrap_or_fixable.rs:92:20 + --> tests/ui/map_unwrap_or_fixable.rs:96:20 | LL | println!("{}", x.map(|y| y + 1).unwrap_or_else(|_| 3)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.map_or_else(|_| 3, |y| y + 1)`