Skip to content

Commit 8293d5e

Browse files
authored
Add needless_type_cast lint (#16139)
Clippy should warn when a const, static, or let binding is defined as one integer type but immediately/ consistently cast to another type at usage sites without any usage of the original type. fixes rust-lang/rust-clippy#16137 changelog: [`needless_type_cast `]: suggests defining bindings as their cast type
2 parents 0cf51b1 + 1232c81 commit 8293d5e

File tree

9 files changed

+760
-2
lines changed

9 files changed

+760
-2
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6688,6 +6688,7 @@ Released 2018-09-13
66886688
[`needless_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
66896689
[`needless_return_with_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return_with_question_mark
66906690
[`needless_splitn`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_splitn
6691+
[`needless_type_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_type_cast
66916692
[`needless_update`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_update
66926693
[`neg_cmp_op_on_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#neg_cmp_op_on_partial_ord
66936694
[`neg_multiply`]: https://rust-lang.github.io/rust-clippy/master/index.html#neg_multiply

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
66

7-
[There are over 750 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
7+
[There are over 800 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
88

99
Lints are divided into categories, each with a default [lint level](https://doc.rust-lang.org/rustc/lints/levels.html).
1010
You can choose how much Clippy is supposed to ~~annoy~~ help you by changing the lint level by category.

book/src/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
A collection of lints to catch common mistakes and improve your
66
[Rust](https://github.com/rust-lang/rust) code.
77

8-
[There are over 750 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
8+
[There are over 800 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
99

1010
Lints are divided into categories, each with a default [lint
1111
level](https://doc.rust-lang.org/rustc/lints/levels.html). You can choose how

clippy_lints/src/casts/mod.rs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ mod fn_to_numeric_cast;
1919
mod fn_to_numeric_cast_any;
2020
mod fn_to_numeric_cast_with_truncation;
2121
mod manual_dangling_ptr;
22+
mod needless_type_cast;
2223
mod ptr_as_ptr;
2324
mod ptr_cast_constness;
2425
mod ref_as_ptr;
@@ -813,6 +814,32 @@ declare_clippy_lint! {
813814
"casting a primitive method pointer to any integer type"
814815
}
815816

817+
declare_clippy_lint! {
818+
/// ### What it does
819+
/// Checks for bindings (constants, statics, or let bindings) that are defined
820+
/// with one numeric type but are consistently cast to a different type in all usages.
821+
///
822+
/// ### Why is this bad?
823+
/// If a binding is always cast to a different type when used, it would be clearer
824+
/// and more efficient to define it with the target type from the start.
825+
///
826+
/// ### Example
827+
/// ```no_run
828+
/// const SIZE: u16 = 15;
829+
/// let arr: [u8; SIZE as usize] = [0; SIZE as usize];
830+
/// ```
831+
///
832+
/// Use instead:
833+
/// ```no_run
834+
/// const SIZE: usize = 15;
835+
/// let arr: [u8; SIZE] = [0; SIZE];
836+
/// ```
837+
#[clippy::version = "1.93.0"]
838+
pub NEEDLESS_TYPE_CAST,
839+
pedantic,
840+
"binding defined with one type but always cast to another"
841+
}
842+
816843
pub struct Casts {
817844
msrv: Msrv,
818845
}
@@ -851,6 +878,7 @@ impl_lint_pass!(Casts => [
851878
AS_POINTER_UNDERSCORE,
852879
MANUAL_DANGLING_PTR,
853880
CONFUSING_METHOD_TO_NUMERIC_CAST,
881+
NEEDLESS_TYPE_CAST,
854882
]);
855883

856884
impl<'tcx> LateLintPass<'tcx> for Casts {
@@ -920,4 +948,8 @@ impl<'tcx> LateLintPass<'tcx> for Casts {
920948
cast_slice_different_sizes::check(cx, expr, self.msrv);
921949
ptr_cast_constness::check_null_ptr_cast_method(cx, expr);
922950
}
951+
952+
fn check_body(&mut self, cx: &LateContext<'tcx>, body: &rustc_hir::Body<'tcx>) {
953+
needless_type_cast::check(cx, body);
954+
}
923955
}
Lines changed: 289 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,289 @@
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::visitors::{Descend, for_each_expr, for_each_expr_without_closures};
3+
use core::ops::ControlFlow;
4+
use rustc_data_structures::fx::FxHashMap;
5+
use rustc_errors::Applicability;
6+
use rustc_hir::def::{DefKind, Res};
7+
use rustc_hir::{BlockCheckMode, Body, Expr, ExprKind, HirId, LetStmt, PatKind, StmtKind, UnsafeSource};
8+
use rustc_lint::LateContext;
9+
use rustc_middle::ty::{Ty, TypeVisitableExt};
10+
use rustc_span::Span;
11+
12+
use super::NEEDLESS_TYPE_CAST;
13+
14+
struct BindingInfo<'a> {
15+
source_ty: Ty<'a>,
16+
ty_span: Span,
17+
}
18+
19+
struct UsageInfo<'a> {
20+
cast_to: Option<Ty<'a>>,
21+
in_generic_context: bool,
22+
}
23+
24+
pub(super) fn check<'a>(cx: &LateContext<'a>, body: &Body<'a>) {
25+
let mut bindings: FxHashMap<HirId, BindingInfo<'a>> = FxHashMap::default();
26+
27+
for_each_expr_without_closures(body.value, |expr| {
28+
match expr.kind {
29+
ExprKind::Block(block, _) => {
30+
for stmt in block.stmts {
31+
if let StmtKind::Let(let_stmt) = stmt.kind {
32+
collect_binding_from_local(cx, let_stmt, &mut bindings);
33+
}
34+
}
35+
},
36+
ExprKind::Let(let_expr) => {
37+
collect_binding_from_let(cx, let_expr, &mut bindings);
38+
},
39+
_ => {},
40+
}
41+
ControlFlow::<()>::Continue(())
42+
});
43+
44+
#[allow(rustc::potential_query_instability)]
45+
let mut binding_vec: Vec<_> = bindings.into_iter().collect();
46+
binding_vec.sort_by_key(|(_, info)| info.ty_span.lo());
47+
48+
for (hir_id, binding_info) in binding_vec {
49+
check_binding_usages(cx, body, hir_id, &binding_info);
50+
}
51+
}
52+
53+
fn collect_binding_from_let<'a>(
54+
cx: &LateContext<'a>,
55+
let_expr: &rustc_hir::LetExpr<'a>,
56+
bindings: &mut FxHashMap<HirId, BindingInfo<'a>>,
57+
) {
58+
if let_expr.ty.is_none()
59+
|| let_expr.span.from_expansion()
60+
|| has_generic_return_type(cx, let_expr.init)
61+
|| contains_unsafe(let_expr.init)
62+
{
63+
return;
64+
}
65+
66+
if let PatKind::Binding(_, hir_id, _, _) = let_expr.pat.kind
67+
&& let Some(ty_hir) = let_expr.ty
68+
{
69+
let ty = cx.typeck_results().pat_ty(let_expr.pat);
70+
if ty.is_numeric() {
71+
bindings.insert(
72+
hir_id,
73+
BindingInfo {
74+
source_ty: ty,
75+
ty_span: ty_hir.span,
76+
},
77+
);
78+
}
79+
}
80+
}
81+
82+
fn collect_binding_from_local<'a>(
83+
cx: &LateContext<'a>,
84+
let_stmt: &LetStmt<'a>,
85+
bindings: &mut FxHashMap<HirId, BindingInfo<'a>>,
86+
) {
87+
if let_stmt.ty.is_none()
88+
|| let_stmt.span.from_expansion()
89+
|| let_stmt
90+
.init
91+
.is_some_and(|init| has_generic_return_type(cx, init) || contains_unsafe(init))
92+
{
93+
return;
94+
}
95+
96+
if let PatKind::Binding(_, hir_id, _, _) = let_stmt.pat.kind
97+
&& let Some(ty_hir) = let_stmt.ty
98+
{
99+
let ty = cx.typeck_results().pat_ty(let_stmt.pat);
100+
if ty.is_numeric() {
101+
bindings.insert(
102+
hir_id,
103+
BindingInfo {
104+
source_ty: ty,
105+
ty_span: ty_hir.span,
106+
},
107+
);
108+
}
109+
}
110+
}
111+
112+
fn contains_unsafe(expr: &Expr<'_>) -> bool {
113+
for_each_expr_without_closures(expr, |e| {
114+
if let ExprKind::Block(block, _) = e.kind
115+
&& let BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) = block.rules
116+
{
117+
return ControlFlow::Break(());
118+
}
119+
ControlFlow::Continue(())
120+
})
121+
.is_some()
122+
}
123+
124+
fn has_generic_return_type(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
125+
match &expr.kind {
126+
ExprKind::Block(block, _) => {
127+
if let Some(tail_expr) = block.expr {
128+
return has_generic_return_type(cx, tail_expr);
129+
}
130+
false
131+
},
132+
ExprKind::If(_, then_block, else_expr) => {
133+
has_generic_return_type(cx, then_block) || else_expr.is_some_and(|e| has_generic_return_type(cx, e))
134+
},
135+
ExprKind::Match(_, arms, _) => arms.iter().any(|arm| has_generic_return_type(cx, arm.body)),
136+
ExprKind::Loop(block, label, ..) => for_each_expr_without_closures(*block, |e| {
137+
match e.kind {
138+
ExprKind::Loop(..) => {
139+
// Unlabeled breaks inside nested loops target the inner loop, not ours
140+
return ControlFlow::Continue(Descend::No);
141+
},
142+
ExprKind::Break(dest, Some(break_expr)) => {
143+
let targets_this_loop =
144+
dest.label.is_none() || dest.label.map(|l| l.ident) == label.map(|l| l.ident);
145+
if targets_this_loop && has_generic_return_type(cx, break_expr) {
146+
return ControlFlow::Break(());
147+
}
148+
},
149+
_ => {},
150+
}
151+
ControlFlow::Continue(Descend::Yes)
152+
})
153+
.is_some(),
154+
ExprKind::MethodCall(..) => {
155+
if let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) {
156+
let sig = cx.tcx.fn_sig(def_id).instantiate_identity();
157+
let ret_ty = sig.output().skip_binder();
158+
return ret_ty.has_param();
159+
}
160+
false
161+
},
162+
ExprKind::Call(callee, _) => {
163+
if let ExprKind::Path(qpath) = &callee.kind {
164+
let res = cx.qpath_res(qpath, callee.hir_id);
165+
if let Res::Def(DefKind::Fn | DefKind::AssocFn, def_id) = res {
166+
let sig = cx.tcx.fn_sig(def_id).instantiate_identity();
167+
let ret_ty = sig.output().skip_binder();
168+
return ret_ty.has_param();
169+
}
170+
}
171+
false
172+
},
173+
_ => false,
174+
}
175+
}
176+
177+
fn is_generic_res(cx: &LateContext<'_>, res: Res) -> bool {
178+
let has_type_params = |def_id| {
179+
cx.tcx
180+
.generics_of(def_id)
181+
.own_params
182+
.iter()
183+
.any(|p| p.kind.is_ty_or_const())
184+
};
185+
match res {
186+
Res::Def(DefKind::Fn | DefKind::AssocFn, def_id) => has_type_params(def_id),
187+
// Ctor → Variant → ADT: constructor's parent is variant, variant's parent is the ADT
188+
Res::Def(DefKind::Ctor(..), def_id) => has_type_params(cx.tcx.parent(cx.tcx.parent(def_id))),
189+
_ => false,
190+
}
191+
}
192+
193+
fn is_cast_in_generic_context<'a>(cx: &LateContext<'a>, cast_expr: &Expr<'a>) -> bool {
194+
let mut current_id = cast_expr.hir_id;
195+
196+
loop {
197+
let parent_id = cx.tcx.parent_hir_id(current_id);
198+
if parent_id == current_id {
199+
return false;
200+
}
201+
202+
let parent = cx.tcx.hir_node(parent_id);
203+
204+
match parent {
205+
rustc_hir::Node::Expr(parent_expr) => {
206+
match &parent_expr.kind {
207+
ExprKind::Closure(_) => return false,
208+
ExprKind::Call(callee, _) => {
209+
if let ExprKind::Path(qpath) = &callee.kind {
210+
let res = cx.qpath_res(qpath, callee.hir_id);
211+
if is_generic_res(cx, res) {
212+
return true;
213+
}
214+
}
215+
},
216+
ExprKind::MethodCall(..) => {
217+
if let Some(def_id) = cx.typeck_results().type_dependent_def_id(parent_expr.hir_id)
218+
&& cx
219+
.tcx
220+
.generics_of(def_id)
221+
.own_params
222+
.iter()
223+
.any(|p| p.kind.is_ty_or_const())
224+
{
225+
return true;
226+
}
227+
},
228+
_ => {},
229+
}
230+
current_id = parent_id;
231+
},
232+
_ => return false,
233+
}
234+
}
235+
}
236+
237+
fn check_binding_usages<'a>(cx: &LateContext<'a>, body: &Body<'a>, hir_id: HirId, binding_info: &BindingInfo<'a>) {
238+
let mut usages = Vec::new();
239+
240+
for_each_expr(cx, body.value, |expr| {
241+
if let ExprKind::Path(ref qpath) = expr.kind
242+
&& !expr.span.from_expansion()
243+
&& let Res::Local(id) = cx.qpath_res(qpath, expr.hir_id)
244+
&& id == hir_id
245+
{
246+
let parent_id = cx.tcx.parent_hir_id(expr.hir_id);
247+
let parent = cx.tcx.hir_node(parent_id);
248+
249+
let usage = if let rustc_hir::Node::Expr(parent_expr) = parent
250+
&& let ExprKind::Cast(..) = parent_expr.kind
251+
&& !parent_expr.span.from_expansion()
252+
{
253+
UsageInfo {
254+
cast_to: Some(cx.typeck_results().expr_ty(parent_expr)),
255+
in_generic_context: is_cast_in_generic_context(cx, parent_expr),
256+
}
257+
} else {
258+
UsageInfo {
259+
cast_to: None,
260+
in_generic_context: false,
261+
}
262+
};
263+
usages.push(usage);
264+
}
265+
ControlFlow::<()>::Continue(())
266+
});
267+
268+
let Some(first_target) = usages
269+
.first()
270+
.and_then(|u| u.cast_to)
271+
.filter(|&t| t != binding_info.source_ty)
272+
.filter(|&t| usages.iter().all(|u| u.cast_to == Some(t) && !u.in_generic_context))
273+
else {
274+
return;
275+
};
276+
277+
span_lint_and_sugg(
278+
cx,
279+
NEEDLESS_TYPE_CAST,
280+
binding_info.ty_span,
281+
format!(
282+
"this binding is defined as `{}` but is always cast to `{}`",
283+
binding_info.source_ty, first_target
284+
),
285+
"consider defining it as",
286+
first_target.to_string(),
287+
Applicability::MaybeIncorrect,
288+
);
289+
}

clippy_lints/src/declared_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
7070
crate::casts::FN_TO_NUMERIC_CAST_ANY_INFO,
7171
crate::casts::FN_TO_NUMERIC_CAST_WITH_TRUNCATION_INFO,
7272
crate::casts::MANUAL_DANGLING_PTR_INFO,
73+
crate::casts::NEEDLESS_TYPE_CAST_INFO,
7374
crate::casts::PTR_AS_PTR_INFO,
7475
crate::casts::PTR_CAST_CONSTNESS_INFO,
7576
crate::casts::REF_AS_PTR_INFO,

0 commit comments

Comments
 (0)