Skip to content

Commit

Permalink
Revert "Auto merge of rust-lang#6568 - Jarcho:redundant_pattern_match…
Browse files Browse the repository at this point in the history 
…ing, r=flip1995"

This reverts commit 7f2068c, reversing
changes made to 1e0a3ff.
  • Loading branch information
@flip1995
flip1995 committed May 5, 2021
1 parent 7e538e3 commit 37b5e81
Show file tree
Hide file tree
Showing 13 changed files with 89 additions and 581 deletions.
231 changes: 24 additions & 207 deletions clippy_lints/src/matches.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -424,12 +424,7 @@ declare_clippy_lint! {
/// **Why is this bad?** It's more concise and clear to just use the proper
/// utility function
///
/// **Known problems:** This will change the drop order for the matched type. Both `if let` and
/// `while let` will drop the value at the end of the block, both `if` and `while` will drop the
/// value before entering the block. For most types this change will not matter, but for a few
/// types this will not be an acceptable change (e.g. locks). See the
/// [reference](https://doc.rust-lang.org/reference/destructors.html#drop-scopes) for more about
/// drop order.
/// **Known problems:** None.
///
/// **Example:**
///
Expand Down Expand Up @@ -1707,206 +1702,55 @@ where
mod redundant_pattern_match {
use super::REDUNDANT_PATTERN_MATCHING;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::{snippet, snippet_with_applicability};
use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, is_type_lang_item, match_type};
use clippy_utils::source::snippet;
use clippy_utils::{is_lang_ctor, is_qpath_def_path, is_trait_method, paths};
use if_chain::if_chain;
use rustc_ast::ast::LitKind;
use rustc_errors::Applicability;
use rustc_hir::LangItem::{OptionNone, OptionSome, PollPending, PollReady, ResultErr, ResultOk};
use rustc_hir::{
intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor},
Arm, Block, Expr, ExprKind, LangItem, MatchSource, Node, PatKind, QPath,
};
use rustc_hir::{Arm, Expr, ExprKind, MatchSource, PatKind, QPath};
use rustc_lint::LateContext;
use rustc_middle::ty::{self, subst::GenericArgKind, Ty};
use rustc_span::sym;

pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if let ExprKind::Match(op, arms, ref match_source) = &expr.kind {
match match_source {
MatchSource::Normal => find_sugg_for_match(cx, expr, op, arms),
MatchSource::IfLetDesugar { contains_else_clause } => {
find_sugg_for_if_let(cx, expr, op, &arms[0], "if", *contains_else_clause)
},
MatchSource::WhileLetDesugar => find_sugg_for_if_let(cx, expr, op, &arms[0], "while", false),
MatchSource::IfLetDesugar { .. } => find_sugg_for_if_let(cx, expr, op, arms, "if"),
MatchSource::WhileLetDesugar => find_sugg_for_if_let(cx, expr, op, arms, "while"),
_ => {},
}
}
}

/// Checks if the drop order for a type matters. Some std types implement drop solely to
/// deallocate memory. For these types, and composites containing them, changing the drop order
/// won't result in any observable side effects.
fn type_needs_ordered_drop(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
if !ty.needs_drop(cx.tcx, cx.param_env) {
false
} else if !cx
.tcx
.lang_items()
.drop_trait()
.map_or(false, |id| implements_trait(cx, ty, id, &[]))
{
// This type doesn't implement drop, so no side effects here.
// Check if any component type has any.
match ty.kind() {
ty::Tuple(_) => ty.tuple_fields().any(|ty| type_needs_ordered_drop(cx, ty)),
ty::Array(ty, _) => type_needs_ordered_drop(cx, ty),
ty::Adt(adt, subs) => adt
.all_fields()
.map(|f| f.ty(cx.tcx, subs))
.any(|ty| type_needs_ordered_drop(cx, ty)),
_ => true,
}
}
// Check for std types which implement drop, but only for memory allocation.
else if is_type_diagnostic_item(cx, ty, sym::vec_type)
|| is_type_lang_item(cx, ty, LangItem::OwnedBox)
|| is_type_diagnostic_item(cx, ty, sym::Rc)
|| is_type_diagnostic_item(cx, ty, sym::Arc)
|| is_type_diagnostic_item(cx, ty, sym::cstring_type)
|| match_type(cx, ty, &paths::BTREEMAP)
|| match_type(cx, ty, &paths::LINKED_LIST)
|| match_type(cx, ty, &paths::WEAK_RC)
|| match_type(cx, ty, &paths::WEAK_ARC)
{
// Check all of the generic arguments.
if let ty::Adt(_, subs) = ty.kind() {
subs.types().any(|ty| type_needs_ordered_drop(cx, ty))
} else {
true
}
} else {
true
}
}

