From 110aecd23eefa603ffd1b5d8f64832230d42b435 Mon Sep 17 00:00:00 2001 From: Gus Wynn Date: Wed, 15 Sep 2021 11:48:34 -0700 Subject: [PATCH] factor into struct, and comments --- compiler/rustc_lint_defs/src/builtin.rs | 6 +- .../src/check/generator_interior.rs | 120 ++++++++---------- .../ui/lint/must_not_suspend/boxed.stderr | 6 +- src/test/ui/lint/must_not_suspend/handled.rs | 28 ++++ src/test/ui/lint/must_not_suspend/ref.stderr | 12 +- .../ui/lint/must_not_suspend/trait.stderr | 12 +- src/test/ui/lint/must_not_suspend/unit.stderr | 6 +- src/test/ui/lint/must_not_suspend/warn.stderr | 6 +- 8 files changed, 104 insertions(+), 92 deletions(-) create mode 100644 src/test/ui/lint/must_not_suspend/handled.rs diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index a192056e244..649ad21385e 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -315,7 +315,7 @@ } 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 @@ /// ### 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) diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 7e4bc08c172..e8a24f01b75 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -126,12 +126,13 @@ fn record( 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,6 +449,16 @@ fn visit_pat(&mut self, pat: &'tcx Pat<'tcx>) { } } +#[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 @@ -455,12 +466,7 @@ 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 @@ -468,36 +474,18 @@ pub fn check_must_not_suspend_ty<'tcx>( // `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,7 +530,7 @@ 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 { @@ -556,9 +538,12 @@ pub fn check_must_not_suspend_ty<'tcx>( }; 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; } @@ -566,17 +551,17 @@ pub fn check_must_not_suspend_ty<'tcx>( 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, ¬e.as_str()); + // FIXME(guswynn): consider formatting this better + err.span_note(data.source_span, ¬e.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(); diff --git a/src/test/ui/lint/must_not_suspend/boxed.stderr b/src/test/ui/lint/must_not_suspend/boxed.stderr index 7bd80405b5d..edc62b6d687 100644 --- a/src/test/ui/lint/must_not_suspend/boxed.stderr +++ b/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(); diff --git a/src/test/ui/lint/must_not_suspend/handled.rs b/src/test/ui/lint/must_not_suspend/handled.rs new file mode 100644 index 00000000000..8714be6449f --- /dev/null +++ b/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() { +} diff --git a/src/test/ui/lint/must_not_suspend/ref.stderr b/src/test/ui/lint/must_not_suspend/ref.stderr index d2a550d7b45..d4c58bcbcd2 100644 --- a/src/test/ui/lint/must_not_suspend/ref.stderr +++ b/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; diff --git a/src/test/ui/lint/must_not_suspend/trait.stderr b/src/test/ui/lint/must_not_suspend/trait.stderr index 8d7bb808764..d19ffddd482 100644 --- a/src/test/ui/lint/must_not_suspend/trait.stderr +++ b/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(); diff --git a/src/test/ui/lint/must_not_suspend/unit.stderr b/src/test/ui/lint/must_not_suspend/unit.stderr index 87e0fd27e70..425c076823d 100644 --- a/src/test/ui/lint/must_not_suspend/unit.stderr +++ b/src/test/ui/lint/must_not_suspend/unit.stderr @@ -1,10 +1,10 @@ -error: `Umm` held across a yield point, but should not be +error: `Umm` held across a suspend point, but should not be --> $DIR/unit.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/unit.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/unit.rs:20:9 | LL | let _guard = bar(); diff --git a/src/test/ui/lint/must_not_suspend/warn.stderr b/src/test/ui/lint/must_not_suspend/warn.stderr index 03b77520c9f..24f52275b43 100644 --- a/src/test/ui/lint/must_not_suspend/warn.stderr +++ b/src/test/ui/lint/must_not_suspend/warn.stderr @@ -1,10 +1,10 @@ -warning: `Umm` held across a yield point, but should not be +warning: `Umm` held across a suspend point, but should not be --> $DIR/warn.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: `#[warn(must_not_suspend)]` on by default note: You gotta use Umm's, ya know? @@ -12,7 +12,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/warn.rs:20:9 | LL | let _guard = bar();