Auto merge of #13165 - Veykril:breakables, r=Veykril

Properly handle break resolution inside non-breakable expressions

We now diagnose invalid `continue` expressions and properly handle things like `async` blocks which prevent labels from resolving further. Cleaned this up since `label_break_value` is on the way to stabilization in rust (🎉 finally) and we weren't handling breaks for blocks properly yet.

6 files changed, 312 insertions, 147 deletions

diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index a70252fa65a..1563ee0b143 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -6,15 +6,15 @@ on:
   pull_request:
   push:
     branches:
-    - auto
-    - try
+      - auto
+      - try
 
 env:
   CARGO_INCREMENTAL: 0
   CARGO_NET_RETRY: 10
   CI: 1
   RUST_BACKTRACE: short
-  RUSTFLAGS: "-D warnings -W unreachable-pub -W rust-2021-compatibility"
+  RUSTFLAGS: "-D warnings -W unreachable-pub -W bare-trait-objects"
   RUSTUP_MAX_RETRIES: 10
 
 jobs:
@@ -31,25 +31,25 @@ jobs:
         os: [ubuntu-latest, windows-latest, macos-latest]
 
     steps:
-    - name: Checkout repository
-      uses: actions/checkout@v3
-      with:
-        ref: ${{ github.event.pull_request.head.sha }}
-        fetch-depth: 20
+      - name: Checkout repository
+        uses: actions/checkout@v3
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+          fetch-depth: 20
 
-    - name: Install Rust toolchain
-      run: |
-        rustup update --no-self-update stable
-        rustup component add rustfmt rust-src
+      - name: Install Rust toolchain
+        run: |
+          rustup update --no-self-update stable
+          rustup component add rustfmt rust-src
 
-    - name: Cache Dependencies
-      uses: Swatinem/rust-cache@ce325b60658c1b38465c06cc965b79baf32c1e72
+      - name: Cache Dependencies
+        uses: Swatinem/rust-cache@ce325b60658c1b38465c06cc965b79baf32c1e72
 
-    - name: Compile
-      run: cargo test --no-run --locked
+      - name: Compile
+        run: cargo test --no-run --locked
 
-    - name: Test
-      run: cargo test -- --nocapture --quiet
+      - name: Test
+        run: cargo test -- --nocapture --quiet
 
   # Weird targets to catch non-portable code
   rust-cross:
@@ -64,25 +64,25 @@ jobs:
       targets_ide: "wasm32-unknown-unknown"
 
     steps:
-    - name: Checkout repository
-      uses: actions/checkout@v3
-
-    - name: Install Rust toolchain
-      run: |
-        rustup update --no-self-update stable
-        rustup target add ${{ env.targets }} ${{ env.targets_ide }}
-
-    - name: Cache Dependencies
-      uses: Swatinem/rust-cache@ce325b60658c1b38465c06cc965b79baf32c1e72
-
-    - name: Check
-      run: |
-        for target in ${{ env.targets }}; do
-          cargo check --target=$target --all-targets
-        done
-        for target in ${{ env.targets_ide }}; do
-          cargo check -p ide --target=$target --all-targets
-        done
+      - name: Checkout repository
+        uses: actions/checkout@v3
+
+      - name: Install Rust toolchain
+        run: |
+          rustup update --no-self-update stable
+          rustup target add ${{ env.targets }} ${{ env.targets_ide }}
+
+      - name: Cache Dependencies
+        uses: Swatinem/rust-cache@ce325b60658c1b38465c06cc965b79baf32c1e72
+
+      - name: Check
+        run: |
+          for target in ${{ env.targets }}; do
+            cargo check --target=$target --all-targets
+          done
+          for target in ${{ env.targets_ide }}; do
+            cargo check -p ide --target=$target --all-targets
+          done
 
   typescript:
     if: github.repository == 'rust-lang/rust-analyzer'
@@ -95,47 +95,47 @@ jobs:
     runs-on: ${{ matrix.os }}
 
     steps:
