For OutsideLoop we should not suggest add 'block label in if block, or we wiil get another err: block label not supported here.

fixes #123261

Author: surechen

compiler/rustc_passes/src/loops.rs

@@ -1,3 +1,4 @@
+use rustc_data_structures::fx::FxHashSet;
 use Context::*;
 
 use rustc_hir as hir;
@@ -9,7 +10,7 @@ use rustc_middle::query::Providers;
 use rustc_middle::ty::TyCtxt;
 use rustc_session::Session;
 use rustc_span::hygiene::DesugaringKind;
-use rustc_span::{BytePos, Span};
+use rustc_span::{BytePos, Span, DUMMY_SP};
 
 use crate::errors::{
     BreakInsideAsyncBlock, BreakInsideClosure, BreakNonLoop, ContinueLabeledBlock, OutsideLoop,
@@ -24,21 +25,33 @@ enum Context {
     Closure(Span),
     AsyncClosure(Span),
     UnlabeledBlock(Span),
+    IfUnlabeledBlock(Span),
     LabeledBlock,
     Constant,
 }
 
-#[derive(Copy, Clone)]
+/// cx_stack is a stack, used for some situation like in `if` block,
+/// we should not suggest adding `'block` label in `if` block,
+/// we can back to outer block and add label there.
+/// fix_label_spans used for recording spans which has added `'block` label,
+/// this can help avoiding suggesting redundant labels.
+#[derive(Clone)]
 struct CheckLoopVisitor<'a, 'tcx> {
     sess: &'a Session,
     tcx: TyCtxt<'tcx>,
-    cx: Context,
+    cx_stack: Vec<Context>,
+    fix_label_spans: FxHashSet<Span>,
 }
 
 fn check_mod_loops(tcx: TyCtxt<'_>, module_def_id: LocalModDefId) {
     tcx.hir().visit_item_likes_in_module(
         module_def_id,
-        &mut CheckLoopVisitor { sess: tcx.sess, tcx, cx: Normal },
+        &mut CheckLoopVisitor {
+            sess: tcx.sess,
+            tcx,
+            cx_stack: vec![Normal],
+            fix_label_spans: Default::default(),
+        },
     );
 }
 
@@ -82,6 +95,35 @@ impl<'a, 'hir> Visitor<'hir> for CheckLoopVisitor<'a, 'hir> {
 
     fn visit_expr(&mut self, e: &'hir hir::Expr<'hir>) {
         match e.kind {
+            hir::ExprKind::If(cond, then, else_opt) => {
+                self.visit_expr(cond);
+                if let hir::ExprKind::Block(ref b, None) = then.kind
+                    && matches!(
+                        self.cx_stack.last().unwrap(),
+                        Normal | Constant | UnlabeledBlock(_)
+                    )
+                {
+                    self.with_context(IfUnlabeledBlock(b.span.shrink_to_lo()), |v| {
+                        v.visit_block(b)
+                    });
+                } else {
+                    self.visit_expr(then);
+                }
+                if let Some(else_expr) = else_opt {
+                    if let hir::ExprKind::Block(ref b, None) = else_expr.kind
+                        && matches!(
+                            self.cx_stack.last().unwrap(),
+                            Normal | Constant | UnlabeledBlock(_)
+                        )
+                    {
+                        self.with_context(IfUnlabeledBlock(b.span.shrink_to_lo()), |v| {
+                            v.visit_block(b)
+                        });
+                    } else {
+                        self.visit_expr(then);
+                    }
+                }
+            }
             hir::ExprKind::Loop(ref b, _, source, _) => {
                 self.with_context(Loop(source), |v| v.visit_block(b));
             }
@@ -102,11 +144,14 @@ impl<'a, 'hir> Visitor<'hir> for CheckLoopVisitor<'a, 'hir> {
             hir::ExprKind::Block(ref b, Some(_label)) => {
                 self.with_context(LabeledBlock, |v| v.visit_block(b));
             }
-            hir::ExprKind::Block(ref b, None) if matches!(self.cx, Fn) => {
+            hir::ExprKind::Block(ref b, None) if matches!(self.cx_stack.last().unwrap(), Fn) => {
                 self.with_context(Normal, |v| v.visit_block(b));
             }
             hir::ExprKind::Block(ref b, None)
-                if matches!(self.cx, Normal | Constant | UnlabeledBlock(_)) =>
+                if matches!(
+                    self.cx_stack.last().unwrap(),
+                    Normal | Constant | UnlabeledBlock(_)
+                ) =>
             {
                 self.with_context(UnlabeledBlock(b.span.shrink_to_lo()), |v| v.visit_block(b));
             }
@@ -179,7 +224,7 @@ impl<'a, 'hir> Visitor<'hir> for CheckLoopVisitor<'a, 'hir> {
                     Some(label) => sp_lo.with_hi(label.ident.span.hi()),
                     None => sp_lo.shrink_to_lo(),
                 };
-                self.require_break_cx("break", e.span, label_sp);
+                self.require_break_cx("break", e.span, label_sp, self.cx_stack.len() - 1);
             }
             hir::ExprKind::Continue(destination) => {
                 self.require_label_in_labeled_block(e.span, &destination, "continue");
@@ -201,7 +246,7 @@ impl<'a, 'hir> Visitor<'hir> for CheckLoopVisitor<'a, 'hir> {
                     }
                     Err(_) => {}
                 }
-                self.require_break_cx("continue", e.span, e.span)
+                self.require_break_cx("continue", e.span, e.span, self.cx_stack.len() - 1)
             }
             _ => intravisit::walk_expr(self, e),
         }
