Implement #[must_not_suspend]

compiler/rustc_feature/src/active.rs

@@ -679,6 +679,10 @@ declare_features! (
     /// Allows `let...else` statements.
     (active, let_else, "1.56.0", Some(87335), None),
 
+    /// Allows the `#[must_not_suspend]` attribute.
+    (active, must_not_suspend, "1.57.0", Some(83310), None),
+
+
     // -------------------------------------------------------------------------
     // feature-group-end: actual feature gates
     // -------------------------------------------------------------------------

compiler/rustc_feature/src/builtin_attrs.rs

@@ -202,6 +202,10 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
     ungated!(forbid, Normal, template!(List: r#"lint1, lint2, ..., /*opt*/ reason = "...""#)),
     ungated!(deny, Normal, template!(List: r#"lint1, lint2, ..., /*opt*/ reason = "...""#)),
     ungated!(must_use, Normal, template!(Word, NameValueStr: "reason")),
+    gated!(
+        must_not_suspend, Normal, template!(Word, NameValueStr: "reason"), must_not_suspend,
+        experimental!(must_not_suspend)
+    ),
     // FIXME(#14407)
     ungated!(
         deprecated, Normal,

compiler/rustc_lint/src/lib.rs

@@ -298,6 +298,7 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) {
         UNUSED_LABELS,
         UNUSED_PARENS,
         UNUSED_BRACES,
+        MUST_NOT_SUSPEND,
         REDUNDANT_SEMICOLONS
     );
 

compiler/rustc_lint_defs/src/builtin.rs

@@ -314,6 +314,44 @@ declare_lint! {
     "imports that are never used"
 }
 
+declare_lint! {
+    /// The `must_not_suspend` lint guards against values that shouldn't be held across suspend points
+    /// (`.await`)
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// #![feature(must_not_suspend)]
+    ///
+    /// #[must_not_suspend]
+    /// struct SyncThing {}
+    ///
+    /// async fn yield_now() {}
+    ///
+    /// pub async fn uhoh() {
+    ///     let guard = SyncThing {};
+    ///     yield_now().await;
+    /// }
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// The `must_not_suspend` lint detects values that are marked with the `#[must_not_suspend]`
+    /// attribute being held across suspend points. A "suspend" point is usually a `.await` in an async
+    /// function.
+    ///
+    /// This attribute can be used to mark values that are semantically incorrect across suspends
+    /// (like certain types of timers), values that have async alternatives, and values that
+    /// regularly cause problems with the `Send`-ness of async fn's returned futures (like
+    /// `MutexGuard`'s)
+    ///
+    pub MUST_NOT_SUSPEND,
+    Warn,
+    "use of a `#[must_not_suspend]` value across a yield point",
+}
+
 declare_lint! {
     /// The `unused_extern_crates` lint guards against `extern crate` items
     /// that are never used.
@@ -2993,6 +3031,7 @@ declare_lint_pass! {
         CENUM_IMPL_DROP_CAST,
         CONST_EVALUATABLE_UNCHECKED,
         INEFFECTIVE_UNSTABLE_TRAIT_IMPL,
+        MUST_NOT_SUSPEND,
         UNINHABITED_STATIC,
         FUNCTION_ITEM_REFERENCES,
         USELESS_DEPRECATED,

compiler/rustc_passes/src/check_attr.rs

@@ -104,6 +104,7 @@ impl CheckAttrVisitor<'tcx> {
                 sym::default_method_body_is_const => {
                     self.check_default_method_body_is_const(attr, span, target)
                 }
+                sym::must_not_suspend => self.check_must_not_suspend(&attr, span, target),
                 sym::rustc_const_unstable
                 | sym::rustc_const_stable
                 | sym::unstable
@@ -1014,6 +1015,21 @@ impl CheckAttrVisitor<'tcx> {
         is_valid
     }
 
+    /// Checks if `#[must_not_suspend]` is applied to a function. Returns `true` if valid.
+    fn check_must_not_suspend(&self, attr: &Attribute, span: &Span, target: Target) -> bool {
+        match target {
+            Target::Struct | Target::Enum | Target::Union | Target::Trait => true,
+            _ => {
+                self.tcx
+                    .sess
+                    .struct_span_err(attr.span, "`must_not_suspend` attribute should be applied to a struct, enum, or trait")
+                        .span_label(*span, "is not a struct, enum, or trait")
+                        .emit();
+                false
+            }
+        }
+    }
+
     /// Checks if `#[cold]` is applied to a non-function. Returns `true` if valid.
     fn check_cold(&self, hir_id: HirId, attr: &Attribute, span: &Span, target: Target) {
         match target {

compiler/rustc_span/src/symbol.rs

@@ -836,6 +836,7 @@ symbols! {
         mul,
         mul_assign,
         mul_with_overflow,
+        must_not_suspend,
         must_use,
         mut_ptr,
         mut_slice_ptr,

compiler/rustc_typeck/src/check/generator_interior.rs

@@ -5,16 +5,19 @@
 
 use super::FnCtxt;
 use rustc_data_structures::fx::{FxHashSet, FxIndexSet};
+use rustc_errors::pluralize;
 use rustc_hir as hir;
 use rustc_hir::def::{CtorKind, DefKind, Res};
 use rustc_hir::def_id::DefId;
 use rustc_hir::hir_id::HirIdSet;
 use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
 use rustc_hir::{Arm, Expr, ExprKind, Guard, HirId, Pat, PatKind};
 use rustc_middle::middle::region::{self, YieldData};
-use rustc_middle::ty::{self, Ty};
+use rustc_middle::ty::{self, Ty, TyCtxt};
+use rustc_span::symbol::sym;
 use rustc_span::Span;
 use smallvec::SmallVec;
+use tracing::debug;
 
 struct InteriorVisitor<'a, 'tcx> {
     fcx: &'a FnCtxt<'a, 'tcx>,
@@ -36,6 +39,7 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> {
     fn record(
         &mut self,
         ty: Ty<'tcx>,
+        hir_id: HirId,
         scope: Option<region::Scope>,
         expr: Option<&'tcx Expr<'tcx>>,
         source_span: Span,
@@ -117,6 +121,20 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> {
             } else {
                 // Insert the type into the ordered set.
                 let scope_span = scope.map(|s| s.span(self.fcx.tcx, self.region_scope_tree));
+
+                check_must_not_suspend_ty(
+                    self.fcx,
+                    ty,
+                    hir_id,
+                    SuspendCheckData {
+                        expr,
+                        source_span,
+                        yield_span: yield_data.span,
+                        plural_len: 1,
+                        ..Default::default()
+                    },
+                );
+
                 self.types.insert(ty::GeneratorInteriorTypeCause {
                     span: source_span,
                     ty: &ty,
@@ -290,7 +308,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> {
         if let PatKind::Binding(..) = pat.kind {
             let scope = self.region_scope_tree.var_scope(pat.hir_id.local_id);
             let ty = self.fcx.typeck_results.borrow().pat_ty(pat);
-            self.record(ty, Some(scope), None, pat.span, false);
+            self.record(ty, pat.hir_id, Some(scope), None, pat.span, false);
         }
     }
 
@@ -342,7 +360,14 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> {
         // If there are adjustments, then record the final type --
         // this is the actual value that is being produced.
         if let Some(adjusted_ty) = self.fcx.typeck_results.borrow().expr_ty_adjusted_opt(expr) {
-            self.record(adjusted_ty, scope, Some(expr), expr.span, guard_borrowing_from_pattern);
+            self.record(
+                adjusted_ty,
+                expr.hir_id,
+                scope,
+                Some(expr),
+                expr.span,
+                guard_borrowing_from_pattern,
+            );
         }
 
         // Also record the unadjusted type (which is the only type if
@@ -380,9 +405,23 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> {
                     tcx.mk_region(ty::RegionKind::ReErased),
                     ty::TypeAndMut { ty, mutbl: hir::Mutability::Not },
                 );
-                self.record(ref_ty, scope, Some(expr), expr.span, guard_borrowing_from_pattern);
+                self.record(
+                    ref_ty,
+                    expr.hir_id,
+                    scope,
+                    Some(expr),
+                    expr.span,
+                    guard_borrowing_from_pattern,
+                );
             }
-            self.record(ty, scope, Some(expr), expr.span, guard_borrowing_from_pattern);
+            self.record(
+                ty,
+                expr.hir_id,
+                scope,
+                Some(expr),
+                expr.span,
+                guard_borrowing_from_pattern,
+            );
         } else {
             self.fcx.tcx.sess.delay_span_bug(expr.span, "no type for node");
         }
@@ -409,3 +448,170 @@ impl<'a, 'tcx> Visitor<'tcx> for ArmPatCollector<'a> {
         }
     }
 }
+
+#[derive(Default)]
+pub struct SuspendCheckData<'a, 'tcx> {
+    expr: Option<&'tcx Expr<'tcx>>,
+    source_span: Span,
+    yield_span: Span,
+    descr_pre: &'a str,
+    descr_post: &'a str,
+    plural_len: usize,
+}
+
+// Returns whether it emitted a diagnostic or not
+// Note that this fn and the proceding one are based on the code
+// for creating must_use diagnostics
+pub fn check_must_not_suspend_ty<'tcx>(
+    fcx: &FnCtxt<'_, 'tcx>,
+    ty: Ty<'tcx>,
+    hir_id: HirId,
+    data: SuspendCheckData<'_, 'tcx>,
+) -> bool {
+    if ty.is_unit()
+    // FIXME: should this check `is_ty_uninhabited_from`. This query is not available in this stage
+    // of typeck (before ReVar and RePlaceholder are removed), but may remove noise, like in
+    // `must_use`
+    // || fcx.tcx.is_ty_uninhabited_from(fcx.tcx.parent_module(hir_id).to_def_id(), ty, fcx.param_env)
+    {
+        return false;
+    }
+
+    let plural_suffix = pluralize!(data.plural_len);
+
+    match *ty.kind() {
+        ty::Adt(..) if ty.is_box() => {
+            let boxed_ty = ty.boxed_ty();
+            let descr_pre = &format!("{}boxed ", data.descr_pre);
+            check_must_not_suspend_ty(fcx, boxed_ty, hir_id, SuspendCheckData { descr_pre, ..data })
+        }
+        ty::Adt(def, _) => check_must_not_suspend_def(fcx.tcx, def.did, hir_id, data),
+        // FIXME: support adding the attribute to TAITs
+        ty::Opaque(def, _) => {
+            let mut has_emitted = false;
+            for &(predicate, _) in fcx.tcx.explicit_item_bounds(def) {
+                // We only look at the `DefId`, so it is safe to skip the binder here.
+                if let ty::PredicateKind::Trait(ref poly_trait_predicate) =
+                    predicate.kind().skip_binder()
+                {
+                    let def_id = poly_trait_predicate.trait_ref.def_id;
+                    let descr_pre = &format!("{}implementer{} of ", data.descr_pre, plural_suffix);
+                    if check_must_not_suspend_def(
+                        fcx.tcx,
+                        def_id,
+                        hir_id,
+                        SuspendCheckData { descr_pre, ..data },
+                    ) {
+                        has_emitted = true;
+                        break;
+                    }
+                }
+            }
+            has_emitted
+        }
+        ty::Dynamic(binder, _) => {
+            let mut has_emitted = false;
+            for predicate in binder.iter() {
+                if let ty::ExistentialPredicate::Trait(ref trait_ref) = predicate.skip_binder() {
+                    let def_id = trait_ref.def_id;
+                    let descr_post = &format!(" trait object{}{}", plural_suffix, data.descr_post);
+                    if check_must_not_suspend_def(
+                        fcx.tcx,
+                        def_id,
+                        hir_id,
+                        SuspendCheckData { descr_post, ..data },
+                    ) {
+                        has_emitted = true;
+                        break;
+                    }
+                }
+            }
+            has_emitted
+        }
+        ty::Tuple(ref tys) => {
+            let mut has_emitted = false;
+            let spans = if let Some(hir::ExprKind::Tup(comps)) = data.expr.map(|e| &e.kind) {
+                debug_assert_eq!(comps.len(), tys.len());
+                comps.iter().map(|e| e.span).collect()
+            } else {
+                vec![]
+            };
+            for (i, ty) in tys.iter().map(|k| k.expect_ty()).enumerate() {
+                let descr_post = &format!(" in tuple element {}", i);
+                let span = *spans.get(i).unwrap_or(&data.source_span);
+                if check_must_not_suspend_ty(
+                    fcx,
+                    ty,
+                    hir_id,
+                    SuspendCheckData { descr_post, source_span: span, ..data },
+                ) {
+                    has_emitted = true;
+                }
+            }
+            has_emitted
+        }
+        ty::Array(ty, len) => {
+            let descr_pre = &format!("{}array{} of ", data.descr_pre, plural_suffix);
+            check_must_not_suspend_ty(
+                fcx,
+                ty,
+                hir_id,
+                SuspendCheckData {
+                    descr_pre,
+                    plural_len: len.try_eval_usize(fcx.tcx, fcx.param_env).unwrap_or(0) as usize
+                        + 1,
+                    ..data
+                },
+            )
+        }
+        _ => false,
+    }
+}
+
+fn check_must_not_suspend_def(
+    tcx: TyCtxt<'_>,
+    def_id: DefId,
+    hir_id: HirId,
+    data: SuspendCheckData<'_, '_>,
+) -> bool {
+    for attr in tcx.get_attrs(def_id).iter() {
+        if attr.has_name(sym::must_not_suspend) {
+            tcx.struct_span_lint_hir(
+                rustc_session::lint::builtin::MUST_NOT_SUSPEND,
+                hir_id,
+                data.source_span,
+                |lint| {
+                    let msg = format!(
+                        "{}`{}`{} held across a suspend point, but should not be",
+                        data.descr_pre,
+                        tcx.def_path_str(def_id),
+                        data.descr_post,
+                    );
+                    let mut err = lint.build(&msg);
+
+                    // add span pointing to the offending yield/await
+                    err.span_label(data.yield_span, "the value is held across this suspend point");
+
+                    // Add optional reason note
+                    if let Some(note) = attr.value_str() {
+                        // FIXME(guswynn): consider formatting this better
+                        err.span_note(data.source_span, &note.as_str());
+                    }
+
+                    // Add some quick suggestions on what to do
+                    // FIXME: can `drop` work as a suggestion here as well?
+                    err.span_help(
+                        data.source_span,
+                        "consider using a block (`{ ... }`) \
+                        to shrink the value's scope, ending before the suspend point",
+                    );
+
+                    err.emit();
+                },
+            );
+
+            return true;
+        }
+    }
+    false
+}