From 707ce2b798ad7bb6c1e0fce0da542d9caef07b56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 19 Jan 2021 15:51:51 -0800 Subject: [PATCH 1/9] Account for labels when suggesting `loop` instead of `while true` --- compiler/rustc_lint/src/builtin.rs | 12 +++- src/test/ui/issues/issue-1962.fixed | 4 +- src/test/ui/issues/issue-1962.rs | 4 +- src/test/ui/issues/issue-1962.stderr | 4 +- src/test/ui/issues/issue-27042.stderr | 2 +- src/test/ui/label/label_misspelled.rs | 22 +++++++ src/test/ui/label/label_misspelled.stderr | 80 ++++++++++++++++++++++- 7 files changed, 116 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_lint/src/builtin.rs b/compiler/rustc_lint/src/builtin.rs index 8cdb33ea3175f..b37660e4a90d3 100644 --- a/compiler/rustc_lint/src/builtin.rs +++ b/compiler/rustc_lint/src/builtin.rs @@ -96,18 +96,24 @@ fn pierce_parens(mut expr: &ast::Expr) -> &ast::Expr { impl EarlyLintPass for WhileTrue { fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &ast::Expr) { - if let ast::ExprKind::While(cond, ..) = &e.kind { + if let ast::ExprKind::While(cond, _, label) = &e.kind { if let ast::ExprKind::Lit(ref lit) = pierce_parens(cond).kind { if let ast::LitKind::Bool(true) = lit.kind { if !lit.span.from_expansion() { let msg = "denote infinite loops with `loop { ... }`"; - let condition_span = cx.sess.source_map().guess_head_span(e.span); + let condition_span = e.span.with_hi(cond.span.hi()); cx.struct_span_lint(WHILE_TRUE, condition_span, |lint| { lint.build(msg) .span_suggestion_short( condition_span, "use `loop`", - "loop".to_owned(), + format!( + "{}loop", + label.map_or_else(String::new, |label| format!( + "{}: ", + label.ident, + )) + ), Applicability::MachineApplicable, ) .emit(); diff --git a/src/test/ui/issues/issue-1962.fixed b/src/test/ui/issues/issue-1962.fixed index b810a90ef37f9..897fd172b29f3 100644 --- a/src/test/ui/issues/issue-1962.fixed +++ b/src/test/ui/issues/issue-1962.fixed @@ -3,8 +3,8 @@ fn main() { let mut i = 0; - loop { //~ ERROR denote infinite loops with `loop + 'a: loop { //~ ERROR denote infinite loops with `loop i += 1; - if i == 5 { break; } + if i == 5 { break 'a; } } } diff --git a/src/test/ui/issues/issue-1962.rs b/src/test/ui/issues/issue-1962.rs index 00d2bbd28506e..71e874100874f 100644 --- a/src/test/ui/issues/issue-1962.rs +++ b/src/test/ui/issues/issue-1962.rs @@ -3,8 +3,8 @@ fn main() { let mut i = 0; - while true { //~ ERROR denote infinite loops with `loop + 'a: while true { //~ ERROR denote infinite loops with `loop i += 1; - if i == 5 { break; } + if i == 5 { break 'a; } } } diff --git a/src/test/ui/issues/issue-1962.stderr b/src/test/ui/issues/issue-1962.stderr index 17142912696a7..4c32a4cf3dd59 100644 --- a/src/test/ui/issues/issue-1962.stderr +++ b/src/test/ui/issues/issue-1962.stderr @@ -1,8 +1,8 @@ error: denote infinite loops with `loop { ... }` --> $DIR/issue-1962.rs:6:5 | -LL | while true { - | ^^^^^^^^^^ help: use `loop` +LL | 'a: while true { + | ^^^^^^^^^^^^^^ help: use `loop` | = note: requested on the command line with `-D while-true` diff --git a/src/test/ui/issues/issue-27042.stderr b/src/test/ui/issues/issue-27042.stderr index 7dee1a6a5f044..59ef28481d0e6 100644 --- a/src/test/ui/issues/issue-27042.stderr +++ b/src/test/ui/issues/issue-27042.stderr @@ -4,7 +4,7 @@ warning: denote infinite loops with `loop { ... }` LL | / 'b: LL | | LL | | while true { break }; // but here we cite the whole loop - | |____________________________^ help: use `loop` + | |__________________^ help: use `loop` | = note: `#[warn(while_true)]` on by default diff --git a/src/test/ui/label/label_misspelled.rs b/src/test/ui/label/label_misspelled.rs index ebfd5642c9fa8..7a8ff0360f6ea 100644 --- a/src/test/ui/label/label_misspelled.rs +++ b/src/test/ui/label/label_misspelled.rs @@ -16,3 +16,25 @@ fn main() { //~^ ERROR cannot find value `for_loop` in this scope }; } + +fn foo() { + 'LOOP: loop { + break LOOP; + //~^ ERROR cannot find value `LOOP` in this scope + }; + 'while_loop: while true { //~ WARN denote infinite loops with + break while_loop; + //~^ ERROR cannot find value `while_loop` in this scope + //~| ERROR `break` with value from a `while` loop + }; + 'while_let: while let Some(_) = Some(()) { + break while_let; + //~^ ERROR cannot find value `while_let` in this scope + //~| ERROR `break` with value from a `while` loop + } + 'for_loop: for _ in 0..3 { + break for_loop; + //~^ ERROR cannot find value `for_loop` in this scope + //~| ERROR `break` with value from a `for` loop + }; +} diff --git a/src/test/ui/label/label_misspelled.stderr b/src/test/ui/label/label_misspelled.stderr index 1368ca4126cdd..8282d3ada32f2 100644 --- a/src/test/ui/label/label_misspelled.stderr +++ b/src/test/ui/label/label_misspelled.stderr @@ -34,6 +34,42 @@ LL | for_loop; | not found in this scope | help: a label with a similar name exists: `'for_loop` +error[E0425]: cannot find value `LOOP` in this scope + --> $DIR/label_misspelled.rs:22:15 + | +LL | break LOOP; + | ^^^^ + | | + | not found in this scope + | help: a label with a similar name exists: `'LOOP` + +error[E0425]: cannot find value `while_loop` in this scope + --> $DIR/label_misspelled.rs:26:15 + | +LL | break while_loop; + | ^^^^^^^^^^ + | | + | not found in this scope + | help: a label with a similar name exists: `'while_loop` + +error[E0425]: cannot find value `while_let` in this scope + --> $DIR/label_misspelled.rs:31:15 + | +LL | break while_let; + | ^^^^^^^^^ + | | + | not found in this scope + | help: a label with a similar name exists: `'while_let` + +error[E0425]: cannot find value `for_loop` in this scope + --> $DIR/label_misspelled.rs:36:15 + | +LL | break for_loop; + | ^^^^^^^^ + | | + | not found in this scope + | help: a label with a similar name exists: `'for_loop` + warning: denote infinite loops with `loop { ... }` --> $DIR/label_misspelled.rs:6:5 | @@ -42,6 +78,46 @@ LL | 'while_loop: while true { | = note: `#[warn(while_true)]` on by default -error: aborting due to 4 previous errors; 1 warning emitted +warning: denote infinite loops with `loop { ... }` + --> $DIR/label_misspelled.rs:25:5 + | +LL | 'while_loop: while true { + | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `loop` + +error[E0571]: `break` with value from a `while` loop + --> $DIR/label_misspelled.rs:26:9 + | +LL | break while_loop; + | ^^^^^^^^^^^^^^^^ can only break with a value inside `loop` or breakable block + | +help: instead, use `break` on its own without a value inside this `while` loop + | +LL | break; + | ^^^^^ + +error[E0571]: `break` with value from a `while` loop + --> $DIR/label_misspelled.rs:31:9 + | +LL | break while_let; + | ^^^^^^^^^^^^^^^ can only break with a value inside `loop` or breakable block + | +help: instead, use `break` on its own without a value inside this `while` loop + | +LL | break; + | ^^^^^ + +error[E0571]: `break` with value from a `for` loop + --> $DIR/label_misspelled.rs:36:9 + | +LL | break for_loop; + | ^^^^^^^^^^^^^^ can only break with a value inside `loop` or breakable block + | +help: instead, use `break` on its own without a value inside this `for` loop + | +LL | break; + | ^^^^^ + +error: aborting due to 11 previous errors; 2 warnings emitted -For more information about this error, try `rustc --explain E0425`. +Some errors have detailed explanations: E0425, E0571. +For more information about an error, try `rustc --explain E0425`. From a701ff981d8768e2b81a053bf9189ff67a2bb7ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 19 Jan 2021 17:51:48 -0800 Subject: [PATCH 2/9] Suggest `'a` when given `a` only when appropriate When encountering a name `a` that isn't resolved, but a label `'a` is found in the current ribs, only suggest `'a` if this name is the value expression of a `break` statement. Solve FIXME. --- compiler/rustc_resolve/src/late.rs | 6 +++ .../rustc_resolve/src/late/diagnostics.rs | 22 ++++++---- src/test/ui/label/label_misspelled.stderr | 44 ++++++++++--------- src/test/ui/loops/loop-break-value.stderr | 4 +- 4 files changed, 46 insertions(+), 30 deletions(-) diff --git a/compiler/rustc_resolve/src/late.rs b/compiler/rustc_resolve/src/late.rs index fff14747e5729..a3a630e14906f 100644 --- a/compiler/rustc_resolve/src/late.rs +++ b/compiler/rustc_resolve/src/late.rs @@ -2266,6 +2266,12 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> { visit::walk_expr(self, expr); } + ExprKind::Break(None, Some(ref e)) => { + // We use this instead of `visit::walk_expr` to keep the parent expr around for + // better + self.resolve_expr(e, Some(&expr)); + } + ExprKind::Let(ref pat, ref scrutinee) => { self.visit_expr(scrutinee); self.resolve_pattern_top(pat, PatternSource::Let); diff --git a/compiler/rustc_resolve/src/late/diagnostics.rs b/compiler/rustc_resolve/src/late/diagnostics.rs index 3945afb4724a8..f8f81c9128772 100644 --- a/compiler/rustc_resolve/src/late/diagnostics.rs +++ b/compiler/rustc_resolve/src/late/diagnostics.rs @@ -547,15 +547,19 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> { for label_rib in &self.label_ribs { for (label_ident, _) in &label_rib.bindings { if format!("'{}", ident) == label_ident.to_string() { - let msg = "a label with a similar name exists"; - // FIXME: consider only emitting this suggestion if a label would be valid here - // which is pretty much only the case for `break` expressions. - err.span_suggestion( - span, - &msg, - label_ident.name.to_string(), - Applicability::MaybeIncorrect, - ); + err.span_label(label_ident.span, "a label with a similar name exists"); + if let PathSource::Expr(Some(Expr { + kind: ExprKind::Break(None, Some(_)), + .. + })) = source + { + err.span_suggestion( + span, + "use the similarly named label", + label_ident.name.to_string(), + Applicability::MaybeIncorrect, + ); + } } } } diff --git a/src/test/ui/label/label_misspelled.stderr b/src/test/ui/label/label_misspelled.stderr index 8282d3ada32f2..7cfb6e8147dd9 100644 --- a/src/test/ui/label/label_misspelled.stderr +++ b/src/test/ui/label/label_misspelled.stderr @@ -1,74 +1,78 @@ error[E0425]: cannot find value `LOOP` in this scope --> $DIR/label_misspelled.rs:3:9 | +LL | 'LOOP: loop { + | ----- a label with a similar name exists LL | LOOP; - | ^^^^ - | | - | not found in this scope - | help: a label with a similar name exists: `'LOOP` + | ^^^^ not found in this scope error[E0425]: cannot find value `while_loop` in this scope --> $DIR/label_misspelled.rs:7:9 | +LL | 'while_loop: while true { + | ----------- a label with a similar name exists LL | while_loop; - | ^^^^^^^^^^ - | | - | not found in this scope - | help: a label with a similar name exists: `'while_loop` + | ^^^^^^^^^^ not found in this scope error[E0425]: cannot find value `while_let` in this scope --> $DIR/label_misspelled.rs:11:9 | +LL | 'while_let: while let Some(_) = Some(()) { + | ---------- a label with a similar name exists LL | while_let; - | ^^^^^^^^^ - | | - | not found in this scope - | help: a label with a similar name exists: `'while_let` + | ^^^^^^^^^ not found in this scope error[E0425]: cannot find value `for_loop` in this scope --> $DIR/label_misspelled.rs:15:9 | +LL | 'for_loop: for _ in 0..3 { + | --------- a label with a similar name exists LL | for_loop; - | ^^^^^^^^ - | | - | not found in this scope - | help: a label with a similar name exists: `'for_loop` + | ^^^^^^^^ not found in this scope error[E0425]: cannot find value `LOOP` in this scope --> $DIR/label_misspelled.rs:22:15 | +LL | 'LOOP: loop { + | ----- a label with a similar name exists LL | break LOOP; | ^^^^ | | | not found in this scope - | help: a label with a similar name exists: `'LOOP` + | help: use the similarly named label: `'LOOP` error[E0425]: cannot find value `while_loop` in this scope --> $DIR/label_misspelled.rs:26:15 | +LL | 'while_loop: while true { + | ----------- a label with a similar name exists LL | break while_loop; | ^^^^^^^^^^ | | | not found in this scope - | help: a label with a similar name exists: `'while_loop` + | help: use the similarly named label: `'while_loop` error[E0425]: cannot find value `while_let` in this scope --> $DIR/label_misspelled.rs:31:15 | +LL | 'while_let: while let Some(_) = Some(()) { + | ---------- a label with a similar name exists LL | break while_let; | ^^^^^^^^^ | | | not found in this scope - | help: a label with a similar name exists: `'while_let` + | help: use the similarly named label: `'while_let` error[E0425]: cannot find value `for_loop` in this scope --> $DIR/label_misspelled.rs:36:15 | +LL | 'for_loop: for _ in 0..3 { + | --------- a label with a similar name exists LL | break for_loop; | ^^^^^^^^ | | | not found in this scope - | help: a label with a similar name exists: `'for_loop` + | help: use the similarly named label: `'for_loop` warning: denote infinite loops with `loop { ... }` --> $DIR/label_misspelled.rs:6:5 diff --git a/src/test/ui/loops/loop-break-value.stderr b/src/test/ui/loops/loop-break-value.stderr index 0237435c8b46a..27104118a63a4 100644 --- a/src/test/ui/loops/loop-break-value.stderr +++ b/src/test/ui/loops/loop-break-value.stderr @@ -1,11 +1,13 @@ error[E0425]: cannot find value `LOOP` in this scope --> $DIR/loop-break-value.rs:95:15 | +LL | 'LOOP: for _ in 0 .. 9 { + | ----- a label with a similar name exists LL | break LOOP; | ^^^^ | | | not found in this scope - | help: a label with a similar name exists: `'LOOP` + | help: use the similarly named label: `'LOOP` warning: denote infinite loops with `loop { ... }` --> $DIR/loop-break-value.rs:26:5 From 060dba67b7523a69223a9aef7c9fe2494ec27535 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 20 Jan 2021 17:15:08 -0800 Subject: [PATCH 3/9] Add loop head span to hir --- compiler/rustc_ast_lowering/src/expr.rs | 19 +++++++++++++++---- compiler/rustc_hir/src/hir.rs | 4 +++- compiler/rustc_hir/src/intravisit.rs | 2 +- compiler/rustc_hir_pretty/src/lib.rs | 2 +- compiler/rustc_mir_build/src/thir/cx/expr.rs | 4 +--- compiler/rustc_passes/src/check_const.rs | 2 +- compiler/rustc_passes/src/liveness.rs | 2 +- compiler/rustc_passes/src/loops.rs | 5 ++--- compiler/rustc_passes/src/region.rs | 2 +- compiler/rustc_resolve/src/late/lifetimes.rs | 2 +- compiler/rustc_typeck/src/check/expr.rs | 2 +- compiler/rustc_typeck/src/expr_use_visitor.rs | 2 +- src/tools/clippy/clippy_lints/src/loops.rs | 8 ++++---- .../clippy_lints/src/needless_continue.rs | 2 +- src/tools/clippy/clippy_lints/src/shadow.rs | 2 +- 15 files changed, 35 insertions(+), 25 deletions(-) diff --git a/compiler/rustc_ast_lowering/src/expr.rs b/compiler/rustc_ast_lowering/src/expr.rs index 1b9ccbd850bee..31360158e2b4e 100644 --- a/compiler/rustc_ast_lowering/src/expr.rs +++ b/compiler/rustc_ast_lowering/src/expr.rs @@ -10,9 +10,9 @@ use rustc_errors::struct_span_err; use rustc_hir as hir; use rustc_hir::def::Res; use rustc_session::parse::feature_err; -use rustc_span::hygiene::ForLoopLoc; use rustc_span::source_map::{respan, DesugaringKind, Span, Spanned}; use rustc_span::symbol::{sym, Ident, Symbol}; +use rustc_span::{hygiene::ForLoopLoc, DUMMY_SP}; use rustc_target::asm; use std::collections::hash_map::Entry; use std::fmt::Write; @@ -102,6 +102,7 @@ impl<'hir> LoweringContext<'_, 'hir> { this.lower_block(body, false), opt_label, hir::LoopSource::Loop, + DUMMY_SP, ) }), ExprKind::TryBlock(ref body) => self.lower_expr_try_block(body), @@ -453,7 +454,12 @@ impl<'hir> LoweringContext<'_, 'hir> { self.expr_match(span, scrutinee, arena_vec![self; then_arm, else_arm], desugar); // `[opt_ident]: loop { ... }` - hir::ExprKind::Loop(self.block_expr(self.arena.alloc(match_expr)), opt_label, source) + hir::ExprKind::Loop( + self.block_expr(self.arena.alloc(match_expr)), + opt_label, + source, + span.with_hi(cond.span.hi()), + ) } /// Desugar `try { ; }` into `{ ; ::std::ops::Try::from_ok() }`, @@ -748,7 +754,7 @@ impl<'hir> LoweringContext<'_, 'hir> { // loop { .. } let loop_expr = self.arena.alloc(hir::Expr { hir_id: loop_hir_id, - kind: hir::ExprKind::Loop(loop_block, None, hir::LoopSource::Loop), + kind: hir::ExprKind::Loop(loop_block, None, hir::LoopSource::Loop, span), span, attrs: ThinVec::new(), }); @@ -1709,7 +1715,12 @@ impl<'hir> LoweringContext<'_, 'hir> { ); // `[opt_ident]: loop { ... }` - let kind = hir::ExprKind::Loop(loop_block, opt_label, hir::LoopSource::ForLoop); + let kind = hir::ExprKind::Loop( + loop_block, + opt_label, + hir::LoopSource::ForLoop, + e.span.with_hi(orig_head_span.hi()), + ); let loop_expr = self.arena.alloc(hir::Expr { hir_id: self.lower_node_id(e.id), kind, diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs index 3673e5c8bf3a5..35170fa7c1d02 100644 --- a/compiler/rustc_hir/src/hir.rs +++ b/compiler/rustc_hir/src/hir.rs @@ -1617,7 +1617,9 @@ pub enum ExprKind<'hir> { /// A conditionless loop (can be exited with `break`, `continue`, or `return`). /// /// I.e., `'label: loop { }`. - Loop(&'hir Block<'hir>, Option