// Extract the generic arguments out of a type
fn try_get_generic_ty(ty: Ty<'_>, index: usize) -> Option<Ty<'_>> {
if_chain! {
if let ty::Adt(_, subs) = ty.kind();
if let Some(sub) = subs.get(index);
if let GenericArgKind::Type(sub_ty) = sub.unpack();
then {
Some(sub_ty)
} else {
None
}
}
}

// Checks if there are any temporaries created in the given expression for which drop order
// matters.
fn temporaries_need_ordered_drop(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
struct V<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
res: bool,
}
impl<'a, 'tcx> Visitor<'tcx> for V<'a, 'tcx> {
type Map = ErasedMap<'tcx>;
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
NestedVisitorMap::None
}

fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
match expr.kind {
// Taking the reference of a value leaves a temporary
// e.g. In `&String::new()` the string is a temporary value.
// Remaining fields are temporary values
// e.g. In `(String::new(), 0).1` the string is a temporary value.
ExprKind::AddrOf(_, _, expr) | ExprKind::Field(expr, _) => {
if !matches!(expr.kind, ExprKind::Path(_)) {
if type_needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(expr)) {
self.res = true;
} else {
self.visit_expr(expr);
}
}
},
// the base type is alway taken by reference.
// e.g. In `(vec![0])[0]` the vector is a temporary value.
ExprKind::Index(base, index) => {
if !matches!(base.kind, ExprKind::Path(_)) {
if type_needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(base)) {
self.res = true;
} else {
self.visit_expr(base);
}
}
self.visit_expr(index);
},
// Method calls can take self by reference.
// e.g. In `String::new().len()` the string is a temporary value.
ExprKind::MethodCall(_, _, [self_arg, args @ ..], _) => {
if !matches!(self_arg.kind, ExprKind::Path(_)) {
let self_by_ref = self
.cx
.typeck_results()
.type_dependent_def_id(expr.hir_id)
.map_or(false, |id| self.cx.tcx.fn_sig(id).skip_binder().inputs()[0].is_ref());
if self_by_ref
&& type_needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(self_arg))
{
self.res = true;
} else {
self.visit_expr(self_arg)
}
}
args.iter().for_each(|arg| self.visit_expr(arg));
},
// Either explicitly drops values, or changes control flow.
ExprKind::DropTemps(_)
| ExprKind::Ret(_)
| ExprKind::Break(..)
| ExprKind::Yield(..)
| ExprKind::Block(Block { expr: None, .. }, _)
| ExprKind::Loop(..) => (),

// Only consider the final expression.
ExprKind::Block(Block { expr: Some(expr), .. }, _) => self.visit_expr(expr),

_ => walk_expr(self, expr),
}
}
}

let mut v = V { cx, res: false };
v.visit_expr(expr);
v.res
}

