`default_numeric_fallback` contains multiple false positives

astra90x commented 2 months ago

Summary

There are a number of cases where the lint default_numeric_fallback is emitted, but no numeric fallback actually occurs due to type inference. I want to use this lint on my projects, but it causes far too many false positives.

Currently, the lint is emitted whenever a numeric literal contains no type suffix, is of the type i32 or f64, and is not a part of a large set of hardcoded cases where type inferences in known to occur. I believe that due to the complexity of type inference in Rust, the lint should instead directly check if rustc is actually doing numeric fallback.

I would like to help work on a PR implementing these changes, but the private rustc crates currently don't seem to provide any sort of interface to check if numeric fallback occurred. I think the ideal implementation should hook into this function and I could think of a few approaches, but I'm not sure what's best. This would also require updating rustc and then clippy, and I'm unsure what the process for that is like.

Furthermore, in my opinion, looking at RFC0212 as mentioned in the lint description, there are still a few common cases where the type fallen back to does not matter at all, and I think it's worth whitelisting those specific cases.

(0..100).map(|_| {}): This include all cases where a range in used as an iterator, but all values out of the iterator is discarded. This is often used to iterate a set number of times, where the type doesn't matter.
number << 8: While fallback does occur here, << and >> are implemented on all integer types with identical behavior.
10 > 0 within a macro: I only include this because it was mentioned here. I have not yet ran into this case in my code.

Lint Name

default_numeric_fallback

Reproducer

I tried this code:

#![warn(clippy::default_numeric_fallback)]

fn id_f64(x: f64) -> f64 {
    x
}

struct A<T>(pub T);

fn main() {
    // inference due to binary operations
    let mut a: f64;
    a = 1.;
    a *= 2.;
    let b = 5i32;
    let _ = a > 2. || b == 3;
    let _ = 2. / 1f64;

    // inference due to if and match
    let x = 10f64;
    let _ = if true { x } else { 10. };
    let _ = match 3i32 {
        3 => x,
        _ => 10.,
    };
    let _ = matches!(5i32, 1..=10);

    // inference due to assignment
    let t = (4., 5);
    let _: (f64, i32) = t;
    let _: A<f64> = A(1.);

    // inference due to functions
    let z = 10.;
    let _ = id_f64(z) == 10.;
    let mut o = None::<f64>;
    let _ = &mut o;
    let _ = o.unwrap_or(1.);
    let mut v: Vec<i32> = vec![];
    let _ = &mut v;
    v.push(1);

    // inference due to array
    let a = [1., 2.];
    let _: f64 = *a.first().unwrap();

    // fallback doesn't have any real effect
    let _ = 1u32 << 8;
    for _ in 0..100 {}
}

I saw this happen: There is a warning on every single unsuffixed numeric literal.

I expected to see this happen: There should be no warning at all.

Version

rustc 1.81.0-nightly (6868c831a 2024-06-30)
binary: rustc
commit-hash: 6868c831a1eb45c5150ff623cef5e42a8b8946d0
commit-date: 2024-06-30
host: x86_64-unknown-linux-gnu
release: 1.81.0-nightly
LLVM version: 18.1.7

Additional Labels

No response

y21 commented 2 months ago

I also experimented with this idea a while ago but then got busy. The two options I was thinking of was adding some kind of Vec<Expr> to TypeckResults for integer literal exprs at which fallback occurred so in clippy we can just go through it and emit a lint without needing any inference logic, or just uplifting it to rustc (which I think makes more sense, because we wouldn't need to add unnecessary things to the compiler that are only ever used by clippy, but maybe it's fine).

astra90x commented 2 months ago

I think the simplest thing for now would be to add it onto TypeckResults and use clippy. AFAICT there's plenty of things inside rustc that's currently used only by clippy, and if we do ever uplift this into rustc (which I'm not sure how well it would fit), it might work pretty similarly anyway.

What I'm uncertain about though is what exactly to add onto TypeckResults. The way integer fallback works seems to be that during inference, it scans for all inferred numeric type "variables" which are unresolved, and at that point I don't see an easy way to find the span of the literal which created this type variable.

The two options which I can think of are to either walk the function body to scan for literals with types matching this type variable, or to track the span of these type variables like regular type variables. The first option doesn't seem clean to me, but the second option requires even more refactoring.

rust-lang / rust-clippy