From 2271376fb224661fd8b7e5e5788217e8fe9e5ecc Mon Sep 17 00:00:00 2001 From: Gus Wynn Date: Sat, 4 Sep 2021 19:36:51 -0700 Subject: [PATCH 1/9] must_not_suspend impl --- compiler/rustc_feature/src/active.rs | 4 + compiler/rustc_feature/src/builtin_attrs.rs | 4 + compiler/rustc_lint/src/lib.rs | 1 + compiler/rustc_lint_defs/src/builtin.rs | 8 + compiler/rustc_passes/src/check_attr.rs | 16 + compiler/rustc_span/src/symbol.rs | 1 + .../src/check/generator_interior.rs | 309 +++++++++++++++++- src/test/ui/lint/must_not_suspend/boxed.rs | 25 ++ .../ui/lint/must_not_suspend/boxed.stderr | 23 ++ .../feature-gate-must_not_suspend.rs | 9 + .../feature-gate-must_not_suspend.stderr | 12 + src/test/ui/lint/must_not_suspend/return.rs | 9 + .../ui/lint/must_not_suspend/return.stderr | 12 + src/test/ui/lint/must_not_suspend/trait.rs | 28 ++ .../ui/lint/must_not_suspend/trait.stderr | 39 +++ src/test/ui/lint/must_not_suspend/unit.rs | 25 ++ src/test/ui/lint/must_not_suspend/unit.stderr | 23 ++ src/test/ui/lint/must_not_suspend/warn.rs | 25 ++ src/test/ui/lint/must_not_suspend/warn.stderr | 19 ++ 19 files changed, 587 insertions(+), 5 deletions(-) create mode 100644 src/test/ui/lint/must_not_suspend/boxed.rs create mode 100644 src/test/ui/lint/must_not_suspend/boxed.stderr create mode 100644 src/test/ui/lint/must_not_suspend/feature-gate-must_not_suspend.rs create mode 100644 src/test/ui/lint/must_not_suspend/feature-gate-must_not_suspend.stderr create mode 100644 src/test/ui/lint/must_not_suspend/return.rs create mode 100644 src/test/ui/lint/must_not_suspend/return.stderr create mode 100644 src/test/ui/lint/must_not_suspend/trait.rs create mode 100644 src/test/ui/lint/must_not_suspend/trait.stderr create mode 100644 src/test/ui/lint/must_not_suspend/unit.rs create mode 100644 src/test/ui/lint/must_not_suspend/unit.stderr create mode 100644 src/test/ui/lint/must_not_suspend/warn.rs create mode 100644 src/test/ui/lint/must_not_suspend/warn.stderr diff --git a/compiler/rustc_feature/src/active.rs b/compiler/rustc_feature/src/active.rs index a3807a2bb9fde..66569270bd21e 100644 --- a/compiler/rustc_feature/src/active.rs +++ b/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 `#[must_not_suspend]`. + (active, must_not_suspend, "1.56.0", Some(83310), None), + + // ------------------------------------------------------------------------- // feature-group-end: actual feature gates // ------------------------------------------------------------------------- diff --git a/compiler/rustc_feature/src/builtin_attrs.rs b/compiler/rustc_feature/src/builtin_attrs.rs index e2aa54a59b202..928a7eb794bd1 100644 --- a/compiler/rustc_feature/src/builtin_attrs.rs +++ b/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, diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index ef4bda666ba06..10285272130cc 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/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 ); diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 8fb678e2d20fb..386435034b6ac 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -314,6 +314,13 @@ declare_lint! { "imports that are never used" } +declare_lint! { + /// [the reference]: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute + 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 +3000,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, diff --git a/compiler/rustc_passes/src/check_attr.rs b/compiler/rustc_passes/src/check_attr.rs index fd438bdc9005a..a31f3fe281ee9 100644 --- a/compiler/rustc_passes/src/check_attr.rs +++ b/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::Fn | Target::Method(..) | Target::ForeignFn | Target::Closure => { + self.tcx + .sess + .struct_span_err(attr.span, "`must_not_suspend` attribute should be applied to a struct, enum, `impl Trait`, or `dyn Trait`") + .span_label(*span, "is a function") + .emit(); + false + } + _ => true, + } + } + /// 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 { diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 24023163cc30e..eecbb9a9cfadc 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/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, diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 5f26e701c0ab7..c5efc30a7c2d4 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -5,6 +5,7 @@ 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; @@ -12,9 +13,11 @@ 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, 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::ParamEnv::empty(), + ty, + hir_id, + expr, + source_span, + yield_data.span, + "", + "", + 1, + ); + 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,263 @@ impl<'a, 'tcx> Visitor<'tcx> for ArmPatCollector<'a> { } } } + +// 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>, + param_env: ty::ParamEnv<'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, +) -> bool { + if ty.is_unit() + || fcx.tcx.is_ty_uninhabited_from(fcx.tcx.parent_module(hir_id).to_def_id(), ty, param_env) + { + return true; + } + + let plural_suffix = pluralize!(plural_len); + + let emitted = 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, + param_env, + boxed_ty, + hir_id, + expr, + source_span, + yield_span, + descr_pre, + descr_post, + plural_len, + ) + } + ty::Adt(def, _) => check_must_not_suspend_def( + fcx.tcx, + def.did, + hir_id, + source_span, + yield_span, + descr_pre, + descr_post, + ), + 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 ", descr_pre, plural_suffix,); + if check_must_not_suspend_def( + fcx.tcx, + def_id, + hir_id, + source_span, + yield_span, + descr_pre, + descr_post, + ) { + 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, descr_post,); + if check_must_not_suspend_def( + fcx.tcx, + def_id, + hir_id, + source_span, + yield_span, + descr_pre, + descr_post, + ) { + has_emitted = true; + break; + } + } + } + has_emitted + } + ty::Tuple(ref tys) => { + let mut has_emitted = false; + /* + let spans = if let hir::ExprKind::Tup(comps) = &expr.kind { + debug_assert_eq!(comps.len(), tys.len()); + comps.iter().map(|e| e.span).collect() + } else { + vec![] + }; + */ + let spans = 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); + if check_must_not_suspend_ty( + fcx, param_env, ty, hir_id, expr, span, yield_span, descr_pre, descr_post, + plural_len, + ) { + has_emitted = true; + } + } + has_emitted + } + ty::Array(ty, len) => match len.try_eval_usize(fcx.tcx, param_env) { + // If the array is empty we don't lint, to avoid false positives + Some(0) | None => false, + // If the array is definitely non-empty, we can do `#[must_use]` checking. + Some(n) => { + let descr_pre = &format!("{}array{} of ", descr_pre, plural_suffix,); + check_must_not_suspend_ty( + fcx, + param_env, + ty, + hir_id, + expr, + source_span, + yield_span, + descr_pre, + descr_post, + n as usize + 1, + ) + } + }, + _ => false, + }; + + // Don't move onto the "return value" path if we already sent a diagnostic + if emitted { + return true; + } + + match expr { + Some(expr) => match expr.kind { + hir::ExprKind::Call(ref callee, _) => { + match callee.kind { + hir::ExprKind::Path(ref qpath) => { + match fcx.typeck_results.borrow().qpath_res(qpath, callee.hir_id) { + Res::Def(DefKind::Fn | DefKind::AssocFn, def_id) => { + check_must_not_suspend_def( + fcx.tcx, + def_id, + hir_id, + source_span, + yield_span, + "return value of ", + "", + ) + } + + // `Res::Local` if it was a closure, for which we + // do not currently support must-not-suspend linting + _ => false, + } + } + _ => false, + } + } + hir::ExprKind::MethodCall(..) => { + if let Some(def_id) = fcx.typeck_results.borrow().type_dependent_def_id(expr.hir_id) + { + check_must_not_suspend_def( + fcx.tcx, + def_id, + hir_id, + source_span, + yield_span, + "return value of ", + "", + ) + } else { + false + } + } + _ => false, + }, + None => false, + } +} + +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, +) -> 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, + |lint| { + let msg = format!( + "{}`{}`{} held across a yield point, but should not be", + descr_pre_path, + tcx.def_path_str(def_id), + descr_post_path + ); + let mut err = lint.build(&msg); + + // Add optional reason note + if let Some(note) = attr.value_str() { + err.note(¬e.as_str()); + } + + // add span pointing to the offending yield/await) + err.span_label(yield_span, "The value is held across this yield point"); + + // Add some quick suggestions on what to do + err.span_help( + source_span, + "`drop` this value before the yield point, or use a block (`{ ... }`) \" + to shrink its scope", + ); + + err.emit(); + }, + ); + + /* + cx.struct_span_lint(UNUSED_MUST_USE, span, |lint| { + let msg = format!( + "unused {}`{}`{} that must be used", + descr_pre_path, + cx.tcx.def_path_str(def_id), + descr_post_path + ); + let mut err = lint.build(&msg); + // check for #[must_use = "..."] + if let Some(note) = attr.value_str() { + err.note(¬e.as_str()); + } + err.emit(); + }); + */ + return true; + } + } + false +} diff --git a/src/test/ui/lint/must_not_suspend/boxed.rs b/src/test/ui/lint/must_not_suspend/boxed.rs new file mode 100644 index 0000000000000..d64d07e5e0df6 --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/boxed.rs @@ -0,0 +1,25 @@ +// edition:2018 +#![feature(must_not_suspend)] +#![deny(must_not_suspend)] + +#[must_not_suspend = "You gotta use Umm's, ya know?"] +struct Umm { + i: i64 +} + + +fn bar() -> Box { + Box::new(Umm { + i: 1 + }) +} + +async fn other() {} + +pub async fn uhoh() { + let _guard = bar(); //~ boxed `Umm` held across + other().await; +} + +fn main() { +} diff --git a/src/test/ui/lint/must_not_suspend/boxed.stderr b/src/test/ui/lint/must_not_suspend/boxed.stderr new file mode 100644 index 0000000000000..c3c23db7d72f7 --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/boxed.stderr @@ -0,0 +1,23 @@ +error: boxed `Umm` held across a yield 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 + | +note: the lint level is defined here + --> $DIR/boxed.rs:3:9 + | +LL | #![deny(must_not_suspend)] + | ^^^^^^^^^^^^^^^^ + = note: You gotta use Umm's, ya know? +help: `drop` this value before the yield point, or use a block (`{ ... }`) " + to shrink its scope + --> $DIR/boxed.rs:20:9 + | +LL | let _guard = bar(); + | ^^^^^^ + +error: aborting due to previous error + diff --git a/src/test/ui/lint/must_not_suspend/feature-gate-must_not_suspend.rs b/src/test/ui/lint/must_not_suspend/feature-gate-must_not_suspend.rs new file mode 100644 index 0000000000000..aff8ff33b65a8 --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/feature-gate-must_not_suspend.rs @@ -0,0 +1,9 @@ +// edition:2018 + +#[must_not_suspend = "You gotta use Umm's, ya know?"] //~ the `#[must_not_suspend]` +struct Umm { + _i: i64 +} + +fn main() { +} diff --git a/src/test/ui/lint/must_not_suspend/feature-gate-must_not_suspend.stderr b/src/test/ui/lint/must_not_suspend/feature-gate-must_not_suspend.stderr new file mode 100644 index 0000000000000..ab20a8be8747d --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/feature-gate-must_not_suspend.stderr @@ -0,0 +1,12 @@ +error[E0658]: the `#[must_not_suspend]` attribute is an experimental feature + --> $DIR/feature-gate-must_not_suspend.rs:3:1 + | +LL | #[must_not_suspend = "You gotta use Umm's, ya know?"] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: see issue #83310 for more information + = help: add `#![feature(must_not_suspend)]` to the crate attributes to enable + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0658`. diff --git a/src/test/ui/lint/must_not_suspend/return.rs b/src/test/ui/lint/must_not_suspend/return.rs new file mode 100644 index 0000000000000..5f80e78937628 --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/return.rs @@ -0,0 +1,9 @@ +// edition:2018 +#![feature(must_not_suspend)] +#![deny(must_not_suspend)] + +#[must_not_suspend] //~ attribute should be +fn foo() -> i32 { + 0 +} +fn main() {} diff --git a/src/test/ui/lint/must_not_suspend/return.stderr b/src/test/ui/lint/must_not_suspend/return.stderr new file mode 100644 index 0000000000000..ff1798320cf8e --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/return.stderr @@ -0,0 +1,12 @@ +error: `must_not_suspend` attribute should be applied to a struct, enum, `impl Trait`, or `dyn Trait` + --> $DIR/return.rs:5:1 + | +LL | #[must_not_suspend] + | ^^^^^^^^^^^^^^^^^^^ +LL | / fn foo() -> i32 { +LL | | 0 +LL | | } + | |_- is a function + +error: aborting due to previous error + diff --git a/src/test/ui/lint/must_not_suspend/trait.rs b/src/test/ui/lint/must_not_suspend/trait.rs new file mode 100644 index 0000000000000..0438e072ce67e --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/trait.rs @@ -0,0 +1,28 @@ +// edition:2018 +#![feature(must_not_suspend)] +#![deny(must_not_suspend)] + +#[must_not_suspend] +trait Wow {} + +impl Wow for i32 {} + +fn r#impl() -> impl Wow { + 1 +} + +fn r#dyn() -> Box { + Box::new(1) +} + +async fn other() {} + +pub async fn uhoh() { + let _guard1 = r#impl(); //~ implementer of `Wow` held across + let _guard2 = r#dyn(); //~ boxed `Wow` trait object held across + + other().await; +} + +fn main() { +} diff --git a/src/test/ui/lint/must_not_suspend/trait.stderr b/src/test/ui/lint/must_not_suspend/trait.stderr new file mode 100644 index 0000000000000..1175cbb919208 --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/trait.stderr @@ -0,0 +1,39 @@ +error: implementer of `Wow` held across a yield 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 + | +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 + --> $DIR/trait.rs:21:9 + | +LL | let _guard1 = r#impl(); + | ^^^^^^^ + +error: boxed `Wow` trait object held across a yield 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 + | +help: `drop` this value before the yield point, or use a block (`{ ... }`) " + to shrink its scope + --> $DIR/trait.rs:22:9 + | +LL | let _guard2 = r#dyn(); + | ^^^^^^^ + +error: aborting due to 2 previous errors + diff --git a/src/test/ui/lint/must_not_suspend/unit.rs b/src/test/ui/lint/must_not_suspend/unit.rs new file mode 100644 index 0000000000000..4e87b801114e9 --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/unit.rs @@ -0,0 +1,25 @@ +// edition:2018 +#![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(); //~ `Umm` held across + other().await; +} + +fn main() { +} diff --git a/src/test/ui/lint/must_not_suspend/unit.stderr b/src/test/ui/lint/must_not_suspend/unit.stderr new file mode 100644 index 0000000000000..cff00dd8ca409 --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/unit.stderr @@ -0,0 +1,23 @@ +error: `Umm` held across a yield 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 + | +note: the lint level is defined here + --> $DIR/unit.rs:3:9 + | +LL | #![deny(must_not_suspend)] + | ^^^^^^^^^^^^^^^^ + = note: You gotta use Umm's, ya know? +help: `drop` this value before the yield point, or use a block (`{ ... }`) " + to shrink its scope + --> $DIR/unit.rs:20:9 + | +LL | let _guard = bar(); + | ^^^^^^ + +error: aborting due to previous error + diff --git a/src/test/ui/lint/must_not_suspend/warn.rs b/src/test/ui/lint/must_not_suspend/warn.rs new file mode 100644 index 0000000000000..d0d7238480759 --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/warn.rs @@ -0,0 +1,25 @@ +// edition:2018 +// run-pass +#![feature(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(); //~ `Umm` held across + other().await; +} + +fn main() { +} diff --git a/src/test/ui/lint/must_not_suspend/warn.stderr b/src/test/ui/lint/must_not_suspend/warn.stderr new file mode 100644 index 0000000000000..bda44d051ee7c --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/warn.stderr @@ -0,0 +1,19 @@ +warning: `Umm` held across a yield 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 + | + = note: `#[warn(must_not_suspend)]` on by default + = note: You gotta use Umm's, ya know? +help: `drop` this value before the yield point, or use a block (`{ ... }`) " + to shrink its scope + --> $DIR/warn.rs:20:9 + | +LL | let _guard = bar(); + | ^^^^^^ + +warning: 1 warning emitted + From 67ee91e77e8106357df63fc2d4b6f04f36ccd2da Mon Sep 17 00:00:00 2001 From: Gus Wynn Date: Sat, 11 Sep 2021 10:50:09 -0700 Subject: [PATCH 2/9] remove attempt at fn call --- .../src/check/generator_interior.rs | 70 +------------------ 1 file changed, 1 insertion(+), 69 deletions(-) diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index c5efc30a7c2d4..118c6e9a27363 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -472,7 +472,7 @@ pub fn check_must_not_suspend_ty<'tcx>( let plural_suffix = pluralize!(plural_len); - let emitted = match *ty.kind() { + match *ty.kind() { ty::Adt(..) if ty.is_box() => { let boxed_ty = ty.boxed_ty(); let descr_pre = &format!("{}boxed ", descr_pre); @@ -589,58 +589,6 @@ pub fn check_must_not_suspend_ty<'tcx>( } }, _ => false, - }; - - // Don't move onto the "return value" path if we already sent a diagnostic - if emitted { - return true; - } - - match expr { - Some(expr) => match expr.kind { - hir::ExprKind::Call(ref callee, _) => { - match callee.kind { - hir::ExprKind::Path(ref qpath) => { - match fcx.typeck_results.borrow().qpath_res(qpath, callee.hir_id) { - Res::Def(DefKind::Fn | DefKind::AssocFn, def_id) => { - check_must_not_suspend_def( - fcx.tcx, - def_id, - hir_id, - source_span, - yield_span, - "return value of ", - "", - ) - } - - // `Res::Local` if it was a closure, for which we - // do not currently support must-not-suspend linting - _ => false, - } - } - _ => false, - } - } - hir::ExprKind::MethodCall(..) => { - if let Some(def_id) = fcx.typeck_results.borrow().type_dependent_def_id(expr.hir_id) - { - check_must_not_suspend_def( - fcx.tcx, - def_id, - hir_id, - source_span, - yield_span, - "return value of ", - "", - ) - } else { - false - } - } - _ => false, - }, - None => false, } } @@ -687,22 +635,6 @@ fn check_must_not_suspend_def( }, ); - /* - cx.struct_span_lint(UNUSED_MUST_USE, span, |lint| { - let msg = format!( - "unused {}`{}`{} that must be used", - descr_pre_path, - cx.tcx.def_path_str(def_id), - descr_post_path - ); - let mut err = lint.build(&msg); - // check for #[must_use = "..."] - if let Some(note) = attr.value_str() { - err.note(¬e.as_str()); - } - err.emit(); - }); - */ return true; } } From 74ea16301e2e6fb96ac8414761cf227775e64dfd Mon Sep 17 00:00:00 2001 From: Gus Wynn Date: Sat, 11 Sep 2021 12:06:05 -0700 Subject: [PATCH 3/9] skip the uninhabitated check and comments --- compiler/rustc_lint_defs/src/builtin.rs | 23 ++++++++++++++++++- compiler/rustc_passes/src/check_attr.rs | 8 +++---- .../src/check/generator_interior.rs | 18 +++++---------- src/test/ui/lint/must_not_suspend/boxed.rs | 2 +- .../feature-gate-must_not_suspend.rs | 2 +- .../ui/lint/must_not_suspend/other_items.rs | 8 +++++++ .../lint/must_not_suspend/other_items.stderr | 10 ++++++++ src/test/ui/lint/must_not_suspend/return.rs | 2 +- .../ui/lint/must_not_suspend/return.stderr | 4 ++-- src/test/ui/lint/must_not_suspend/trait.rs | 4 ++-- src/test/ui/lint/must_not_suspend/unit.rs | 2 +- src/test/ui/lint/must_not_suspend/warn.rs | 2 +- 12 files changed, 59 insertions(+), 26 deletions(-) create mode 100644 src/test/ui/lint/must_not_suspend/other_items.rs create mode 100644 src/test/ui/lint/must_not_suspend/other_items.stderr diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 386435034b6ac..0463cb2b76553 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -315,7 +315,28 @@ declare_lint! { } declare_lint! { - /// [the reference]: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute + /// 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 + /// function. + /// + /// This attribute can be used to mark values that are semantically incorrect across yields + /// (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) + /// + /// ### Example + /// + /// ```rust + /// #[must_not_suspend] + /// struct SyncThing {} + /// + /// async fn yield() {} + /// + /// pub async fn uhoh() { + /// let guard = SyncThing {}; + /// yield().await; + /// } + /// ``` pub MUST_NOT_SUSPEND, Warn, "Use of a `#[must_not_suspend]` value across a yield point", diff --git a/compiler/rustc_passes/src/check_attr.rs b/compiler/rustc_passes/src/check_attr.rs index a31f3fe281ee9..3e59fc4f55159 100644 --- a/compiler/rustc_passes/src/check_attr.rs +++ b/compiler/rustc_passes/src/check_attr.rs @@ -1018,15 +1018,15 @@ impl CheckAttrVisitor<'tcx> { /// 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::Fn | Target::Method(..) | Target::ForeignFn | Target::Closure => { + 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, `impl Trait`, or `dyn Trait`") - .span_label(*span, "is a function") + .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 } - _ => true, } } diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 118c6e9a27363..c4c09a55a6e00 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -124,7 +124,6 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { check_must_not_suspend_ty( self.fcx, - ty::ParamEnv::empty(), ty, hir_id, expr, @@ -454,7 +453,6 @@ impl<'a, 'tcx> Visitor<'tcx> for ArmPatCollector<'a> { // for creating must_use diagnostics pub fn check_must_not_suspend_ty<'tcx>( fcx: &FnCtxt<'_, 'tcx>, - param_env: ty::ParamEnv<'tcx>, ty: Ty<'tcx>, hir_id: HirId, expr: Option<&'tcx Expr<'tcx>>, @@ -464,8 +462,10 @@ pub fn check_must_not_suspend_ty<'tcx>( descr_post: &str, plural_len: usize, ) -> bool { + debug!("FOUND TYPE: {:?}", ty); if ty.is_unit() - || fcx.tcx.is_ty_uninhabited_from(fcx.tcx.parent_module(hir_id).to_def_id(), ty, param_env) + // || fcx.tcx.is_ty_uninhabited_from(fcx.tcx.parent_module(hir_id).to_def_id(), ty, fcx.param_env) + // FIXME: should this check is_ty_uninhabited_from { return true; } @@ -478,7 +478,6 @@ pub fn check_must_not_suspend_ty<'tcx>( let descr_pre = &format!("{}boxed ", descr_pre); check_must_not_suspend_ty( fcx, - param_env, boxed_ty, hir_id, expr, @@ -547,28 +546,24 @@ pub fn check_must_not_suspend_ty<'tcx>( } ty::Tuple(ref tys) => { let mut has_emitted = false; - /* - let spans = if let hir::ExprKind::Tup(comps) = &expr.kind { + let spans = if let Some(hir::ExprKind::Tup(comps)) = expr.map(|e| &e.kind) { debug_assert_eq!(comps.len(), tys.len()); comps.iter().map(|e| e.span).collect() } else { vec![] }; - */ - let spans = 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); if check_must_not_suspend_ty( - fcx, param_env, ty, hir_id, expr, span, yield_span, descr_pre, descr_post, - plural_len, + fcx, ty, hir_id, expr, span, yield_span, descr_pre, descr_post, plural_len, ) { has_emitted = true; } } has_emitted } - ty::Array(ty, len) => match len.try_eval_usize(fcx.tcx, param_env) { + ty::Array(ty, len) => match len.try_eval_usize(fcx.tcx, fcx.param_env) { // If the array is empty we don't lint, to avoid false positives Some(0) | None => false, // If the array is definitely non-empty, we can do `#[must_use]` checking. @@ -576,7 +571,6 @@ pub fn check_must_not_suspend_ty<'tcx>( let descr_pre = &format!("{}array{} of ", descr_pre, plural_suffix,); check_must_not_suspend_ty( fcx, - param_env, ty, hir_id, expr, diff --git a/src/test/ui/lint/must_not_suspend/boxed.rs b/src/test/ui/lint/must_not_suspend/boxed.rs index d64d07e5e0df6..1f823fc559d40 100644 --- a/src/test/ui/lint/must_not_suspend/boxed.rs +++ b/src/test/ui/lint/must_not_suspend/boxed.rs @@ -17,7 +17,7 @@ fn bar() -> Box { async fn other() {} pub async fn uhoh() { - let _guard = bar(); //~ boxed `Umm` held across + let _guard = bar(); //~ ERROR boxed `Umm` held across other().await; } diff --git a/src/test/ui/lint/must_not_suspend/feature-gate-must_not_suspend.rs b/src/test/ui/lint/must_not_suspend/feature-gate-must_not_suspend.rs index aff8ff33b65a8..1554408c174ce 100644 --- a/src/test/ui/lint/must_not_suspend/feature-gate-must_not_suspend.rs +++ b/src/test/ui/lint/must_not_suspend/feature-gate-must_not_suspend.rs @@ -1,6 +1,6 @@ // edition:2018 -#[must_not_suspend = "You gotta use Umm's, ya know?"] //~ the `#[must_not_suspend]` +#[must_not_suspend = "You gotta use Umm's, ya know?"] //~ ERROR the `#[must_not_suspend]` struct Umm { _i: i64 } diff --git a/src/test/ui/lint/must_not_suspend/other_items.rs b/src/test/ui/lint/must_not_suspend/other_items.rs new file mode 100644 index 0000000000000..5aa1abb14d3fa --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/other_items.rs @@ -0,0 +1,8 @@ +// edition:2018 +#![feature(must_not_suspend)] +#![deny(must_not_suspend)] + +#[must_not_suspend] //~ ERROR attribute should be +mod inner {} + +fn main() {} diff --git a/src/test/ui/lint/must_not_suspend/other_items.stderr b/src/test/ui/lint/must_not_suspend/other_items.stderr new file mode 100644 index 0000000000000..41c8896921b0f --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/other_items.stderr @@ -0,0 +1,10 @@ +error: `must_not_suspend` attribute should be applied to a struct, enum, or trait + --> $DIR/other_items.rs:5:1 + | +LL | #[must_not_suspend] + | ^^^^^^^^^^^^^^^^^^^ +LL | mod inner {} + | ------------ is not a struct, enum, or trait + +error: aborting due to previous error + diff --git a/src/test/ui/lint/must_not_suspend/return.rs b/src/test/ui/lint/must_not_suspend/return.rs index 5f80e78937628..5b1fa5e272118 100644 --- a/src/test/ui/lint/must_not_suspend/return.rs +++ b/src/test/ui/lint/must_not_suspend/return.rs @@ -2,7 +2,7 @@ #![feature(must_not_suspend)] #![deny(must_not_suspend)] -#[must_not_suspend] //~ attribute should be +#[must_not_suspend] //~ ERROR attribute should be fn foo() -> i32 { 0 } diff --git a/src/test/ui/lint/must_not_suspend/return.stderr b/src/test/ui/lint/must_not_suspend/return.stderr index ff1798320cf8e..fdada85eb4d1c 100644 --- a/src/test/ui/lint/must_not_suspend/return.stderr +++ b/src/test/ui/lint/must_not_suspend/return.stderr @@ -1,4 +1,4 @@ -error: `must_not_suspend` attribute should be applied to a struct, enum, `impl Trait`, or `dyn Trait` +error: `must_not_suspend` attribute should be applied to a struct, enum, or trait --> $DIR/return.rs:5:1 | LL | #[must_not_suspend] @@ -6,7 +6,7 @@ LL | #[must_not_suspend] LL | / fn foo() -> i32 { LL | | 0 LL | | } - | |_- is a function + | |_- is not a struct, enum, or trait error: aborting due to previous error diff --git a/src/test/ui/lint/must_not_suspend/trait.rs b/src/test/ui/lint/must_not_suspend/trait.rs index 0438e072ce67e..6c911cb4b0f09 100644 --- a/src/test/ui/lint/must_not_suspend/trait.rs +++ b/src/test/ui/lint/must_not_suspend/trait.rs @@ -18,8 +18,8 @@ fn r#dyn() -> Box { async fn other() {} pub async fn uhoh() { - let _guard1 = r#impl(); //~ implementer of `Wow` held across - let _guard2 = r#dyn(); //~ boxed `Wow` trait object held across + let _guard1 = r#impl(); //~ ERROR implementer of `Wow` held across + let _guard2 = r#dyn(); //~ ERROR boxed `Wow` trait object held across other().await; } diff --git a/src/test/ui/lint/must_not_suspend/unit.rs b/src/test/ui/lint/must_not_suspend/unit.rs index 4e87b801114e9..d3a19f704324d 100644 --- a/src/test/ui/lint/must_not_suspend/unit.rs +++ b/src/test/ui/lint/must_not_suspend/unit.rs @@ -17,7 +17,7 @@ fn bar() -> Umm { async fn other() {} pub async fn uhoh() { - let _guard = bar(); //~ `Umm` held across + let _guard = bar(); //~ ERROR `Umm` held across other().await; } diff --git a/src/test/ui/lint/must_not_suspend/warn.rs b/src/test/ui/lint/must_not_suspend/warn.rs index d0d7238480759..50a696ba52322 100644 --- a/src/test/ui/lint/must_not_suspend/warn.rs +++ b/src/test/ui/lint/must_not_suspend/warn.rs @@ -17,7 +17,7 @@ fn bar() -> Umm { async fn other() {} pub async fn uhoh() { - let _guard = bar(); //~ `Umm` held across + let _guard = bar(); //~ WARNING `Umm` held across other().await; } From 461a0f3da4acd6dfea31b9f12c1e14405b055438 Mon Sep 17 00:00:00 2001 From: Gus Wynn Date: Sat, 11 Sep 2021 12:24:40 -0700 Subject: [PATCH 4/9] array comment + test for references --- .../src/check/generator_interior.rs | 34 +++++++-------- src/test/ui/lint/must_not_suspend/ref.rs | 30 ++++++++++++++ src/test/ui/lint/must_not_suspend/ref.stderr | 41 +++++++++++++++++++ 3 files changed, 85 insertions(+), 20 deletions(-) create mode 100644 src/test/ui/lint/must_not_suspend/ref.rs create mode 100644 src/test/ui/lint/must_not_suspend/ref.stderr diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index c4c09a55a6e00..1b63d6d974148 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -462,7 +462,6 @@ pub fn check_must_not_suspend_ty<'tcx>( descr_post: &str, plural_len: usize, ) -> bool { - debug!("FOUND TYPE: {:?}", ty); if ty.is_unit() // || fcx.tcx.is_ty_uninhabited_from(fcx.tcx.parent_module(hir_id).to_def_id(), ty, fcx.param_env) // FIXME: should this check is_ty_uninhabited_from @@ -563,25 +562,20 @@ pub fn check_must_not_suspend_ty<'tcx>( } has_emitted } - ty::Array(ty, len) => match len.try_eval_usize(fcx.tcx, fcx.param_env) { - // If the array is empty we don't lint, to avoid false positives - Some(0) | None => false, - // If the array is definitely non-empty, we can do `#[must_use]` checking. - Some(n) => { - let descr_pre = &format!("{}array{} of ", descr_pre, plural_suffix,); - check_must_not_suspend_ty( - fcx, - ty, - hir_id, - expr, - source_span, - yield_span, - descr_pre, - descr_post, - n as usize + 1, - ) - } - }, + ty::Array(ty, len) => { + let descr_pre = &format!("{}array{} of ", 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, + ) + } _ => false, } } diff --git a/src/test/ui/lint/must_not_suspend/ref.rs b/src/test/ui/lint/must_not_suspend/ref.rs new file mode 100644 index 0000000000000..89fd73c187e90 --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/ref.rs @@ -0,0 +1,30 @@ +// edition:2018 +#![feature(must_not_suspend)] +#![deny(must_not_suspend)] + +#[must_not_suspend = "You gotta use Umm's, ya know?"] +struct Umm { + i: i64 +} + +struct Bar { + u: Umm, +} + +async fn other() {} + +impl Bar { + async fn uhoh(&mut self) { + let guard = &mut self.u; //~ ERROR `Umm` held across + //~^ ERROR `Umm` held across + + other().await; + + *guard = Umm { + i: 2 + } + } +} + +fn main() { +} diff --git a/src/test/ui/lint/must_not_suspend/ref.stderr b/src/test/ui/lint/must_not_suspend/ref.stderr new file mode 100644 index 0000000000000..91c91a4b545f8 --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/ref.stderr @@ -0,0 +1,41 @@ +error: `Umm` held across a yield 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 + | +note: the lint level is defined here + --> $DIR/ref.rs:3:9 + | +LL | #![deny(must_not_suspend)] + | ^^^^^^^^^^^^^^^^ + = note: You gotta use Umm's, ya know? +help: `drop` this value before the yield point, or use a block (`{ ... }`) " + to shrink its scope + --> $DIR/ref.rs:18:26 + | +LL | let guard = &mut self.u; + | ^^^^^^ + +error: `Umm` held across a yield 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 + | + = note: You gotta use Umm's, ya know? +help: `drop` this value before the yield point, or use a block (`{ ... }`) " + to shrink its scope + --> $DIR/ref.rs:18:26 + | +LL | let guard = &mut self.u; + | ^^^^^^ + +error: aborting due to 2 previous errors + From ee1d2ea3b73d07d95d5c22ccd1e884e3674fcec6 Mon Sep 17 00:00:00 2001 From: Gus Wynn Date: Sat, 11 Sep 2021 12:39:33 -0700 Subject: [PATCH 5/9] fix doctests --- compiler/rustc_lint_defs/src/builtin.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 0463cb2b76553..781ac6a14a4b3 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -327,14 +327,16 @@ declare_lint! { /// ### Example /// /// ```rust + /// #![feature(must_not_suspend)] + /// /// #[must_not_suspend] /// struct SyncThing {} /// - /// async fn yield() {} + /// async fn yield_now() {} /// /// pub async fn uhoh() { /// let guard = SyncThing {}; - /// yield().await; + /// yield_now().await; /// } /// ``` pub MUST_NOT_SUSPEND, From 2af1ebfbdf01e4aa43fd9b1b5af602b545101e1f Mon Sep 17 00:00:00 2001 From: Gus Wynn Date: Mon, 13 Sep 2021 08:19:40 -0700 Subject: [PATCH 6/9] error formatting and fix build --- compiler/rustc_feature/src/active.rs | 4 +-- compiler/rustc_lint_defs/src/builtin.rs | 26 ++++++++++++------- .../src/check/generator_interior.rs | 15 ++++++----- .../ui/lint/must_not_suspend/boxed.stderr | 11 +++++--- src/test/ui/lint/must_not_suspend/ref.stderr | 22 ++++++++++------ .../ui/lint/must_not_suspend/trait.stderr | 10 +++---- src/test/ui/lint/must_not_suspend/unit.stderr | 11 +++++--- src/test/ui/lint/must_not_suspend/warn.stderr | 11 +++++--- 8 files changed, 67 insertions(+), 43 deletions(-) diff --git a/compiler/rustc_feature/src/active.rs b/compiler/rustc_feature/src/active.rs index 66569270bd21e..6ab925cfa9b7f 100644 --- a/compiler/rustc_feature/src/active.rs +++ b/compiler/rustc_feature/src/active.rs @@ -679,8 +679,8 @@ declare_features! ( /// Allows `let...else` statements. (active, let_else, "1.56.0", Some(87335), None), - /// Allows `#[must_not_suspend]`. - (active, must_not_suspend, "1.56.0", Some(83310), None), + /// Allows the `#[must_not_suspend]` attribute. + (active, must_not_suspend, "1.57.0", Some(83310), None), // ------------------------------------------------------------------------- diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 781ac6a14a4b3..a192056e244bd 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -315,14 +315,8 @@ declare_lint! { } declare_lint! { - /// 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 - /// function. - /// - /// This attribute can be used to mark values that are semantically incorrect across yields - /// (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) + /// The `must_not_suspend` lint guards against values that shouldn't be held across yield points + /// (`.await`) /// /// ### Example /// @@ -339,9 +333,23 @@ declare_lint! { /// yield_now().await; /// } /// ``` + /// + /// {{produces}} + /// + /// ### 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 + /// function. + /// + /// This attribute can be used to mark values that are semantically incorrect across yields + /// (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", + "use of a `#[must_not_suspend]` value across a yield point", } declare_lint! { diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 1b63d6d974148..7e4bc08c1723c 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -463,8 +463,10 @@ pub fn check_must_not_suspend_ty<'tcx>( plural_len: usize, ) -> 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) - // FIXME: should this check is_ty_uninhabited_from { return true; } @@ -496,6 +498,7 @@ pub fn check_must_not_suspend_ty<'tcx>( descr_pre, descr_post, ), + // FIXME: support adding the attribute to TAITs ty::Opaque(def, _) => { let mut has_emitted = false; for &(predicate, _) in fcx.tcx.explicit_item_bounds(def) { @@ -604,18 +607,18 @@ fn check_must_not_suspend_def( ); 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"); + // Add optional reason note if let Some(note) = attr.value_str() { - err.note(¬e.as_str()); + err.span_note(source_span, ¬e.as_str()); } - // add span pointing to the offending yield/await) - err.span_label(yield_span, "The value is held across this yield point"); - // Add some quick suggestions on what to do err.span_help( source_span, - "`drop` this value before the yield point, or use a block (`{ ... }`) \" + "`drop` this value before the yield point, or use a block (`{ ... }`) \ to shrink its scope", ); diff --git a/src/test/ui/lint/must_not_suspend/boxed.stderr b/src/test/ui/lint/must_not_suspend/boxed.stderr index c3c23db7d72f7..7bd80405b5de6 100644 --- a/src/test/ui/lint/must_not_suspend/boxed.stderr +++ b/src/test/ui/lint/must_not_suspend/boxed.stderr @@ -4,16 +4,19 @@ error: boxed `Umm` held across a yield point, but should not be LL | let _guard = bar(); | ^^^^^^ LL | other().await; - | ------------- The value is held across this yield point + | ------------- the value is held across this yield point | note: the lint level is defined here --> $DIR/boxed.rs:3:9 | LL | #![deny(must_not_suspend)] | ^^^^^^^^^^^^^^^^ - = note: You gotta use Umm's, ya know? -help: `drop` this value before the yield point, or use a block (`{ ... }`) " - to shrink its scope +note: You gotta use Umm's, ya know? + --> $DIR/boxed.rs:20:9 + | +LL | let _guard = bar(); + | ^^^^^^ +help: `drop` this value before the yield point, or use a block (`{ ... }`) to shrink its scope --> $DIR/boxed.rs:20:9 | LL | let _guard = bar(); diff --git a/src/test/ui/lint/must_not_suspend/ref.stderr b/src/test/ui/lint/must_not_suspend/ref.stderr index 91c91a4b545f8..d2a550d7b45d4 100644 --- a/src/test/ui/lint/must_not_suspend/ref.stderr +++ b/src/test/ui/lint/must_not_suspend/ref.stderr @@ -5,16 +5,19 @@ LL | let guard = &mut self.u; | ^^^^^^ ... LL | other().await; - | ------------- The value is held across this yield point + | ------------- the value is held across this yield point | note: the lint level is defined here --> $DIR/ref.rs:3:9 | LL | #![deny(must_not_suspend)] | ^^^^^^^^^^^^^^^^ - = note: You gotta use Umm's, ya know? -help: `drop` this value before the yield point, or use a block (`{ ... }`) " - to shrink its scope +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 --> $DIR/ref.rs:18:26 | LL | let guard = &mut self.u; @@ -27,11 +30,14 @@ LL | let guard = &mut self.u; | ^^^^^^ ... LL | other().await; - | ------------- The value is held across this yield point + | ------------- the value is held across this yield point | - = note: You gotta use Umm's, ya know? -help: `drop` this value before the yield point, or use a block (`{ ... }`) " - to shrink its scope +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 --> $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 1175cbb919208..8d7bb80876440 100644 --- a/src/test/ui/lint/must_not_suspend/trait.stderr +++ b/src/test/ui/lint/must_not_suspend/trait.stderr @@ -5,15 +5,14 @@ LL | let _guard1 = r#impl(); | ^^^^^^^ ... LL | other().await; - | ------------- The value is held across this yield point + | ------------- the value is held across this yield 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: `drop` this value before the yield point, or use a block (`{ ... }`) to shrink its scope --> $DIR/trait.rs:21:9 | LL | let _guard1 = r#impl(); @@ -26,10 +25,9 @@ LL | let _guard2 = r#dyn(); | ^^^^^^^ LL | LL | other().await; - | ------------- The value is held across this yield point + | ------------- the value is held across this yield point | -help: `drop` this value before the yield point, or use a block (`{ ... }`) " - to shrink its scope +help: `drop` this value before the yield point, or use a block (`{ ... }`) to shrink its scope --> $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 cff00dd8ca409..87e0fd27e70f2 100644 --- a/src/test/ui/lint/must_not_suspend/unit.stderr +++ b/src/test/ui/lint/must_not_suspend/unit.stderr @@ -4,16 +4,19 @@ error: `Umm` held across a yield point, but should not be LL | let _guard = bar(); | ^^^^^^ LL | other().await; - | ------------- The value is held across this yield point + | ------------- the value is held across this yield point | note: the lint level is defined here --> $DIR/unit.rs:3:9 | LL | #![deny(must_not_suspend)] | ^^^^^^^^^^^^^^^^ - = note: You gotta use Umm's, ya know? -help: `drop` this value before the yield point, or use a block (`{ ... }`) " - to shrink its scope +note: You gotta use Umm's, ya know? + --> $DIR/unit.rs:20:9 + | +LL | let _guard = bar(); + | ^^^^^^ +help: `drop` this value before the yield point, or use a block (`{ ... }`) to shrink its scope --> $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 bda44d051ee7c..03b77520c9f9f 100644 --- a/src/test/ui/lint/must_not_suspend/warn.stderr +++ b/src/test/ui/lint/must_not_suspend/warn.stderr @@ -4,12 +4,15 @@ warning: `Umm` held across a yield point, but should not be LL | let _guard = bar(); | ^^^^^^ LL | other().await; - | ------------- The value is held across this yield point + | ------------- the value is held across this yield point | = note: `#[warn(must_not_suspend)]` on by default - = note: You gotta use Umm's, ya know? -help: `drop` this value before the yield point, or use a block (`{ ... }`) " - to shrink its scope +note: You gotta use Umm's, ya know? + --> $DIR/warn.rs:20:9 + | +LL | let _guard = bar(); + | ^^^^^^ +help: `drop` this value before the yield point, or use a block (`{ ... }`) to shrink its scope --> $DIR/warn.rs:20:9 | LL | let _guard = bar(); From 110aecd23eefa603ffd1b5d8f64832230d42b435 Mon Sep 17 00:00:00 2001 From: Gus Wynn Date: Wed, 15 Sep 2021 11:48:34 -0700 Subject: [PATCH 7/9] 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 a192056e244bd..649ad21385e5e 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/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) diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 7e4bc08c1723c..e8a24f01b7563 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/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,6 +449,16 @@ 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 @@ -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 7bd80405b5de6..edc62b6d687ad 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 0000000000000..8714be6449f92 --- /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 d2a550d7b45d4..d4c58bcbcd280 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 8d7bb80876440..d19ffddd482e0 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 87e0fd27e70f2..425c076823d2f 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 03b77520c9f9f..24f52275b430a 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(); From f1021bf05459ca5e84a580895565cab9663ed839 Mon Sep 17 00:00:00 2001 From: Gus Wynn Date: Sat, 18 Sep 2021 12:23:16 -0700 Subject: [PATCH 8/9] generic test --- .../src/check/generator_interior.rs | 3 ++ src/test/ui/lint/must_not_suspend/generic.rs | 21 +++++++++++++ .../ui/lint/must_not_suspend/generic.stderr | 31 +++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 src/test/ui/lint/must_not_suspend/generic.rs create mode 100644 src/test/ui/lint/must_not_suspend/generic.stderr diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index e8a24f01b7563..ac67d2b93c57b 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -462,6 +462,9 @@ pub struct SuspendCheckData<'a, 'tcx> { // 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 +// +// Note that this technique was chosen over things like a `Suspend` marker trait +// as it is simpler and has precendent in the compiler pub fn check_must_not_suspend_ty<'tcx>( fcx: &FnCtxt<'_, 'tcx>, ty: Ty<'tcx>, diff --git a/src/test/ui/lint/must_not_suspend/generic.rs b/src/test/ui/lint/must_not_suspend/generic.rs new file mode 100644 index 0000000000000..94457e375400e --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/generic.rs @@ -0,0 +1,21 @@ +// edition:2018 +#![feature(must_not_suspend)] +#![deny(must_not_suspend)] + +#[must_not_suspend] +struct No {} + +async fn shushspend() {} + +async fn wheeee(t: T) { + shushspend().await; + drop(t); +} + +async fn yes() { + wheeee(No {}).await; //~ ERROR `No` held across + //~^ ERROR `No` held across +} + +fn main() { +} diff --git a/src/test/ui/lint/must_not_suspend/generic.stderr b/src/test/ui/lint/must_not_suspend/generic.stderr new file mode 100644 index 0000000000000..d853ba720a3ad --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/generic.stderr @@ -0,0 +1,31 @@ +error: `No` held across a suspend point, but should not be + --> $DIR/generic.rs:16:12 + | +LL | wheeee(No {}).await; + | -------^^^^^------- the value is held across this suspend point + | +note: the lint level is defined here + --> $DIR/generic.rs:3:9 + | +LL | #![deny(must_not_suspend)] + | ^^^^^^^^^^^^^^^^ +help: consider using a block (`{ ... }`) to shrink the value's scope, ending before the suspend point + --> $DIR/generic.rs:16:12 + | +LL | wheeee(No {}).await; + | ^^^^^ + +error: `No` held across a suspend point, but should not be + --> $DIR/generic.rs:16:12 + | +LL | wheeee(No {}).await; + | -------^^^^^------- the value is held across this suspend point + | +help: consider using a block (`{ ... }`) to shrink the value's scope, ending before the suspend point + --> $DIR/generic.rs:16:12 + | +LL | wheeee(No {}).await; + | ^^^^^ + +error: aborting due to 2 previous errors + From 08e026675ee38bb4ca81106e766a6456c8b7382e Mon Sep 17 00:00:00 2001 From: Gus Wynn Date: Sat, 18 Sep 2021 13:00:36 -0700 Subject: [PATCH 9/9] deduplication --- .../src/check/generator_interior.rs | 29 ++++++++++------- src/test/ui/lint/must_not_suspend/dedup.rs | 20 ++++++++++++ .../ui/lint/must_not_suspend/dedup.stderr | 19 ++++++++++++ src/test/ui/lint/must_not_suspend/generic.rs | 9 +++--- .../ui/lint/must_not_suspend/generic.stderr | 31 ------------------- src/test/ui/lint/must_not_suspend/ref.rs | 1 - src/test/ui/lint/must_not_suspend/ref.stderr | 24 ++------------ 7 files changed, 62 insertions(+), 71 deletions(-) create mode 100644 src/test/ui/lint/must_not_suspend/dedup.rs create mode 100644 src/test/ui/lint/must_not_suspend/dedup.stderr delete mode 100644 src/test/ui/lint/must_not_suspend/generic.stderr diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index ac67d2b93c57b..5ad9bdbe68db0 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -33,6 +33,7 @@ struct InteriorVisitor<'a, 'tcx> { /// that they may succeed the said yield point in the post-order. guard_bindings: SmallVec<[SmallVec<[HirId; 4]>; 1]>, guard_bindings_set: HirIdSet, + linted_values: HirIdSet, } impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { @@ -122,18 +123,21 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { // 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() - }, - ); + if !self.linted_values.contains(&hir_id) { + 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.linted_values.insert(hir_id); + } self.types.insert(ty::GeneratorInteriorTypeCause { span: source_span, @@ -181,6 +185,7 @@ pub fn resolve_interior<'a, 'tcx>( prev_unresolved_span: None, guard_bindings: <_>::default(), guard_bindings_set: <_>::default(), + linted_values: <_>::default(), }; intravisit::walk_body(&mut visitor, body); diff --git a/src/test/ui/lint/must_not_suspend/dedup.rs b/src/test/ui/lint/must_not_suspend/dedup.rs new file mode 100644 index 0000000000000..040fff5a5a5a8 --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/dedup.rs @@ -0,0 +1,20 @@ +// edition:2018 +#![feature(must_not_suspend)] +#![deny(must_not_suspend)] + +#[must_not_suspend] +struct No {} + +async fn shushspend() {} + +async fn wheeee(t: T) { + shushspend().await; + drop(t); +} + +async fn yes() { + wheeee(No {}).await; //~ ERROR `No` held across +} + +fn main() { +} diff --git a/src/test/ui/lint/must_not_suspend/dedup.stderr b/src/test/ui/lint/must_not_suspend/dedup.stderr new file mode 100644 index 0000000000000..542b7a3bc7e98 --- /dev/null +++ b/src/test/ui/lint/must_not_suspend/dedup.stderr @@ -0,0 +1,19 @@ +error: `No` held across a suspend point, but should not be + --> $DIR/dedup.rs:16:12 + | +LL | wheeee(No {}).await; + | -------^^^^^------- the value is held across this suspend point + | +note: the lint level is defined here + --> $DIR/dedup.rs:3:9 + | +LL | #![deny(must_not_suspend)] + | ^^^^^^^^^^^^^^^^ +help: consider using a block (`{ ... }`) to shrink the value's scope, ending before the suspend point + --> $DIR/dedup.rs:16:12 + | +LL | wheeee(No {}).await; + | ^^^^^ + +error: aborting due to previous error + diff --git a/src/test/ui/lint/must_not_suspend/generic.rs b/src/test/ui/lint/must_not_suspend/generic.rs index 94457e375400e..b3effa020c48f 100644 --- a/src/test/ui/lint/must_not_suspend/generic.rs +++ b/src/test/ui/lint/must_not_suspend/generic.rs @@ -1,4 +1,7 @@ // edition:2018 +// run-pass +// +// this test shows a case where the lint doesn't fire in generic code #![feature(must_not_suspend)] #![deny(must_not_suspend)] @@ -12,10 +15,6 @@ async fn wheeee(t: T) { drop(t); } -async fn yes() { - wheeee(No {}).await; //~ ERROR `No` held across - //~^ ERROR `No` held across -} - fn main() { + let _fut = wheeee(No {}); } diff --git a/src/test/ui/lint/must_not_suspend/generic.stderr b/src/test/ui/lint/must_not_suspend/generic.stderr deleted file mode 100644 index d853ba720a3ad..0000000000000 --- a/src/test/ui/lint/must_not_suspend/generic.stderr +++ /dev/null @@ -1,31 +0,0 @@ -error: `No` held across a suspend point, but should not be - --> $DIR/generic.rs:16:12 - | -LL | wheeee(No {}).await; - | -------^^^^^------- the value is held across this suspend point - | -note: the lint level is defined here - --> $DIR/generic.rs:3:9 - | -LL | #![deny(must_not_suspend)] - | ^^^^^^^^^^^^^^^^ -help: consider using a block (`{ ... }`) to shrink the value's scope, ending before the suspend point - --> $DIR/generic.rs:16:12 - | -LL | wheeee(No {}).await; - | ^^^^^ - -error: `No` held across a suspend point, but should not be - --> $DIR/generic.rs:16:12 - | -LL | wheeee(No {}).await; - | -------^^^^^------- the value is held across this suspend point - | -help: consider using a block (`{ ... }`) to shrink the value's scope, ending before the suspend point - --> $DIR/generic.rs:16:12 - | -LL | wheeee(No {}).await; - | ^^^^^ - -error: aborting due to 2 previous errors - diff --git a/src/test/ui/lint/must_not_suspend/ref.rs b/src/test/ui/lint/must_not_suspend/ref.rs index 89fd73c187e90..738dd9e04655c 100644 --- a/src/test/ui/lint/must_not_suspend/ref.rs +++ b/src/test/ui/lint/must_not_suspend/ref.rs @@ -16,7 +16,6 @@ async fn other() {} impl Bar { async fn uhoh(&mut self) { let guard = &mut self.u; //~ ERROR `Umm` held across - //~^ ERROR `Umm` held across other().await; diff --git a/src/test/ui/lint/must_not_suspend/ref.stderr b/src/test/ui/lint/must_not_suspend/ref.stderr index d4c58bcbcd280..78b44b00625d1 100644 --- a/src/test/ui/lint/must_not_suspend/ref.stderr +++ b/src/test/ui/lint/must_not_suspend/ref.stderr @@ -3,7 +3,7 @@ error: `Umm` held across a suspend point, but should not be | LL | let guard = &mut self.u; | ^^^^^^ -... +LL | LL | other().await; | ------------- the value is held across this suspend point | @@ -23,25 +23,5 @@ help: consider using a block (`{ ... }`) to shrink the value's scope, ending bef LL | let guard = &mut self.u; | ^^^^^^ -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 suspend point - | -note: You gotta use Umm's, ya know? - --> $DIR/ref.rs:18:26 - | -LL | let guard = &mut self.u; - | ^^^^^^ -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: aborting due to 2 previous errors +error: aborting due to previous error