Skip to content

Commit 6bce1ce

Browse files
profetiaprofetia
committed
fix: branches-sharing-code suggests wrongly on const and static 
1 parent c1f6124 commit 6bce1ce

File tree

4 files changed

+330
-27
lines changed

4 files changed

+330
-27
lines changed

‎clippy_lints/src/ifs/branches_sharing_code.rs‎

Lines changed: 52 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use clippy_utils::{
99
use core::iter;
1010
use core::ops::ControlFlow;
1111
use rustc_errors::Applicability;
12-
use rustc_hir::{Block, Expr, ExprKind, HirId, HirIdSet, LetStmt, Node, Stmt, StmtKind, intravisit};
12+
use rustc_hir::{Block, Expr, ExprKind, HirId, HirIdSet, ItemKind, LetStmt, Node, Stmt, StmtKind, UseKind, intravisit};
1313
use rustc_lint::LateContext;
1414
use rustc_span::hygiene::walk_chain;
1515
use rustc_span::source_map::SourceMap;
@@ -108,6 +108,7 @@ struct BlockEq {
108108
/// The name and id of every local which can be moved at the beginning and the end.
109109
moved_locals: Vec<(HirId, Symbol)>,
110110
}
111+
111112
impl BlockEq {
112113
fn start_span(&self, b: &Block<'_>, sm: &SourceMap) -> Option<Span> {
113114
match &b.stmts[..self.start_end_eq] {
@@ -129,20 +130,32 @@ impl BlockEq {
129130
}
130131

131132
/// If the statement is a local, checks if the bound names match the expected list of names.
132-
fn eq_binding_names(s: &Stmt<'_>, names: &[(HirId, Symbol)]) -> bool {
133-
if let StmtKind::Let(l) = s.kind {
134-
let mut i = 0usize;
135-
let mut res = true;
136-
l.pat.each_binding_or_first(&mut |_, _, _, name| {
137-
if names.get(i).is_some_and(|&(_, n)| n == name.name) {
138-
i += 1;
139-
} else {
140-
res = false;
141-
}
142-
});
143-
res && i == names.len()
144-
} else {
145-
false
133+
fn eq_binding_names(cx: &LateContext<'_>, s: &Stmt<'_>, names: &[(HirId, Symbol)]) -> bool {
134+
match s.kind {
135+
StmtKind::Let(l) => {
136+
let mut i = 0usize;
137+
let mut res = true;
138+
l.pat.each_binding_or_first(&mut |_, _, _, name| {
139+
if names.get(i).is_some_and(|&(_, n)| n == name.name) {
140+
i += 1;
141+
} else {
142+
res = false;
143+
}
144+
});
145+
res && i == names.len()
146+
},
147+
StmtKind::Item(item_id)
148+
if let item = cx.tcx.hir_item(item_id)
149+
&& let ItemKind::Static(_, ident, ..)
150+
| ItemKind::Const(ident, ..)
151+
| ItemKind::Fn { ident, .. }
152+
| ItemKind::TyAlias(ident, ..)
153+
| ItemKind::Use(_, UseKind::Single(ident))
154+
| ItemKind::Mod(ident, _) = item.kind =>
155+
{
156+
names.last().is_some_and(|&(_, n)| n == ident.name)
157+
},
158+
_ => false,
146159
}
147160
}
148161

@@ -164,6 +177,7 @@ fn modifies_any_local<'tcx>(cx: &LateContext<'tcx>, s: &'tcx Stmt<'_>, locals: &
164177
/// Checks if the given statement should be considered equal to the statement in the same
165178
/// position for each block.
166179
fn eq_stmts(
180+
cx: &LateContext<'_>,
167181
stmt: &Stmt<'_>,
168182
blocks: &[&Block<'_>],
169183
get_stmt: impl for<'a> Fn(&'a Block<'a>) -> Option<&'a Stmt<'a>>,
@@ -178,7 +192,7 @@ fn eq_stmts(
178192
let new_bindings = &moved_bindings[old_count..];
179193
blocks
180194
.iter()
181-
.all(|b| get_stmt(b).is_some_and(|s| eq_binding_names(s, new_bindings)))
195+
.all(|b| get_stmt(b).is_some_and(|s| eq_binding_names(cx, s, new_bindings)))
182196
} else {
183197
true
184198
}) && blocks.iter().all(|b| get_stmt(b).is_some_and(|s| eq.eq_stmt(s, stmt)))
@@ -218,7 +232,7 @@ fn scan_block_for_eq<'tcx>(
218232
return true;
219233
}
220234
modifies_any_local(cx, stmt, &cond_locals)
221-
|| !eq_stmts(stmt, blocks, |b| b.stmts.get(i), &mut eq, &mut moved_locals)
235+
|| !eq_stmts(cx, stmt, blocks, |b| b.stmts.get(i), &mut eq, &mut moved_locals)
222236
})
223237
.map_or(block.stmts.len(), |(i, stmt)| {
224238
adjust_by_closest_callsite(i, stmt, block.stmts[..i].iter().enumerate().rev())
@@ -279,6 +293,7 @@ fn scan_block_for_eq<'tcx>(
279293
}))
280294
.fold(end_search_start, |init, (stmt, offset)| {
281295
if eq_stmts(
296+
cx,
282297
stmt,
283298
blocks,
284299
|b| b.stmts.get(b.stmts.len() - offset),
@@ -290,11 +305,26 @@ fn scan_block_for_eq<'tcx>(
290305
// Clear out all locals seen at the end so far. None of them can be moved.
291306
let stmts = &blocks[0].stmts;
292307
for stmt in &stmts[stmts.len() - init..=stmts.len() - offset] {
293-
if let StmtKind::Let(l) = stmt.kind {
294-
l.pat.each_binding_or_first(&mut |_, id, _, _| {
295-
// FIXME(rust/#120456) - is `swap_remove` correct?
296-
eq.locals.swap_remove(&id);
297-
});
308+
match stmt.kind {
309+
StmtKind::Let(l) => {
310+
l.pat.each_binding_or_first(&mut |_, id, _, _| {
311+
// FIXME(rust/#120456) - is `swap_remove` correct?
312+
eq.locals.swap_remove(&id);
313+
});
314+
},
315+
StmtKind::Item(item_id) => {
316+
let item = cx.tcx.hir_item(item_id);
317+
if let ItemKind::Static(..)
318+
| ItemKind::Const(..)
319+
| ItemKind::Fn { .. }
320+
| ItemKind::TyAlias(..)
321+
| ItemKind::Use(..)
322+
| ItemKind::Mod(..) = item.kind
323+
{
324+
eq.local_items.swap_remove(&item.owner_id.to_def_id());
325+
}
326+
},
327+
_ => {},
298328
}
299329
}
300330
moved_locals.truncate(moved_locals_at_start);

‎clippy_utils/src/hir_utils.rs‎

Lines changed: 157 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, FnRetTy, FnSig, GenericArg, GenericArgs, GenericParam, GenericParamKind, GenericParamSource,
14+
Generics, HirId, HirIdMap, InlineAsmOperand, ItemId, ItemKind, LetExpr, Lifetime, LifetimeKind, LifetimeParamKind,
15+
Node, ParamName, Pat, PatExpr, PatExprKind, PatField, PatKind, Path, PathSegment, PrimTy, QPath, Stmt, StmtKind,
16+
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,144 @@ 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) | (UseKind::ListStem, UseKind::ListStem) => true,
226+
_ => false,
227+
}
228+
},
229+
(ItemKind::Mod(l_ident, l_mod), ItemKind::Mod(r_ident, r_mod)) => {
230+
l_ident.name == r_ident.name && over(l_mod.item_ids, r_mod.item_ids, |l, r| self.eq_item(*l, *r))
231+
},
232+
_ => false,
233+
};
234+
if eq {
235+
self.local_items.insert(l.owner_id.to_def_id(), r.owner_id.to_def_id());
236+
}
237+
eq
238+
}
239+
240+
fn eq_fn_sig(&mut self, left: &FnSig<'_>, right: &FnSig<'_>) -> bool {
241+
left.header.safety == right.header.safety
242+
&& left.header.constness == right.header.constness
243+
&& left.header.asyncness == right.header.asyncness
244+
&& left.header.abi == right.header.abi
245+
&& self.eq_fn_decl(left.decl, right.decl)
246+
}
247+
248+
fn eq_fn_decl(&mut self, left: &FnDecl<'_>, right: &FnDecl<'_>) -> bool {
249+
over(left.inputs, right.inputs, |l, r| self.eq_ty(l, r))
250+
&& (match (left.output, right.output) {
251+
(FnRetTy::DefaultReturn(_), FnRetTy::DefaultReturn(_)) => true,
252+
(FnRetTy::Return(l_ty), FnRetTy::Return(r_ty)) => self.eq_ty(l_ty, r_ty),
253+
_ => false,
254+
})
255+
&& left.c_variadic == right.c_variadic
256+
&& left.implicit_self == right.implicit_self
257+
&& left.lifetime_elision_allowed == right.lifetime_elision_allowed
258+
}
259+
260+
fn eq_generics(&mut self, left: &Generics<'_>, right: &Generics<'_>) -> bool {
261+
self.eq_generics_param(left.params, right.params)
262+
}
263+
264+
fn eq_generics_param(&mut self, left: &[GenericParam<'_>], right: &[GenericParam<'_>]) -> bool {
265+
over(left, right, |l, r| {
266+
(match (l.name, r.name) {
267+
(ParamName::Plain(l_ident), ParamName::Plain(r_ident))
268+
| (ParamName::Error(l_ident), ParamName::Error(r_ident)) => l_ident.name == r_ident.name,
269+
(ParamName::Fresh, ParamName::Fresh) => true,
270+
_ => false,
271+
}) && l.pure_wrt_drop == r.pure_wrt_drop
272+
&& self.eq_generics_param_kind(&l.kind, &r.kind)
273+
&& (matches!(
274+
(l.source, r.source),
275+
(GenericParamSource::Generics, GenericParamSource::Generics)
276+
| (GenericParamSource::Binder, GenericParamSource::Binder)
277+
))
278+
})
279+
}
280+
281+
fn eq_generics_param_kind(&mut self, left: &GenericParamKind<'_>, right: &GenericParamKind<'_>) -> bool {
282+
match (left, right) {
283+
(GenericParamKind::Lifetime { kind: l_kind }, GenericParamKind::Lifetime { kind: r_kind }) => {
284+
match (l_kind, r_kind) {
285+
(LifetimeParamKind::Explicit, LifetimeParamKind::Explicit)
286+
| (LifetimeParamKind::Error, LifetimeParamKind::Error) => true,
287+
(LifetimeParamKind::Elided(l_lifetime_kind), LifetimeParamKind::Elided(r_lifetime_kind)) => {
288+
l_lifetime_kind == r_lifetime_kind
289+
},
290+
_ => false,
291+
}
292+
},
293+
(
294+
GenericParamKind::Type {
295+
default: l_default,
296+
synthetic: l_synthetic,
297+
},
298+
GenericParamKind::Type {
299+
default: r_default,
300+
synthetic: r_synthetic,
301+
},
302+
) => both(*l_default, *r_default, |l, r| self.eq_ty(l, r)) && l_synthetic == r_synthetic,
303+
(
304+
GenericParamKind::Const {
305+
ty: l_ty,
306+
default: l_default,
307+
},
308+
GenericParamKind::Const {
309+
ty: r_ty,
310+
default: r_default,
311+
},
312+
) => self.eq_ty(l_ty, r_ty) && both(*l_default, *r_default, |l, r| self.eq_const_arg(l, r)),
171313
_ => false,
172314
}
173315
}
@@ -564,6 +706,17 @@ impl HirEqInterExpr<'_, '_, '_> {
564706
match (left.res, right.res) {
565707
(Res::Local(l), Res::Local(r)) => l == r || self.locals.get(&l) == Some(&r),
566708
(Res::Local(_), _) | (_, Res::Local(_)) => false,
709+
(Res::Def(l_kind, l), Res::Def(r_kind, r))
710+
if l_kind == r_kind
711+
&& let DefKind::Const
712+
| DefKind::Static { .. }
713+
| DefKind::Fn
714+
| DefKind::TyAlias
715+
| DefKind::Use
716+
| DefKind::Mod = l_kind =>
717+
{
718+
(l == r || self.local_items.get(&l) == Some(&r)) && self.eq_path_segments(left.segments, right.segments)
719+
},
567720
_ => self.eq_path_segments(left.segments, right.segments),
568721
}
569722
}

‎tests/ui/branches_sharing_code/shared_at_bottom.rs‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,3 +300,55 @@ fn issue15004() {
300300
//~^ branches_sharing_code
301301
};
302302
}
303+
304+
pub fn issue15347<T>() -> isize {
305+
if false {
306+
static A: isize = 4;
307+
return A;
308+
} else {
309+
static A: isize = 5;
310+
return A;
311+
}
312+
313+
if false {
314+
//~^ branches_sharing_code
315+
type ISize = isize;
316+
return ISize::MAX;
317+
} else {
318+
type ISize = isize;
319+
return ISize::MAX;
320+
}
321+
322+
if false {
323+
//~^ branches_sharing_code
324+
fn foo() -> isize {
325+
4
326+
}
327+
return foo();
328+
} else {
329+
fn foo() -> isize {
330+
4
331+
}
332+
return foo();
333+
}
334+
335+
if false {
336+
//~^ branches_sharing_code
337+
use std::num::NonZeroIsize;
338+
return NonZeroIsize::new(4).unwrap().get();
339+
} else {
340+
use std::num::NonZeroIsize;
341+
return NonZeroIsize::new(4).unwrap().get();
342+
}
343+
344+
if false {
345+
//~^ branches_sharing_code
346+
const B: isize = 5;
347+
return B;
348+
} else {
349+
const B: isize = 5;
350+
return B;
351+
}
352+
353+
todo!()
354+
}

0 commit comments

Comments
 (0)