Rollup merge of #73726 - davidtwco:issue-73541-labelled-break-through-closure-async, r=petrochenkov

resolve: disallow labelled breaks/continues through closures/async blocks

Fixes #73541.

This PR modifies name resolution to prohibit labelled breaks/continues through closures or async blocks, fixing an ICE. In addition, it improves the diagnostics surrounding labelled breaks/continues through closures or async blocks by informing the user if the label exists in an parent scope and telling them that won't work.

r? @petrochenkov (resolve)
cc @estebank (diagnostic changes) @tmandry (issue is from `wg-async-foundations`)

Author: Manishearth

src/librustc_error_codes/error_codes.rs

@@ -448,6 +448,7 @@ E0763: include_str!("./error_codes/E0763.md"),
 E0764: include_str!("./error_codes/E0764.md"),
 E0765: include_str!("./error_codes/E0765.md"),
 E0766: include_str!("./error_codes/E0766.md"),
+E0767: include_str!("./error_codes/E0767.md"),
 ;
 //  E0006, // merged with E0005
 //  E0008, // cannot bind by-move into a pattern guard

src/librustc_error_codes/error_codes/E0767.md

@@ -0,0 +1,20 @@
+An unreachable label was used.
+
+Erroneous code example:
+
+```compile_fail,E0767
+'a: loop {
+    || {
+        loop { break 'a } // error: use of unreachable label `'a`
+    };
+}
+```
+
+Ensure that the label is within scope. Labels are not reachable through
+functions, closures, async blocks or modules. Example:
+
+```
+'a: loop {
+    break 'a; // ok!
+}
+```

src/librustc_passes/liveness.rs

@@ -1123,16 +1123,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
 
                 match target {
                     Some(b) => self.propagate_through_opt_expr(opt_expr.as_ref().map(|e| &**e), b),
-                    None => {
-                        // FIXME: This should have been checked earlier. Once this is fixed,
-                        // replace with `delay_span_bug`. (#62480)
-                        self.ir
-                            .tcx
-                            .sess
-                            .struct_span_err(expr.span, "`break` to unknown label")
-                            .emit();
-                        rustc_errors::FatalError.raise()
-                    }
+                    None => span_bug!(expr.span, "`break` to unknown label"),
                 }
             }
 

src/librustc_resolve/diagnostics.rs

@@ -33,6 +33,10 @@ type Res = def::Res<ast::NodeId>;
 /// A vector of spans and replacements, a message and applicability.
 crate type Suggestion = (Vec<(Span, String)>, String, Applicability);
 
+/// Potential candidate for an undeclared or out-of-scope label - contains the ident of a
+/// similarly named label and whether or not it is reachable.
+crate type LabelSuggestion = (Ident, bool);
+
 crate struct TypoSuggestion {
     pub candidate: Symbol,
     pub res: Res,
@@ -282,24 +286,39 @@ impl<'a> Resolver<'a> {
                 err.span_label(span, "used in a pattern more than once");
                 err
             }
-            ResolutionError::UndeclaredLabel(name, lev_candidate) => {
+            ResolutionError::UndeclaredLabel { name, suggestion } => {
                 let mut err = struct_span_err!(
                     self.session,
                     span,
                     E0426,
                     "use of undeclared label `{}`",
                     name
                 );
-                if let Some(lev_candidate) = lev_candidate {
-                    err.span_suggestion(
-                        span,
-                        "a label with a similar name exists in this scope",
-                        lev_candidate.to_string(),
-                        Applicability::MaybeIncorrect,
-                    );
-                } else {
-                    err.span_label(span, format!("undeclared label `{}`", name));
+
+                err.span_label(span, format!("undeclared label `{}`", name));
+
+                match suggestion {
+                    // A reachable label with a similar name exists.
+                    Some((ident, true)) => {
+                        err.span_label(ident.span, "a label with a similar name is reachable");
+                        err.span_suggestion(
+                            span,
+                            "try using similarly named label",
+                            ident.name.to_string(),
+                            Applicability::MaybeIncorrect,
+                        );
+                    }
+                    // An unreachable label with a similar name exists.
+                    Some((ident, false)) => {
+                        err.span_label(
+                            ident.span,
+                            "a label with a similar name exists but is unreachable",
+                        );
+                    }
+                    // No similarly-named labels exist.
+                    None => (),
                 }
+
                 err
             }
             ResolutionError::SelfImportsOnlyAllowedWithin { root, span_with_rename } => {
@@ -433,6 +452,45 @@ impl<'a> Resolver<'a> {
                 err.span_label(span, "`Self` in type parameter default".to_string());
                 err
             }
+            ResolutionError::UnreachableLabel { name, definition_span, suggestion } => {
+                let mut err = struct_span_err!(
+                    self.session,
+                    span,
+                    E0767,
+                    "use of unreachable label `{}`",
+                    name,
+                );
+
+                err.span_label(definition_span, "unreachable label defined here");
+                err.span_label(span, format!("unreachable label `{}`", name));
+                err.note(
+                    "labels are unreachable through functions, closures, async blocks and modules",
+                );
+
+                match suggestion {
+                    // A reachable label with a similar name exists.
+                    Some((ident, true)) => {
+                        err.span_label(ident.span, "a label with a similar name is reachable");
+                        err.span_suggestion(
+                            span,
+                            "try using similarly named label",
+                            ident.name.to_string(),
+                            Applicability::MaybeIncorrect,
+                        );
+                    }
+                    // An unreachable label with a similar name exists.
+                    Some((ident, false)) => {
+                        err.span_label(
+                            ident.span,
+                            "a label with a similar name exists but is also unreachable",
+                        );
+                    }
+                    // No similarly-named labels exist.
+                    None => (),
+                }
+
+                err
+            }
         }
     }
 

src/librustc_resolve/late.rs

@@ -13,7 +13,6 @@ use crate::{ResolutionError, Resolver, Segment, UseError};
 
 use rustc_ast::ast::*;
 use rustc_ast::ptr::P;
-use rustc_ast::util::lev_distance::find_best_match_for_name;
 use rustc_ast::visit::{self, AssocCtxt, FnCtxt, FnKind, Visitor};
 use rustc_ast::{unwrap_or, walk_list};
 use rustc_ast_lowering::ResolverAstLowering;
@@ -101,6 +100,9 @@ crate enum RibKind<'a> {
     /// upvars).
     AssocItemRibKind,
 
+    /// We passed through a closure. Disallow labels.
+    ClosureOrAsyncRibKind,
+
     /// We passed through a function definition. Disallow upvars.
     /// Permit only those const parameters that are specified in the function's generics.
     FnItemRibKind,
@@ -124,11 +126,15 @@ crate enum RibKind<'a> {
 }
 
 impl RibKind<'_> {
-    // Whether this rib kind contains generic parameters, as opposed to local
-    // variables.
+    /// Whether this rib kind contains generic parameters, as opposed to local
+    /// variables.
     crate fn contains_params(&self) -> bool {
         match self {
-            NormalRibKind | FnItemRibKind | ConstantItemRibKind | ModuleRibKind(_)
+            NormalRibKind
+            | ClosureOrAsyncRibKind
+            | FnItemRibKind
+            | ConstantItemRibKind
+            | ModuleRibKind(_)
             | MacroDefinition(_) => false,
             AssocItemRibKind | ItemRibKind(_) | ForwardTyParamBanRibKind => true,
         }
@@ -474,7 +480,8 @@ impl<'a, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> {
             // Bail if there's no body.
             FnKind::Fn(.., None) => return visit::walk_fn(self, fn_kind, sp),
             FnKind::Fn(FnCtxt::Free | FnCtxt::Foreign, ..) => FnItemRibKind,
-            FnKind::Fn(FnCtxt::Assoc(_), ..) | FnKind::Closure(..) => NormalRibKind,
+            FnKind::Fn(FnCtxt::Assoc(_), ..) => NormalRibKind,
+            FnKind::Closure(..) => ClosureOrAsyncRibKind,
         };
         let previous_value =
             replace(&mut self.diagnostic_metadata.current_function, Some((fn_kind, sp)));
@@ -725,37 +732,81 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
         }
     }
 
-    /// Searches the current set of local scopes for labels. Returns the first non-`None` label that
-    /// is returned by the given predicate function
-    ///
-    /// Stops after meeting a closure.
-    fn search_label<P, R>(&self, mut ident: Ident, pred: P) -> Option<R>
-    where
-        P: Fn(&Rib<'_, NodeId>, Ident) -> Option<R>,
-    {
-        for rib in self.label_ribs.iter().rev() {
-            match rib.kind {
-                NormalRibKind => {}
+    /// Searches the current set of local scopes for labels. Returns the `NodeId` of the resolved
+    /// label and reports an error if the label is not found or is unreachable.
+    fn resolve_label(&self, mut label: Ident) -> Option<NodeId> {
+        let mut suggestion = None;
+
+        // Preserve the original span so that errors contain "in this macro invocation"
+        // information.
+        let original_span = label.span;
+
+        for i in (0..self.label_ribs.len()).rev() {
+            let rib = &self.label_ribs[i];
+
+            if let MacroDefinition(def) = rib.kind {
                 // If an invocation of this macro created `ident`, give up on `ident`
                 // and switch to `ident`'s source from the macro definition.
-                MacroDefinition(def) => {
-                    if def == self.r.macro_def(ident.span.ctxt()) {
-                        ident.span.remove_mark();
-                    }
-                }
-                _ => {
-                    // Do not resolve labels across function boundary
-                    return None;
+                if def == self.r.macro_def(label.span.ctxt()) {
+                    label.span.remove_mark();
                 }
             }
-            let r = pred(rib, ident);
-            if r.is_some() {
-                return r;
+
+            let ident = label.normalize_to_macro_rules();
+            if let Some((ident, id)) = rib.bindings.get_key_value(&ident) {
+                return if self.is_label_valid_from_rib(i) {
+                    Some(*id)
+                } else {
+                    self.r.report_error(
+                        original_span,
+                        ResolutionError::UnreachableLabel {
+                            name: &label.name.as_str(),
+                            definition_span: ident.span,
+                            suggestion,
+                        },
+                    );
+
+                    None
+                };
             }
+
+            // Diagnostics: Check if this rib contains a label with a similar name, keep track of
+            // the first such label that is encountered.
+            suggestion = suggestion.or_else(|| self.suggestion_for_label_in_rib(i, label));
         }
+
+        self.r.report_error(
+            original_span,
+            ResolutionError::UndeclaredLabel { name: &label.name.as_str(), suggestion },
+        );
         None
     }
 
+    /// Determine whether or not a label from the `rib_index`th label rib is reachable.
+    fn is_label_valid_from_rib(&self, rib_index: usize) -> bool {
+        let ribs = &self.label_ribs[rib_index + 1..];
+
+        for rib in ribs {
+            match rib.kind {
+                NormalRibKind | MacroDefinition(..) => {
+                    // Nothing to do. Continue.
+                }
+
+                AssocItemRibKind
+                | ClosureOrAsyncRibKind
+                | FnItemRibKind
+                | ItemRibKind(..)
+                | ConstantItemRibKind
+                | ModuleRibKind(..)
+                | ForwardTyParamBanRibKind => {
+                    return false;
+                }
+            }
+        }
+
+        true
+    }
+
     fn resolve_adt(&mut self, item: &'ast Item, generics: &'ast Generics) {
         debug!("resolve_adt");
         self.with_current_self_item(item, |this| {
@@ -2044,35 +2095,10 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
             }
 
             ExprKind::Break(Some(label), _) | ExprKind::Continue(Some(label)) => {
-                let node_id = self.search_label(label.ident, |rib, ident| {
-                    rib.bindings.get(&ident.normalize_to_macro_rules()).cloned()
-                });
-                match node_id {
-                    None => {
-                        // Search again for close matches...
-                        // Picks the first label that is "close enough", which is not necessarily
-                        // the closest match
-                        let close_match = self.search_label(label.ident, |rib, ident| {
-                            let names = rib.bindings.iter().filter_map(|(id, _)| {
-                                if id.span.ctxt() == label.ident.span.ctxt() {
-                                    Some(&id.name)
-                                } else {
-                                    None
-                                }
-                            });
-                            find_best_match_for_name(names, &ident.as_str(), None)
-                        });
-                        self.r.record_partial_res(expr.id, PartialRes::new(Res::Err));
-                        self.r.report_error(
-                            label.ident.span,
-                            ResolutionError::UndeclaredLabel(&label.ident.as_str(), close_match),
-                        );
-                    }
-                    Some(node_id) => {
-                        // Since this res is a label, it is never read.
-                        self.r.label_res_map.insert(expr.id, node_id);
-                        self.diagnostic_metadata.unused_labels.remove(&node_id);
-                    }
+                if let Some(node_id) = self.resolve_label(label.ident) {
+                    // Since this res is a label, it is never read.
+                    self.r.label_res_map.insert(expr.id, node_id);
+                    self.diagnostic_metadata.unused_labels.remove(&node_id);
                 }
 
                 // visit `break` argument if any
@@ -2144,21 +2170,26 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
             // closure are detected as upvars rather than normal closure arg usages.
             ExprKind::Closure(_, Async::Yes { .. }, _, ref fn_decl, ref body, _span) => {
                 self.with_rib(ValueNS, NormalRibKind, |this| {
-                    // Resolve arguments:
-                    this.resolve_params(&fn_decl.inputs);
-                    // No need to resolve return type --
-                    // the outer closure return type is `FnRetTy::Default`.
+                    this.with_label_rib(ClosureOrAsyncRibKind, |this| {
+                        // Resolve arguments:
+                        this.resolve_params(&fn_decl.inputs);
+                        // No need to resolve return type --
+                        // the outer closure return type is `FnRetTy::Default`.
 
-                    // Now resolve the inner closure
-                    {
-                        // No need to resolve arguments: the inner closure has none.
-                        // Resolve the return type:
-                        visit::walk_fn_ret_ty(this, &fn_decl.output);
-                        // Resolve the body
-                        this.visit_expr(body);
-                    }
+                        // Now resolve the inner closure
+                        {
+                            // No need to resolve arguments: the inner closure has none.
+                            // Resolve the return type:
+                            visit::walk_fn_ret_ty(this, &fn_decl.output);
+                            // Resolve the body
+                            this.visit_expr(body);
+                        }
+                    })
                 });
             }
+            ExprKind::Async(..) | ExprKind::Closure(..) => {
+                self.with_label_rib(ClosureOrAsyncRibKind, |this| visit::walk_expr(this, expr));
+            }
             _ => {
                 visit::walk_expr(self, expr);
             }