Implement suggestion when array of 1 range used as argument of type Range

[IoaNNUwU]

Fixes: #147944

fn main() {
    let mut vec = vec![1, 2, 3, 4, 5];
    vec.drain([..3]);
}

Using array of 1 range instead of range itself as argument of impl RangeBounds now additionally produces this suggestion:

help: consider removing `[]`
   |
LL -     vec.drain([..3]);
LL +     vec.drain(..3);
   |

[rustbot]

r? @madsmtm

rustbot has assigned @madsmtm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[madsmtm]

This seems like a mistake that a new user could reasonably hit if they haven't internalized that item[..x] means Index[Mut] with Range parameter, and think that the ..x syntax must be wrapped in [].

So +1 on adding the diagnostic. Have a few comments on the implementation, see below.

View changes since this review

[rustbot]

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.

[IoaNNUwU]

I used let pattern chain instead of early returns similarly how other functions in this file do this to make it even less verbose.

I edited the tests to include this example:

let range = [1..5];
let mut vec = vec![1, 2, 3, 4, 5];
vec.drain(range);

And I have a question about this. Should compiler give an error in this case or is it outside of a scope for such suggestion?

Right now suggest_remove_brackets_from_range only works for inline ranges - for that it checks if snippet starts and ends with [, ]. It this the right solution?

I also wonder if something like this should be a part of this PR:

let range_wrong: Range<usize> = [1..5];
//               ^^^^^^^^^^^^   ^^^^^^ array [Range<usize>; 1]
//               struct Range<usize>

[IoaNNUwU]

@rustbot label -S-waiting-on-author +S-waiting-on-review

[madsmtm]

Sorry for the delay.

I used let pattern chain instead of early returns similarly how other functions in this file do this to make it even less verbose.

Thanks, I like it so far!

I answered the rest of your questions in inline comments.

View changes since this review

[madsmtm]

I edited the tests to include this example:

let range = [1..5];
let mut vec = vec![1, 2, 3, 4, 5];
vec.drain(range);

And I have a question about this. Should compiler give an error in this case or is it outside of a scope for such suggestion?

I think it's fine to leave this as future work, maybe with the note like this instead:

Suggested change

	// Should not produce this suggestion:
	let range = [1..5];
	let mut vec = vec![1, 2, 3, 4, 5];
	vec.drain(range);
	//~^ ERROR: the trait bound `[std::ops::Range<{integer}>; 1]: RangeBounds<usize>` is not satisfied [E0277]
	// Does not produce this suggestion (but could in the future):
	let range = [1..5];
	let mut vec = vec![1, 2, 3, 4, 5];
	vec.drain(range);
	//~^ ERROR: the trait bound `[std::ops::Range<{integer}>; 1]: RangeBounds<usize>` is not satisfied [E0277]
	// Should not produce this suggestion:

[madsmtm]

I also wonder if something like this should be a part of this PR:

let range_wrong: Range<usize> = [1..5];
//               ^^^^^^^^^^^^   ^^^^^^ array [Range<usize>; 1]
//               struct Range<usize>

I'm fine with it not being so, though maybe you could add it as a test as well?

[madsmtm]

Right now suggest_remove_brackets_from_range only works for inline ranges - for that it checks if snippet starts and ends with [, ]. It this the right solution?

Well, it does make it considerably easier to construct the suggestion, and there is precedence for it in suggest_block_to_brackets, so I think it's fine.

[madsmtm]

I just realized, we already have something that describes whether a type is a valid range: the RangeBounds trait.

So maybe we can use something like this instead:

Suggested change

	&& let ty::Adt(adt_def, _) = inner_type.kind()
	&& self.tcx.is_range(adt_def.did())
	// NOTE: Assumes that arrays do not implement `RangeBounds`.
	&& let Some(range_bounds) = self.tcx.get_diagnostic_item(sym::RangeBounds)
	&& self.type_implements_trait(range_bounds, [inner_type], param_env).must_apply_modulo_regions()

And then we can get rid of the is_range function?

[madsmtm]

Could you add a test that calls a different method than Vec::drain?

And possibly also a test like:

let my_range = 1..3;
vec.drain([my_range]);