Skip to content

Commit 8fbd252

Browse files
profetiaprofetia
committed
fix: branches-sharing-code suggests wrongly on const and static 
1 parent 9a2076e commit 8fbd252

File tree

4 files changed

+349
-27
lines changed

4 files changed

+349
-27
lines changed

‎clippy_lints/src/copies.rs

Lines changed: 52 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ use clippy_utils::{
1212
use core::iter;
1313
use core::ops::ControlFlow;
1414
use rustc_errors::Applicability;
15-
use rustc_hir::{Block, Expr, ExprKind, HirId, HirIdSet, LetStmt, Node, Stmt, StmtKind, intravisit};
15+
use rustc_hir::{Block, Expr, ExprKind, HirId, HirIdSet, ItemKind, LetStmt, Node, Stmt, StmtKind, UseKind, intravisit};
1616
use rustc_lint::{LateContext, LateLintPass};
1717
use rustc_middle::ty::TyCtxt;
1818
use rustc_session::impl_lint_pass;
@@ -303,6 +303,7 @@ struct BlockEq {
303303
/// The name and id of every local which can be moved at the beginning and the end.
304304
moved_locals: Vec<(HirId, Symbol)>,
305305
}
306+
306307
impl BlockEq {
307308
fn start_span(&self, b: &Block<'_>, sm: &SourceMap) -> Option<Span> {
308309
match &b.stmts[..self.start_end_eq] {
@@ -324,20 +325,32 @@ impl BlockEq {
324325
}
325326

326327
/// If the statement is a local, checks if the bound names match the expected list of names.
327-
fn eq_binding_names(s: &Stmt<'_>, names: &[(HirId, Symbol)]) -> bool {
328-
if let StmtKind::Let(l) = s.kind {
329-
let mut i = 0usize;
330-
let mut res = true;
331-
l.pat.each_binding_or_first(&mut |_, _, _, name| {
332-
if names.get(i).is_some_and(|&(_, n)| n == name.name) {
333-
i += 1;
334-
} else {
335-
res = false;
336-
}
337-
});
338-
res && i == names.len()
339-
} else {
340-
false
328+
fn eq_binding_names(cx: &LateContext<'_>, s: &Stmt<'_>, names: &[(HirId, Symbol)]) -> bool {
329+
match s.kind {
330+
StmtKind::Let(l) => {
331+
let mut i = 0usize;
332+
let mut res = true;
333+
l.pat.each_binding_or_first(&mut |_, _, _, name| {
334+
if names.get(i).is_some_and(|&(_, n)| n == name.name) {
335+
i += 1;
336+
} else {
337+
res = false;
338+
}
339+
});
340+
res && i == names.len()
341+
},
342+
StmtKind::Item(item_id)
343+
if let item = cx.tcx.hir_item(item_id)
344+
&& let ItemKind::Static(_, ident, ..)
345+
| ItemKind::Const(ident, ..)
346+
| ItemKind::Fn { ident, .. }
347+
| ItemKind::TyAlias(ident, ..)
348+
| ItemKind::Use(_, UseKind::Single(ident))
349+
| ItemKind::Mod(ident, _) = item.kind =>
350+
{
351+
names.last().is_some_and(|&(_, n)| n == ident.name)
352+
},
353+
_ => false,
341354
}
342355
}
343356

@@ -359,6 +372,7 @@ fn modifies_any_local<'tcx>(cx: &LateContext<'tcx>, s: &'tcx Stmt<'_>, locals: &
359372
/// Checks if the given statement should be considered equal to the statement in the same position
360373
/// for each block.
361374
fn eq_stmts(
375+
cx: &LateContext<'_>,
362376
stmt: &Stmt<'_>,
363377
blocks: &[&Block<'_>],
364378
get_stmt: impl for<'a> Fn(&'a Block<'a>) -> Option<&'a Stmt<'a>>,
@@ -373,7 +387,7 @@ fn eq_stmts(
373387
let new_bindings = &moved_bindings[old_count..];
374388
blocks
375389
.iter()
376-
.all(|b| get_stmt(b).is_some_and(|s| eq_binding_names(s, new_bindings)))
390+
.all(|b| get_stmt(b).is_some_and(|s| eq_binding_names(cx, s, new_bindings)))
377391
} else {
378392
true
379393
}) && blocks.iter().all(|b| get_stmt(b).is_some_and(|s| eq.eq_stmt(s, stmt)))
@@ -413,7 +427,7 @@ fn scan_block_for_eq<'tcx>(
413427
return true;
414428
}
415429
modifies_any_local(cx, stmt, &cond_locals)
416-
|| !eq_stmts(stmt, blocks, |b| b.stmts.get(i), &mut eq, &mut moved_locals)
430+
|| !eq_stmts(cx, stmt, blocks, |b| b.stmts.get(i), &mut eq, &mut moved_locals)
417431
})
418432
.map_or(block.stmts.len(), |(i, stmt)| {
419433
adjust_by_closest_callsite(i, stmt, block.stmts[..i].iter().enumerate().rev())
@@ -474,6 +488,7 @@ fn scan_block_for_eq<'tcx>(
474488
}))
475489
.fold(end_search_start, |init, (stmt, offset)| {
476490
if eq_stmts(
491+
cx,
477492
stmt,
478493
blocks,
479494
|b| b.stmts.get(b.stmts.len() - offset),
@@ -485,11 +500,26 @@ fn scan_block_for_eq<'tcx>(
485500
// Clear out all locals seen at the end so far. None of them can be moved.
486501
let stmts = &blocks[0].stmts;
487502
for stmt in &stmts[stmts.len() - init..=stmts.len() - offset] {
488-
if let StmtKind::Let(l) = stmt.kind {
489-
l.pat.each_binding_or_first(&mut |_, id, _, _| {
490-
// FIXME(rust/#120456) - is `swap_remove` correct?
491-
eq.locals.swap_remove(&id);
492-
});
503+
match stmt.kind {
504+
StmtKind::Let(l) => {
505+
l.pat.each_binding_or_first(&mut |_, id, _, _| {
506+
// FIXME(rust/#120456) - is `swap_remove` correct?
507+
eq.locals.swap_remove(&id);
508+
});
509+
},
510+
StmtKind::Item(item_id) => {
511+
let item = cx.tcx.hir_item(item_id);
512+
if let ItemKind::Static(..)
513+
| ItemKind::Const(..)
514+
| ItemKind::Fn { .. }
515+
| ItemKind::TyAlias(..)
516+
| ItemKind::Use(..)
517+
| ItemKind::Mod(..) = item.kind
518+
{
519+
eq.local_items.swap_remove(&item.owner_id.to_def_id());
520+
}
521+
},
522+
_ => {},
493523
}
494524
}
495525
moved_locals.truncate(moved_locals_at_start);

‎clippy_utils/src/hir_utils.rs

Lines changed: 176 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,16 @@ use crate::source::{SpanRange, SpanRangeExt, walk_span_to_context};
44
use crate::tokenize_with_text;
55
use rustc_ast::ast;
66
use rustc_ast::ast::InlineAsmTemplatePiece;
7-
use rustc_data_structures::fx::FxHasher;
7+
use rustc_data_structures::fx::{FxHasher, FxIndexMap};
88
use rustc_hir::MatchSource::TryDesugar;
99
use rustc_hir::def::{DefKind, Res};
10+
use rustc_hir::def_id::DefId;
1011
use rustc_hir::{
1112
AssocItemConstraint, BinOpKind, BindingMode, Block, BodyId, Closure, ConstArg, ConstArgKind, Expr, ExprField,
12-
ExprKind, FnRetTy, GenericArg, GenericArgs, HirId, HirIdMap, InlineAsmOperand, LetExpr, Lifetime, LifetimeKind,
13-
Node, Pat, PatExpr, PatExprKind, PatField, PatKind, Path, PathSegment, PrimTy, QPath, Stmt, StmtKind,
14-
StructTailExpr, TraitBoundModifiers, Ty, TyKind, TyPat, TyPatKind,
13+
ExprKind, FnDecl, FnHeader, FnRetTy, FnSig, GenericArg, GenericArgs, GenericParam, GenericParamKind,
14+
GenericParamSource, Generics, HirId, HirIdMap, InlineAsmOperand, ItemId, ItemKind, LetExpr, Lifetime, LifetimeKind,
15+
LifetimeParamKind, Node, ParamName, Pat, PatExpr, PatExprKind, PatField, PatKind, Path, PathSegment, PrimTy, QPath,
16+
Stmt, StmtKind, StructTailExpr, TraitBoundModifiers, Ty, TyKind, TyPat, TyPatKind, UseKind,
1517
};
1618
use rustc_lexer::{FrontmatterAllowed, TokenKind, tokenize};
1719
use rustc_lint::LateContext;
@@ -106,6 +108,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> {
106108
left_ctxt: SyntaxContext::root(),
107109
right_ctxt: SyntaxContext::root(),
108110
locals: HirIdMap::default(),
111+
local_items: FxIndexMap::default(),
109112
}
110113
}
111114

@@ -144,6 +147,7 @@ pub struct HirEqInterExpr<'a, 'b, 'tcx> {
144147
// right. For example, when comparing `{ let x = 1; x + 2 }` and `{ let y = 1; y + 2 }`,
145148
// these blocks are considered equal since `x` is mapped to `y`.
146149
pub locals: HirIdMap<HirId>,
150+
pub local_items: FxIndexMap<DefId, DefId>,
147151
}
148152

149153
impl HirEqInterExpr<'_, '_, '_> {
@@ -168,6 +172,163 @@ impl HirEqInterExpr<'_, '_, '_> {
168172
&& self.eq_pat(l.pat, r.pat)
169173
},
170174
(StmtKind::Expr(l), StmtKind::Expr(r)) | (StmtKind::Semi(l), StmtKind::Semi(r)) => self.eq_expr(l, r),
175+
(StmtKind::Item(l), StmtKind::Item(r)) => self.eq_item(*l, *r),
176+
_ => false,
177+
}
178+
}
179+
180+
pub fn eq_item(&mut self, l: ItemId, r: ItemId) -> bool {
181+
let left = self.inner.cx.tcx.hir_item(l);
182+
let right = self.inner.cx.tcx.hir_item(r);
183+
let eq = match (left.kind, right.kind) {
184+
(
185+
ItemKind::Const(l_ident, l_generics, l_ty, l_body),
186+
ItemKind::Const(r_ident, r_generics, r_ty, r_body),
187+
) => {
188+
l_ident.name == r_ident.name
189+
&& self.eq_generics(l_generics, r_generics)
190+
&& self.eq_ty(l_ty, r_ty)
191+
&& self.eq_body(l_body, r_body)
192+
},
193+
(ItemKind::Static(l_mut, l_ident, l_ty, l_body), ItemKind::Static(r_mut, r_ident, r_ty, r_body)) => {
194+
l_mut == r_mut && l_ident.name == r_ident.name && self.eq_ty(l_ty, r_ty) && self.eq_body(l_body, r_body)
195+
},
196+
(
197+
ItemKind::Fn {
198+
sig: l_sig,
199+
ident: l_ident,
200+
generics: l_generics,
201+
body: l_body,
202+
has_body: l_has_body,
203+
},
204+
ItemKind::Fn {
205+
sig: r_sig,
206+
ident: r_ident,
207+
generics: r_generics,
208+
body: r_body,
209+
has_body: r_has_body,
210+
},
211+
) => {
212+
l_ident.name == r_ident.name
213+
&& self.eq_fn_sig(&l_sig, &r_sig)
214+
&& self.eq_generics(l_generics, r_generics)
215+
&& (l_has_body == r_has_body)
216+
&& self.eq_body(l_body, r_body)
217+
},
218+
(ItemKind::TyAlias(l_ident, l_generics, l_ty), ItemKind::TyAlias(r_ident, r_generics, r_ty)) => {
219+
l_ident.name == r_ident.name && self.eq_generics(l_generics, r_generics) && self.eq_ty(l_ty, r_ty)
220+
},
221+
(ItemKind::Use(l_path, l_kind), ItemKind::Use(r_path, r_kind)) => {
222+
self.eq_path_segments(l_path.segments, r_path.segments)
223+
&& match (l_kind, r_kind) {
224+
(UseKind::Single(l_ident), UseKind::Single(r_ident)) => l_ident.name == r_ident.name,
225+
(UseKind::Glob, UseKind::Glob) => true,
226+
(UseKind::ListStem, UseKind::ListStem) => true,
227+
_ => false,
228+
}
229+
},
230+
(ItemKind::Mod(l_ident, l_mod), ItemKind::Mod(r_ident, r_mod)) => {
231+
l_ident.name == r_ident.name && over(l_mod.item_ids, r_mod.item_ids, |l, r| self.eq_item(*l, *r))
232+
},
233+
_ => false,
234+
};
235+
if eq {
236+
self.local_items.insert(l.owner_id.to_def_id(), r.owner_id.to_def_id());
237+
}
238+
eq
239+
}
240+
241+
fn eq_fn_sig(&mut self, left: &FnSig<'_>, right: &FnSig<'_>) -> bool {
242+
self.eq_fn_header(&left.header, &right.header) && self.eq_fn_decl(&left.decl, &right.decl)
243+
}
244+
245+
fn eq_fn_header(&mut self, left: &FnHeader, right: &FnHeader) -> bool {
246+
left.safety == right.safety
247+
&& left.constness == right.constness
248+
&& left.asyncness == right.asyncness
249+
&& left.abi == right.abi
250+
}
251+
252+
fn eq_fn_decl(&mut self, left: &FnDecl<'_>, right: &FnDecl<'_>) -> bool {
253+
over(left.inputs, right.inputs, |l, r| self.eq_ty(l, r))
254+
&& (match (left.output, right.output) {
255+
(FnRetTy::DefaultReturn(_), FnRetTy::DefaultReturn(_)) => true,
256+
(FnRetTy::Return(l_ty), FnRetTy::Return(r_ty)) => self.eq_ty(l_ty, r_ty),
257+
_ => false,
258+
})
259+
&& left.c_variadic == right.c_variadic
260+
&& left.implicit_self == right.implicit_self
261+
&& left.lifetime_elision_allowed == right.lifetime_elision_allowed
262+
}
263+
264+
fn eq_generics(&mut self, left: &Generics<'_>, right: &Generics<'_>) -> bool {
265+
self.eq_generics_param(left.params, right.params)
266+
}
267+
268+
fn eq_generics_param(&mut self, left: &[GenericParam<'_>], right: &[GenericParam<'_>]) -> bool {
269+
over(left, right, |l, r| {
270+
self.eq_generics_param_name(&l.name, &r.name)
271+
&& l.pure_wrt_drop == r.pure_wrt_drop
272+
&& self.eq_generics_param_kind(&l.kind, &r.kind)
273+
&& self.eq_generic_param_source(&l.source, &r.source)
274+
})
275+
}
276+
277+
fn eq_generics_param_name(&mut self, left: &ParamName, right: &ParamName) -> bool {
278+
match (left, right) {
279+
(ParamName::Plain(l_ident), ParamName::Plain(r_ident)) => l_ident.name == r_ident.name,
280+
(ParamName::Error(l_ident), ParamName::Error(r_ident)) => l_ident.name == r_ident.name,
281+
(ParamName::Fresh, ParamName::Fresh) => true,
282+
_ => false,
283+
}
284+
}
285+
286+
fn eq_generics_param_kind(&mut self, left: &GenericParamKind<'_>, right: &GenericParamKind<'_>) -> bool {
287+
match (left, right) {
288+
(GenericParamKind::Lifetime { kind: l_kind }, GenericParamKind::Lifetime { kind: r_kind }) => {
289+
match (l_kind, r_kind) {
290+
(LifetimeParamKind::Explicit, LifetimeParamKind::Explicit) => true,
291+
(LifetimeParamKind::Elided(l_lifetime_kind), LifetimeParamKind::Elided(r_lifetime_kind)) => {
292+
l_lifetime_kind == r_lifetime_kind
293+
},
294+
(LifetimeParamKind::Error, LifetimeParamKind::Error) => true,
295+
_ => false,
296+
}
297+
},
298+
(
299+
GenericParamKind::Type {
300+
default: l_default,
301+
synthetic: l_synthetic,
302+
},
303+
GenericParamKind::Type {
304+
default: r_default,
305+
synthetic: r_synthetic,
306+
},
307+
) => both(*l_default, *r_default, |l, r| self.eq_ty(l, r)) && l_synthetic == r_synthetic,
308+
(
309+
GenericParamKind::Const {
310+
ty: l_ty,
311+
default: l_default,
312+
synthetic: l_synthetic,
313+
},
314+
GenericParamKind::Const {
315+
ty: r_ty,
316+
default: r_default,
317+
synthetic: r_synthetic,
318+
},
319+
) => {
320+
self.eq_ty(&l_ty, &r_ty)
321+
&& both(*l_default, *r_default, |l, r| self.eq_const_arg(l, r))
322+
&& l_synthetic == r_synthetic
323+
},
324+
_ => false,
325+
}
326+
}
327+
328+
fn eq_generic_param_source(&mut self, left: &GenericParamSource, right: &GenericParamSource) -> bool {
329+
match (left, right) {
330+
(GenericParamSource::Generics, GenericParamSource::Generics) => true,
331+
(GenericParamSource::Binder, GenericParamSource::Binder) => true,
171332
_ => false,
172333
}
173334
}
@@ -562,6 +723,17 @@ impl HirEqInterExpr<'_, '_, '_> {
562723
match (left.res, right.res) {
563724
(Res::Local(l), Res::Local(r)) => l == r || self.locals.get(&l) == Some(&r),
564725
(Res::Local(_), _) | (_, Res::Local(_)) => false,
726+
(Res::Def(l_kind, l), Res::Def(r_kind, r))
727+
if l_kind == r_kind
728+
&& let DefKind::Const
729+
| DefKind::Static { .. }
730+
| DefKind::Fn
731+
| DefKind::TyAlias
732+
| DefKind::Use
733+
| DefKind::Mod = l_kind =>
734+
{
735+
(l == r || self.local_items.get(&l) == Some(&r)) && self.eq_path_segments(left.segments, right.segments)
736+
},
565737
_ => self.eq_path_segments(left.segments, right.segments),
566738
}
567739
}

0 commit comments

Comments
 (0)