From: bors Date: Wed, 22 Sep 2021 06:43:33 +0000 (+0000) Subject: Auto merge of #88865 - guswynn:must_not_suspend, r=oli-obk X-Git-Url: https://git.lizzy.rs/?a=commitdiff_plain;h=ce45663e14dac3f0f58be698cc530bc2e6e21682;hp=-c;p=rust.git Auto merge of #88865 - guswynn:must_not_suspend, r=oli-obk Implement `#[must_not_suspend]` implements #83310 Some notes on the impl: 1. The code that searches for the attribute on the ADT is basically copied from the `must_use` lint. It's not shared, as the logic did diverge 2. The RFC does specify that the attribute can be placed on fn's (and fn-like objects), like `must_use`. I think this is a direct copy from the `must_use` reference definition. This implementation does NOT support this, as I felt that ADT's (+ `impl Trait` + `dyn Trait`) cover the usecase's people actually want on the RFC, and adding an imp for the fn call case would be significantly harder. The `must_use` impl can do a single check at fn call stmt time, but `must_not_suspend` would need to answer the question: "for some value X with type T, find any fn call that COULD have produced this value". That would require significant changes to `generator_interior.rs`, and I would need mentorship on that. `@eholk` and I are discussing it. 3. `@estebank` do you know a way I can make the user-provided `reason` note pop out? right now it seems quite hidden Also, I am not sure if we should run perf on this r? `@nikomatsakis` --- ce45663e14dac3f0f58be698cc530bc2e6e21682 diff --combined compiler/rustc_feature/src/active.rs index efa93c18636,6ab925cfa9b..2baf70197dc --- a/compiler/rustc_feature/src/active.rs +++ b/compiler/rustc_feature/src/active.rs @@@ -37,6 -37,7 +37,6 @@@ macro_rules! declare_features since: $ver, issue: to_nonzero($issue), edition: $edition, - description: concat!($($doc,)*), } ),+]; @@@ -638,6 -639,9 +638,6 @@@ declare_features! /// Allows specifying the as-needed link modifier (active, native_link_modifiers_as_needed, "1.53.0", Some(81490), None), - /// Allows unnamed fields of struct and union type - (incomplete, unnamed_fields, "1.53.0", Some(49804), None), - /// Allows qualified paths in struct expressions, struct patterns and tuple struct patterns. (active, more_qualified_paths, "1.54.0", Some(86935), None), @@@ -675,6 -679,10 +675,10 @@@ /// Allows `let...else` statements. (active, let_else, "1.56.0", Some(87335), None), + /// Allows the `#[must_not_suspend]` attribute. + (active, must_not_suspend, "1.57.0", Some(83310), None), + + // ------------------------------------------------------------------------- // feature-group-end: actual feature gates // ------------------------------------------------------------------------- diff --combined compiler/rustc_feature/src/builtin_attrs.rs index f74ea0e0c4d,928a7eb794b..f3eaf2645f5 --- a/compiler/rustc_feature/src/builtin_attrs.rs +++ b/compiler/rustc_feature/src/builtin_attrs.rs @@@ -202,6 -202,10 +202,10 @@@ pub const BUILTIN_ATTRIBUTES: &[Builtin 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, @@@ -453,9 -457,6 +457,9 @@@ ), // Enumerates "identity-like" conversion methods to suggest on type mismatch. rustc_attr!(rustc_conversion_suggestion, Normal, template!(Word), INTERNAL_UNSTABLE), + // Prevents field reads in the marked trait or method to be considered + // during dead code analysis. + rustc_attr!(rustc_trivial_field_reads, Normal, template!(Word), INTERNAL_UNSTABLE), // ========================================================================== // Internal attributes, Const related: diff --combined compiler/rustc_lint_defs/src/builtin.rs index b14abb9e5db,649ad21385e..5830ce26fea --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@@ -314,6 -314,44 +314,44 @@@ declare_lint! "imports that are never used" } + declare_lint! { + /// The `must_not_suspend` lint guards against values that shouldn't be held across suspend points + /// (`.await`) + /// + /// ### Example + /// + /// ```rust + /// #![feature(must_not_suspend)] + /// + /// #[must_not_suspend] + /// struct SyncThing {} + /// + /// async fn yield_now() {} + /// + /// pub async fn uhoh() { + /// let guard = SyncThing {}; + /// yield_now().await; + /// } + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// The `must_not_suspend` lint detects values that are marked with the `#[must_not_suspend]` + /// attribute being held across suspend points. A "suspend" point is usually a `.await` in an async + /// function. + /// + /// This attribute can be used to mark values that are semantically incorrect across suspends + /// (like certain types of timers), values that have async alternatives, and values that + /// regularly cause problems with the `Send`-ness of async fn's returned futures (like + /// `MutexGuard`'s) + /// + pub MUST_NOT_SUSPEND, + Warn, + "use of a `#[must_not_suspend]` value across a yield point", + } + declare_lint! { /// The `unused_extern_crates` lint guards against `extern crate` items /// that are never used. @@@ -1584,7 -1622,7 +1622,7 @@@ declare_lint! /// /// ### Example /// - /// ```rust + /// ```rust,edition2018 /// trait Trait { } /// /// fn takes_trait_object(_: Box) { @@@ -2993,6 -3031,7 +3031,7 @@@ declare_lint_pass! CENUM_IMPL_DROP_CAST, CONST_EVALUATABLE_UNCHECKED, INEFFECTIVE_UNSTABLE_TRAIT_IMPL, + MUST_NOT_SUSPEND, UNINHABITED_STATIC, FUNCTION_ITEM_REFERENCES, USELESS_DEPRECATED, @@@ -3010,7 -3049,6 +3049,7 @@@ UNSUPPORTED_CALLING_CONVENTIONS, BREAK_WITH_LABEL_AND_LOOP, UNUSED_ATTRIBUTES, + NON_EXHAUSTIVE_OMITTED_PATTERNS, ] } @@@ -3313,7 -3351,7 +3352,7 @@@ declare_lint! /// /// ### Example /// - /// ```rust,compile_fail + /// ```rust,edition2018,compile_fail /// #![deny(rust_2021_prefixes_incompatible_syntax)] /// /// macro_rules! m { @@@ -3333,8 -3371,6 +3372,8 @@@ /// /// This lint suggests to add whitespace between the `z` and `"hey"` tokens /// to keep them separated in Rust 2021. + // Allow this lint -- rustdoc doesn't yet support threading edition into this lint's parser. + #[allow(rustdoc::invalid_rust_codeblocks)] pub RUST_2021_PREFIXES_INCOMPATIBLE_SYNTAX, Allow, "identifiers that will be parsed as a prefix in Rust 2021", @@@ -3419,56 -3455,3 +3458,56 @@@ declare_lint! Warn, "`break` expression with label and unlabeled loop as value expression" } + +declare_lint! { + /// The `non_exhaustive_omitted_patterns` lint detects when a wildcard (`_` or `..`) in a + /// pattern for a `#[non_exhaustive]` struct or enum is reachable. + /// + /// ### Example + /// + /// ```rust,ignore (needs separate crate) + /// // crate A + /// #[non_exhaustive] + /// pub enum Bar { + /// A, + /// B, // added variant in non breaking change + /// } + /// + /// // in crate B + /// match Bar::A { + /// Bar::A => {}, + /// #[warn(non_exhaustive_omitted_patterns)] + /// _ => {}, + /// } + /// ``` + /// + /// This will produce: + /// + /// ```text + /// warning: reachable patterns not covered of non exhaustive enum + /// --> $DIR/reachable-patterns.rs:70:9 + /// | + /// LL | _ => {} + /// | ^ pattern `B` not covered + /// | + /// note: the lint level is defined here + /// --> $DIR/reachable-patterns.rs:69:16 + /// | + /// LL | #[warn(non_exhaustive_omitted_patterns)] + /// | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + /// = help: ensure that all possible cases are being handled by adding the suggested match arms + /// = note: the matched value is of type `Bar` and the `non_exhaustive_omitted_patterns` attribute was found + /// ``` + /// + /// ### Explanation + /// + /// Structs and enums tagged with `#[non_exhaustive]` force the user to add a + /// (potentially redundant) wildcard when pattern-matching, to allow for future + /// addition of fields or variants. The `non_exhaustive_omitted_patterns` lint + /// detects when such a wildcard happens to actually catch some fields/variants. + /// In other words, when the match without the wildcard would not be exhaustive. + /// This lets the user be informed if new fields/variants were added. + pub NON_EXHAUSTIVE_OMITTED_PATTERNS, + Allow, + "detect when patterns of types marked `non_exhaustive` are missed", +} diff --combined compiler/rustc_span/src/symbol.rs index 322bea3806c,eecbb9a9cfa..7c2a09e0a32 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@@ -5,7 -5,6 +5,7 @@@ use rustc_arena::DroplessArena; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::stable_hasher::{HashStable, StableHasher, ToStableHashKey}; +use rustc_data_structures::sync::Lock; use rustc_macros::HashStable_Generic; use rustc_serialize::{Decodable, Decoder, Encodable, Encoder}; @@@ -837,6 -836,7 +837,7 @@@ symbols! mul, mul_assign, mul_with_overflow, + must_not_suspend, must_use, mut_ptr, mut_slice_ptr, @@@ -924,7 -924,6 +925,7 @@@ panic_2021, panic_abort, panic_bounds_check, + panic_display, panic_fmt, panic_handler, panic_impl, @@@ -936,6 -935,7 +937,6 @@@ panic_unwind, panicking, param_attrs, - parent_trait, partial_cmp, partial_ord, passes, @@@ -1135,7 -1135,6 +1136,7 @@@ rustc_synthetic, rustc_test_marker, rustc_then_this_would_need, + rustc_trivial_field_reads, rustc_unsafe_specialization_marker, rustc_variance, rustdoc, @@@ -1212,7 -1211,6 +1213,7 @@@ simd_select_bitmask, simd_shl, simd_shr, + simd_shuffle, simd_sub, simd_trunc, simd_xor, @@@ -1361,6 -1359,7 +1362,6 @@@ unix, unlikely, unmarked_api, - unnamed_fields, unpin, unreachable, unreachable_code, @@@ -1420,7 -1419,6 +1421,7 @@@ wrapping_sub, wreg, write_bytes, + write_str, x87_reg, xer, xmm_reg, @@@ -1625,15 -1623,14 +1626,15 @@@ impl Symbol /// Maps a string to its interned representation. pub fn intern(string: &str) -> Self { - with_interner(|interner| interner.intern(string)) + with_session_globals(|session_globals| session_globals.symbol_interner.intern(string)) } /// Convert to a `SymbolStr`. This is a slowish operation because it /// requires locking the symbol interner. pub fn as_str(self) -> SymbolStr { - with_interner(|interner| unsafe { - SymbolStr { string: std::mem::transmute::<&str, &str>(interner.get(self)) } + with_session_globals(|session_globals| { + let symbol_str = session_globals.symbol_interner.get(self); + unsafe { SymbolStr { string: std::mem::transmute::<&str, &str>(symbol_str) } } }) } @@@ -1642,7 -1639,7 +1643,7 @@@ } pub fn len(self) -> usize { - with_interner(|interner| interner.get(self).len()) + with_session_globals(|session_globals| session_globals.symbol_interner.get(self).len()) } pub fn is_empty(self) -> bool { @@@ -1699,19 -1696,13 +1700,19 @@@ impl ToStableHashKey for Symb } } +#[derive(Default)] +pub(crate) struct Interner(Lock); + // The `&'static str`s in this type actually point into the arena. // // The `FxHashMap`+`Vec` pair could be replaced by `FxIndexSet`, but #75278 // found that to regress performance up to 2% in some cases. This might be // revisited after further improvements to `indexmap`. +// +// This type is private to prevent accidentally constructing more than one `Interner` on the same +// thread, which makes it easy to mixup `Symbol`s between `Interner`s. #[derive(Default)] -pub struct Interner { +struct InternerInner { arena: DroplessArena, names: FxHashMap<&'static str, Symbol>, strings: Vec<&'static str>, @@@ -1719,38 -1710,37 +1720,38 @@@ impl Interner { fn prefill(init: &[&'static str]) -> Self { - Interner { + Interner(Lock::new(InternerInner { strings: init.into(), names: init.iter().copied().zip((0..).map(Symbol::new)).collect(), ..Default::default() - } + })) } #[inline] - pub fn intern(&mut self, string: &str) -> Symbol { - if let Some(&name) = self.names.get(string) { + fn intern(&self, string: &str) -> Symbol { + let mut inner = self.0.lock(); + if let Some(&name) = inner.names.get(string) { return name; } - let name = Symbol::new(self.strings.len() as u32); + let name = Symbol::new(inner.strings.len() as u32); // `from_utf8_unchecked` is safe since we just allocated a `&str` which is known to be // UTF-8. let string: &str = - unsafe { str::from_utf8_unchecked(self.arena.alloc_slice(string.as_bytes())) }; + unsafe { str::from_utf8_unchecked(inner.arena.alloc_slice(string.as_bytes())) }; // It is safe to extend the arena allocation to `'static` because we only access // these while the arena is still alive. let string: &'static str = unsafe { &*(string as *const str) }; - self.strings.push(string); - self.names.insert(string, name); + inner.strings.push(string); + inner.names.insert(string, name); name } // Get the symbol as a string. `Symbol::as_str()` should be used in // preference to this function. - pub fn get(&self, symbol: Symbol) -> &str { - self.strings[symbol.0.as_usize()] + fn get(&self, symbol: Symbol) -> &str { + self.0.lock().strings[symbol.0.as_usize()] } } @@@ -1881,6 -1871,11 +1882,6 @@@ impl Ident } } -#[inline] -fn with_interner T>(f: F) -> T { - with_session_globals(|session_globals| f(&mut *session_globals.symbol_interner.lock())) -} - /// An alternative to [`Symbol`], useful when the chars within the symbol need to /// be accessed. It deliberately has limited functionality and should only be /// used for temporary values. diff --combined compiler/rustc_typeck/src/check/generator_interior.rs index 3da9fd159a7,5ad9bdbe68d..2910ce6de68 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@@ -1,10 -1,11 +1,11 @@@ //! This calculates the types which has storage which lives across a suspension point in a //! generator from the perspective of typeck. The actual types used at runtime -//! is calculated in `rustc_mir::transform::generator` and may be a subset of the +//! is calculated in `rustc_const_eval::transform::generator` and may be a subset of the //! types computed here. 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 +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>, @@@ -30,12 -33,14 +33,14 @@@ /// 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> { fn record( &mut self, ty: Ty<'tcx>, + hir_id: HirId, scope: Option, expr: Option<&'tcx Expr<'tcx>>, source_span: Span, @@@ -117,6 -122,23 +122,23 @@@ } else { // Insert the type into the ordered set. let scope_span = scope.map(|s| s.span(self.fcx.tcx, self.region_scope_tree)); + + 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, ty: &ty, @@@ -163,6 -185,7 +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); @@@ -290,7 -313,7 +313,7 @@@ impl<'a, 'tcx> Visitor<'tcx> for Interi 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 -365,14 +365,14 @@@ // 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 -410,23 +410,23 @@@ 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 -453,173 +453,173 @@@ impl<'a, 'tcx> Visitor<'tcx> for ArmPat } } } + + #[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 + // + // 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>, + hir_id: HirId, + data: SuspendCheckData<'_, 'tcx>, + ) -> bool { + if ty.is_unit() + // FIXME: should this check `is_ty_uninhabited_from`. This query is not available in this stage + // of typeck (before ReVar and RePlaceholder are removed), but may remove noise, like in + // `must_use` + // || fcx.tcx.is_ty_uninhabited_from(fcx.tcx.parent_module(hir_id).to_def_id(), ty, fcx.param_env) + { + return false; + } + + let plural_suffix = pluralize!(data.plural_len); + + match *ty.kind() { + ty::Adt(..) if ty.is_box() => { + let boxed_ty = ty.boxed_ty(); + let descr_pre = &format!("{}boxed ", data.descr_pre); + check_must_not_suspend_ty(fcx, boxed_ty, hir_id, SuspendCheckData { descr_pre, ..data }) + } + ty::Adt(def, _) => check_must_not_suspend_def(fcx.tcx, def.did, hir_id, data), + // FIXME: support adding the attribute to TAITs + ty::Opaque(def, _) => { + let mut has_emitted = false; + for &(predicate, _) in fcx.tcx.explicit_item_bounds(def) { + // We only look at the `DefId`, so it is safe to skip the binder here. + if let ty::PredicateKind::Trait(ref poly_trait_predicate) = + predicate.kind().skip_binder() + { + let def_id = poly_trait_predicate.trait_ref.def_id; + let descr_pre = &format!("{}implementer{} of ", data.descr_pre, plural_suffix); + if check_must_not_suspend_def( + fcx.tcx, + def_id, + hir_id, + SuspendCheckData { descr_pre, ..data }, + ) { + has_emitted = true; + break; + } + } + } + has_emitted + } + ty::Dynamic(binder, _) => { + let mut has_emitted = false; + for predicate in binder.iter() { + if let ty::ExistentialPredicate::Trait(ref trait_ref) = predicate.skip_binder() { + let def_id = trait_ref.def_id; + let descr_post = &format!(" trait object{}{}", plural_suffix, data.descr_post); + if check_must_not_suspend_def( + fcx.tcx, + def_id, + hir_id, + SuspendCheckData { descr_post, ..data }, + ) { + has_emitted = true; + break; + } + } + } + has_emitted + } + ty::Tuple(ref tys) => { + let mut has_emitted = false; + let spans = if let Some(hir::ExprKind::Tup(comps)) = data.expr.map(|e| &e.kind) { + debug_assert_eq!(comps.len(), tys.len()); + comps.iter().map(|e| e.span).collect() + } else { + vec![] + }; + for (i, ty) in tys.iter().map(|k| k.expect_ty()).enumerate() { + let descr_post = &format!(" in tuple element {}", i); + let span = *spans.get(i).unwrap_or(&data.source_span); + if check_must_not_suspend_ty( + fcx, + ty, + hir_id, + SuspendCheckData { descr_post, source_span: span, ..data }, + ) { + has_emitted = true; + } + } + has_emitted + } + ty::Array(ty, len) => { + let descr_pre = &format!("{}array{} of ", data.descr_pre, plural_suffix); + check_must_not_suspend_ty( + fcx, + ty, + hir_id, + SuspendCheckData { + descr_pre, + plural_len: len.try_eval_usize(fcx.tcx, fcx.param_env).unwrap_or(0) as usize + + 1, + ..data + }, + ) + } + _ => false, + } + } + + fn check_must_not_suspend_def( + tcx: TyCtxt<'_>, + def_id: DefId, + hir_id: HirId, + data: SuspendCheckData<'_, '_>, + ) -> bool { + for attr in tcx.get_attrs(def_id).iter() { + if attr.has_name(sym::must_not_suspend) { + tcx.struct_span_lint_hir( + rustc_session::lint::builtin::MUST_NOT_SUSPEND, + hir_id, + data.source_span, + |lint| { + let msg = format!( + "{}`{}`{} held across a suspend point, but should not be", + data.descr_pre, + tcx.def_path_str(def_id), + data.descr_post, + ); + let mut err = lint.build(&msg); + + // add span pointing to the offending yield/await + err.span_label(data.yield_span, "the value is held across this suspend point"); + + // Add optional reason note + if let Some(note) = attr.value_str() { + // FIXME(guswynn): consider formatting this better + err.span_note(data.source_span, ¬e.as_str()); + } + + // Add some quick suggestions on what to do + // FIXME: can `drop` work as a suggestion here as well? + err.span_help( + data.source_span, + "consider using a block (`{ ... }`) \ + to shrink the value's scope, ending before the suspend point", + ); + + err.emit(); + }, + ); + + return true; + } + } + false + }