mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #12610 - ARandomDev99:manual_unwrap_or_default-12564, r=dswij

Browse Source 
[`manual_unwrap_or_default`]: Check for Default trait implementation in initial condition when linting and use `IfLetOrMatch`

Fixes #12564

changelog: Fix [`manual_unwrap_or_default`] false positive when initial `match`/`if let` condition doesn't implement `Default` but the return type does.
This commit is contained in:
bors 2024-04-08 07:24:17 +00:00
commit 1c9e96536b
4 changed files with 62 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

95
clippy_lints/src/manual_unwrap_or_default.rs
View File

@ -1,13 +1,14 @@
use clippy_utils::sugg::Sugg;
use rustc_errors::Applicability;
use rustc_hir::def::Res;
use rustc_hir::{Arm, Expr, ExprKind, HirId, LangItem, MatchSource, Pat, PatKind, QPath};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::ty::GenericArgKind;
use rustc_session::declare_lint_pass;
use rustc_span::sym;
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet_opt;
use clippy_utils::higher::IfLetOrMatch;
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::implements_trait;
use clippy_utils::{in_constant, is_default_equivalent, peel_blocks, span_contains_comment};
@ -105,19 +106,39 @@ fn get_some_and_none_bodies<'tcx>(
}
}
fn handle_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
let ExprKind::Match(match_expr, [arm1, arm2], MatchSource::Normal | MatchSource::ForLoopDesugar) = expr.kind else {
return false;
#[allow(clippy::needless_pass_by_value)]
fn handle<'tcx>(cx: &LateContext<'tcx>, if_let_or_match: IfLetOrMatch<'tcx>, expr: &'tcx Expr<'tcx>) {
// Get expr_name ("if let" or "match" depending on kind of expression), the condition, the body for
// the some arm, the body for the none arm and the binding id of the some arm
let (expr_name, condition, body_some, body_none, binding_id) = match if_let_or_match {
IfLetOrMatch::Match(condition, [arm1, arm2], MatchSource::Normal | MatchSource::ForLoopDesugar)
// Make sure there are no guards to keep things simple
if arm1.guard.is_none()
&& arm2.guard.is_none()
// Get the some and none bodies and the binding id of the some arm
&& let Some(((body_some, binding_id), body_none)) = get_some_and_none_bodies(cx, arm1, arm2) =>
{
("match", condition, body_some, body_none, binding_id)
},
IfLetOrMatch::IfLet(condition, pat, if_expr, Some(else_expr), _)
if let Some(binding_id) = get_some(cx, pat) =>
{
("if let", condition, if_expr, else_expr, binding_id)
},
_ => {
// All other cases (match with number of arms != 2, if let without else, etc.)
return;
},
};
// We don't want conditions on the arms to simplify things.
if arm1.guard.is_none()
&& arm2.guard.is_none()
// We check that the returned type implements the `Default` trait.
&& let match_ty = cx.typeck_results().expr_ty(expr)
&& let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
&& implements_trait(cx, match_ty, default_trait_id, &[])
// We now get the bodies for both the `Some` and `None` arms.
&& let Some(((body_some, binding_id), body_none)) = get_some_and_none_bodies(cx, arm1, arm2)
// We check if the return type of the expression implements Default.
let expr_type = cx.typeck_results().expr_ty(expr);
if let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
&& implements_trait(cx, expr_type, default_trait_id, &[])
// We check if the initial condition implements Default.
&& let Some(condition_ty) = cx.typeck_results().expr_ty(condition).walk().nth(1)
&& let GenericArgKind::Type(condition_ty) = condition_ty.unpack()
&& implements_trait(cx, condition_ty, default_trait_id, &[])
// We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`.
&& let ExprKind::Path(QPath::Resolved(_, path)) = peel_blocks(body_some).kind
&& let Res::Local(local_id) = path.res
@ -125,8 +146,9 @@ fn handle_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
// We now check the `None` arm is calling a method equivalent to `Default::default`.
&& let body_none = peel_blocks(body_none)
&& is_default_equivalent(cx, body_none)
&& let Some(receiver) = Sugg::hir_opt(cx, match_expr).map(Sugg::maybe_par)
&& let Some(receiver) = Sugg::hir_opt(cx, condition).map(Sugg::maybe_par)
{
// Machine applicable only if there are no comments present
let applicability = if span_contains_comment(cx.sess().source_map(), expr.span) {
Applicability::MaybeIncorrect
} else {
@ -136,48 +158,12 @@ fn handle_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
cx,
MANUAL_UNWRAP_OR_DEFAULT,
expr.span,
"match can be simplified with `.unwrap_or_default()`",
format!("{expr_name} can be simplified with `.unwrap_or_default()`"),
"replace it with",
format!("{receiver}.unwrap_or_default()"),
applicability,
);
}
true
}
fn handle_if_let<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
if let ExprKind::If(cond, if_block, Some(else_expr)) = expr.kind
&& let ExprKind::Let(let_) = cond.kind
&& let ExprKind::Block(_, _) = else_expr.kind
// We check that the returned type implements the `Default` trait.
&& let match_ty = cx.typeck_results().expr_ty(expr)
&& let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
&& implements_trait(cx, match_ty, default_trait_id, &[])
&& let Some(binding_id) = get_some(cx, let_.pat)
// We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`.
&& let ExprKind::Path(QPath::Resolved(_, path)) = peel_blocks(if_block).kind
&& let Res::Local(local_id) = path.res
&& local_id == binding_id
// We now check the `None` arm is calling a method equivalent to `Default::default`.
&& let body_else = peel_blocks(else_expr)
&& is_default_equivalent(cx, body_else)
&& let Some(if_let_expr_snippet) = snippet_opt(cx, let_.init.span)
{
let applicability = if span_contains_comment(cx.sess().source_map(), expr.span) {
Applicability::MaybeIncorrect
} else {
Applicability::MachineApplicable
};
span_lint_and_sugg(
cx,
MANUAL_UNWRAP_OR_DEFAULT,
expr.span,
"if let can be simplified with `.unwrap_or_default()`",
"replace it with",
format!("{if_let_expr_snippet}.unwrap_or_default()"),
applicability,
);
}
}
impl<'tcx> LateLintPass<'tcx> for ManualUnwrapOrDefault {
@ -185,8 +171,9 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
if expr.span.from_expansion() || in_constant(cx, expr.hir_id) {
return;
}
if !handle_match(cx, expr) {
handle_if_let(cx, expr);
// Call handle only if the expression is `if let` or `match`
if let Some(if_let_or_match) = IfLetOrMatch::parse(cx, expr) {
handle(cx, if_let_or_match, expr);
}
}
}