-    - name: Checkout repository
-      uses: actions/checkout@v3
-
-    - name: Install Nodejs
-      uses: actions/setup-node@v1
-      with:
-        node-version: 16.x
-
-    - name: Install xvfb
-      if: matrix.os == 'ubuntu-latest'
-      run: sudo apt-get install -y xvfb
-
-    - run: npm ci
-      working-directory: ./editors/code
-
-#    - run: npm audit || { sleep 10 && npm audit; } || { sleep 30 && npm audit; }
-#      if: runner.os == 'Linux'
-#      working-directory: ./editors/code
-
-    - run: npm run lint
-      working-directory: ./editors/code
-
-    - name: Run VS Code tests (Linux)
-      if: matrix.os == 'ubuntu-latest'
-      env:
-        VSCODE_CLI: 1
-      run: xvfb-run npm test
-      working-directory: ./editors/code
-
-    - name: Run VS Code tests (Windows)
-      if: matrix.os == 'windows-latest'
-      env:
-        VSCODE_CLI: 1
-      run: npm test
-      working-directory: ./editors/code
-
-    - run: npm run pretest
-      working-directory: ./editors/code
-
-    - run: npm run package --scripts-prepend-node-path
-      working-directory: ./editors/code
+      - name: Checkout repository
+        uses: actions/checkout@v3
+
+      - name: Install Nodejs
+        uses: actions/setup-node@v1
+        with:
+          node-version: 16.x
+
+      - name: Install xvfb
+        if: matrix.os == 'ubuntu-latest'
+        run: sudo apt-get install -y xvfb
+
+      - run: npm ci
+        working-directory: ./editors/code
+
+      #    - run: npm audit || { sleep 10 && npm audit; } || { sleep 30 && npm audit; }
+      #      if: runner.os == 'Linux'
+      #      working-directory: ./editors/code
+
+      - run: npm run lint
+        working-directory: ./editors/code
+
+      - name: Run VS Code tests (Linux)
+        if: matrix.os == 'ubuntu-latest'
+        env:
+          VSCODE_CLI: 1
+        run: xvfb-run npm test
+        working-directory: ./editors/code
+
+      - name: Run VS Code tests (Windows)
+        if: matrix.os == 'windows-latest'
+        env:
+          VSCODE_CLI: 1
+        run: npm test
+        working-directory: ./editors/code
+
+      - run: npm run pretest
+        working-directory: ./editors/code
+
+      - run: npm run package --scripts-prepend-node-path
+        working-directory: ./editors/code
 
   end-success:
     name: bors build finished
diff --git a/crates/hir-ty/src/infer.rs b/crates/hir-ty/src/infer.rs
index 5df48e5fdcb..10ffde87eef 100644
--- a/crates/hir-ty/src/infer.rs
+++ b/crates/hir-ty/src/infer.rs
@@ -182,7 +182,7 @@ pub(crate) type InferResult<T> = Result<InferOk<T>, TypeError>;
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub enum InferenceDiagnostic {
     NoSuchField { expr: ExprId },
-    BreakOutsideOfLoop { expr: ExprId },
+    BreakOutsideOfLoop { expr: ExprId, is_break: bool },
     MismatchedArgCount { call_expr: ExprId, expected: usize, found: usize },
 }
 
@@ -418,18 +418,45 @@ pub(crate) struct InferenceContext<'a> {
 
 #[derive(Clone, Debug)]
 struct BreakableContext {
+    /// Whether this context contains at least one break expression.
     may_break: bool,
+    /// The coercion target of the context.
     coerce: CoerceMany,
+    /// The optional label of the context.
     label: Option<name::Name>,
+    kind: BreakableKind,
+}
+
+#[derive(Clone, Debug)]
+enum BreakableKind {
+    Block,
+    Loop,
+    /// A border is something like an async block, closure etc. Anything that prevents
+    /// breaking/continuing through
+    Border,
 }
 
 fn find_breakable<'c>(
     ctxs: &'c mut [BreakableContext],
     label: Option<&name::Name>,
 ) -> Option<&'c mut BreakableContext> {
