Propose to exchange ranges only when it is safe to do so #14432

samueltardieu · 2025-03-18T13:15:21Z

To avoid false positives, the range_plus_one and range_minus_one lints will restrict themselves to situations where the iterator types can be easily switched from exclusive to inclusive or vice-versa. This includes situations where the range is used as an iterator, or is used for indexing.

On the other hand, assignments of the range to variables, including automatically typed ones or wildcards, will no longer trigger the lint. However, the cases where such an assignment would benefit from the lint are probably rare.

In a second commit, the range_plus_one and range_minus_one logic are unified, in order to properly emit parentheses around the suggestion when needed.

Fix #3307
Fix #9908

changelog: [range_plus_one, range_minus_one]: restrict lint to cases where it is safe to switch the range type

Edit: as a consequence, this led to the removal of three #[expect(clippy::range_plus_one)] in the Clippy sources to avoid those false positives.

rustbot · 2025-03-18T13:15:26Z

r? @llogiq

rustbot has assigned @llogiq.
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

samueltardieu · 2025-03-18T13:38:34Z

This could also be extended to situations where the ranges are used as an argument to a function/method call which accepts an impl or dyn trait implemented by all kind of ranges:

Letting this as a future enhancement removes current false positives.
Doing it in this PR limits the number of places where the lint triggered before but won't trigger now.

samueltardieu · 2025-03-31T13:39:31Z

Rebased on master

samueltardieu · 2025-03-31T14:43:23Z

Putting to draft, I want to double-check the lintcheck, it looks like we get some false negatives there.
@rustbot author

samueltardieu · 2025-03-31T16:41:02Z

Much better. I added checking if the range to be modified is a method or function call argument, and if the target range would be suitable as well.

@rustbot review

samueltardieu · 2025-04-13T15:51:12Z

r? clippy

samueltardieu · 2025-04-15T21:28:54Z

I added a second commit to fix an issue with parentheses lacking from some suggestions identified in a comment made tonight in #9908.

samueltardieu · 2025-05-16T19:14:27Z

r? clippy

samueltardieu · 2025-05-17T17:38:14Z

Rebased

Jarcho

Thank you for fixing this as it's quite annoying currently.

Jarcho · 2025-05-21T17:18:35Z

clippy_lints/src/ranges.rs

+    if let ExprKind::Index(_, index, _) = parent_expr.kind
+        && index.hir_id == expr.hir_id
+    {
+        return true;
    }
+
+    false
+}


This needs to check if Index<RangeType> is implemented for the indexed type.

Done, and I checked this for Index or IndexMut, depending of what we need.

clippy_lints/src/ranges.rs

Jarcho · 2025-05-21T19:15:26Z

clippy_lints/src/ranges.rs

+        let expr_ty = inputs[input_idx];
+        // Check that the `expr` type is present only one, otherwise modifying just one of them it might be
+        // risky if they are referenced using the same generic type for example.
+        if inputs.iter().enumerate().all(|(n, ty)| n == input_idx || *ty != expr_ty)


This will need a TypeVisitor over all other input parameters. Foo<T> and such needs to be detected.

Done with ty.walk(), I don't think we need a visitor, if the range type doesn't appear within the generic arguments it doesn't matter if it is present, for example in a struct field of another argument, as they will not be used together (since we check that the range type is not referenced per-se but through constraints).

TypeVisitor actually visits the same things as Ty::walk does, but walk is the better thing here and I totally forgotten it existed.

clippy_lints/src/ranges.rs

samueltardieu · 2025-06-18T03:58:00Z

Ping @Jarcho

To limit false positives, the `range_plus_one` and `range_minus_one` lints will restrict themselves to situations where the iterator types can be easily switched from exclusive to inclusive or vice-versa. This includes situations where the range is used as an iterator, or is used for indexing. Also, when the range is used as a function or method call argument and the parameter type is generic over traits implemented by both kind of ranges, the lint will trigger. On the other hand, assignments of the range to variables, including automatically typed ones or wildcards, will no longer trigger the lint. However, the cases where such an assignment would benefit from the lint are probably rare. This fix doesn't prevent false positives from happening. A more complete fix would check if the obligations can be satisfied by the new proposed range type.