@@ -213,15 +258,14 @@ impl<'a, 'hir> CheckLoopVisitor<'a, 'hir> {
     where
         F: FnOnce(&mut CheckLoopVisitor<'a, 'hir>),
     {
-        let old_cx = self.cx;
-        self.cx = cx;
+        self.cx_stack.push(cx);
         f(self);
-        self.cx = old_cx;
+        self.cx_stack.pop();
     }
 
-    fn require_break_cx(&self, name: &str, span: Span, break_span: Span) {
+    fn require_break_cx(&mut self, name: &str, span: Span, break_span: Span, cx_pos: usize) {
         let is_break = name == "break";
-        match self.cx {
+        match self.cx_stack[cx_pos] {
             LabeledBlock | Loop(_) => {}
             Closure(closure_span) => {
                 self.sess.dcx().emit_err(BreakInsideClosure { span, closure_span, name });
@@ -230,10 +274,18 @@ impl<'a, 'hir> CheckLoopVisitor<'a, 'hir> {
                 self.sess.dcx().emit_err(BreakInsideAsyncBlock { span, closure_span, name });
             }
             UnlabeledBlock(block_span) if is_break && block_span.eq_ctxt(break_span) => {
-                let suggestion = Some(OutsideLoopSuggestion { block_span, break_span });
+                let suggestion = if !self.fix_label_spans.contains(&block_span) {
+                    Some(OutsideLoopSuggestion { block_span, break_span })
+                } else {
+                    Some(OutsideLoopSuggestion { block_span: DUMMY_SP, break_span })
+                };
                 self.sess.dcx().emit_err(OutsideLoop { span, name, is_break, suggestion });
+                self.fix_label_spans.insert(block_span);
+            }
+            IfUnlabeledBlock(_) if is_break => {
+                self.require_break_cx(name, span, break_span, cx_pos - 1);
             }
-            Normal | Constant | Fn | UnlabeledBlock(_) => {
+            Normal | Constant | Fn | UnlabeledBlock(_) | IfUnlabeledBlock(_) => {
                 self.sess.dcx().emit_err(OutsideLoop { span, name, is_break, suggestion: None });
             }
         }
@@ -246,7 +298,7 @@ impl<'a, 'hir> CheckLoopVisitor<'a, 'hir> {
         cf_type: &str,
     ) -> bool {
         if !span.is_desugaring(DesugaringKind::QuestionMark)
-            && self.cx == LabeledBlock
+            && *self.cx_stack.last().unwrap() == LabeledBlock
             && label.label.is_none()
         {
             self.sess.dcx().emit_err(UnlabeledInLabeledBlock { span, cf_type });

tests/ui/loops/loop-if-else-break-issue-123261.fixed

@@ -0,0 +1,21 @@
+//@ run-rustfix
+
+#![allow(unused)]
+
+fn main() {
+    let n = 1;
+    let x = 'block: {
+        if n == 0 {
+            break 'block 1; //~ ERROR [E0268]
+        } else {
+            break 'block 2; //~ ERROR [E0268]
+        }
+    };
+
+    let y = 'block: {
+        if n == 0 {
+            break 'block 1; //~ ERROR [E0268]
+        }
+        break 'block 0; //~ ERROR [E0268]
+    };
+}

tests/ui/loops/loop-if-else-break-issue-123261.rs

@@ -0,0 +1,21 @@
+//@ run-rustfix
+
+#![allow(unused)]
+
+fn main() {
+    let n = 1;
+    let x = {
+        if n == 0 {
+            break 1; //~ ERROR [E0268]
+        } else {
+            break 2; //~ ERROR [E0268]
+        }
+    };
+
+    let y = {
+        if n == 0 {
+            break 1; //~ ERROR [E0268]
+        }
+        break 0; //~ ERROR [E0268]
+    };
+}

tests/ui/loops/loop-if-else-break-issue-123261.stderr

@@ -0,0 +1,59 @@
+error[E0268]: `break` outside of a loop or labeled block
+  --> $DIR/loop-if-else-break-issue-123261.rs:9:13
+   |
+LL |             break 1;
+   |             ^^^^^^^ cannot `break` outside of a loop or labeled block
+   |
+help: consider labeling this block to be able to break within it
+   |
+LL ~     let x = 'block: {
+LL |         if n == 0 {
+LL ~             break 'block 1;
+   |
+
+error[E0268]: `break` outside of a loop or labeled block
+  --> $DIR/loop-if-else-break-issue-123261.rs:11:13
+   |
+LL |             break 2;
+   |             ^^^^^^^ cannot `break` outside of a loop or labeled block
+   |
+help: consider labeling this block to be able to break within it
+   |
+LL ~ 'block: //@ run-rustfix
+LL | 
+ ...
+LL |         } else {
+LL ~             break 'block 2;
+   |
+
+error[E0268]: `break` outside of a loop or labeled block
+  --> $DIR/loop-if-else-break-issue-123261.rs:17:13
+   |
+LL |             break 1;
+   |             ^^^^^^^ cannot `break` outside of a loop or labeled block
+   |
+help: consider labeling this block to be able to break within it
+   |
+LL ~     let y = 'block: {
+LL |         if n == 0 {
+LL ~             break 'block 1;
+   |
+
+error[E0268]: `break` outside of a loop or labeled block
+  --> $DIR/loop-if-else-break-issue-123261.rs:19:9
+   |
+LL |         break 0;
+   |         ^^^^^^^ cannot `break` outside of a loop or labeled block
+   |
+help: consider labeling this block to be able to break within it
+   |
+LL ~ 'block: //@ run-rustfix
+LL | 
+ ...
+LL |         }
+LL ~         break 'block 0;
+   |
+
+error: aborting due to 4 previous errors
+
+For more information about this error, try `rustc --explain E0268`.