Skip to content

Commit 95396f6

Browse files
borsbors
committed
Auto merge of #8617 - Alexendoo:relax-needless-late-init, r=giraffate
`needless_late_init`: ignore `if let`, `let mut` and significant drops No longer lints `if let`, personal taste on this one is pretty split, so it probably shouldn't be warning by default. Fixes #8613 ```rust let x = if let Some(n) = y { n } else { 1 } ``` No longer lints `let mut`, things like the following are not uncommon and look fine as they are https://github.com/shepmaster/twox-hash/blob/b169c16d86eb8ea4a296b0acb9d00ca7e3c3005f/src/sixty_four.rs#L88-L93 Avoids changing the drop order in an observable way, where the type of `x` has a drop with side effects and something between `x` and the first use also does, e.g. https://github.com/denoland/rusty_v8/blob/48cc6cb791cac57d22fab1a2feaa92da8ddc9a68/tests/test_api.rs#L159-L167 The implementation of `type_needs_ordered_drop_inner` was changed a bit, it now uses `Ty::has_significant_drop` and reordered the ifs to check diagnostic name before checking the implicit drop impl changelog: [`needless_late_init`]: No longer lints `if let` statements, `let mut` bindings and no longer significantly changes drop order
2 parents 94623ee + 1d1fecf commit 95396f6

8 files changed

+221
-139
lines changed

‎clippy_lints/src/matches/redundant_pattern_match.rs

Lines changed: 6 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,16 @@ use super::REDUNDANT_PATTERN_MATCHING;
22
use clippy_utils::diagnostics::span_lint_and_then;
33
use clippy_utils::source::snippet;
44
use clippy_utils::sugg::Sugg;
5-
use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, is_type_lang_item, match_type};
5+
use clippy_utils::ty::needs_ordered_drop;
66
use clippy_utils::{higher, match_def_path};
77
use clippy_utils::{is_lang_ctor, is_trait_method, paths};
88
use if_chain::if_chain;
99
use rustc_ast::ast::LitKind;
10-
use rustc_data_structures::fx::FxHashSet;
1110
use rustc_errors::Applicability;
1211
use rustc_hir::LangItem::{OptionNone, PollPending};
1312
use rustc_hir::{
1413
intravisit::{walk_expr, Visitor},
15-
Arm, Block, Expr, ExprKind, LangItem, Node, Pat, PatKind, QPath, UnOp,
14+
Arm, Block, Expr, ExprKind, Node, Pat, PatKind, QPath, UnOp,
1615
};
1716
use rustc_lint::LateContext;
1817
use rustc_middle::ty::{self, subst::GenericArgKind, DefIdTree, Ty};
@@ -32,59 +31,6 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
3231
}
3332
}
3433

