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

[asomers]

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]

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]

+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]

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]

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

[jyn514]

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]

@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]

@rustbot label +L-suggestion-causes-error

[ghost]

@rustbot claim

[sendittothenewts]

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]

@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]

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]

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]

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.