factor into struct, and comments

Author: guswynn

compiler/rustc_lint_defs/src/builtin.rs

@@ -315,7 +315,7 @@ declare_lint! {
 }
 
 declare_lint! {
-    /// The `must_not_suspend` lint guards against values that shouldn't be held across yield points
+    /// The `must_not_suspend` lint guards against values that shouldn't be held across suspend points
     /// (`.await`)
     ///
     /// ### Example
@@ -339,10 +339,10 @@ declare_lint! {
     /// ### Explanation
     ///
     /// The `must_not_suspend` lint detects values that are marked with the `#[must_not_suspend]`
-    /// attribute being held across yield points. A "yield" point is usually a `.await` in an async
+    /// 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 yields
+    /// 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)

compiler/rustc_typeck/src/check/generator_interior.rs

@@ -126,12 +126,13 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> {
                     self.fcx,
                     ty,
                     hir_id,
-                    expr,
-                    source_span,
-                    yield_data.span,
-                    "",
-                    "",
-                    1,
+                    SuspendCheckData {
+                        expr,
+                        source_span,
+                        yield_span: yield_data.span,
+                        plural_len: 1,
+                        ..Default::default()
+                    },
                 );
 
                 self.types.insert(ty::GeneratorInteriorTypeCause {
@@ -448,56 +449,43 @@ 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,
-    expr: Option<&'tcx Expr<'tcx>>,
-    source_span: Span,
-    yield_span: Span,
-    descr_pre: &str,
-    descr_post: &str,
-    plural_len: usize,
+    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 true;
+        return false;
     }
 
-    let plural_suffix = pluralize!(plural_len);
+    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 ", descr_pre);
-            check_must_not_suspend_ty(
-                fcx,
-                boxed_ty,
-                hir_id,
-                expr,
-                source_span,
-                yield_span,
-                descr_pre,
-                descr_post,
-                plural_len,
-            )
+            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,
-            source_span,
-            yield_span,
-            descr_pre,
-            descr_post,
-        ),
+        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;
@@ -507,15 +495,12 @@ pub fn check_must_not_suspend_ty<'tcx>(
                     predicate.kind().skip_binder()
                 {
                     let def_id = poly_trait_predicate.trait_ref.def_id;
-                    let descr_pre = &format!("{}implementer{} of ", descr_pre, plural_suffix,);
+                    let descr_pre = &format!("{}implementer{} of ", data.descr_pre, plural_suffix);
                     if check_must_not_suspend_def(
                         fcx.tcx,
                         def_id,
                         hir_id,
-                        source_span,
-                        yield_span,
-                        descr_pre,
-                        descr_post,
+                        SuspendCheckData { descr_pre, ..data },
                     ) {
                         has_emitted = true;
                         break;
@@ -529,15 +514,12 @@ pub fn check_must_not_suspend_ty<'tcx>(
             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, descr_post,);
+                    let descr_post = &format!(" trait object{}{}", plural_suffix, data.descr_post);
                     if check_must_not_suspend_def(
                         fcx.tcx,
                         def_id,
                         hir_id,
-                        source_span,
-                        yield_span,
-                        descr_pre,
-                        descr_post,
+                        SuspendCheckData { descr_post, ..data },
                     ) {
                         has_emitted = true;
                         break;
@@ -548,35 +530,38 @@ pub fn check_must_not_suspend_ty<'tcx>(
         }
         ty::Tuple(ref tys) => {
             let mut has_emitted = false;
-            let spans = if let Some(hir::ExprKind::Tup(comps)) = expr.map(|e| &e.kind) {
+            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(&source_span);
+                let span = *spans.get(i).unwrap_or(&data.source_span);
                 if check_must_not_suspend_ty(
-                    fcx, ty, hir_id, expr, span, yield_span, descr_pre, descr_post, plural_len,
+                    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 ", descr_pre, plural_suffix,);
+            let descr_pre = &format!("{}array{} of ", data.descr_pre, plural_suffix);
             check_must_not_suspend_ty(
                 fcx,
                 ty,
                 hir_id,
-                expr,
-                source_span,
-                yield_span,
-                descr_pre,
-                descr_post,
-                len.try_eval_usize(fcx.tcx, fcx.param_env).unwrap_or(0) as usize + 1,
+                SuspendCheckData {
+                    descr_pre,
+                    plural_len: len.try_eval_usize(fcx.tcx, fcx.param_env).unwrap_or(0) as usize
+                        + 1,
+                    ..data
+                },
             )
         }
         _ => false,
@@ -587,39 +572,38 @@ fn check_must_not_suspend_def(
     tcx: TyCtxt<'_>,
     def_id: DefId,
     hir_id: HirId,
-    source_span: Span,
-    yield_span: Span,
-    descr_pre_path: &str,
-    descr_post_path: &str,
+    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,
-                source_span,
+                data.source_span,
                 |lint| {
                     let msg = format!(
-                        "{}`{}`{} held across a yield point, but should not be",
-                        descr_pre_path,
+                        "{}`{}`{} held across a suspend point, but should not be",
+                        data.descr_pre,
                         tcx.def_path_str(def_id),
-                        descr_post_path
+                        data.descr_post,
                     );
                     let mut err = lint.build(&msg);
 
                     // add span pointing to the offending yield/await
-                    err.span_label(yield_span, "the value is held across this yield point");
+                    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() {
-                        err.span_note(source_span, &note.as_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(
-                        source_span,
-                        "`drop` this value before the yield point, or use a block (`{ ... }`) \
-                        to shrink its scope",
+                        data.source_span,
+                        "consider using a block (`{ ... }`) \
+                        to shrink the value's scope, ending before the suspend point",
                     );
 
                     err.emit();

src/test/ui/lint/must_not_suspend/boxed.stderr

@@ -1,10 +1,10 @@
-error: boxed `Umm` held across a yield point, but should not be
+error: boxed `Umm` held across a suspend point, but should not be
   --> $DIR/boxed.rs:20:9
    |
 LL |     let _guard = bar();
    |         ^^^^^^
 LL |     other().await;
-   |     ------------- the value is held across this yield point
+   |     ------------- the value is held across this suspend point
    |
 note: the lint level is defined here
   --> $DIR/boxed.rs:3:9
@@ -16,7 +16,7 @@ note: You gotta use Umm's, ya know?
    |
 LL |     let _guard = bar();
    |         ^^^^^^
-help: `drop` this value before the yield point, or use a block (`{ ... }`) to shrink its scope
+help: consider using a block (`{ ... }`) to shrink the value's scope, ending before the suspend point
   --> $DIR/boxed.rs:20:9
    |
 LL |     let _guard = bar();

src/test/ui/lint/must_not_suspend/handled.rs

@@ -0,0 +1,28 @@
+// edition:2018
+// run-pass
+#![feature(must_not_suspend)]
+#![deny(must_not_suspend)]
+
+#[must_not_suspend = "You gotta use Umm's, ya know?"]
+struct Umm {
+    _i: i64
+}
+
+
+fn bar() -> Umm {
+    Umm {
+        _i: 1
+    }
+}
+
+async fn other() {}
+
+pub async fn uhoh() {
+    {
+        let _guard = bar();
+    }
+    other().await;
+}
+
+fn main() {
+}

src/test/ui/lint/must_not_suspend/ref.stderr

@@ -1,11 +1,11 @@
-error: `Umm` held across a yield point, but should not be
+error: `Umm` held across a suspend point, but should not be
   --> $DIR/ref.rs:18:26
    |
 LL |         let guard = &mut self.u;
    |                          ^^^^^^
 ...
 LL |         other().await;
-   |         ------------- the value is held across this yield point
+   |         ------------- the value is held across this suspend point
    |
 note: the lint level is defined here
   --> $DIR/ref.rs:3:9
@@ -17,27 +17,27 @@ note: You gotta use Umm's, ya know?
    |
 LL |         let guard = &mut self.u;
    |                          ^^^^^^
-help: `drop` this value before the yield point, or use a block (`{ ... }`) to shrink its scope
+help: consider using a block (`{ ... }`) to shrink the value's scope, ending before the suspend point
   --> $DIR/ref.rs:18:26
    |
 LL |         let guard = &mut self.u;
    |                          ^^^^^^
 
-error: `Umm` held across a yield point, but should not be
+error: `Umm` held across a suspend point, but should not be
   --> $DIR/ref.rs:18:26
    |
 LL |         let guard = &mut self.u;
    |                          ^^^^^^
 ...
 LL |         other().await;
-   |         ------------- the value is held across this yield point
+   |         ------------- the value is held across this suspend point
    |
 note: You gotta use Umm's, ya know?
   --> $DIR/ref.rs:18:26
    |
 LL |         let guard = &mut self.u;
    |                          ^^^^^^
-help: `drop` this value before the yield point, or use a block (`{ ... }`) to shrink its scope
+help: consider using a block (`{ ... }`) to shrink the value's scope, ending before the suspend point
   --> $DIR/ref.rs:18:26
    |
 LL |         let guard = &mut self.u;

src/test/ui/lint/must_not_suspend/trait.stderr

@@ -1,33 +1,33 @@
-error: implementer of `Wow` held across a yield point, but should not be
+error: implementer of `Wow` held across a suspend point, but should not be
   --> $DIR/trait.rs:21:9
    |
 LL |     let _guard1 = r#impl();
    |         ^^^^^^^
 ...
 LL |     other().await;
-   |     ------------- the value is held across this yield point
+   |     ------------- the value is held across this suspend point
    |
 note: the lint level is defined here
   --> $DIR/trait.rs:3:9
    |
 LL | #![deny(must_not_suspend)]
    |         ^^^^^^^^^^^^^^^^
-help: `drop` this value before the yield point, or use a block (`{ ... }`) to shrink its scope
+help: consider using a block (`{ ... }`) to shrink the value's scope, ending before the suspend point
   --> $DIR/trait.rs:21:9
    |
 LL |     let _guard1 = r#impl();
    |         ^^^^^^^
 
-error: boxed `Wow` trait object held across a yield point, but should not be
+error: boxed `Wow` trait object held across a suspend point, but should not be
   --> $DIR/trait.rs:22:9
    |
 LL |     let _guard2 = r#dyn();
    |         ^^^^^^^
 LL | 
 LL |     other().await;
-   |     ------------- the value is held across this yield point
+   |     ------------- the value is held across this suspend point
    |
-help: `drop` this value before the yield point, or use a block (`{ ... }`) to shrink its scope
+help: consider using a block (`{ ... }`) to shrink the value's scope, ending before the suspend point
   --> $DIR/trait.rs:22:9
    |
 LL |     let _guard2 = r#dyn();