Add needless_type_cast lint
#16139
Add needless_type_cast lint
#16139
Conversation
rustbot
added
needs-fcp
S-waiting-on-review
|
No changes for e40774c |
|
Looking at Lintcheck, at least the Judging by the comment in I don't really know how one would go on about recognizing such FPs, so maybe this lint would just need to be |
|
r? clippy |
| let_expr: &rustc_hir::LetExpr<'a>, | ||
| bindings: &mut FxHashMap<HirId, BindingInfo<'a>>, | ||
| ) { | ||
| if let_expr.ty.is_none() { |
There was a problem hiding this comment.
This lint should have a from_expansion check to avoid linting code in macros.
Yes, it's possible that there may be wins, but often macros have been written a certain way for a reason, and this might reduce the noise the lint makes.
There was a problem hiding this comment.
cd30783 added checks to skip:
- macro-expanded bindings, usages, and casts
- casts inside generic function/method calls
- bindings initialized from functions with generic return types
- generic constructors (e.g., Some(a as i32))
Also, track usages inside closures, handle nested blocks, expand test coverage.
| if usages.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| if !usages.iter().all(|u| u.is_cast) { | ||
| return; | ||
| } | ||
|
|
||
| let Some(first_target) = usages.first().and_then(|u| u.cast_to) else { | ||
| return; | ||
| }; | ||
|
|
||
| if !usages.iter().all(|u| u.cast_to == Some(first_target)) { | ||
| return; | ||
| } | ||
|
|
||
| if first_target == binding_info.source_ty { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
All of those checks could be one if-let chain.
tests/ui/needless_type_cast.fixed
Outdated
| #![allow(clippy::no_effect, clippy::unnecessary_cast, unused)] | ||
|
|
||
| fn main() { | ||
| // Should lint: let binding always cast to i32 |
There was a problem hiding this comment.
Those tests only cover local variables and a few binary ops. What about function and method calls? Especially if those functions might be generic, and doubly especially if they have a generic return type, like .sum().
Yeah, I'd assume in a case like this: let a: u8 = some_iterator.next().unwrap();
let _ = a as i32;Here, |
|
this still appears to be a false positive: The transmute has no type annotations, so the return type is inferred from the outside. Changing that would change behavior and potentially cause undefined behavior. So apparently your check for generic return types misses this function. One thing we may want to do (in addition to fixing the generic return type check) is to omit the lint if there is any |
I guess since the actual issue is about generic return types being hidden inside blocks and not about unsafe itself, omitting the lint if there is any |
|
In that case, at least the check for generics should peel blocks (even unsafe ones). I still don't think that we should lint this in unsafe code at all. The risk of getting something wrong leading to catastrophic results just isn't worth the benefit of code simplification. |
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
5 participants
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 #16137
changelog: [
needless_type_cast]: suggests defining bindings as their cast type