Those lints share the detection logic structure, but differed, probably because of an oversight, in lint emission: only one of them would take care of emitting parentheses around the suggestion if required. Factor the code into one function which checks for the right condition and emits the lint in an identical way.

samueltardieu · 2025-06-18T04:42:51Z

Rebased since GenericArg::unpack() was renamed to GenericArg::kind() in rustc.

Jarcho

Thank you.

rustbot assigned llogiq Mar 18, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 18, 2025

samueltardieu force-pushed the push-vzpslusykwsk branch from 977ab28 to 5560c51 Compare March 18, 2025 13:22

samueltardieu force-pushed the push-vzpslusykwsk branch from 5560c51 to 184736f Compare March 31, 2025 13:38

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 31, 2025

samueltardieu marked this pull request as draft March 31, 2025 14:43

samueltardieu force-pushed the push-vzpslusykwsk branch 2 times, most recently from db80969 to f532213 Compare March 31, 2025 16:40

samueltardieu marked this pull request as ready for review March 31, 2025 16:40

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 31, 2025

rustbot assigned blyxyas and unassigned llogiq Apr 13, 2025

samueltardieu mentioned this pull request Apr 15, 2025

range_minus_one changes range type and causes type error #9908

Closed

samueltardieu force-pushed the push-vzpslusykwsk branch from 443ee08 to 28b490f Compare April 15, 2025 21:39

rustbot assigned Jarcho and unassigned blyxyas May 16, 2025

This comment has been minimized.

Sign in to view

samueltardieu force-pushed the push-vzpslusykwsk branch from 28b490f to 3867236 Compare May 17, 2025 17:38

Jarcho requested changes May 21, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels May 21, 2025

samueltardieu force-pushed the push-vzpslusykwsk branch from 3867236 to 2044877 Compare May 23, 2025 11:08

samueltardieu requested a review from Jarcho May 23, 2025 11:11

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 23, 2025

samueltardieu force-pushed the push-vzpslusykwsk branch 4 times, most recently from 0a28296 to 4c23383 Compare May 26, 2025 12:07

samueltardieu added 2 commits June 18, 2025 06:41

samueltardieu force-pushed the push-vzpslusykwsk branch from 4c23383 to 15fc993 Compare June 18, 2025 04:41

Jarcho approved these changes Jul 16, 2025

View reviewed changes

Jarcho added this pull request to the merge queue Jul 16, 2025

Merged via the queue into rust-lang:master with commit e113e66 Jul 16, 2025
11 checks passed

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 16, 2025

samueltardieu deleted the push-vzpslusykwsk branch July 16, 2025 17:05

Propose to exchange ranges only when it is safe to do so #14432

Propose to exchange ranges only when it is safe to do so #14432

Uh oh!

Conversation

samueltardieu commented Mar 18, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Mar 18, 2025

Uh oh!

samueltardieu commented Mar 18, 2025

Uh oh!

samueltardieu commented Mar 31, 2025

Uh oh!

samueltardieu commented Mar 31, 2025

Uh oh!

samueltardieu commented Mar 31, 2025

Uh oh!

samueltardieu commented Apr 13, 2025

Uh oh!

samueltardieu commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samueltardieu commented May 16, 2025

Uh oh!

This comment has been minimized.

samueltardieu commented May 17, 2025

Uh oh!

Jarcho left a comment

Choose a reason for hiding this comment

Uh oh!

Jarcho May 21, 2025

Choose a reason for hiding this comment

Uh oh!

samueltardieu May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jarcho May 21, 2025

Choose a reason for hiding this comment

Uh oh!

samueltardieu May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Jarcho May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samueltardieu commented Jun 18, 2025

Uh oh!

samueltardieu commented Jun 18, 2025

Uh oh!

Jarcho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

samueltardieu commented Mar 18, 2025 •

edited by rustbot

Loading

samueltardieu commented Apr 15, 2025 •

edited

Loading