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

surechen · surechen · commit efbf143387d2 · 2024-04-09T16:45:39.000+08:00
fixes #123261
diff --git a/compiler/rustc_passes/src/errors.rs b/compiler/rustc_passes/src/errors.rs
@@ -1091,7 +1091,7 @@ pub struct BreakInsideAsyncBlock<'a> {
 pub struct OutsideLoop<'a> {
     #[primary_span]
     #[label]
-    pub span: Span,
+    pub spans: Vec<Span>,
     pub name: &'a str,
     pub is_break: bool,
     #[subdiagnostic]
@@ -1103,7 +1103,7 @@ pub struct OutsideLoopSuggestion {
     #[suggestion_part(code = "'block: ")]
     pub block_span: Span,
     #[suggestion_part(code = " 'block")]
-    pub break_span: Span,
+    pub break_spans: Vec<Span>,
 }
 
 #[derive(Diagnostic)]
diff --git a/compiler/rustc_passes/src/loops.rs b/compiler/rustc_passes/src/loops.rs
@@ -1,3 +1,4 @@
+use rustc_data_structures::fx::FxHashMap;
 use Context::*;
 
 use rustc_hir as hir;
@@ -24,22 +25,40 @@ enum Context {
     Closure(Span),
     AsyncClosure(Span),
     UnlabeledBlock(Span),
+    IfUnlabeledBlock(Span),
     LabeledBlock,
     Constant,
 }
 
-#[derive(Copy, Clone)]
+#[derive(Clone)]
+struct BlockInfo {
+    name: String,
+    spans: Vec<Span>,
+    suggs: Vec<Span>,
+}
+
+#[derive(Clone)]
 struct CheckLoopVisitor<'a, 'tcx> {
     sess: &'a Session,
     tcx: TyCtxt<'tcx>,
-    cx: Context,
+    // Used for diagnostic like when in a `if` block with some `break`s,
+    // we should not suggest adding `'block` label in `if` block,
+    // we can back to outer block and add label there.
+    cx_stack: Vec<Context>,
+    blocks: Vec<Span>,
+    block_breaks: FxHashMap<Span, BlockInfo>,
 }
 
 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 },
-    );
+    let mut check = CheckLoopVisitor {
+        sess: tcx.sess,
+        tcx,
+        cx_stack: vec![Normal],
+        blocks: Default::default(),
+        block_breaks: Default::default(),
+    };
+    tcx.hir().visit_item_likes_in_module(module_def_id, &mut check);
+    check.report_outside_loop_error();
 }
 
 pub(crate) fn provide(providers: &mut Providers) {
@@ -82,6 +101,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(_) | IfUnlabeledBlock(_)
+                    )
+                {
+                    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(_) | IfUnlabeledBlock(_)
+                        )
+                    {
+                        self.with_context(IfUnlabeledBlock(b.span.shrink_to_lo()), |v| {
+                            v.visit_block(b)
+                        });
+                    } else {
+                        self.visit_expr(else_expr);
+                    }
+                }
+            }
             hir::ExprKind::Loop(ref b, _, source, _) => {
                 self.with_context(Loop(source), |v| v.visit_block(b));
             }
@@ -102,11 +150,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 +230,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 +252,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 +264,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,11 +280,31 @@ 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 });
-                self.sess.dcx().emit_err(OutsideLoop { span, name, is_break, suggestion });
+                if let Some(block) = self.block_breaks.get_mut(&block_span) {
+                    block.spans.push(span);
+                    block.suggs.push(break_span);
+                } else {
+                    self.blocks.push(block_span);
+                    self.block_breaks.insert(
+                        block_span,
+                        BlockInfo {
+                            name: name.to_string(),
+                            spans: vec![span],
+                            suggs: vec![break_span],
+                        },
+                    );
+                }
+            }
+            IfUnlabeledBlock(_) if is_break => {
+                self.require_break_cx(name, span, break_span, cx_pos - 1);
             }
-            Normal | Constant | Fn | UnlabeledBlock(_) => {
-                self.sess.dcx().emit_err(OutsideLoop { span, name, is_break, suggestion: None });
+            Normal | Constant | Fn | UnlabeledBlock(_) | IfUnlabeledBlock(_) => {
+                self.sess.dcx().emit_err(OutsideLoop {
+                    spans: vec![span],
+                    name,
+                    is_break,
+                    suggestion: None,
+                });
             }
         }
     }
@@ -246,12 +316,28 @@ 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 });
             return true;
         }
         false
     }
+
+    fn report_outside_loop_error(&mut self) {
+        self.blocks.iter().for_each(|s| {
+            if let Some(block) = self.block_breaks.get(s) {
+                self.sess.dcx().emit_err(OutsideLoop {
+                    spans: block.spans.clone(),
+                    name: &block.name,
+                    is_break: true,
+                    suggestion: Some(OutsideLoopSuggestion {
+                        block_span: *s,
+                        break_spans: block.suggs.clone(),
+                    }),
+                });
+            }
+        });
+    }
 }