35-
/// Checks if the drop order for a type matters. Some std types implement drop solely to
36-
/// deallocate memory. For these types, and composites containing them, changing the drop order
37-
/// won't result in any observable side effects.
38-
fn type_needs_ordered_drop<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
39-
type_needs_ordered_drop_inner(cx, ty, &mut FxHashSet::default())
40-
}
41-
42-
fn type_needs_ordered_drop_inner<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, seen: &mut FxHashSet<Ty<'tcx>>) -> bool {
43-
if !seen.insert(ty) {
44-
return false;
45-
}
46-
if !ty.needs_drop(cx.tcx, cx.param_env) {
47-
false
48-
} else if !cx
49-
.tcx
50-
.lang_items()
51-
.drop_trait()
52-
.map_or(false, |id| implements_trait(cx, ty, id, &[]))
53-
{
54-
// This type doesn't implement drop, so no side effects here.
55-
// Check if any component type has any.
56-
match ty.kind() {
57-
ty::Tuple(fields) => fields.iter().any(|ty| type_needs_ordered_drop_inner(cx, ty, seen)),
58-
ty::Array(ty, _) => type_needs_ordered_drop_inner(cx, *ty, seen),
59-
ty::Adt(adt, subs) => adt
60-
.all_fields()
61-
.map(|f| f.ty(cx.tcx, subs))
62-
.any(|ty| type_needs_ordered_drop_inner(cx, ty, seen)),
63-
_ => true,
64-
}
65-
}
66-
// Check for std types which implement drop, but only for memory allocation.
67-
else if is_type_diagnostic_item(cx, ty, sym::Vec)
68-
|| is_type_lang_item(cx, ty, LangItem::OwnedBox)
69-
|| is_type_diagnostic_item(cx, ty, sym::Rc)
70-
|| is_type_diagnostic_item(cx, ty, sym::Arc)
71-
|| is_type_diagnostic_item(cx, ty, sym::cstring_type)
72-
|| is_type_diagnostic_item(cx, ty, sym::BTreeMap)
73-
|| is_type_diagnostic_item(cx, ty, sym::LinkedList)
74-
|| match_type(cx, ty, &paths::WEAK_RC)
75-
|| match_type(cx, ty, &paths::WEAK_ARC)
76-
{
77-
// Check all of the generic arguments.
78-
if let ty::Adt(_, subs) = ty.kind() {
79-
subs.types().any(|ty| type_needs_ordered_drop_inner(cx, ty, seen))
80-
} else {
81-
true
82-
}
83-
} else {
84-
true
85-
}
86-
}
87-
8834
// Extract the generic arguments out of a type
8935
fn try_get_generic_ty(ty: Ty<'_>, index: usize) -> Option<Ty<'_>> {
9036
if_chain! {
@@ -115,7 +61,7 @@ fn temporaries_need_ordered_drop<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<
11561
// e.g. In `(String::new(), 0).1` the string is a temporary value.
11662
ExprKind::AddrOf(_, _, expr) | ExprKind::Field(expr, _) => {
11763
if !matches!(expr.kind, ExprKind::Path(_)) {
118-
if type_needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(expr)) {
64+
if needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(expr)) {
11965
self.res = true;
12066
} else {
12167
self.visit_expr(expr);
@@ -126,7 +72,7 @@ fn temporaries_need_ordered_drop<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<
12672
// e.g. In `(vec![0])[0]` the vector is a temporary value.
12773
ExprKind::Index(base, index) => {
12874
if !matches!(base.kind, ExprKind::Path(_)) {
129-
if type_needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(base)) {
75+
if needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(base)) {
13076
self.res = true;
13177
} else {
13278
self.visit_expr(base);
@@ -143,7 +89,7 @@ fn temporaries_need_ordered_drop<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<
14389
.typeck_results()
14490
.type_dependent_def_id(expr.hir_id)
14591
.map_or(false, |id| self.cx.tcx.fn_sig(id).skip_binder().inputs()[0].is_ref());
146-
if self_by_ref && type_needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(self_arg)) {
92+
if self_by_ref && needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(self_arg)) {
14793
self.res = true;
14894
} else {
14995
self.visit_expr(self_arg);
@@ -243,7 +189,7 @@ fn find_sugg_for_if_let<'tcx>(
243189
// scrutinee would be, so they have to be considered as well.
244190
// e.g. in `if let Some(x) = foo.lock().unwrap().baz.as_ref() { .. }` the lock will be held
245191
// for the duration if body.
246-
let needs_drop = type_needs_ordered_drop(cx, check_ty) || temporaries_need_ordered_drop(cx, let_expr);
192+
let needs_drop = needs_ordered_drop(cx, check_ty) || temporaries_need_ordered_drop(cx, let_expr);
247193

248194
// check that `while_let_on_iterator` lint does not trigger
249195
if_chain! {

‎clippy_lints/src/needless_late_init.rs

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::path_to_local;
33
use clippy_utils::source::snippet_opt;
4-
use clippy_utils::visitors::{expr_visitor, is_local_used};
4+
use clippy_utils::ty::needs_ordered_drop;
5+
use clippy_utils::visitors::{expr_visitor, expr_visitor_no_bodies, is_local_used};
56
use rustc_errors::Applicability;
67
use rustc_hir::intravisit::Visitor;
7-
use rustc_hir::{Block, Expr, ExprKind, HirId, Local, LocalSource, MatchSource, Node, Pat, PatKind, Stmt, StmtKind};
8+
use rustc_hir::{
9+
BindingAnnotation, Block, Expr, ExprKind, HirId, Local, LocalSource, MatchSource, Node, Pat, PatKind, Stmt,
10+
StmtKind,
11+
};
812
use rustc_lint::{LateContext, LateLintPass};
913
use rustc_session::{declare_lint_pass, declare_tool_lint};
1014
use rustc_span::Span;
@@ -73,6 +77,31 @@ fn contains_assign_expr<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'tcx>) ->
7377
seen
7478
}
7579

80+
fn contains_let(cond: &Expr<'_>) -> bool {
81+
let mut seen = false;
82+
expr_visitor_no_bodies(|expr| {
83+
if let ExprKind::Let(_) = expr.kind {
84+
seen = true;
85+
}
86+
87+
!seen
88+
})
89+
.visit_expr(cond);
90+
91+
seen
92+
}
93+
94+
fn stmt_needs_ordered_drop(cx: &LateContext<'_>, stmt: &Stmt<'_>) -> bool {
95+
let StmtKind::Local(local) = stmt.kind else { return false };
96+
!local.pat.walk_short(|pat| {
97+
if let PatKind::Binding(.., None) = pat.kind {
98+
!needs_ordered_drop(cx, cx.typeck_results().pat_ty(pat))
99+
} else {
100+
true
101+
}
102+
})
103+
}
104+
76105
#[derive(Debug)]
77106
struct LocalAssign {
78107
lhs_id: HirId,
@@ -187,11 +216,14 @@ fn first_usage<'tcx>(
187216
local_stmt_id: HirId,
188217
block: &'tcx Block<'tcx>,
189218
) -> Option<Usage<'tcx>> {
219+
let significant_drop = needs_ordered_drop(cx, cx.typeck_results().node_type(binding_id));
220+
190221
block
191222
.stmts
192223
.iter()
193224
.skip_while(|stmt| stmt.hir_id != local_stmt_id)
194225
.skip(1)
226+
.take_while(|stmt| !significant_drop || !stmt_needs_ordered_drop(cx, stmt))
195227
.find(|&stmt| is_local_used(cx, stmt, binding_id))
196228
.and_then(|stmt| match stmt.kind {
197229
StmtKind::Expr(expr) => Some(Usage {
@@ -258,7 +290,7 @@ fn check<'tcx>(
258290
},
259291
);
260292
},
261-
ExprKind::If(_, then_expr, Some(else_expr)) => {
293+
ExprKind::If(cond, then_expr, Some(else_expr)) if !contains_let(cond) => {
262294
let (applicability, suggestions) = assignment_suggestions(cx, binding_id, [then_expr, else_expr])?;
263295

264296
span_lint_and_then(
@@ -338,7 +370,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessLateInit {
338370
if let Local {
339371
init: None,
340372
pat: &Pat {
341-
kind: PatKind::Binding(_, binding_id, _, None),
373+
kind: PatKind::Binding(BindingAnnotation::Unannotated, binding_id, _, None),
342374
..
343375
},
344376
source: LocalSource::Normal,

‎clippy_utils/src/ty.rs

Lines changed: 68 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,11 @@
33
#![allow(clippy::module_name_repetitions)]
44

55
use rustc_ast::ast::Mutability;
6-
use rustc_data_structures::fx::FxHashMap;
6+
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
77
use rustc_hir as hir;
88
use rustc_hir::def::{CtorKind, DefKind, Res};
99
use rustc_hir::def_id::DefId;
10-
use rustc_hir::{Expr, TyKind, Unsafety};
10+
use rustc_hir::{Expr, LangItem, TyKind, Unsafety};
1111
use rustc_infer::infer::TyCtxtInferExt;
1212
use rustc_lint::LateContext;
1313
use rustc_middle::mir::interpret::{ConstValue, Scalar};
@@ -22,7 +22,7 @@ use rustc_trait_selection::infer::InferCtxtExt;
2222
use rustc_trait_selection::traits::query::normalize::AtExt;
2323
use std::iter;
2424

25-
use crate::{match_def_path, must_use_attr, path_res};
25+
use crate::{match_def_path, must_use_attr, path_res, paths};
2626

2727
// Checks if the given type implements copy.
2828
pub fn is_copy<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
@@ -83,6 +83,20 @@ pub fn get_associated_type<'tcx>(
8383
})
8484
}
8585

86+
/// Get the diagnostic name of a type, e.g. `sym::HashMap`. To check if a type
87+
/// implements a trait marked with a diagnostic item use [`implements_trait`].
88+
///
89+
/// For a further exploitation what diagnostic items are see [diagnostic items] in
90+
/// rustc-dev-guide.
91+
///
92+
/// [Diagnostic Items]: https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-items.html
93+
pub fn get_type_diagnostic_name(cx: &LateContext<'_>, ty: Ty<'_>) -> Option<Symbol> {
94+
match ty.kind() {
95+
ty::Adt(adt, _) => cx.tcx.get_diagnostic_name(adt.did()),
96+
_ => None,
97+
}
98+
}
99+
86100
/// Returns true if ty has `iter` or `iter_mut` methods
87101
pub fn has_iter_method(cx: &LateContext<'_>, probably_ref_ty: Ty<'_>) -> Option<Symbol> {
88102
// FIXME: instead of this hard-coded list, we should check if `<adt>::iter`
@@ -319,6 +333,57 @@ pub fn match_type(cx: &LateContext<'_>, ty: Ty<'_>, path: &[&str]) -> bool {
319333
}
320334
}
321335

336+
/// Checks if the drop order for a type matters. Some std types implement drop solely to
337+
/// deallocate memory. For these types, and composites containing them, changing the drop order
338+
/// won't result in any observable side effects.
339+
pub fn needs_ordered_drop<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
340+
fn needs_ordered_drop_inner<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, seen: &mut FxHashSet<Ty<'tcx>>) -> bool {
341+
if !seen.insert(ty) {
342+
return false;
343+
}
344+
if !ty.has_significant_drop(cx.tcx, cx.param_env) {
345+
false
346+
}
347+
// Check for std types which implement drop, but only for memory allocation.
348+
else if is_type_lang_item(cx, ty, LangItem::OwnedBox)
349+
|| matches!(
350+
get_type_diagnostic_name(cx, ty),
351+
Some(sym::HashSet | sym::Rc | sym::Arc | sym::cstring_type)
352+
)
353+
|| match_type(cx, ty, &paths::WEAK_RC)
354+
|| match_type(cx, ty, &paths::WEAK_ARC)
355+
{
356+
// Check all of the generic arguments.
357+
if let ty::Adt(_, subs) = ty.kind() {
358+
subs.types().any(|ty| needs_ordered_drop_inner(cx, ty, seen))
359+
} else {
360+
true
361+
}
362+
} else if !cx
363+
.tcx
364+
.lang_items()
365+
.drop_trait()
366+
.map_or(false, |id| implements_trait(cx, ty, id, &[]))
367+
{
368+
// This type doesn't implement drop, so no side effects here.
369+
// Check if any component type has any.
370+
match ty.kind() {
371+
ty::Tuple(fields) => fields.iter().any(|ty| needs_ordered_drop_inner(cx, ty, seen)),
372+
ty::Array(ty, _) => needs_ordered_drop_inner(cx, *ty, seen),
373+
ty::Adt(adt, subs) => adt
374+
.all_fields()
375+
.map(|f| f.ty(cx.tcx, subs))
376+
.any(|ty| needs_ordered_drop_inner(cx, ty, seen)),
377+
_ => true,
378+
}
379+
} else {
380+
true
381+
}
382+
}
383+
384+
needs_ordered_drop_inner(cx, ty, &mut FxHashSet::default())
385+
}
386+
322387
/// Peels off all references on the type. Returns the underlying type and the number of references
323388
/// removed.
324389
pub fn peel_mid_ty_refs(ty: Ty<'_>) -> (Ty<'_>, usize) {

0 commit comments

Comments
 (0)