rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.43k stars 1.54k forks source link

range_plus_one lint wrongly suggests using RangeInclusive when Range is required and the other way around #3307

Open asomers opened 6 years ago

asomers commented 6 years ago

The range_plus_one lint suggests replacing Range syntax with RangeInclusive syntax. That's fine if you're immediately going to loop. But it's invalid if the Range in question is going to be assigned to a variable of type Range. For example, clippy will suggest replacing this code:

struct Foo {
    r: Range<u32>
}

fn bar() {
    let x = 0;
    let foo = Foo{r: x..x+1};
}

with this:

struct Foo {
    r: Range<u32>
}

fn bar() {
    let x = 0;
    let foo = Foo{r: x..=x};
}

But that fails to compile

2263 |     let foo = Foo{r: x..=x};
     |                      ^^^^^ expected struct `std::ops::Range`, found struct `std::ops::RangeInclusive`                                                                
     |
     = note: expected type `std::ops::Range<u32>`
                found type `std::ops::RangeInclusive<{integer}>`
clippy 0.0.212 (32b1d1fc 2018-10-05)
jonasbb commented 5 years ago

Another example using rayon:

The following program compiles, but clippy produces a warning on it:

extern crate rayon;
use rayon::prelude::*;
fn main() {
    (0..1 + 1).into_par_iter();
}

The warning is:

warning: an inclusive range would be more readable
 --> src/main.rs:4:5
  |
4 |     (0..1 + 1).into_par_iter();
  |     ^^^^^^^^^^ help: use: `(0..=1)`
  |
  = note: #[warn(clippy::range_plus_one)] on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#range_plus_one

However, applying the suggestion results in the following error:

error[E0599]: no method named `into_par_iter` found for type `std::ops::RangeInclusive<{integer}>` in the current scope
 --> src/main.rs:4:13
  |
4 |     (0..=1).into_par_iter();
  |             ^^^^^^^^^^^^^
  |
  = note: the method `into_par_iter` exists but the following trait bounds were not satisfied:
          `std::ops::RangeInclusive<{integer}> : rayon::iter::IntoParallelIterator`
          `&std::ops::RangeInclusive<{integer}> : rayon::iter::IntoParallelIterator`
          `&mut std::ops::RangeInclusive<{integer}> : rayon::iter::IntoParallelIterator`

See also the rayon documentation for ranges.

oxalica commented 5 years ago

+1 for this issue.

I also met it when calling a function requiring Range. Example:

fn foo(_: std::ops::Range<i32>) {}

fn main() {
    let x = 1;
    foo(x..(x + 1));
}

I don't think implementing a generic function for all ranges would be always better.

flip1995 commented 4 years ago

Citing #4898 to keep traack of this in a single issue:

warning: an exclusive range would be more readable
  --> src/main.rs:55:21
   |
55 |         init_range: 0..=(sidx_offset - 1),
   |                     ^^^^^^^^^^^^^^^^^^^^^ help: use: `0..sidx_offset`
   |
   = note: `#[warn(clippy::range_minus_one)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#range_minus_one

However, init_range in the struct in question, is defined as an InclusiveRange. (By design, matching HTTP-Range).

So the same problem exists the other way around.

kvark commented 4 years ago

We hit this in gfx-rs since the code expects Range specifically and fails upon turning the expression into RangeInclusive.

jyn514 commented 4 years ago

This also happens when returning a value from a function.

pub struct Span {
    offset: usize,
    length: usize,
}

impl From<Span> for RangeInclusive<usize> {
    fn from(span: Span) -> Self {
        // clippy wants me to put span.offset..(span.offset + span.length) here
        span.offset..=(span.offset + span.length - 1)
    }
}
krishna-veerareddy commented 4 years ago

@flip1995 Hey I would like to tackle this one but I am not sure how we would know if the type of an expression can be changed or not. Any suggestions?

ThibsG commented 3 years ago

@rustbot label +L-suggestion-causes-error

ghost commented 2 years ago

@rustbot claim

sendittothenewts commented 5 months ago

I've just tripped this lint in some code that uses the .end() method, so applying the suggestion would've introduced an off-by-one bug that would not have been caught by the compiler, unlike the examples mentioned above. That strikes me as pretty pernicious even for a pedantic allow-by-default lint.

Of course, some of the blame for that could probably be directed at Range and RangeInclusive reusing the same .end() name for two subtly different semantics, but that's another matter.

flip1995 commented 5 months ago

@sendittothenewts Can you post a small code snippet where you ran into this issue? Doesn't have to be the original code, just a snippet that illustrates the problem.

sendittothenewts commented 5 months ago

Ah yes, since end in only a method on RangeInclusive, and a field on the other Range types, there actually would be a compile failure in the specific case I mentioned, and you'd have to apply the compiler's next suggestion to remove the method call brackets before the off-by-one bug appears.

However, the following slight variation does exhibit changed run-time behaviour without any more warnings:

fn main() {
    let names = ["fred", "barney", "wilma"];
    let bound = ..=names.len() - 1; // clippy suggests `..names.len()` instead

    println!("{}", bound.end); // Would print "3" instead of "2"
    println!("{}", names[bound.end - 1]); // Would print "wilma" instead of "barney"
    println!("{}", names[bound.end]); // Would panic instead of printing "wilma"
}
flip1995 commented 5 months ago

I see, so the main problem is with RangeToInclusive (note the To). Maybe the lint should at least emit a note pointing out this potential problem. The applicability of this lint should definitely be MaybeIncorrect.

sendittothenewts commented 5 months ago

Yes, that's right. Without the To, the problem is still there, but at least there happens to be another compiler error to bring it to your attention.