diff --git a/tests/ui/loops/loop-if-else-break-issue-123261.fixed b/tests/ui/loops/loop-if-else-break-issue-123261.fixed
@@ -0,0 +1,31 @@
+//@ run-rustfix
+
+#![allow(unused)]
+
+fn main() {
+    let n = 1;
+    let m = 2;
+    let x = 'block: {
+        if n == 0 {
+            break 'block 1; //~ ERROR [E0268]
+        } else {
+            break 'block 2;
+        }
+    };
+
+    let y = 'block: {
+        if n == 0 {
+            break 'block 1; //~ ERROR [E0268]
+        }
+        break 'block 0;
+    };
+
+    let z = 'block: {
+        if n == 0 {
+            if m > 1 {
+                break 'block 3; //~ ERROR [E0268]
+            }
+        }
+        break 'block 1;
+    };
+}
diff --git a/tests/ui/loops/loop-if-else-break-issue-123261.rs b/tests/ui/loops/loop-if-else-break-issue-123261.rs
@@ -0,0 +1,31 @@
+//@ run-rustfix
+
+#![allow(unused)]
+
+fn main() {
+    let n = 1;
+    let m = 2;
+    let x = {
+        if n == 0 {
+            break 1; //~ ERROR [E0268]
+        } else {
+            break 2;
+        }
+    };
+
+    let y = {
+        if n == 0 {
+            break 1; //~ ERROR [E0268]
+        }
+        break 0;
+    };
+
+    let z = {
+        if n == 0 {
+            if m > 1 {
+                break 3; //~ ERROR [E0268]
+            }
+        }
+        break 1;
+    };
+}
diff --git a/tests/ui/loops/loop-if-else-break-issue-123261.stderr b/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:10:13
+   |
+LL |             break 1;
+   |             ^^^^^^^ cannot `break` outside of a loop or labeled block
+LL |         } else {
+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 ~     let x = 'block: {
+LL |         if n == 0 {
+LL ~             break 'block 1;
+LL |         } else {
+LL ~             break 'block 2;
+   |
+
+error[E0268]: `break` outside of a loop or labeled block
+  --> $DIR/loop-if-else-break-issue-123261.rs:18:13
+   |
+LL |             break 1;
+   |             ^^^^^^^ cannot `break` outside of a loop or labeled block
+LL |         }
+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 ~     let y = 'block: {
+LL |         if n == 0 {
+LL ~             break 'block 1;
+LL |         }
+LL ~         break 'block 0;
+   |
+
+error[E0268]: `break` outside of a loop or labeled block
+  --> $DIR/loop-if-else-break-issue-123261.rs:26:17
+   |
+LL |                 break 3;
+   |                 ^^^^^^^ cannot `break` outside of a loop or labeled block
+...
+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 z = 'block: {
+LL |         if n == 0 {
+LL |             if m > 1 {
+LL ~                 break 'block 3;
+LL |             }
+LL |         }
+LL ~         break 'block 1;
+   |
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0268`.
diff --git a/tests/ui/parser/break-in-unlabeled-block-in-macro.stderr b/tests/ui/parser/break-in-unlabeled-block-in-macro.stderr
@@ -20,6 +20,23 @@ LL |         foo!(());
    |
    = note: this error originates in the macro `foo` (in Nightly builds, run with -Z macro-backtrace for more info)
 
+error[E0268]: `break` outside of a loop or labeled block
+  --> $DIR/break-in-unlabeled-block-in-macro.rs:33:17
+   |
+LL |         foo!(=> break ());
+   |                 ^^^^^^^^ cannot `break` outside of a loop or labeled block
+
+error[E0268]: `break` outside of a loop or labeled block
+  --> $DIR/break-in-unlabeled-block-in-macro.rs:38:17
+   |
+LL |                 break ()
+   |                 ^^^^^^^^ cannot `break` outside of a loop or labeled block
+...
+LL |         bar!()
+   |         ------ in this macro invocation
+   |
+   = note: this error originates in the macro `bar` (in Nightly builds, run with -Z macro-backtrace for more info)
+
 error[E0268]: `break` outside of a loop or labeled block
   --> $DIR/break-in-unlabeled-block-in-macro.rs:27:19
    |
@@ -47,23 +64,6 @@ help: consider labeling this block to be able to break within it
 LL |         'block: { break 'block $e; }
    |         +++++++         ++++++
 
-error[E0268]: `break` outside of a loop or labeled block
-  --> $DIR/break-in-unlabeled-block-in-macro.rs:33:17
-   |
-LL |         foo!(=> break ());
-   |                 ^^^^^^^^ cannot `break` outside of a loop or labeled block
-
-error[E0268]: `break` outside of a loop or labeled block
-  --> $DIR/break-in-unlabeled-block-in-macro.rs:38:17
-   |
-LL |                 break ()
-   |                 ^^^^^^^^ cannot `break` outside of a loop or labeled block
-...
-LL |         bar!()
-   |         ------ in this macro invocation
-   |
-   = note: this error originates in the macro `bar` (in Nightly builds, run with -Z macro-backtrace for more info)
-
 error: aborting due to 6 previous errors
 
 For more information about this error, try `rustc --explain E0268`.