10
tests/ui/manual_unwrap_or_default.fixed
View File

@ -16,6 +16,16 @@ fn main() {
let x: Option<Vec<String>> = None;
x.unwrap_or_default();
// Issue #12564
// No error as &Vec<_> doesn't implement std::default::Default
let mut map = std::collections::HashMap::from([(0, vec![0; 3]), (1, vec![1; 3]), (2, vec![2])]);
let x: &[_] = if let Some(x) = map.get(&0) { x } else { &[] };
// Same code as above written using match.
let x: &[_] = match map.get(&0) {
Some(x) => x,
None => &[],
};
}
// Issue #12531

10
tests/ui/manual_unwrap_or_default.rs
View File

@ -37,6 +37,16 @@ fn main() {
} else {
Vec::default()
};
// Issue #12564
// No error as &Vec<_> doesn't implement std::default::Default
let mut map = std::collections::HashMap::from([(0, vec![0; 3]), (1, vec![1; 3]), (2, vec![2])]);
let x: &[_] = if let Some(x) = map.get(&0) { x } else { &[] };
// Same code as above written using match.
let x: &[_] = match map.get(&0) {
Some(x) => x,
None => &[],
};
}
// Issue #12531

2
tests/ui/manual_unwrap_or_default.stderr
View File

@ -53,7 +53,7 @@ LL | | };
| |_____^ help: replace it with: `x.unwrap_or_default()`
error: match can be simplified with `.unwrap_or_default()`
--> tests/ui/manual_unwrap_or_default.rs:46:20
--> tests/ui/manual_unwrap_or_default.rs:56:20
|
LL | Some(_) => match *b {
| ____________________^