never_loop does not understand break-with-label

[dhardy]

Lint name: never_loop

I tried this code:

            let response = 'outer: loop {
                for child in self.widgets.iter_mut() {
                    if id <= child.widget.id() {
                        let r = child.widget.send(mgr, id, child_event);
                        break 'outer (child.key.clone(), r);
                    }
                }
                debug_assert!(false, "SendEvent::send: bad WidgetId");
                return Response::Unhandled;
            };

(The code sample is from here.)

I expected to see this happen: no report. I consider this a valid pattern as an (ugly) stand-in until (if ever) Rust gets for-break-with-default.

Instead, this happened:

error: this loop never actually loops
   --> src/widget/view/list_view.rs:552:28

Meta

• cargo clippy -V: clippy 0.1.53 (53cb7b0 2021-06-17)
• rustc -Vv:

rustc 1.53.0 (53cb7b09b 2021-06-17)
binary: rustc
commit-hash: 53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b
commit-date: 2021-06-17
host: x86_64-unknown-linux-gnu
release: 1.53.0
LLVM version: 12.0.1

[Jarcho]

This particular case could be done with find:

let response = match self.widgets.iter_mut().find(|child| id <= child.widget.id()) {
    Some(child) => {
        let r = child.widget.send(mgr, id, child_event);
        (child.key.clone(), r)
    }
    None => {
        debug_assert!(false, "SendEvent::send: bad WidgetId");
        return Response::Unhandled;
    }
}

More generally something like this would work:

let mut iter = foo.iter();
let bar = loop {
    if let Some(bar) = iter.next() {
        if some_complex_thing() {
            break bar;
        }
    } else {
        // handle missing case here
    }
};

There's at least some debate as to which one is better in the general case, but if find works it should be used.

[dhardy]

This particular case could be done with find:

I believe so. Semantically however find conveys the idea of finding any matching candidate whereas for ... conveys do something in a certain order. For this code that order is important, and while the code is equivalent, the semantics are still reason enough to use find.

Which is quite off topic here, the point merely being justification for using for over find.

The second example is an alternative, though may require explicit drop(iter); after the last line. It is debatable whether it is better.

This whole conversation could be avoided if we had labelled blocks (let response = 'outer: { ... };). Perhaps until then the never_loop lint should be suppressed for labelled loops?

[camsteffen]

I consider this a valid pattern as an (ugly) stand-in until (if ever) Rust gets for-break-with-default.

I don't think Clippy should make exceptions for such cases. You admitted it is ugly and Clippy did its job by agreeing with you. Whether the code should be changed or #[allow]ed is a matter of personal preference.

[HKalbasi]

This is a workaround for RFC 2046 and I'm not aware for a better and less ugly alternative. Maybe we can detect if tool chain is nightly, suggest block labels and if it is stable, don't emit warning (just for labeled loops with labeled breaks)?

[dhardy]

@HKalbasi code can't use nightly features if (as usual) stable compiler support is desired. And there are reasons beyond novel features to use a nightly compiler, e.g. compile speed and proc-macro diagnostics. (So suggesting use of nightly features is a bad idea.)

[HKalbasi]

If it is not good we can disable this warning in nightly as well.

[oxalica]

I think we should allow label-break usages in replacement of label_break_value, since it's not yet stabilized. My use case is almost exact the same as the example of RFC 2046 lable_break_value. I think it's acceptable at least.

let some_value = 'got: loop {
    if let Some(...) = some_complex_condition() {
        break 'got some_value;
    }
    intermediate();
    if some_other_condition() {
        return some_value; // Yes, I returns. So it's not easy to extract this loop into a function.
    }
    intermediate();
    if let Ok(...) = some_extreme_condition() {
        break 'got some_value;
    }
    return otherwise;
};

[kartva]

Also ran into this while working on Rust Analyzer. Is there any progress on fixing this? I'd be happy to try my hand.

[xFrednet]

There isn't any progress AFAIK and nobody is assigned to the issue. So if you'd like to, you can claim it (@rustbot claim)

[kartva]

The progress on rust-lang/rust#48594 (comment) seems to be nearly finished, so I don't think working on temporarily allowing a hacky workaround around a feature that seems it's going to be stabilized very soon seems like a good idea. I would be happy to revisit this in case the stabilization is delayed though.

[tommythorn]

I literally just hit this [1] and was appalled that Clippy complained and didn't offer a workaround. The message "The loop is useless" is technically false as without the "loop" the code doesn't work. I'm glad to see a better solution is forthcoming, but regardless the Clippy message is wrong and unhelpful.
[1] https://users.rust-lang.org/t/breaking-out-of-for/79893/5

[traviscross]

The problem here is that this is a correctness lint. If we wanted to express that this pattern is simply ugly, then that would need to be a style lint (farcically, clippy-hates-break-with-label). Even after label-break-value is stabilized in Rust 1.65, this correctness lint will remain incorrect.

People who write #![allow(clippy::style)] should be able to ignore Clippy's opinion about stylistic matters while benefiting from its other useful lints.

[kartva]

Hmm, from what I understand it's labelled correctness as never looping in for loop (i.e. breaking out unconditionally) is far more likely to be an outright error (someone forgot to write the condition for breaking) instead of being a workaround for label-break-value not being stabilized yet,

Though now that it is stabilized, it might be a good idea to add a properly worded suggestion for converting to a labelled block instead.

[kartva]

This should now be fixed with #9837 and #9858.

[xFrednet]

Perfect. Thank you! Then I'll close this :)