+    let mut ctxs = ctxs
+        .iter_mut()
+        .rev()
+        .take_while(|it| matches!(it.kind, BreakableKind::Block | BreakableKind::Loop));
+    match label {
+        Some(_) => ctxs.find(|ctx| ctx.label.as_ref() == label),
+        None => ctxs.find(|ctx| matches!(ctx.kind, BreakableKind::Loop)),
+    }
+}
+
+fn find_continuable<'c>(
+    ctxs: &'c mut [BreakableContext],
+    label: Option<&name::Name>,
+) -> Option<&'c mut BreakableContext> {
     match label {
-        Some(_) => ctxs.iter_mut().rev().find(|ctx| ctx.label.as_ref() == label),
-        None => ctxs.last_mut(),
+        Some(_) => find_breakable(ctxs, label).filter(|it| matches!(it.kind, BreakableKind::Loop)),
+        None => find_breakable(ctxs, label),
     }
 }
 
diff --git a/crates/hir-ty/src/infer/expr.rs b/crates/hir-ty/src/infer/expr.rs
index a42a00ea598..f3f4ee65bb2 100644
--- a/crates/hir-ty/src/infer/expr.rs
+++ b/crates/hir-ty/src/infer/expr.rs
@@ -10,7 +10,7 @@ use chalk_ir::{
     cast::Cast, fold::Shift, DebruijnIndex, GenericArgData, Mutability, TyVariableKind,
 };
 use hir_def::{
-    expr::{ArithOp, Array, BinaryOp, CmpOp, Expr, ExprId, Literal, Statement, UnaryOp},
+    expr::{ArithOp, Array, BinaryOp, CmpOp, Expr, ExprId, LabelId, Literal, Statement, UnaryOp},
     generics::TypeOrConstParamData,
     path::{GenericArg, GenericArgs},
     resolver::resolver_for_expr,
@@ -23,7 +23,7 @@ use syntax::ast::RangeOp;
 use crate::{
     autoderef::{self, Autoderef},
     consteval,
-    infer::coerce::CoerceMany,
+    infer::{coerce::CoerceMany, find_continuable, BreakableKind},
     lower::{
         const_or_path_to_chalk, generic_arg_to_chalk, lower_to_chalk_mutability, ParamLoweringMode,
     },
@@ -120,32 +120,37 @@ impl<'a> InferenceContext<'a> {
                 let ty = match label {
                     Some(_) => {
                         let break_ty = self.table.new_type_var();
-                        self.breakables.push(BreakableContext {
-                            may_break: false,
-                            coerce: CoerceMany::new(break_ty.clone()),
-                            label: label.map(|label| self.body[label].name.clone()),
-                        });
-                        let ty = self.infer_block(
-                            tgt_expr,
-                            statements,
-                            *tail,
-                            &Expectation::has_type(break_ty),
+                        let (breaks, ty) = self.with_breakable_ctx(
+                            BreakableKind::Block,
+                            break_ty.clone(),
+                            *label,
+                            |this| {
+                                this.infer_block(
+                                    tgt_expr,
+                                    statements,
+                                    *tail,
+                                    &Expectation::has_type(break_ty),
+                                )
+                            },
                         );
-                        let ctxt = self.breakables.pop().expect("breakable stack broken");
-                        if ctxt.may_break {
-                            ctxt.coerce.complete()
-                        } else {
-                            ty
-                        }
+                        breaks.unwrap_or(ty)
                     }
                     None => self.infer_block(tgt_expr, statements, *tail, expected),
                 };
                 self.resolver = old_resolver;
                 ty
             }
-            Expr::Unsafe { body } | Expr::Const { body } => self.infer_expr(*body, expected),
+            Expr::Unsafe { body } => self.infer_expr(*body, expected),
+            Expr::Const { body } => {
+                self.with_breakable_ctx(BreakableKind::Border, self.err_ty(), None, |this| {
+                    this.infer_expr(*body, expected)
+                })
+                .1
+            }
             Expr::TryBlock { body } => {
-                let _inner = self.infer_expr(*body, expected);
+                self.with_breakable_ctx(BreakableKind::Border, self.err_ty(), None, |this| {
+                    let _inner = this.infer_expr(*body, expected);
+                });
                 // FIXME should be std::result::Result<{inner}, _>
                 self.err_ty()
             }
@@ -154,7 +159,10 @@ impl<'a> InferenceContext<'a> {
                 let prev_diverges = mem::replace(&mut self.diverges, Diverges::Maybe);
                 let prev_ret_ty = mem::replace(&mut self.return_ty, ret_ty.clone());
 
-                let inner_ty = self.infer_expr_coerce(*body, &Expectation::has_type(ret_ty));
+                let (_, inner_ty) =
+                    self.with_breakable_ctx(BreakableKind::Border, self.err_ty(), None, |this| {
+                        this.infer_expr_coerce(*body, &Expectation::has_type(ret_ty))
+                    });
 
                 self.diverges = prev_diverges;
                 self.return_ty = prev_ret_ty;
@@ -166,54 +174,44 @@ impl<'a> InferenceContext<'a> {
                 TyKind::OpaqueType(opaque_ty_id, Substitution::from1(Interner, inner_ty))
                     .intern(Interner)
             }
-            Expr::Loop { body, label } => {
-                self.breakables.push(BreakableContext {
-                    may_break: false,
-                    coerce: CoerceMany::new(self.table.new_type_var()),
-                    label: label.map(|label| self.body[label].name.clone()),
-                });
-                self.infer_expr(*body, &Expectation::has_type(TyBuilder::unit()));
-
-                let ctxt = self.breakables.pop().expect("breakable stack broken");
+            &Expr::Loop { body, label } => {
+                let ty = self.table.new_type_var();
+                let (breaks, ()) =
+                    self.with_breakable_ctx(BreakableKind::Loop, ty, label, |this| {
+                        this.infer_expr(body, &Expectation::has_type(TyBuilder::unit()));
+                    });
 
-                if ctxt.may_break {
-                    self.diverges = Diverges::Maybe;
-                    ctxt.coerce.complete()
-                } else {
-                    TyKind::Never.intern(Interner)
+                match breaks {
+                    Some(breaks) => {
+                        self.diverges = Diverges::Maybe;
+                        breaks
+                    }
+                    None => TyKind::Never.intern(Interner),
                 }
             }
-            Expr::While { condition, body, label } => {
-                self.breakables.push(BreakableContext {
-                    may_break: false,
-                    coerce: CoerceMany::new(self.err_ty()),
-                    label: label.map(|label| self.body[label].name.clone()),
+            &Expr::While { condition, body, label } => {
+                self.with_breakable_ctx(BreakableKind::Loop, self.err_ty(), label, |this| {
+                    this.infer_expr(
+                        condition,
+                        &Expectation::has_type(TyKind::Scalar(Scalar::Bool).intern(Interner)),
+                    );
+                    this.infer_expr(body, &Expectation::has_type(TyBuilder::unit()));
                 });
-                self.infer_expr(
-                    *condition,
-                    &Expectation::has_type(TyKind::Scalar(Scalar::Bool).intern(Interner)),
-                );
-                self.infer_expr(*body, &Expectation::has_type(TyBuilder::unit()));
-                let _ctxt = self.breakables.pop().expect("breakable stack broken");
+
                 // the body may not run, so it diverging doesn't mean we diverge
                 self.diverges = Diverges::Maybe;
                 TyBuilder::unit()
             }
-            Expr::For { iterable, body, pat, label } => {
-                let iterable_ty = self.infer_expr(*iterable, &Expectation::none());
-
-                self.breakables.push(BreakableContext {
-                    may_break: false,
-                    coerce: CoerceMany::new(self.err_ty()),
-                    label: label.map(|label| self.body[label].name.clone()),
-                });
+            &Expr::For { iterable, body, pat, label } => {
+                let iterable_ty = self.infer_expr(iterable, &Expectation::none());
                 let pat_ty =
                     self.resolve_associated_type(iterable_ty, self.resolve_into_iter_item());
 
-                self.infer_pat(*pat, &pat_ty, BindingMode::default());
+                self.infer_pat(pat, &pat_ty, BindingMode::default());
+                self.with_breakable_ctx(BreakableKind::Loop, self.err_ty(), label, |this| {
+                    this.infer_expr(body, &Expectation::has_type(TyBuilder::unit()));
+                });
 
-                self.infer_expr(*body, &Expectation::has_type(TyBuilder::unit()));
-                let _ctxt = self.breakables.pop().expect("breakable stack broken");
                 // the body may not run, so it diverging doesn't mean we diverge
                 self.diverges = Diverges::Maybe;
                 TyBuilder::unit()
@@ -269,7 +267,9 @@ impl<'a> InferenceContext<'a> {
                 let prev_diverges = mem::replace(&mut self.diverges, Diverges::Maybe);
                 let prev_ret_ty = mem::replace(&mut self.return_ty, ret_ty.clone());
 
-                self.infer_expr_coerce(*body, &Expectation::has_type(ret_ty));
+                self.with_breakable_ctx(BreakableKind::Border, self.err_ty(), None, |this| {
+                    this.infer_expr_coerce(*body, &Expectation::has_type(ret_ty));
+                });
 
                 self.diverges = prev_diverges;
                 self.return_ty = prev_ret_ty;
@@ -372,7 +372,15 @@ impl<'a> InferenceContext<'a> {
                 let resolver = resolver_for_expr(self.db.upcast(), self.owner, tgt_expr);
                 self.infer_path(&resolver, p, tgt_expr.into()).unwrap_or_else(|| self.err_ty())
             }
-            Expr::Continue { .. } => TyKind::Never.intern(Interner),
+            Expr::Continue { label } => {
+                if let None = find_continuable(&mut self.breakables, label.as_ref()) {
+                    self.push_diagnostic(InferenceDiagnostic::BreakOutsideOfLoop {
+                        expr: tgt_expr,
+                        is_break: false,
+                    });
+                };
+                TyKind::Never.intern(Interner)
+            }
             Expr::Break { expr, label } => {
                 let mut coerce = match find_breakable(&mut self.breakables, label.as_ref()) {
                     Some(ctxt) => {
@@ -400,6 +408,7 @@ impl<'a> InferenceContext<'a> {
                 } else {
                     self.push_diagnostic(InferenceDiagnostic::BreakOutsideOfLoop {
                         expr: tgt_expr,
+                        is_break: true,
                     });
                 };
 
@@ -1472,4 +1481,20 @@ impl<'a> InferenceContext<'a> {
             },
         })
     }
+
+    fn with_breakable_ctx<T>(
+        &mut self,
+        kind: BreakableKind,
+        ty: 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 }
+        });
+        let res = cb(self);
+        let ctx = self.breakables.pop().expect("breakable stack broken");
+        (ctx.may_break.then(|| ctx.coerce.complete()), res)
+    }
 }
diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs
index 50374f4b3fe..5edc16d8bce 100644
--- a/crates/hir/src/diagnostics.rs
+++ b/crates/hir/src/diagnostics.rs
@@ -124,6 +124,7 @@ pub struct NoSuchField {
 #[derive(Debug)]
 pub struct BreakOutsideOfLoop {
     pub expr: InFile<AstPtr<ast::Expr>>,
+    pub is_break: bool,
 }
 
 #[derive(Debug)]
diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs
index 6dccf2ed20b..e4bb63a8647 100644
--- a/crates/hir/src/lib.rs
+++ b/crates/hir/src/lib.rs
@@ -1216,11 +1216,11 @@ impl DefWithBody {
                     let field = source_map.field_syntax(*expr);
                     acc.push(NoSuchField { field }.into())
                 }
-                hir_ty::InferenceDiagnostic::BreakOutsideOfLoop { expr } => {
+                &hir_ty::InferenceDiagnostic::BreakOutsideOfLoop { expr, is_break } => {
                     let expr = source_map
-                        .expr_syntax(*expr)
+                        .expr_syntax(expr)
                         .expect("break outside of loop in synthetic syntax");
-                    acc.push(BreakOutsideOfLoop { expr }.into())
+                    acc.push(BreakOutsideOfLoop { expr, is_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 d12594a4ce5..0c92e706b39 100644
--- a/crates/ide-diagnostics/src/handlers/break_outside_of_loop.rs
+++ b/crates/ide-diagnostics/src/handlers/break_outside_of_loop.rs
@@ -7,9 +7,10 @@ pub(crate) fn break_outside_of_loop(
     ctx: &DiagnosticsContext<'_>,
     d: &hir::BreakOutsideOfLoop,
 ) -> Diagnostic {
+    let construct = if d.is_break { "break" } else { "continue" };
     Diagnostic::new(
         "break-outside-of-loop",
-        "break outside of loop",
+        format!("{construct} outside of loop"),
         ctx.sema.diagnostics_display_range(d.expr.clone().map(|it| it.into())).range,
     )
 }
@@ -19,11 +20,122 @@ mod tests {
     use crate::tests::check_diagnostics;
 
     #[test]
-    fn break_outside_of_loop() {
+    fn outside_of_loop() {
         check_diagnostics(
             r#"
-fn foo() { break; }
-         //^^^^^ error: break outside of loop
+fn foo() {
+    break;
+  //^^^^^ error: break outside of loop
+    break 'a;
+  //^^^^^^^^ error: break outside of loop
+    continue;
+  //^^^^^^^^ error: continue outside of loop
+    continue 'a;
+  //^^^^^^^^^^^ error: continue outside of loop
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn try_blocks_are_borders() {
+        check_diagnostics(
+            r#"
+fn foo() {
+    'a: loop {
+        try {
+                break;
+              //^^^^^ error: break outside of loop
+                break 'a;
+              //^^^^^^^^ error: break outside of loop
+                continue;
+              //^^^^^^^^ error: continue outside of loop
+                continue 'a;
+              //^^^^^^^^^^^ error: continue outside of loop
+        };
+    }
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn async_blocks_are_borders() {
+        check_diagnostics(
+            r#"
+fn foo() {
+    'a: loop {
+        try {
+                break;
+              //^^^^^ error: break outside of loop
+                break 'a;
+              //^^^^^^^^ error: break outside of loop
+                continue;
+              //^^^^^^^^ error: continue outside of loop
+                continue 'a;
+              //^^^^^^^^^^^ error: continue outside of loop
+        };
+    }
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn closures_are_borders() {
+        check_diagnostics(
+            r#"
+fn foo() {
+    'a: loop {
+        try {
+                break;
+              //^^^^^ error: break outside of loop
+                break 'a;
+              //^^^^^^^^ error: break outside of loop
+                continue;
+              //^^^^^^^^ error: continue outside of loop
+                continue 'a;
+              //^^^^^^^^^^^ error: continue outside of loop
+        };
+    }
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn blocks_pass_through() {
+        check_diagnostics(
+            r#"
+fn foo() {
+    'a: loop {
+        {
+            break;
+            break 'a;
+            continue;
+            continue 'a;
+        }
+    }
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn label_blocks() {
+        check_diagnostics(
+            r#"
+fn foo() {
+    'a: {
+        break;
+      //^^^^^ error: break outside of loop
+        break 'a;
+        continue;
+      //^^^^^^^^ error: continue outside of loop
+        continue 'a;
+      //^^^^^^^^^^^ error: continue outside of loop
+    }
+}
 "#,
         );
     }