Diagnose value breaks in incorrect breakables

5 files changed, 71 insertions, 38 deletions

diff --git a/crates/hir-ty/src/infer.rs b/crates/hir-ty/src/infer.rs
index d3e7f6e7dca..cca787befee 100644
--- a/crates/hir-ty/src/infer.rs
+++ b/crates/hir-ty/src/infer.rs
@@ -167,7 +167,8 @@ pub enum InferenceDiagnostic {
     NoSuchField { expr: ExprId },
     PrivateField { expr: ExprId, field: FieldId },
     PrivateAssocItem { id: ExprOrPatId, item: AssocItemId },
-    BreakOutsideOfLoop { expr: ExprId, is_break: bool },
+    // FIXME: Make this proper
+    BreakOutsideOfLoop { expr: ExprId, is_break: bool, bad_value_break: bool },
     MismatchedArgCount { call_expr: ExprId, expected: usize, found: usize },
 }
 
@@ -411,7 +412,7 @@ struct BreakableContext {
     /// Whether this context contains at least one break expression.
     may_break: bool,
     /// The coercion target of the context.
-    coerce: CoerceMany,
+    coerce: Option<CoerceMany>,
     /// The optional label of the context.
     label: Option<name::Name>,
     kind: BreakableKind,
diff --git a/crates/hir-ty/src/infer/expr.rs b/crates/hir-ty/src/infer/expr.rs
index b650afe9963..e64b020c7fb 100644
--- a/crates/hir-ty/src/infer/expr.rs
+++ b/crates/hir-ty/src/infer/expr.rs
@@ -133,7 +133,7 @@ impl<'a> InferenceContext<'a> {
                         let break_ty = self.table.new_type_var();
                         let (breaks, ty) = self.with_breakable_ctx(
                             BreakableKind::Block,
-                            break_ty.clone(),
+                            Some(break_ty.clone()),
                             *label,
                             |this| {
                                 this.infer_block(
@@ -153,7 +153,7 @@ impl<'a> InferenceContext<'a> {
             }
             Expr::Unsafe { body } => self.infer_expr(*body, expected),
             Expr::Const { body } => {
-                self.with_breakable_ctx(BreakableKind::Border, self.err_ty(), None, |this| {
+                self.with_breakable_ctx(BreakableKind::Border, None, None, |this| {
                     this.infer_expr(*body, expected)
                 })
                 .1
@@ -169,7 +169,7 @@ impl<'a> InferenceContext<'a> {
                 let ok_ty =
                     self.resolve_associated_type(try_ty.clone(), self.resolve_ops_try_output());
 
-                self.with_breakable_ctx(BreakableKind::Block, ok_ty.clone(), None, |this| {
+                self.with_breakable_ctx(BreakableKind::Block, Some(ok_ty.clone()), None, |this| {
                     this.infer_expr(*body, &Expectation::has_type(ok_ty));
                 });
 
@@ -183,7 +183,7 @@ impl<'a> InferenceContext<'a> {
                     mem::replace(&mut self.return_coercion, Some(CoerceMany::new(ret_ty.clone())));
 
                 let (_, inner_ty) =
-                    self.with_breakable_ctx(BreakableKind::Border, self.err_ty(), None, |this| {
+                    self.with_breakable_ctx(BreakableKind::Border, None, None, |this| {
                         this.infer_expr_coerce(*body, &Expectation::has_type(ret_ty))
                     });
 
@@ -203,7 +203,7 @@ impl<'a> InferenceContext<'a> {
                 // let ty = expected.coercion_target_type(&mut self.table);
                 let ty = self.table.new_type_var();
                 let (breaks, ()) =
-                    self.with_breakable_ctx(BreakableKind::Loop, ty, label, |this| {
+                    self.with_breakable_ctx(BreakableKind::Loop, Some(ty), label, |this| {
                         this.infer_expr(body, &Expectation::HasType(TyBuilder::unit()));
                     });
 
@@ -216,7 +216,7 @@ impl<'a> InferenceContext<'a> {
                 }
             }
             &Expr::While { condition, body, label } => {
-                self.with_breakable_ctx(BreakableKind::Loop, self.err_ty(), label, |this| {
+                self.with_breakable_ctx(BreakableKind::Loop, None, label, |this| {
                     this.infer_expr(
                         condition,
                         &Expectation::HasType(this.result.standard_types.bool_.clone()),
@@ -236,7 +236,7 @@ impl<'a> InferenceContext<'a> {
                     self.resolve_associated_type(into_iter_ty, self.resolve_iterator_item());
 
                 self.infer_top_pat(pat, &pat_ty);
-                self.with_breakable_ctx(BreakableKind::Loop, self.err_ty(), label, |this| {
+                self.with_breakable_ctx(BreakableKind::Loop, None, label, |this| {
                     this.infer_expr(body, &Expectation::HasType(TyBuilder::unit()));
                 });
 
@@ -321,7 +321,7 @@ impl<'a> InferenceContext<'a> {
                 let prev_resume_yield_tys =
                     mem::replace(&mut self.resume_yield_tys, resume_yield_tys);
 
-                self.with_breakable_ctx(BreakableKind::Border, self.err_ty(), None, |this| {
+                self.with_breakable_ctx(BreakableKind::Border, None, None, |this| {
                     this.infer_return(*body);
                 });
 
@@ -439,42 +439,50 @@ impl<'a> InferenceContext<'a> {
                     self.push_diagnostic(InferenceDiagnostic::BreakOutsideOfLoop {
                         expr: tgt_expr,
                         is_break: false,
+                        bad_value_break: false,
                     });
                 };
                 self.result.standard_types.never.clone()
             }
             Expr::Break { expr, label } => {
                 let val_ty = if let Some(expr) = *expr {
-                    let opt_coerce_to = find_breakable(&mut self.breakables, label.as_ref())
-                        .map(|ctxt| ctxt.coerce.expected_ty());
-                    self.infer_expr_inner(
-                        expr,
-                        &Expectation::HasType(opt_coerce_to.unwrap_or_else(|| self.err_ty())),
-                    )
+                    let opt_coerce_to = match find_breakable(&mut self.breakables, label.as_ref()) {
+                        Some(ctxt) => match &ctxt.coerce {
+                            Some(coerce) => coerce.expected_ty(),
+                            None => {
+                                self.push_diagnostic(InferenceDiagnostic::BreakOutsideOfLoop {
+                                    expr: tgt_expr,
+                                    is_break: true,
+                                    bad_value_break: true,
+                                });
+                                self.err_ty()
+                            }
+                        },
+                        None => self.err_ty(),
+                    };
+                    self.infer_expr_inner(expr, &Expectation::HasType(opt_coerce_to))
                 } else {
                     TyBuilder::unit()
                 };
 
                 match find_breakable(&mut self.breakables, label.as_ref()) {
-                    Some(ctxt) => {
-                        // avoiding the borrowck
-                        let mut coerce = mem::replace(
-                            &mut ctxt.coerce,
-                            CoerceMany::new(expected.coercion_target_type(&mut self.table)),
-                        );
-
-                        // FIXME: create a synthetic `()` during lowering so we have something to refer to here?
-                        coerce.coerce(self, *expr, &val_ty);
-
-                        let ctxt = find_breakable(&mut self.breakables, label.as_ref())
-                            .expect("breakable stack changed during coercion");
-                        ctxt.coerce = coerce;
-                        ctxt.may_break = true;
-                    }
+                    Some(ctxt) => match ctxt.coerce.take() {
+                        Some(mut coerce) => {
+                            coerce.coerce(self, *expr, &val_ty);
+
+                            // Avoiding borrowck
+                            let ctxt = find_breakable(&mut self.breakables, label.as_ref())
+                                .expect("breakable stack changed during coercion");
+                            ctxt.may_break = true;
+                            ctxt.coerce = Some(coerce);
+                        }
+                        None => ctxt.may_break = true,
+                    },
                     None => {
                         self.push_diagnostic(InferenceDiagnostic::BreakOutsideOfLoop {
                             expr: tgt_expr,
                             is_break: true,
+                            bad_value_break: false,
                         });
                     }
                 }
@@ -1712,16 +1720,16 @@ impl<'a> InferenceContext<'a> {
     fn with_breakable_ctx<T>(
         &mut self,
         kind: BreakableKind,
-        ty: Ty,
+        ty: Option<Ty>,
         label: Option<LabelId>,
         cb: impl FnOnce(&mut Self) -> T,
     ) -> (Option<Ty>, T) {
         self.breakables.push({
             let label = label.map(|label| self.body[label].name.clone());
-            BreakableContext { kind, may_break: false, coerce: CoerceMany::new(ty), label }
+            BreakableContext { kind, may_break: false, coerce: ty.map(CoerceMany::new), label }
         });
         let res = cb(self);
         let ctx = self.breakables.pop().expect("breakable stack broken");
-        (ctx.may_break.then(|| ctx.coerce.complete(self)), res)
+        (if ctx.may_break { ctx.coerce.map(|ctx| ctx.complete(self)) } else { None }, res)
     }
 }
diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs
index 3b2591e8a10..bb7468d4660 100644
--- a/crates/hir/src/diagnostics.rs
+++ b/crates/hir/src/diagnostics.rs
@@ -140,6 +140,7 @@ pub struct PrivateField {
 pub struct BreakOutsideOfLoop {
     pub expr: InFile<AstPtr<ast::Expr>>,
     pub is_break: bool,
+    pub bad_value_break: bool,
 }
 
 #[derive(Debug)]
diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs
index 846ad4007ce..c9ae12d8d33 100644
--- a/crates/hir/src/lib.rs
+++ b/crates/hir/src/lib.rs
@@ -1373,11 +1373,15 @@ impl DefWithBody {
                     let field = source_map.field_syntax(*expr);
                     acc.push(NoSuchField { field }.into())
                 }
-                &hir_ty::InferenceDiagnostic::BreakOutsideOfLoop { expr, is_break } => {
+                &hir_ty::InferenceDiagnostic::BreakOutsideOfLoop {
+                    expr,
+                    is_break,
+                    bad_value_break,
+                } => {
                     let expr = source_map
                         .expr_syntax(expr)
                         .expect("break outside of loop in synthetic syntax");
-                    acc.push(BreakOutsideOfLoop { expr, is_break }.into())
+                    acc.push(BreakOutsideOfLoop { expr, is_break, bad_value_break }.into())
                 }
                 hir_ty::InferenceDiagnostic::MismatchedArgCount { call_expr, expected, found } => {
                     match source_map.expr_syntax(*call_expr) {
diff --git a/crates/ide-diagnostics/src/handlers/break_outside_of_loop.rs b/crates/ide-diagnostics/src/handlers/break_outside_of_loop.rs
index 10e637979f2..114face2dca 100644
--- a/crates/ide-diagnostics/src/handlers/break_outside_of_loop.rs
+++ b/crates/ide-diagnostics/src/handlers/break_outside_of_loop.rs
@@ -7,10 +7,15 @@ pub(crate) fn break_outside_of_loop(
     ctx: &DiagnosticsContext<'_>,
     d: &hir::BreakOutsideOfLoop,
 ) -> Diagnostic {
-    let construct = if d.is_break { "break" } else { "continue" };
+    let message = if d.bad_value_break {
+        "can't break with a value in this position".to_owned()
+    } else {
+        let construct = if d.is_break { "break" } else { "continue" };
+        format!("{construct} outside of loop")
+    };
     Diagnostic::new(
         "break-outside-of-loop",
-        format!("{construct} outside of loop"),
+        message,
         ctx.sema.diagnostics_display_range(d.expr.clone().map(|it| it.into())).range,
     )
 }
@@ -135,4 +140,18 @@ fn foo() {
 "#,
         );
     }
+
+    #[test]
+    fn value_break_in_for_loop() {
+        check_diagnostics(
+            r#"
+fn test() {
+    for _ in [()] {
+        break 3;
+     // ^^^^^^^ error: can't break with a value in this position
+    }
+}
+"#,
+        );
+    }
 }