fn find_sugg_for_if_let<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
op: &'tcx Expr<'tcx>,
arm: &Arm<'_>,
op: &Expr<'_>,
arms: &[Arm<'_>],
keyword: &'static str,
has_else: bool,
) {
// also look inside refs
let mut kind = &arm.pat.kind;
let mut kind = &arms[0].pat.kind;
// if we have &None for example, peel it so we can detect "if let None = x"
if let PatKind::Ref(inner, _mutability) = kind {
kind = &inner.kind;
}
let op_ty = cx.typeck_results().expr_ty(op);
// Determine which function should be used, and the type contained by the corresponding
// variant.
let (good_method, inner_ty) = match kind {
let good_method = match kind {
PatKind::TupleStruct(ref path, [sub_pat], _) => {
if let PatKind::Wild = sub_pat.kind {
if is_lang_ctor(cx, path, ResultOk) {
("is_ok()", try_get_generic_ty(op_ty, 0).unwrap_or(op_ty))
"is_ok()"
} else if is_lang_ctor(cx, path, ResultErr) {
("is_err()", try_get_generic_ty(op_ty, 1).unwrap_or(op_ty))
"is_err()"
} else if is_lang_ctor(cx, path, OptionSome) {
("is_some()", op_ty)
"is_some()"
} else if is_lang_ctor(cx, path, PollReady) {
("is_ready()", op_ty)
"is_ready()"
} else if is_qpath_def_path(cx, path, sub_pat.hir_id, &paths::IPADDR_V4) {
("is_ipv4()", op_ty)
"is_ipv4()"
} else if is_qpath_def_path(cx, path, sub_pat.hir_id, &paths::IPADDR_V6) {
("is_ipv6()", op_ty)
"is_ipv6()"
} else {
return;
}
Expand All @@ -1915,36 +1759,17 @@ mod redundant_pattern_match {
}
},
PatKind::Path(ref path) => {
let method = if is_lang_ctor(cx, path, OptionNone) {
if is_lang_ctor(cx, path, OptionNone) {
"is_none()"
} else if is_lang_ctor(cx, path, PollPending) {
"is_pending()"
} else {
return;
};
// `None` and `Pending` don't have an inner type.
(method, cx.tcx.types.unit)
}
},
_ => return,
};

// If this is the last expression in a block or there is an else clause then the whole
// type needs to be considered, not just the inner type of the branch being matched on.
// Note the last expression in a block is dropped after all local bindings.
let check_ty = if has_else
|| (keyword == "if" && matches!(cx.tcx.hir().parent_iter(expr.hir_id).next(), Some((_, Node::Block(..)))))
{
op_ty
} else {
inner_ty
};

// All temporaries created in the scrutinee expression are dropped at the same time as the
// scrutinee would be, so they have to be considered as well.
// e.g. in `if let Some(x) = foo.lock().unwrap().baz.as_ref() { .. }` the lock will be held
// for the duration if body.
let needs_drop = type_needs_ordered_drop(cx, check_ty) || temporaries_need_ordered_drop(cx, op);

// check that `while_let_on_iterator` lint does not trigger
if_chain! {
if keyword == "while";
Expand All @@ -1963,7 +1788,7 @@ mod redundant_pattern_match {
span_lint_and_then(
cx,
REDUNDANT_PATTERN_MATCHING,
arm.pat.span,
arms[0].pat.span,
&format!("redundant pattern matching, consider using `{}`", good_method),
|diag| {
// while let ... = ... { ... }
Expand All @@ -1977,20 +1802,12 @@ mod redundant_pattern_match {
// while let ... = ... { ... }
// ^^^^^^^^^^^^^^^^^^^
let span = expr_span.until(op_span.shrink_to_hi());

let mut app = if needs_drop {
Applicability::MaybeIncorrect
} else {
Applicability::MachineApplicable
};
let sugg = snippet_with_applicability(cx, op_span, "_", &mut app);

diag.span_suggestion(span, "try this", format!("{} {}.{}", keyword, sugg, good_method), app);

if needs_drop {
diag.note("this will change drop order of the result, as well as all temporaries");
diag.note("add `#[allow(clippy::redundant_pattern_matching)]` if this is important");
}
diag.span_suggestion(
span,
"try this",
format!("{} {}.{}", keyword, snippet(cx, op_span, "_"), good_method),
Applicability::MachineApplicable, // snippet
);
},
);
}
Expand Down
58 changes: 0 additions & 58 deletions tests/ui/redundant_pattern_matching_drop_order.fixed
View file

This file was deleted.

Loading

0 comments on commit 37b5e81

Please sign in to comment.