Unused loop (break) name gives no compiler warning

[BartMassey]

Currently, code like this

'unused_name: for i in 0..2 {
    println!("{}", i);
}

is silently accepted. As with unused function parameters or variable declarations, this is usually indicative of a mistake. A warning should be issued unless the break name begins with _.

Do I need to treat this as a feature request and file an RFC? Please say "no" — the RFC process is heavyweight as hell. I think it's just a bug report 😄.

[abonander]

I can mentor this one, though I'll have to look at how the other unused-* lints work before I can provide instructions. Should be an easy task overall though.

Addendum: yeah, should be easyish. I'll bang out some mentoring instructions later today when I'm back at a computer if this hasn't been closed as needs-RFC. I think it's fine to implement as-is.

[abonander]

(READY) Mentoring Instructions

So all the lints that call out unused elements reside in src/librustc_lint/unused.rs. We're going to implement another in a very similar vein to UnusedImportBraces; early-pass lints are done when no type inference information is necessary, so they're good when you only really need to look at the source tree itself like we're going to do here.

This lint should be pretty easy, we'll start just like with UnusedImportBraces and call declare_lint! with the lint information:

declare_lint! {
    UNUSED_LOOP_LABEL,
    Warn,
    "warns on unused labels for loops",
}

#[derive(Copy, Clone)]
pub struct UnusedLoopLabel(Vec<Label>);

impl UnusedLoopLabel {
    // stateful lints are expected to have a `new()` constructor
    pub fn new() -> Self {
        UnusedLoopLabel(vec![])
    }
}

impl LintPass for UnusedLoopLabel {
    fn get_lints(&self) -> LintArray {
        lint_array!(UNUSED_LOOP_LABEL)
    }
}

And then we're going to implement EarlyLintPass, overriding the check_expr() and check_expr_post() methods:

impl EarlyLintPass for UnusedLoopLabel {
    fn check_expr(&mut self, _: &EarlyContext, expr: &ast::Expr) {
        // match `expr.kind`, looking for any of the loop kinds and pushing their `Label`s to `self.0`
        // also look for `Continue` or `Break`, and find/remove their labels from `self.0` in reverse order
        // (since loop labels can shadow each other we want to remove the innermost label as that's
        //  the one that'll be actually used)
        // use `label.ident.name` for comparison
    }
   
    // called after returning from recursing into nested expressions,
    //  this is where we emit the lint message
    fn check_expr_post(&mut self, ctxt: &EarlyContext, expr: &ast::Expr) {
        // look at loop kinds in `expr` again, if that loop's label is in `self.0` then emit a lint message like this:
        ctxt.span_lint(UNUSED_LOOP_LABEL, label.ident.span, "unused loop label");
        // (make sure to search in reverse order again, and also remove the label from the vec)
    }
}

Next, add to the following two macro invocations, where it fits alphabetically:

• add UnusedLoopLabel in this one: https://github.com/rust-lang/rust/blob/master/src/librustc_lint/lib.rs#L111
• and UNUSED_LOOP_LABEL in this one: https://github.com/rust-lang/rust/blob/master/src/librustc_lint/lib.rs#L161-L179

And then you'll implement a UI test by creating src/test/ui/lint/unused_loop_label.rs. You can copy the MIT license headers from another file in that folder. That test should cover every loop kind, so while, while let, for and loop. For every line where a warning is supposed to appear, add the following comment under it:

//~^ WARN unused loop label

Now, run x.py test ui --test-args <filename>. It'll give you an error about a missing file with the .stderr extension. Just copy the expected contents it gives you there (including the two trailing blank lines) and paste them into a new file with that name. Run the command again and it should pass. That's it!

[abonander]

@estebank A complicating issue came up. Loop labels don't have their own spans in the AST, so a complete implementation of this lint (for optimal diagnostics) would also add span info to ast::Label which would need to touch the parser, and that might be a bit dense for a beginner. I can write the instructions for it but it could be a bit involved.

Pfft, nevermind, idents have spans.

[kylestach]

I'd like to take this issue.

[BartMassey]

Speaking just for myself, @KyleStach1678, I'd be happy for you to take this. I was just about to fork and start on it, but honestly I don't really have the time to tackle it right now, so I'm glad to hear I might not have to. :-)

[BartMassey]

Thanks huge! That was hilariously fast turnaround. Super-impressive.

[BartMassey]

I'm confused, though. In the end, the default for unused labels looks like it was changed to allow? The whole point was to have it default to warn… Am I missing something?

[petrochenkov]

@BartMassey
See #50763 (comment).
Someone needs to send a PR making the lint warn-by-default and that PR will have to go through some collective approval process from the lang team and possibly crater run.

[BartMassey]

Ah, missed that bit, thanks. Who sends that PR?

[kylestach]

@BartMassey if no one was planning to, I could open that PR. Not sure if it would be more appropriate for someone on the lang team to open that up instead though?

[petrochenkov]

@KyleStach1678
Please do it, lang team people are mostly busy with their own stuff.

[BartMassey]

See Issue #66324 for an attempt to revive the change to have the lint warn by default.