Add lint checks for unused loop labels #50763

kylestach · 2018-05-15T06:40:18Z

Previously no warning was generated when a loop label was written out but never used (i.e. in a break or continue statement):

fn main() {
    'unused_label: loop {}
}

would compile without complaint.

This fix introduces -W unused_loop_label, which generates the following warning message for the above snippet:

warning: unused loop label
 --> main.rs:2:5
  |
2 |     'unused_label: loop {}
  |     ^^^^^^^^^^^^^
  |
  = note: #[warn(unused_loop_label)] on by default

Fixes: #50751.

rust-highfive · 2018-05-15T06:40:23Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

abonander · 2018-05-15T07:14:00Z

src/librustc_lint/lib.rs

@@ -176,7 +177,8 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
                    UNUSED_DOC_COMMENT,
                    UNUSED_EXTERN_CRATES,
                    UNUSED_FEATURES,
-                    UNUSED_PARENS);
+                    UNUSED_PARENS,
+                    UNUSED_LOOP_LABEL);


This should be one line up so it's in alphabetical order.

abonander · 2018-05-15T07:14:41Z

src/librustc_lint/unused.rs

+            ast::ExprKind::While(_, _, Some(ref label))
+            | ast::ExprKind::WhileLet(_, _, _, Some(ref label))
+            | ast::ExprKind::ForLoop(_, _, _, Some(ref label))
+            | ast::ExprKind::Loop(_, Some(ref label)) => {


ref shouldn't be necessary on these lines since ast::Label is copyable.

abonander · 2018-05-15T07:18:10Z

src/librustc_lint/unused.rs

+            }
+            ast::ExprKind::Break(Some(ref label), _) | ast::ExprKind::Continue(Some(ref label)) => {
+                'remove_used_label: for i in (0..self.0.len()).rev() {
+                    if self.0.get(i).unwrap().ident.name == label.ident.name {


This should be doable with self.0.iter().enumerate().rev().find() but there's not much difference in clarity except most people prefer to see iterator chains.

abonander · 2018-05-15T07:24:22Z

src/librustc_lint/unused.rs

+            | ast::ExprKind::ForLoop(_, _, _, Some(ref label))
+            | ast::ExprKind::Loop(_, Some(ref label)) => if !self.0.is_empty() {
+                {
+                    let unused_label = self.0.last().unwrap();


I'd rather combine this and the !is_empty() check into if let Some(unused_label) = self.0.last()

In fact this should probably be self.0.pop() instead and push the label back if the equality check falls though.

est31 · 2018-05-15T08:58:46Z

Hmm I wonder about which more general name to choose as soon blocks can bear labels too: #50045

It'd be cool to have a more general name (unused_labels?) and error message etc from the start so that my PR doesn't have to change the name, only add capabilities to the lint.

est31 · 2018-05-15T08:59:27Z

src/librustc_lint/unused.rs

+}
+
+#[derive(Clone)]
+pub struct UnusedLoopLabel(pub vec::Vec<ast::Label>);


Vec is part of the prelude so you don't need vec::Vec here, Vec is enough. use std::vec; isn't needed either.

abonander · 2018-05-15T09:18:39Z

@est31 I considered including block labels in my mentoring instructions but they seem like a much more niche feature. I don't think I've seen them in the wild for quite a long time, what are they even useful for again? Can you use them to actually name a lifetime?

est31 · 2018-05-15T09:26:01Z

@abonander there is only a merged RFC for labeled blocks, but no implementation in the compiler. The PR to add compiler support is linked above. The point I made above was to forestall renames of the lint in the future if that PR merges. So it is entirely fine if this PR still keeps the loop in its name, I'll just rename it then... as long as no stable release shipped with the lint that should have no consequences.

petrochenkov · 2018-05-15T09:33:21Z

src/librustc_lint/unused.rs

+            }
+            ast::ExprKind::Break(Some(ref label), _) | ast::ExprKind::Continue(Some(ref label)) => {
+                'remove_used_label: for i in (0..self.0.len()).rev() {
+                    if self.0.get(i).unwrap().ident.name == label.ident.name {


Comparing names is not correct in general because macro hygiene applies to labels as well.
This lint should not attempt to repeat the name resolution pass (librustc_resolve/lib.rs), it should become a part of it - each label definition (fn with_resolved_label) should add that label's id into "unused" set and then each label use (fn search_label) should remove the definition it resolves to from the set.

petrochenkov · 2018-05-15T09:38:49Z

Hmm I wonder about which more general name to choose as soon blocks can bear labels too: #50045
It'd be cool to have a more general name (unused_labels?) and error message etc from the start so that my PR doesn't have to change the name, only add capabilities to the lint.

I agree that unused_loop_label is overly specific and doesn't fit the lint naming style, unused_labels is the way to go.

nagisa · 2018-05-15T21:42:58Z

I remember us being wary of adding new lints to the compiler because they are a breaking change. What makes this case different?

estebank · 2018-05-15T22:32:08Z

Please remove the file src/test/ui/lint/unused_loop_label.stdout, as it's empty.

abonander · 2018-05-15T22:33:08Z

@nagisa https://github.com/arcnmx/rust-rfcs/blob/master/text/1105-api-evolution.md#lints

Since lint #[deny] errors don't stop compilation of dependencies the only build breakage would occur for the local crate where the author can immediately address it.

petrochenkov · 2018-05-15T22:41:18Z

@estebank

Please remove the file src/test/ui/lint/unused_loop_label.stdout, as it's empty.

That would make a good tidy check: #50785

estebank

It looks good to me, one nitpick inline.

estebank · 2018-05-16T00:54:43Z

src/test/ui/lint/unused_label.rs

+    // unreachable_code errors everywhere else
+    'unused_loop_label: loop {
+        //~^ WARN unused label
+    }


Can you add a case for something along the lines of

fn main() { 'used_many: loop { if true { break 'used_many; } else { break 'used_many; } } }

The code as it is seems to be doing the right thing for this, but want to make sure we don't have a regression in the future.

estebank · 2018-05-16T00:56:14Z

src/librustc_lint/unused.rs

+            | ast::ExprKind::WhileLet(_, _, _, Some(label))
+            | ast::ExprKind::ForLoop(_, _, _, Some(label))
+            | ast::ExprKind::Loop(_, Some(label)) => {
+                self.0.push(label);


Can you add a comment along the lines of "adding all labels as we encounter their definitions"

estebank · 2018-05-16T00:57:12Z

src/librustc_lint/unused.rs

+            }
+            ast::ExprKind::Break(Some(label), _) | ast::ExprKind::Continue(Some(label)) => {
+                if let Some(index) = self.0.iter().rposition(|&l| l.ident == label.ident) {
+                    self.0.remove(index);


Add another comment along the lines of "Remove labels as they're used. If they're used more than once they will already have been removed, as expected".

estebank · 2018-05-16T01:01:40Z

src/librustc_lint/unused.rs

+                        ctxt.span_lint(UNUSED_LABEL, label.ident.span, "unused label");
+                    } else {
+                        self.0.push(unused_label);
+                    }


I'm not sure this is correct. If you have multiple nested scopes with labels, check_expr_post is going to start going down, right? If this is the case, then the lint will only warn on the bottom-most label. I'm not entirely sure. That being said, I feel this would be easier to follow as a check_crate_post and merely iterate through &self.0 idents and emit the lint.

Yeah, this approach definitely has an issue with what you mentioned earlier:

fn main() { 'used_many_shadowed: loop { 'used_many_shadowed: loop { if true { break 'used_many_shadowed; } else { break 'used_many_shadowed; } } } }

Should warn on the first 'used_many_shadowed label, but won't. I've implemented a fix using a Vec<(Label, bool)> to keep track of when each Label is used instead of just removing it, but honestly it's looking like @petrochenkov 's suggestion to run this check within librustc_resolve is probably the way to go.

Can you add a test case for the following, which should be allowed:

fn main() { 'used_many: loop { if true { break 'used_many; } else { break 'used_many; } } }

rust-highfive · 2018-05-16T03:48:55Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:05:22] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:22] tidy error: /checkout/src/librustc_lint/unused.rs:499: line longer than 100 chars
[00:05:24] some tidy checks failed
[00:05:24] 
[00:05:24] 
[00:05:24] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:24] 
[00:05:24] 
[00:05:24] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:24] Build completed unsuccessfully in 0:02:12
[00:05:24] Build completed unsuccessfully in 0:02:12
[00:05:24] make: *** [tidy] Error 1
[00:05:24] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:019ee4d5
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

petrochenkov · 2018-05-16T09:40:43Z

@KyleStach1678
Could you address this now hidden comment (#50763 (comment)) and make the check use resolution results and also rename the lint into idiomatic unused_labels (see https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#lints).

abonander · 2018-05-16T10:16:16Z

@petrochenkov Can you go into more detail about your alternative solution? I don't understand resolve as well as I do the syntax/lint part of the frontend so I won't be much help here.

nagisa · 2018-05-17T04:56:40Z

It only applies to cargo (or build systems that do not neglect to use --cap-lint) and it does not work that way for end users who do cargo install.

…

On Wed, May 16, 2018, 01:33 Austin Bonander ***@***.***> wrote: @nagisa <https://github.com/nagisa> https://github.com/arcnmx/rust-rfcs/blob/master/text/1105-api-evolution.md#lints Since lint #[deny] errors don't stop compilation of dependencies the only build breakage would occur for the local crate where the author can immediately address it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50763 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApc0n0M07YpDJ0EbSnZ5HoAb0wcsTvyks5ty1fBgaJpZM4T--FL> .

est31 · 2018-05-17T08:18:09Z

Now that #50045 merged, you can implement this lint for labeled blocks as well. All you need to do is to rebase the PR onto current master and add a ast::ExprKind::Block case to your matches.

petrochenkov · 2018-05-17T10:25:13Z

@abonander

Can you go into more detail about your alternative solution?

struct Resolver needs to get a set of unused labels: unused_labels: FxHashMap<NodeId, Span>.
Then fn with_resolved_label would add label ids into that set and label ids found by fn search_label would be removed from that set.
Then fn check_crate in librustc_resolve/check_unused.rs loops through resolver.unused_labels and reports lints.

kylestach · 2018-05-18T00:45:12Z

@petrochenkov as far as I can tell Labels are identified by a NodeId rather than from a DefId, at least according to the definiton of Def (unless I'm misreading that, which is very possible). Does that sound correct?

petrochenkov · 2018-05-18T00:56:37Z

@KyleStach1678
Oh, sorry, it's NodeId of course, it was a copy-paste error on my side.

… multiple times

This functionality is being reimplemented in the resolver phase This reverts commit 503a69e844970476b27bf1ac7be951bb22194f50.

This reverts commit b9257e2ca161b1bf5aae9d6b667f4d0c6b8d7be6.

This reverts commit b2a2450cf2925498e1c3bdd781aa1978f9eb00e5.

petrochenkov · 2018-05-19T11:32:37Z

@bors r+

bors · 2018-05-19T11:32:38Z

📌 Commit 6da64a7 has been approved by petrochenkov

bors · 2018-05-19T14:52:36Z

⌛ Testing commit 6da64a7 with merge 1b240da...

Add lint checks for unused loop labels Previously no warning was generated when a loop label was written out but never used (i.e. in a `break` or `continue` statement): ```rust fn main() { 'unused_label: loop {} } ``` would compile without complaint. This fix introduces `-W unused_loop_label`, which generates the following warning message for the above snippet: ``` warning: unused loop label --> main.rs:2:5 | 2 | 'unused_label: loop {} | ^^^^^^^^^^^^^ | = note: #[warn(unused_loop_label)] on by default ``` Fixes: #50751.

bors · 2018-05-19T17:03:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 1b240da to master...

rust-highfive assigned estebank May 15, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 15, 2018

abonander reviewed May 15, 2018

View reviewed changes

est31 reviewed May 15, 2018

View reviewed changes

petrochenkov reviewed May 15, 2018

View reviewed changes

estebank approved these changes May 16, 2018

View reviewed changes

petrochenkov self-assigned this May 16, 2018

abonander mentioned this pull request May 16, 2018

Add lint for multiple associated types #50682

Merged

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2018

kylestach added 15 commits May 18, 2018 16:57

Add lint checks for unused loop labels

d67628e

Rename test to unused_label and remove empty stdout file

7c4aa73

Add additional test case to unused_label lint

bb867d3

Rename unused_loop_label to unused_label and fix/clean up lint logic

acd6ab8

Add test case for shadowed labels, with the inner broken multiple times

0f27412

Fix ignored unused outer label when inner label shadows and is broken…

4de4e61

… multiple times

Fix formatting in unused label linting to make tidy checks pass

167cede

Revert "Add lint checks for unused loop labels"

7676982

This functionality is being reimplemented in the resolver phase This reverts commit 503a69e844970476b27bf1ac7be951bb22194f50.

Reimplement unused_labels lint as a compiler builtin in the resolver

88f4063

Add tests for new labeled blocks for unused_labels

702f1dd

Remove unused label in mir

f488d2b

Allow unused_labels in some compile-fail tests

a336aa9

Revert "Allow unused_labels in some compile-fail tests"

335f91f

This reverts commit b9257e2ca161b1bf5aae9d6b667f4d0c6b8d7be6.

Revert "Remove unused label in mir"

5586cd8

This reverts commit b2a2450cf2925498e1c3bdd781aa1978f9eb00e5.

Default unused_labels to allow, move to "unused"

6da64a7

kylestach force-pushed the unused-loop-label branch from b9257e2 to 6da64a7 Compare May 19, 2018 00:49

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 19, 2018

bors merged commit 6da64a7 into rust-lang:master May 19, 2018

This was referenced May 19, 2018

Implementation of RFC 2086 - Allow Irrefutable Let patterns #49469

Merged

Add a lint for unused const fn results #50805

Closed

Fix naming conventions for new lints #50879

Merged

Add deprecation lint for duplicated macro_exports #50143

Merged

petrochenkov mentioned this pull request May 19, 2018

Unused loop (break) name gives no compiler warning #50751

Closed

BartMassey mentioned this pull request Nov 12, 2019

unused_labels should be reported as warnings #66324

Closed

Add lint checks for unused loop labels #50763

Add lint checks for unused loop labels #50763

Conversation

kylestach commented May 15, 2018

rust-highfive commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

est31 commented May 15, 2018

Choose a reason for hiding this comment

abonander commented May 15, 2018 • edited Loading

est31 commented May 15, 2018

petrochenkov May 15, 2018 • edited Loading

Choose a reason for hiding this comment

petrochenkov commented May 15, 2018

nagisa commented May 15, 2018

estebank commented May 15, 2018

abonander commented May 15, 2018

petrochenkov commented May 15, 2018

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rust-highfive commented May 16, 2018

petrochenkov commented May 16, 2018

abonander commented May 16, 2018 • edited Loading

nagisa commented May 17, 2018 via email

est31 commented May 17, 2018

petrochenkov commented May 17, 2018 • edited Loading

kylestach commented May 18, 2018 • edited Loading

petrochenkov commented May 18, 2018

petrochenkov commented May 19, 2018

bors commented May 19, 2018

bors commented May 19, 2018

bors commented May 19, 2018

abonander commented May 15, 2018 •

edited

Loading

petrochenkov May 15, 2018 •

edited

Loading

abonander commented May 16, 2018 •

edited

Loading

petrochenkov commented May 17, 2018 •

edited

Loading

kylestach commented May 18, 2018 •

edited

Loading