add new lint: for_unbounded_range

[Erk-]

View all comments

This adds a Clippy lint for the possible confusing behaviour pointed out in rust-lang/rust#150084 and rust-lang/rust#111514, and discussed in rust-lang/libs-team#304.

While no consensus have been reached in the API Change Proposal linked above I thought that it would still make sense to add a Clippy lint for the time being.

Open questions:

• Which group should it be in?
• Should it give warnings for AsciiChar, Ipv4Addr and Ipv6Addr? (Currently these will always panic when running over the max value)

changelog: new lint: [for_unbounded_range]

[rustbot]

r? @dswij

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

[ada4a]

LGTM:) just one thing

View changes since this review

[ada4a]

r? clippy

[samueltardieu]

Looking for break targeting the loop and for return might limit the number of false positives.

View changes since this review

[samueltardieu]

I think it would be more correct to say "will panick or wrap around", as it will panick using the dev profile.

In both cases, the suggestion changes the semantics and should be MaybeIncorrect.

[Erk-]

changed it to "for loops over unbounded ranges will wrap around and may panic, consider using start..=MAX instead"

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Erk-]

Looking for break targeting the loop and for return might limit the number of false positives.

I have implemented this using the LoopVisitor from the infinite loop checker.

I am not really sure I agree if it should be a thing as I personally think a loop over a unbounded integer/char range is always wrong, even if you break out of it at a later point.

[Erk-]

@rustbot ready

[Erk-]

@samueltardieu Do you have time to give it another look?

[samueltardieu]

Note that the following code will make an incorrect suggestion, as the size of std::ffi::c_long depends on the platform, and the actual primitive type will be suggested instead:

    for i in 0.. {
        //~^ for_unbounded_range
        do_something::<std::ffi::c_long>(i);
    }

but since this is MaybeIncorrect I'm fine with that.

View changes since this review

[rustbot]

This lint has been nominated for inclusion.

A FCP topic has been created on Zulip.

[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.

[Erk-]

@rustbot ready

[Erk-]

What would you say the type of i is?

Yeah that makes sense, it would not really be ergonomic to say that it was the unification of those two (which is probably not the fully correct terminology either), but I guess that does also mean that you currently could not write a (at least late) lint to guard against that behavior.

Almost ready!

I think that should be the rest, I rebased it as well to make sure everything was up to date.

[samueltardieu]

Can you please squash the commits together?

View changes since this review

[Erk-]

Can you please squash the commits together?

Done

Moot

[Erk-]

@rustbot ready

[github-actions]

Lintcheck changes for 4858a73

Lint	Added	Removed	Changed
clippy::multiple_crate_versions	0	0	1

This comment will be updated if you push new changes