From aa523a9b4b42662c721fc747aa2cc278797195f2 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 13 Jan 2022 12:38:08 -0500 Subject: Fix errors on blanket impls by ignoring the children of their generated implementations --- src/librustdoc/clean/types.rs | 8 ++++++++ src/librustdoc/json/mod.rs | 6 +++++- src/test/rustdoc-json/impls/blanket_with_local.rs | 14 ++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 src/test/rustdoc-json/impls/blanket_with_local.rs (limited to 'src') diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 00c6e38839f..c47a2c7ca03 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -75,6 +75,14 @@ impl ItemId { } } + #[inline] + crate fn is_local_impl(self) -> bool { + match self { + ItemId::Blanket { impl_id, .. } => impl_id.is_local(), + ItemId::Auto { .. } | ItemId::DefId(_) | ItemId::Primitive(_, _) => false, + } + } + #[inline] #[track_caller] crate fn expect_def_id(self) -> DefId { diff --git a/src/librustdoc/json/mod.rs b/src/librustdoc/json/mod.rs index 88a5c0c5ca2..c1efefc322e 100644 --- a/src/librustdoc/json/mod.rs +++ b/src/librustdoc/json/mod.rs @@ -172,7 +172,11 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> { /// implementations filled out before they're inserted. fn item(&mut self, item: clean::Item) -> Result<(), Error> { // Flatten items that recursively store other items - item.kind.inner_items().for_each(|i| self.item(i.clone()).unwrap()); + // We skip local blanket implementations, as we'll have already seen the actual generic + // impl, and the generated ones don't need documenting. + if !item.def_id.is_local_impl() { + item.kind.inner_items().for_each(|i| self.item(i.clone()).unwrap()); + } let id = item.def_id; if let Some(mut new_item) = self.convert_item(item) { diff --git a/src/test/rustdoc-json/impls/blanket_with_local.rs b/src/test/rustdoc-json/impls/blanket_with_local.rs new file mode 100644 index 00000000000..88d1477f0b2 --- /dev/null +++ b/src/test/rustdoc-json/impls/blanket_with_local.rs @@ -0,0 +1,14 @@ +// Test for the ICE in rust/83718 +// A blanket impl plus a local type together shouldn't result in mismatched ID issues + +// @has method_abi.json "$.index[*][?(@.name=='Load')]" +pub trait Load { + fn load() {} +} + +impl

Load for P { + fn load() {} +} + +// @has - "$.index[*][?(@.name=='Wrapper')]" +pub struct Wrapper {} -- cgit 1.4.1-3-g733a5 From 74f0e582becf704e6e9ccf0c1956bcbc7cc3703d Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 13 Jan 2022 13:25:11 -0500 Subject: Fix typo in test --- src/test/rustdoc-json/impls/blanket_with_local.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/test/rustdoc-json/impls/blanket_with_local.rs b/src/test/rustdoc-json/impls/blanket_with_local.rs index 88d1477f0b2..963ea2fe5ae 100644 --- a/src/test/rustdoc-json/impls/blanket_with_local.rs +++ b/src/test/rustdoc-json/impls/blanket_with_local.rs @@ -1,7 +1,7 @@ // Test for the ICE in rust/83718 // A blanket impl plus a local type together shouldn't result in mismatched ID issues -// @has method_abi.json "$.index[*][?(@.name=='Load')]" +// @has blanket_with_local.json "$.index[*][?(@.name=='Load')]" pub trait Load { fn load() {} } -- cgit 1.4.1-3-g733a5 From a6aa3cb2c10c986eb231378e154766ce38f99a49 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 13 Jan 2022 14:40:28 -0500 Subject: inline ItemId method, clarify comments a bit --- src/librustdoc/clean/types.rs | 8 -------- src/librustdoc/json/mod.rs | 13 ++++++++++--- 2 files changed, 10 insertions(+), 11 deletions(-) (limited to 'src') diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index c47a2c7ca03..00c6e38839f 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -75,14 +75,6 @@ impl ItemId { } } - #[inline] - crate fn is_local_impl(self) -> bool { - match self { - ItemId::Blanket { impl_id, .. } => impl_id.is_local(), - ItemId::Auto { .. } | ItemId::DefId(_) | ItemId::Primitive(_, _) => false, - } - } - #[inline] #[track_caller] crate fn expect_def_id(self) -> DefId { diff --git a/src/librustdoc/json/mod.rs b/src/librustdoc/json/mod.rs index c1efefc322e..0f80c8a5b4c 100644 --- a/src/librustdoc/json/mod.rs +++ b/src/librustdoc/json/mod.rs @@ -171,10 +171,17 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> { /// the hashmap because certain items (traits and types) need to have their mappings for trait /// implementations filled out before they're inserted. fn item(&mut self, item: clean::Item) -> Result<(), Error> { + // We skip children of local blanket implementations, as we'll have already seen the actual + // generic impl, and the generated ones don't need documenting. + let local_blanket_impl = match item.def_id { + clean::ItemId::Blanket { impl_id, .. } => impl_id.is_local(), + clean::ItemId::Auto { .. } + | clean::ItemId::DefId(_) + | clean::ItemId::Primitive(_, _) => false, + }; + // Flatten items that recursively store other items - // We skip local blanket implementations, as we'll have already seen the actual generic - // impl, and the generated ones don't need documenting. - if !item.def_id.is_local_impl() { + if !local_blanket_impl { item.kind.inner_items().for_each(|i| self.item(i.clone()).unwrap()); } -- cgit 1.4.1-3-g733a5 From aafcbf1e70defb145542ec66a98d581c23899e76 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 13 Jan 2022 14:43:32 -0500 Subject: Update comment to make it a FIXME --- src/librustdoc/json/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/librustdoc/json/mod.rs b/src/librustdoc/json/mod.rs index 0f80c8a5b4c..c95570c4b3b 100644 --- a/src/librustdoc/json/mod.rs +++ b/src/librustdoc/json/mod.rs @@ -171,8 +171,10 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> { /// the hashmap because certain items (traits and types) need to have their mappings for trait /// implementations filled out before they're inserted. fn item(&mut self, item: clean::Item) -> Result<(), Error> { - // We skip children of local blanket implementations, as we'll have already seen the actual - // generic impl, and the generated ones don't need documenting. + // FIXME(CraftSpider): We skip children of local blanket implementations, as we'll have + // already seen the actual generic impl, and the generated ones don't need documenting. + // This is necessary due to the visibility, return type, and self arg of the generated + // impls not quite matching, and will no longer be necessary when the mismatch is fixed. let local_blanket_impl = match item.def_id { clean::ItemId::Blanket { impl_id, .. } => impl_id.is_local(), clean::ItemId::Auto { .. } -- cgit 1.4.1-3-g733a5 From 474e091160a5704ba6c07dee2d7aa789736ca857 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 13 Jan 2022 14:46:04 -0500 Subject: Move FIXME to if statement --- src/librustdoc/json/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/librustdoc/json/mod.rs b/src/librustdoc/json/mod.rs index c95570c4b3b..7f9d04d237e 100644 --- a/src/librustdoc/json/mod.rs +++ b/src/librustdoc/json/mod.rs @@ -171,10 +171,6 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> { /// the hashmap because certain items (traits and types) need to have their mappings for trait /// implementations filled out before they're inserted. fn item(&mut self, item: clean::Item) -> Result<(), Error> { - // FIXME(CraftSpider): We skip children of local blanket implementations, as we'll have - // already seen the actual generic impl, and the generated ones don't need documenting. - // This is necessary due to the visibility, return type, and self arg of the generated - // impls not quite matching, and will no longer be necessary when the mismatch is fixed. let local_blanket_impl = match item.def_id { clean::ItemId::Blanket { impl_id, .. } => impl_id.is_local(), clean::ItemId::Auto { .. } @@ -183,6 +179,10 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> { }; // Flatten items that recursively store other items + // FIXME(CraftSpider): We skip children of local blanket implementations, as we'll have + // already seen the actual generic impl, and the generated ones don't need documenting. + // This is necessary due to the visibility, return type, and self arg of the generated + // impls not quite matching, and will no longer be necessary when the mismatch is fixed. if !local_blanket_impl { item.kind.inner_items().for_each(|i| self.item(i.clone()).unwrap()); } -- cgit 1.4.1-3-g733a5 From 4be32f896a7fc7e9db8b92132b147870bd57bc9b Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 8 Oct 2021 15:09:20 -0700 Subject: Add test case for #57478 --- src/test/ui/generator/issue-57478.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 src/test/ui/generator/issue-57478.rs (limited to 'src') diff --git a/src/test/ui/generator/issue-57478.rs b/src/test/ui/generator/issue-57478.rs new file mode 100644 index 00000000000..592632cd351 --- /dev/null +++ b/src/test/ui/generator/issue-57478.rs @@ -0,0 +1,14 @@ +#![feature(negative_impls, generators)] + +struct Foo; +impl !Send for Foo {} + +fn main() { + assert_send(|| { + let guard = Foo; + drop(guard); + yield; + }) +} + +fn assert_send(_: T) {} -- cgit 1.4.1-3-g733a5 From f712df8c5dfa14a01525dc28f38d731eedcc7263 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 20 Oct 2021 16:42:53 -0700 Subject: Track drop points in generator_interior This change adds the basic infrastructure for tracking drop ranges in generator interior analysis, which allows us to exclude dropped types from the generator type. Not yet complete, but many of the async/await and generator tests pass. The main missing piece is tracking branching control flow (e.g. around an `if` expression). The patch does include support, however, for multiple yields in th e same block. Issue #57478 --- compiler/rustc_passes/src/region.rs | 1 - .../rustc_typeck/src/check/generator_interior.rs | 190 +++++++++++++++++---- src/test/ui/async-await/async-fn-nonsend.rs | 8 +- src/test/ui/async-await/unresolved_type_param.rs | 16 -- .../ui/async-await/unresolved_type_param.stderr | 50 +----- src/test/ui/generator/issue-57478.rs | 2 + 6 files changed, 168 insertions(+), 99 deletions(-) (limited to 'src') diff --git a/compiler/rustc_passes/src/region.rs b/compiler/rustc_passes/src/region.rs index db699a56645..f8989200d7e 100644 --- a/compiler/rustc_passes/src/region.rs +++ b/compiler/rustc_passes/src/region.rs @@ -255,7 +255,6 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h hir::ExprKind::AssignOp(..) | hir::ExprKind::Index(..) | hir::ExprKind::Unary(..) - | hir::ExprKind::Call(..) | hir::ExprKind::MethodCall(..) => { // FIXME(https://github.com/rust-lang/rfcs/issues/811) Nested method calls // diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index fb6e11dbfb7..a406a1a8ecd 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -3,7 +3,10 @@ //! is calculated in `rustc_const_eval::transform::generator` and may be a subset of the //! types computed here. +use crate::expr_use_visitor::{self, ExprUseVisitor}; + use super::FnCtxt; +use hir::HirIdMap; use rustc_data_structures::fx::{FxHashSet, FxIndexSet}; use rustc_errors::pluralize; use rustc_hir as hir; @@ -34,6 +37,7 @@ struct InteriorVisitor<'a, 'tcx> { guard_bindings: SmallVec<[SmallVec<[HirId; 4]>; 1]>, guard_bindings_set: HirIdSet, linted_values: HirIdSet, + drop_ranges: HirIdMap, } impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { @@ -48,9 +52,11 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { ) { use rustc_span::DUMMY_SP; + let ty = self.fcx.resolve_vars_if_possible(ty); + debug!( - "generator_interior: attempting to record type {:?} {:?} {:?} {:?}", - ty, scope, expr, source_span + "attempting to record type ty={:?}; hir_id={:?}; scope={:?}; expr={:?}; source_span={:?}; expr_count={:?}", + ty, hir_id, scope, expr, source_span, self.expr_count, ); let live_across_yield = scope @@ -68,6 +74,14 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { yield_data.expr_and_pat_count, self.expr_count, source_span ); + match self.drop_ranges.get(&hir_id) { + Some(range) if range.contains(yield_data.expr_and_pat_count) => { + debug!("value is dropped at yield point; not recording"); + return None + } + _ => (), + } + // If it is a borrowing happening in the guard, // it needs to be recorded regardless because they // do live across this yield point. @@ -85,7 +99,6 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { }); if let Some(yield_data) = live_across_yield { - let ty = self.fcx.resolve_vars_if_possible(ty); debug!( "type in expr = {:?}, scope = {:?}, type = {:?}, count = {}, yield_span = {:?}", expr, scope, ty, self.expr_count, yield_data.span @@ -154,7 +167,6 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { self.expr_count, expr.map(|e| e.span) ); - let ty = self.fcx.resolve_vars_if_possible(ty); if let Some((unresolved_type, unresolved_type_span)) = self.fcx.unresolved_type_vars(&ty) { @@ -166,6 +178,39 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { } } } + + fn visit_call( + &mut self, + call_expr: &'tcx Expr<'tcx>, + callee: &'tcx Expr<'tcx>, + args: &'tcx [Expr<'tcx>], + ) { + match &callee.kind { + ExprKind::Path(qpath) => { + let res = self.fcx.typeck_results.borrow().qpath_res(qpath, callee.hir_id); + match res { + // Direct calls never need to keep the callee `ty::FnDef` + // ZST in a temporary, so skip its type, just in case it + // can significantly complicate the generator type. + Res::Def( + DefKind::Fn | DefKind::AssocFn | DefKind::Ctor(_, CtorKind::Fn), + _, + ) => { + // NOTE(eddyb) this assumes a path expression has + // no nested expressions to keep track of. + self.expr_count += 1; + + // Record the rest of the call expression normally. + for arg in args { + self.visit_expr(arg); + } + } + _ => intravisit::walk_expr(self, call_expr), + } + } + _ => intravisit::walk_expr(self, call_expr), + } + } } pub fn resolve_interior<'a, 'tcx>( @@ -176,6 +221,20 @@ pub fn resolve_interior<'a, 'tcx>( kind: hir::GeneratorKind, ) { let body = fcx.tcx.hir().body(body_id); + + let mut drop_range_visitor = DropRangeVisitor::default(); + + // Run ExprUseVisitor to find where values are consumed. + ExprUseVisitor::new( + &mut drop_range_visitor, + &fcx.infcx, + def_id.expect_local(), + fcx.param_env, + &fcx.typeck_results.borrow(), + ) + .consume_body(body); + intravisit::walk_body(&mut drop_range_visitor, body); + let mut visitor = InteriorVisitor { fcx, types: FxIndexSet::default(), @@ -186,6 +245,7 @@ pub fn resolve_interior<'a, 'tcx>( guard_bindings: <_>::default(), guard_bindings_set: <_>::default(), linted_values: <_>::default(), + drop_ranges: drop_range_visitor.drop_ranges, }; intravisit::walk_body(&mut visitor, body); @@ -313,32 +373,9 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { let mut guard_borrowing_from_pattern = false; + match &expr.kind { - ExprKind::Call(callee, args) => match &callee.kind { - ExprKind::Path(qpath) => { - let res = self.fcx.typeck_results.borrow().qpath_res(qpath, callee.hir_id); - match res { - // Direct calls never need to keep the callee `ty::FnDef` - // ZST in a temporary, so skip its type, just in case it - // can significantly complicate the generator type. - Res::Def( - DefKind::Fn | DefKind::AssocFn | DefKind::Ctor(_, CtorKind::Fn), - _, - ) => { - // NOTE(eddyb) this assumes a path expression has - // no nested expressions to keep track of. - self.expr_count += 1; - - // Record the rest of the call expression normally. - for arg in *args { - self.visit_expr(arg); - } - } - _ => intravisit::walk_expr(self, expr), - } - } - _ => intravisit::walk_expr(self, expr), - }, + ExprKind::Call(callee, args) => self.visit_call(expr, callee, args), ExprKind::Path(qpath) => { intravisit::walk_expr(self, expr); let res = self.fcx.typeck_results.borrow().qpath_res(qpath, expr.hir_id); @@ -617,3 +654,98 @@ fn check_must_not_suspend_def( } false } + +/// This struct facilitates computing the ranges for which a place is uninitialized. +#[derive(Default)] +struct DropRangeVisitor { + consumed_places: HirIdSet, + drop_ranges: HirIdMap, + expr_count: usize, +} + +impl DropRangeVisitor { + fn record_drop(&mut self, hir_id: HirId) { + debug!("marking {:?} as dropped at {}", hir_id, self.expr_count); + self.drop_ranges.insert(hir_id, DropRange { dropped_at: self.expr_count }); + } + + /// ExprUseVisitor's consume callback doesn't go deep enough for our purposes in all + /// expressions. This method consumes a little deeper into the expression when needed. + fn consume_expr(&mut self, expr: &hir::Expr<'_>) { + self.record_drop(expr.hir_id); + match expr.kind { + hir::ExprKind::Path(hir::QPath::Resolved( + _, + hir::Path { res: hir::def::Res::Local(hir_id), .. }, + )) => { + self.record_drop(*hir_id); + } + _ => (), + } + } +} + +impl<'tcx> expr_use_visitor::Delegate<'tcx> for DropRangeVisitor { + fn consume( + &mut self, + place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>, + diag_expr_id: hir::HirId, + ) { + debug!("consume {:?}; diag_expr_id={:?}", place_with_id, diag_expr_id); + self.consumed_places.insert(place_with_id.hir_id); + } + + fn borrow( + &mut self, + _place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>, + _diag_expr_id: hir::HirId, + _bk: rustc_middle::ty::BorrowKind, + ) { + } + + fn mutate( + &mut self, + _assignee_place: &expr_use_visitor::PlaceWithHirId<'tcx>, + _diag_expr_id: hir::HirId, + ) { + } + + fn fake_read( + &mut self, + _place: expr_use_visitor::Place<'tcx>, + _cause: rustc_middle::mir::FakeReadCause, + _diag_expr_id: hir::HirId, + ) { + } +} + +impl<'tcx> Visitor<'tcx> for DropRangeVisitor { + type Map = intravisit::ErasedMap<'tcx>; + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } + + fn visit_expr(&mut self, expr: &Expr<'_>) { + intravisit::walk_expr(self, expr); + + self.expr_count += 1; + + if self.consumed_places.contains(&expr.hir_id) { + self.consume_expr(expr); + } + } +} + +struct DropRange { + /// The post-order id of the point where this expression is dropped. + /// + /// We can consider the value dropped at any post-order id greater than dropped_at. + dropped_at: usize, +} + +impl DropRange { + fn contains(&self, id: usize) -> bool { + id >= self.dropped_at + } +} diff --git a/src/test/ui/async-await/async-fn-nonsend.rs b/src/test/ui/async-await/async-fn-nonsend.rs index 845941200fc..4dd36e7f0f0 100644 --- a/src/test/ui/async-await/async-fn-nonsend.rs +++ b/src/test/ui/async-await/async-fn-nonsend.rs @@ -18,7 +18,7 @@ async fn fut() {} async fn fut_arg(_: T) {} async fn local_dropped_before_await() { - // FIXME: it'd be nice for this to be allowed in a `Send` `async fn` + // this is okay now because of the drop let x = non_send(); drop(x); fut().await; @@ -36,7 +36,7 @@ async fn non_send_temporary_in_match() { } async fn non_sync_with_method_call() { - // FIXME: it'd be nice for this to work. + let f: &mut std::fmt::Formatter = panic!(); if non_sync().fmt(f).unwrap() == () { fut().await; @@ -47,9 +47,9 @@ fn assert_send(_: impl Send) {} pub fn pass_assert() { assert_send(local_dropped_before_await()); - //~^ ERROR future cannot be sent between threads safely + assert_send(non_send_temporary_in_match()); //~^ ERROR future cannot be sent between threads safely assert_send(non_sync_with_method_call()); - //~^ ERROR future cannot be sent between threads safely + } diff --git a/src/test/ui/async-await/unresolved_type_param.rs b/src/test/ui/async-await/unresolved_type_param.rs index 85d868c2703..79c043b701d 100644 --- a/src/test/ui/async-await/unresolved_type_param.rs +++ b/src/test/ui/async-await/unresolved_type_param.rs @@ -8,24 +8,8 @@ async fn bar() -> () {} async fn foo() { bar().await; //~^ ERROR type inside `async fn` body must be known in this context - //~| ERROR type inside `async fn` body must be known in this context - //~| ERROR type inside `async fn` body must be known in this context - //~| ERROR type inside `async fn` body must be known in this context - //~| ERROR type inside `async fn` body must be known in this context //~| NOTE cannot infer type for type parameter `T` - //~| NOTE cannot infer type for type parameter `T` - //~| NOTE cannot infer type for type parameter `T` - //~| NOTE cannot infer type for type parameter `T` - //~| NOTE cannot infer type for type parameter `T` - //~| NOTE the type is part of the `async fn` body because of this `await` - //~| NOTE the type is part of the `async fn` body because of this `await` //~| NOTE the type is part of the `async fn` body because of this `await` - //~| NOTE the type is part of the `async fn` body because of this `await` - //~| NOTE the type is part of the `async fn` body because of this `await` - //~| NOTE in this expansion of desugaring of `await` - //~| NOTE in this expansion of desugaring of `await` - //~| NOTE in this expansion of desugaring of `await` - //~| NOTE in this expansion of desugaring of `await` //~| NOTE in this expansion of desugaring of `await` } fn main() {} diff --git a/src/test/ui/async-await/unresolved_type_param.stderr b/src/test/ui/async-await/unresolved_type_param.stderr index 8c0ecb8785d..853e53ed69d 100644 --- a/src/test/ui/async-await/unresolved_type_param.stderr +++ b/src/test/ui/async-await/unresolved_type_param.stderr @@ -10,54 +10,6 @@ note: the type is part of the `async fn` body because of this `await` LL | bar().await; | ^^^^^^ -error[E0698]: type inside `async fn` body must be known in this context - --> $DIR/unresolved_type_param.rs:9:5 - | -LL | bar().await; - | ^^^ cannot infer type for type parameter `T` declared on the function `bar` - | -note: the type is part of the `async fn` body because of this `await` - --> $DIR/unresolved_type_param.rs:9:10 - | -LL | bar().await; - | ^^^^^^ - -error[E0698]: type inside `async fn` body must be known in this context - --> $DIR/unresolved_type_param.rs:9:5 - | -LL | bar().await; - | ^^^ cannot infer type for type parameter `T` declared on the function `bar` - | -note: the type is part of the `async fn` body because of this `await` - --> $DIR/unresolved_type_param.rs:9:10 - | -LL | bar().await; - | ^^^^^^ - -error[E0698]: type inside `async fn` body must be known in this context - --> $DIR/unresolved_type_param.rs:9:5 - | -LL | bar().await; - | ^^^ cannot infer type for type parameter `T` declared on the function `bar` - | -note: the type is part of the `async fn` body because of this `await` - --> $DIR/unresolved_type_param.rs:9:10 - | -LL | bar().await; - | ^^^^^^ - -error[E0698]: type inside `async fn` body must be known in this context - --> $DIR/unresolved_type_param.rs:9:5 - | -LL | bar().await; - | ^^^ cannot infer type for type parameter `T` declared on the function `bar` - | -note: the type is part of the `async fn` body because of this `await` - --> $DIR/unresolved_type_param.rs:9:10 - | -LL | bar().await; - | ^^^^^^ - -error: aborting due to 5 previous errors +error: aborting due to previous error For more information about this error, try `rustc --explain E0698`. diff --git a/src/test/ui/generator/issue-57478.rs b/src/test/ui/generator/issue-57478.rs index 592632cd351..39710febdb9 100644 --- a/src/test/ui/generator/issue-57478.rs +++ b/src/test/ui/generator/issue-57478.rs @@ -1,3 +1,5 @@ +// check-pass + #![feature(negative_impls, generators)] struct Foo; -- cgit 1.4.1-3-g733a5 From c4dee401700170c95c649682d62ad150d6b5fdeb Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 22 Oct 2021 12:45:02 -0700 Subject: Track drops across multiple yields --- compiler/rustc_middle/src/middle/region.rs | 6 +-- compiler/rustc_passes/src/region.rs | 11 +++++- .../rustc_typeck/src/check/generator_interior.rs | 46 +++++++++++----------- src/test/ui/generator/drop-yield-twice.rs | 15 +++++++ src/test/ui/generator/drop-yield-twice.stderr | 25 ++++++++++++ 5 files changed, 75 insertions(+), 28 deletions(-) create mode 100644 src/test/ui/generator/drop-yield-twice.rs create mode 100644 src/test/ui/generator/drop-yield-twice.stderr (limited to 'src') diff --git a/compiler/rustc_middle/src/middle/region.rs b/compiler/rustc_middle/src/middle/region.rs index 39ca41c92ff..75dd223d014 100644 --- a/compiler/rustc_middle/src/middle/region.rs +++ b/compiler/rustc_middle/src/middle/region.rs @@ -308,7 +308,7 @@ pub struct ScopeTree { /// The reason is that semantically, until the `box` expression returns, /// the values are still owned by their containing expressions. So /// we'll see that `&x`. - pub yield_in_scope: FxHashMap, + pub yield_in_scope: FxHashMap>, /// The number of visit_expr and visit_pat calls done in the body. /// Used to sanity check visit_expr/visit_pat call count when @@ -423,8 +423,8 @@ impl ScopeTree { /// Checks whether the given scope contains a `yield`. If so, /// returns `Some(YieldData)`. If not, returns `None`. - pub fn yield_in_scope(&self, scope: Scope) -> Option { - self.yield_in_scope.get(&scope).cloned() + pub fn yield_in_scope(&self, scope: Scope) -> Option<&Vec> { + self.yield_in_scope.get(&scope) } /// Gives the number of expressions visited in a body. diff --git a/compiler/rustc_passes/src/region.rs b/compiler/rustc_passes/src/region.rs index f8989200d7e..8b22c46f01b 100644 --- a/compiler/rustc_passes/src/region.rs +++ b/compiler/rustc_passes/src/region.rs @@ -365,7 +365,8 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h let target_scopes = visitor.fixup_scopes.drain(start_point..); for scope in target_scopes { - let mut yield_data = visitor.scope_tree.yield_in_scope.get_mut(&scope).unwrap(); + let mut yield_data = + visitor.scope_tree.yield_in_scope.get_mut(&scope).unwrap().last_mut().unwrap(); let count = yield_data.expr_and_pat_count; let span = yield_data.span; @@ -428,7 +429,13 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h }; let data = YieldData { span, expr_and_pat_count: visitor.expr_and_pat_count, source: *source }; - visitor.scope_tree.yield_in_scope.insert(scope, data); + match visitor.scope_tree.yield_in_scope.get_mut(&scope) { + Some(yields) => yields.push(data), + None => { + visitor.scope_tree.yield_in_scope.insert(scope, vec![data]); + } + } + if visitor.pessimistic_yield { debug!("resolve_expr in pessimistic_yield - marking scope {:?} for fixup", scope); visitor.fixup_scopes.push(scope); diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index a406a1a8ecd..ba4080031a2 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -69,29 +69,29 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { // // See the mega-comment at `yield_in_scope` for a proof. - debug!( - "comparing counts yield: {} self: {}, source_span = {:?}", - yield_data.expr_and_pat_count, self.expr_count, source_span - ); - - match self.drop_ranges.get(&hir_id) { - Some(range) if range.contains(yield_data.expr_and_pat_count) => { - debug!("value is dropped at yield point; not recording"); - return None - } - _ => (), - } - - // If it is a borrowing happening in the guard, - // it needs to be recorded regardless because they - // do live across this yield point. - if guard_borrowing_from_pattern - || yield_data.expr_and_pat_count >= self.expr_count - { - Some(yield_data) - } else { - None - } + yield_data + .iter() + .find(|yield_data| { + debug!( + "comparing counts yield: {} self: {}, source_span = {:?}", + yield_data.expr_and_pat_count, self.expr_count, source_span + ); + + match self.drop_ranges.get(&hir_id) { + Some(range) if range.contains(yield_data.expr_and_pat_count) => { + debug!("value is dropped at yield point; not recording"); + return false; + } + _ => (), + } + + // If it is a borrowing happening in the guard, + // it needs to be recorded regardless because they + // do live across this yield point. + guard_borrowing_from_pattern + || yield_data.expr_and_pat_count >= self.expr_count + }) + .cloned() }) }) .unwrap_or_else(|| { diff --git a/src/test/ui/generator/drop-yield-twice.rs b/src/test/ui/generator/drop-yield-twice.rs new file mode 100644 index 00000000000..f484cbb8d67 --- /dev/null +++ b/src/test/ui/generator/drop-yield-twice.rs @@ -0,0 +1,15 @@ +#![feature(negative_impls, generators)] + +struct Foo(i32); +impl !Send for Foo {} + +fn main() { + assert_send(|| { //~ ERROR generator cannot be sent between threads safely + let guard = Foo(42); + yield; + drop(guard); + yield; + }) +} + +fn assert_send(_: T) {} diff --git a/src/test/ui/generator/drop-yield-twice.stderr b/src/test/ui/generator/drop-yield-twice.stderr new file mode 100644 index 00000000000..f821f2f4005 --- /dev/null +++ b/src/test/ui/generator/drop-yield-twice.stderr @@ -0,0 +1,25 @@ +error: generator cannot be sent between threads safely + --> $DIR/drop-yield-twice.rs:7:5 + | +LL | assert_send(|| { + | ^^^^^^^^^^^ generator is not `Send` + | + = help: within `[generator@$DIR/drop-yield-twice.rs:7:17: 12:6]`, the trait `Send` is not implemented for `Foo` +note: generator is not `Send` as this value is used across a yield + --> $DIR/drop-yield-twice.rs:9:9 + | +LL | let guard = Foo(42); + | ----- has type `Foo` which is not `Send` +LL | yield; + | ^^^^^ yield occurs here, with `guard` maybe used later +... +LL | }) + | - `guard` is later dropped here +note: required by a bound in `assert_send` + --> $DIR/drop-yield-twice.rs:15:19 + | +LL | fn assert_send(_: T) {} + | ^^^^ required by this bound in `assert_send` + +error: aborting due to previous error + -- cgit 1.4.1-3-g733a5 From f664cfc47cdfaa83a1fd35e6e6a3fcdb692286ae Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 22 Oct 2021 15:49:38 -0700 Subject: Make generator and async-await tests pass The main change needed to make this work is to do a pessimistic over- approximation for AssignOps. The existing ScopeTree analysis in region.rs works by doing both left to right and right to left order and then choosing the most conservative ordering. This behavior is needed because AssignOp's evaluation order depends on whether it is a primitive type or an overloaded operator, which runs as a method call. This change mimics the same behavior as region.rs in generator_interior.rs. Issue #57478 --- .../rustc_typeck/src/check/generator_interior.rs | 139 +++++++++++++++------ src/test/ui/async-await/async-fn-nonsend.rs | 5 +- 2 files changed, 106 insertions(+), 38 deletions(-) (limited to 'src') diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index ba4080031a2..baeb78139ac 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -15,6 +15,7 @@ use rustc_hir::def_id::DefId; use rustc_hir::hir_id::HirIdSet; use rustc_hir::intravisit::{self, Visitor}; use rustc_hir::{Arm, Expr, ExprKind, Guard, HirId, Pat, PatKind}; +use rustc_middle::hir::place::{Place, PlaceBase}; use rustc_middle::middle::region::{self, YieldData}; use rustc_middle::ty::{self, Ty, TyCtxt}; use rustc_span::symbol::sym; @@ -222,30 +223,37 @@ pub fn resolve_interior<'a, 'tcx>( ) { let body = fcx.tcx.hir().body(body_id); - let mut drop_range_visitor = DropRangeVisitor::default(); - - // Run ExprUseVisitor to find where values are consumed. - ExprUseVisitor::new( - &mut drop_range_visitor, - &fcx.infcx, - def_id.expect_local(), - fcx.param_env, - &fcx.typeck_results.borrow(), - ) - .consume_body(body); - intravisit::walk_body(&mut drop_range_visitor, body); - - let mut visitor = InteriorVisitor { - fcx, - types: FxIndexSet::default(), - region_scope_tree: fcx.tcx.region_scope_tree(def_id), - expr_count: 0, - kind, - prev_unresolved_span: None, - guard_bindings: <_>::default(), - guard_bindings_set: <_>::default(), - linted_values: <_>::default(), - drop_ranges: drop_range_visitor.drop_ranges, + let mut visitor = { + let mut drop_range_visitor = DropRangeVisitor { + consumed_places: <_>::default(), + borrowed_places: <_>::default(), + drop_ranges: vec![<_>::default()], + expr_count: 0, + }; + + // Run ExprUseVisitor to find where values are consumed. + ExprUseVisitor::new( + &mut drop_range_visitor, + &fcx.infcx, + def_id.expect_local(), + fcx.param_env, + &fcx.typeck_results.borrow(), + ) + .consume_body(body); + intravisit::walk_body(&mut drop_range_visitor, body); + + InteriorVisitor { + fcx, + types: FxIndexSet::default(), + region_scope_tree: fcx.tcx.region_scope_tree(def_id), + expr_count: 0, + kind, + prev_unresolved_span: None, + guard_bindings: <_>::default(), + guard_bindings_set: <_>::default(), + linted_values: <_>::default(), + drop_ranges: drop_range_visitor.drop_ranges.pop().unwrap(), + } }; intravisit::walk_body(&mut visitor, body); @@ -656,17 +664,37 @@ fn check_must_not_suspend_def( } /// This struct facilitates computing the ranges for which a place is uninitialized. -#[derive(Default)] struct DropRangeVisitor { consumed_places: HirIdSet, - drop_ranges: HirIdMap, + borrowed_places: HirIdSet, + drop_ranges: Vec>, expr_count: usize, } impl DropRangeVisitor { fn record_drop(&mut self, hir_id: HirId) { - debug!("marking {:?} as dropped at {}", hir_id, self.expr_count); - self.drop_ranges.insert(hir_id, DropRange { dropped_at: self.expr_count }); + let drop_ranges = self.drop_ranges.last_mut().unwrap(); + if self.borrowed_places.contains(&hir_id) { + debug!("not marking {:?} as dropped because it is borrowed at some point", hir_id); + } else if self.consumed_places.contains(&hir_id) { + debug!("marking {:?} as dropped at {}", hir_id, self.expr_count); + drop_ranges.insert(hir_id, DropRange { dropped_at: self.expr_count }); + } + } + + fn push_drop_scope(&mut self) { + self.drop_ranges.push(<_>::default()); + } + + fn pop_and_merge_drop_scope(&mut self) { + let mut old_last = self.drop_ranges.pop().unwrap(); + let drop_ranges = self.drop_ranges.last_mut().unwrap(); + for (k, v) in old_last.drain() { + match drop_ranges.get(&k).cloned() { + Some(v2) => drop_ranges.insert(k, v.intersect(&v2)), + None => drop_ranges.insert(k, v), + }; + } } /// ExprUseVisitor's consume callback doesn't go deep enough for our purposes in all @@ -685,6 +713,14 @@ impl DropRangeVisitor { } } +fn place_hir_id(place: &Place<'_>) -> Option { + match place.base { + PlaceBase::Rvalue | PlaceBase::StaticItem => None, + PlaceBase::Local(hir_id) + | PlaceBase::Upvar(ty::UpvarId { var_path: ty::UpvarPath { hir_id }, .. }) => Some(hir_id), + } +} + impl<'tcx> expr_use_visitor::Delegate<'tcx> for DropRangeVisitor { fn consume( &mut self, @@ -693,14 +729,16 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for DropRangeVisitor { ) { debug!("consume {:?}; diag_expr_id={:?}", place_with_id, diag_expr_id); self.consumed_places.insert(place_with_id.hir_id); + place_hir_id(&place_with_id.place).map(|place| self.consumed_places.insert(place)); } fn borrow( &mut self, - _place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>, + place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>, _diag_expr_id: hir::HirId, _bk: rustc_middle::ty::BorrowKind, ) { + place_hir_id(&place_with_id.place).map(|place| self.borrowed_places.insert(place)); } fn mutate( @@ -726,17 +764,44 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor { NestedVisitorMap::None } - fn visit_expr(&mut self, expr: &Expr<'_>) { - intravisit::walk_expr(self, expr); + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + match expr.kind { + ExprKind::AssignOp(_, lhs, rhs) => { + // These operations are weird because their order of evaluation depends on whether + // the operator is overloaded. In a perfect world, we'd just ask the type checker + // whether this is a method call, but we also need to match the expression IDs + // from RegionResolutionVisitor. RegionResolutionVisitor doesn't know the order, + // so it runs both orders and picks the most conservative. We'll mirror that here. + let mut old_count = self.expr_count; + intravisit::walk_expr(self, lhs); + intravisit::walk_expr(self, rhs); + + self.push_drop_scope(); + std::mem::swap(&mut old_count, &mut self.expr_count); + intravisit::walk_expr(self, rhs); + intravisit::walk_expr(self, lhs); + + // We should have visited the same number of expressions in either order. + assert_eq!(old_count, self.expr_count); + + self.pop_and_merge_drop_scope(); + } + _ => intravisit::walk_expr(self, expr), + } self.expr_count += 1; + self.consume_expr(expr); + } - if self.consumed_places.contains(&expr.hir_id) { - self.consume_expr(expr); - } + fn visit_pat(&mut self, pat: &'tcx Pat<'tcx>) { + intravisit::walk_pat(self, pat); + + // Increment expr_count here to match what InteriorVisitor expects. + self.expr_count += 1; } } +#[derive(Clone)] struct DropRange { /// The post-order id of the point where this expression is dropped. /// @@ -745,7 +810,11 @@ struct DropRange { } impl DropRange { + fn intersect(&self, other: &Self) -> Self { + Self { dropped_at: self.dropped_at.max(other.dropped_at) } + } + fn contains(&self, id: usize) -> bool { - id >= self.dropped_at + id > self.dropped_at } } diff --git a/src/test/ui/async-await/async-fn-nonsend.rs b/src/test/ui/async-await/async-fn-nonsend.rs index 4dd36e7f0f0..210d9ff3f2d 100644 --- a/src/test/ui/async-await/async-fn-nonsend.rs +++ b/src/test/ui/async-await/async-fn-nonsend.rs @@ -36,7 +36,7 @@ async fn non_send_temporary_in_match() { } async fn non_sync_with_method_call() { - + // FIXME: it would be nice for this to work let f: &mut std::fmt::Formatter = panic!(); if non_sync().fmt(f).unwrap() == () { fut().await; @@ -47,9 +47,8 @@ fn assert_send(_: impl Send) {} pub fn pass_assert() { assert_send(local_dropped_before_await()); - assert_send(non_send_temporary_in_match()); //~^ ERROR future cannot be sent between threads safely assert_send(non_sync_with_method_call()); - + //~^ ERROR future cannot be sent between threads safely } -- cgit 1.4.1-3-g733a5 From f246c0b116cdbbad570c23c5745aa01f6f3f64a0 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Mon, 25 Oct 2021 17:01:24 -0700 Subject: Attribute drop to parent expression of the consume point This is needed to handle cases like `[a, b.await, c]`. `ExprUseVisitor` considers `a` to be consumed when it is passed to the array, but the array is not quite live yet at that point. This means we were missing the `a` value across the await point. Attributing drops to the parent expression means we do not consider the value consumed until the consuming expression has finished. Issue #57478 --- .../rustc_typeck/src/check/generator_interior.rs | 70 +++++++++++++++------- src/test/ui/async-await/unresolved_type_param.rs | 8 +++ .../ui/async-await/unresolved_type_param.stderr | 26 +++++++- src/test/ui/lint/must_not_suspend/dedup.rs | 2 +- src/test/ui/lint/must_not_suspend/dedup.stderr | 12 ++-- 5 files changed, 87 insertions(+), 31 deletions(-) (limited to 'src') diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index baeb78139ac..92dea92a0bc 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -6,7 +6,7 @@ use crate::expr_use_visitor::{self, ExprUseVisitor}; use super::FnCtxt; -use hir::HirIdMap; +use hir::{HirIdMap, Node}; use rustc_data_structures::fx::{FxHashSet, FxIndexSet}; use rustc_errors::pluralize; use rustc_hir as hir; @@ -15,6 +15,7 @@ use rustc_hir::def_id::DefId; use rustc_hir::hir_id::HirIdSet; use rustc_hir::intravisit::{self, Visitor}; use rustc_hir::{Arm, Expr, ExprKind, Guard, HirId, Pat, PatKind}; +use rustc_middle::hir::map::Map; use rustc_middle::hir::place::{Place, PlaceBase}; use rustc_middle::middle::region::{self, YieldData}; use rustc_middle::ty::{self, Ty, TyCtxt}; @@ -225,6 +226,7 @@ pub fn resolve_interior<'a, 'tcx>( let mut visitor = { let mut drop_range_visitor = DropRangeVisitor { + hir: fcx.tcx.hir(), consumed_places: <_>::default(), borrowed_places: <_>::default(), drop_ranges: vec![<_>::default()], @@ -664,19 +666,28 @@ fn check_must_not_suspend_def( } /// This struct facilitates computing the ranges for which a place is uninitialized. -struct DropRangeVisitor { - consumed_places: HirIdSet, +struct DropRangeVisitor<'tcx> { + hir: Map<'tcx>, + /// Maps a HirId to a set of HirIds that are dropped by that node. + consumed_places: HirIdMap, borrowed_places: HirIdSet, drop_ranges: Vec>, expr_count: usize, } -impl DropRangeVisitor { +impl DropRangeVisitor<'tcx> { + fn mark_consumed(&mut self, consumer: HirId, target: HirId) { + if !self.consumed_places.contains_key(&consumer) { + self.consumed_places.insert(consumer, <_>::default()); + } + self.consumed_places.get_mut(&consumer).map(|places| places.insert(target)); + } + fn record_drop(&mut self, hir_id: HirId) { let drop_ranges = self.drop_ranges.last_mut().unwrap(); if self.borrowed_places.contains(&hir_id) { debug!("not marking {:?} as dropped because it is borrowed at some point", hir_id); - } else if self.consumed_places.contains(&hir_id) { + } else { debug!("marking {:?} as dropped at {}", hir_id, self.expr_count); drop_ranges.insert(hir_id, DropRange { dropped_at: self.expr_count }); } @@ -700,15 +711,24 @@ impl DropRangeVisitor { /// ExprUseVisitor's consume callback doesn't go deep enough for our purposes in all /// expressions. This method consumes a little deeper into the expression when needed. fn consume_expr(&mut self, expr: &hir::Expr<'_>) { - self.record_drop(expr.hir_id); - match expr.kind { - hir::ExprKind::Path(hir::QPath::Resolved( - _, - hir::Path { res: hir::def::Res::Local(hir_id), .. }, - )) => { - self.record_drop(*hir_id); + debug!("consuming expr {:?}, count={}", expr.hir_id, self.expr_count); + let places = self + .consumed_places + .get(&expr.hir_id) + .map_or(vec![], |places| places.iter().cloned().collect()); + for place in places { + self.record_drop(place); + if let Some(Node::Expr(expr)) = self.hir.find(place) { + match expr.kind { + hir::ExprKind::Path(hir::QPath::Resolved( + _, + hir::Path { res: hir::def::Res::Local(hir_id), .. }, + )) => { + self.record_drop(*hir_id); + } + _ => (), + } } - _ => (), } } } @@ -721,15 +741,19 @@ fn place_hir_id(place: &Place<'_>) -> Option { } } -impl<'tcx> expr_use_visitor::Delegate<'tcx> for DropRangeVisitor { +impl<'tcx> expr_use_visitor::Delegate<'tcx> for DropRangeVisitor<'tcx> { fn consume( &mut self, place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId, ) { - debug!("consume {:?}; diag_expr_id={:?}", place_with_id, diag_expr_id); - self.consumed_places.insert(place_with_id.hir_id); - place_hir_id(&place_with_id.place).map(|place| self.consumed_places.insert(place)); + let parent = match self.hir.find_parent_node(place_with_id.hir_id) { + Some(parent) => parent, + None => place_with_id.hir_id, + }; + debug!("consume {:?}; diag_expr_id={:?}, using parent {:?}", place_with_id, diag_expr_id, parent); + self.mark_consumed(parent, place_with_id.hir_id); + place_hir_id(&place_with_id.place).map(|place| self.mark_consumed(parent, place)); } fn borrow( @@ -757,7 +781,7 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for DropRangeVisitor { } } -impl<'tcx> Visitor<'tcx> for DropRangeVisitor { +impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { type Map = intravisit::ErasedMap<'tcx>; fn nested_visit_map(&mut self) -> NestedVisitorMap { @@ -766,20 +790,20 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor { fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { match expr.kind { - ExprKind::AssignOp(_, lhs, rhs) => { + ExprKind::AssignOp(_op, lhs, rhs) => { // These operations are weird because their order of evaluation depends on whether // the operator is overloaded. In a perfect world, we'd just ask the type checker // whether this is a method call, but we also need to match the expression IDs // from RegionResolutionVisitor. RegionResolutionVisitor doesn't know the order, // so it runs both orders and picks the most conservative. We'll mirror that here. let mut old_count = self.expr_count; - intravisit::walk_expr(self, lhs); - intravisit::walk_expr(self, rhs); + self.visit_expr(lhs); + self.visit_expr(rhs); self.push_drop_scope(); std::mem::swap(&mut old_count, &mut self.expr_count); - intravisit::walk_expr(self, rhs); - intravisit::walk_expr(self, lhs); + self.visit_expr(rhs); + self.visit_expr(lhs); // We should have visited the same number of expressions in either order. assert_eq!(old_count, self.expr_count); diff --git a/src/test/ui/async-await/unresolved_type_param.rs b/src/test/ui/async-await/unresolved_type_param.rs index 79c043b701d..d313691b388 100644 --- a/src/test/ui/async-await/unresolved_type_param.rs +++ b/src/test/ui/async-await/unresolved_type_param.rs @@ -8,8 +8,16 @@ async fn bar() -> () {} async fn foo() { bar().await; //~^ ERROR type inside `async fn` body must be known in this context + //~| ERROR type inside `async fn` body must be known in this context + //~| ERROR type inside `async fn` body must be known in this context //~| NOTE cannot infer type for type parameter `T` + //~| NOTE cannot infer type for type parameter `T` + //~| NOTE cannot infer type for type parameter `T` + //~| NOTE the type is part of the `async fn` body because of this `await` //~| NOTE the type is part of the `async fn` body because of this `await` + //~| NOTE the type is part of the `async fn` body because of this `await` + //~| NOTE in this expansion of desugaring of `await` + //~| NOTE in this expansion of desugaring of `await` //~| NOTE in this expansion of desugaring of `await` } fn main() {} diff --git a/src/test/ui/async-await/unresolved_type_param.stderr b/src/test/ui/async-await/unresolved_type_param.stderr index 853e53ed69d..6a268bcda62 100644 --- a/src/test/ui/async-await/unresolved_type_param.stderr +++ b/src/test/ui/async-await/unresolved_type_param.stderr @@ -10,6 +10,30 @@ note: the type is part of the `async fn` body because of this `await` LL | bar().await; | ^^^^^^ -error: aborting due to previous error +error[E0698]: type inside `async fn` body must be known in this context + --> $DIR/unresolved_type_param.rs:9:5 + | +LL | bar().await; + | ^^^ cannot infer type for type parameter `T` declared on the function `bar` + | +note: the type is part of the `async fn` body because of this `await` + --> $DIR/unresolved_type_param.rs:9:5 + | +LL | bar().await; + | ^^^^^^^^^^^ + +error[E0698]: type inside `async fn` body must be known in this context + --> $DIR/unresolved_type_param.rs:9:5 + | +LL | bar().await; + | ^^^ cannot infer type for type parameter `T` declared on the function `bar` + | +note: the type is part of the `async fn` body because of this `await` + --> $DIR/unresolved_type_param.rs:9:5 + | +LL | bar().await; + | ^^^^^^^^^^^ + +error: aborting due to 3 previous errors For more information about this error, try `rustc --explain E0698`. diff --git a/src/test/ui/lint/must_not_suspend/dedup.rs b/src/test/ui/lint/must_not_suspend/dedup.rs index 040fff5a5a5..81a08579bb7 100644 --- a/src/test/ui/lint/must_not_suspend/dedup.rs +++ b/src/test/ui/lint/must_not_suspend/dedup.rs @@ -13,7 +13,7 @@ async fn wheeee(t: T) { } async fn yes() { - wheeee(No {}).await; //~ ERROR `No` held across + 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 index bc1b611299a..d1513747452 100644 --- a/src/test/ui/lint/must_not_suspend/dedup.stderr +++ b/src/test/ui/lint/must_not_suspend/dedup.stderr @@ -1,8 +1,8 @@ error: `No` held across a suspend point, but should not be - --> $DIR/dedup.rs:16:12 + --> $DIR/dedup.rs:16:13 | -LL | wheeee(No {}).await; - | ^^^^^ ------ the value is held across this suspend point +LL | wheeee(&No {}).await; + | --------^^^^^------- the value is held across this suspend point | note: the lint level is defined here --> $DIR/dedup.rs:3:9 @@ -10,10 +10,10 @@ note: the lint level is defined here 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 + --> $DIR/dedup.rs:16:13 | -LL | wheeee(No {}).await; - | ^^^^^ +LL | wheeee(&No {}).await; + | ^^^^^ error: aborting due to previous error -- cgit 1.4.1-3-g733a5 From aa029d4bbe78fafbffdebb398a767941459d9d4e Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 27 Oct 2021 17:46:08 -0700 Subject: Support conditional drops This adds support for branching and merging control flow and uses this to correctly handle the case where a value is dropped in one branch of an if expression but not another. There are other cases we need to handle, which will come in follow up patches. Issue #57478 --- Cargo.lock | 1 + compiler/rustc_typeck/Cargo.toml | 1 + .../rustc_typeck/src/check/generator_interior.rs | 224 ++++++++++++++++++--- src/test/ui/generator/drop-if.rs | 22 ++ 4 files changed, 220 insertions(+), 28 deletions(-) create mode 100644 src/test/ui/generator/drop-if.rs (limited to 'src') diff --git a/Cargo.lock b/Cargo.lock index 529e17b158f..dfe3db5907a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4383,6 +4383,7 @@ dependencies = [ name = "rustc_typeck" version = "0.0.0" dependencies = [ + "itertools 0.9.0", "rustc_arena", "rustc_ast", "rustc_attr", diff --git a/compiler/rustc_typeck/Cargo.toml b/compiler/rustc_typeck/Cargo.toml index 7e570e151c5..1da106d7002 100644 --- a/compiler/rustc_typeck/Cargo.toml +++ b/compiler/rustc_typeck/Cargo.toml @@ -8,6 +8,7 @@ test = false doctest = false [dependencies] +itertools = "0.9" rustc_arena = { path = "../rustc_arena" } tracing = "0.1" rustc_macros = { path = "../rustc_macros" } diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 92dea92a0bc..6144cbbd8dd 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -3,10 +3,13 @@ //! is calculated in `rustc_const_eval::transform::generator` and may be a subset of the //! types computed here. +use std::mem; + use crate::expr_use_visitor::{self, ExprUseVisitor}; use super::FnCtxt; use hir::{HirIdMap, Node}; +use itertools::Itertools; use rustc_data_structures::fx::{FxHashSet, FxIndexSet}; use rustc_errors::pluralize; use rustc_hir as hir; @@ -24,6 +27,9 @@ use rustc_span::Span; use smallvec::SmallVec; use tracing::debug; +#[cfg(test)] +mod tests; + struct InteriorVisitor<'a, 'tcx> { fcx: &'a FnCtxt<'a, 'tcx>, types: FxIndexSet>, @@ -80,7 +86,9 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { ); match self.drop_ranges.get(&hir_id) { - Some(range) if range.contains(yield_data.expr_and_pat_count) => { + Some(range) + if range.is_dropped_at(yield_data.expr_and_pat_count) => + { debug!("value is dropped at yield point; not recording"); return false; } @@ -229,7 +237,7 @@ pub fn resolve_interior<'a, 'tcx>( hir: fcx.tcx.hir(), consumed_places: <_>::default(), borrowed_places: <_>::default(), - drop_ranges: vec![<_>::default()], + drop_ranges: <_>::default(), expr_count: 0, }; @@ -254,7 +262,7 @@ pub fn resolve_interior<'a, 'tcx>( guard_bindings: <_>::default(), guard_bindings_set: <_>::default(), linted_values: <_>::default(), - drop_ranges: drop_range_visitor.drop_ranges.pop().unwrap(), + drop_ranges: drop_range_visitor.drop_ranges, } }; intravisit::walk_body(&mut visitor, body); @@ -671,7 +679,7 @@ struct DropRangeVisitor<'tcx> { /// Maps a HirId to a set of HirIds that are dropped by that node. consumed_places: HirIdMap, borrowed_places: HirIdSet, - drop_ranges: Vec>, + drop_ranges: HirIdMap, expr_count: usize, } @@ -684,28 +692,42 @@ impl DropRangeVisitor<'tcx> { } fn record_drop(&mut self, hir_id: HirId) { - let drop_ranges = self.drop_ranges.last_mut().unwrap(); + let drop_ranges = &mut self.drop_ranges; if self.borrowed_places.contains(&hir_id) { debug!("not marking {:?} as dropped because it is borrowed at some point", hir_id); } else { debug!("marking {:?} as dropped at {}", hir_id, self.expr_count); - drop_ranges.insert(hir_id, DropRange { dropped_at: self.expr_count }); + drop_ranges.insert(hir_id, DropRange::new(self.expr_count)); } } - fn push_drop_scope(&mut self) { - self.drop_ranges.push(<_>::default()); + fn swap_drop_ranges(&mut self, mut other: HirIdMap) -> HirIdMap { + mem::swap(&mut self.drop_ranges, &mut other); + other } - fn pop_and_merge_drop_scope(&mut self) { - let mut old_last = self.drop_ranges.pop().unwrap(); - let drop_ranges = self.drop_ranges.last_mut().unwrap(); - for (k, v) in old_last.drain() { - match drop_ranges.get(&k).cloned() { - Some(v2) => drop_ranges.insert(k, v.intersect(&v2)), - None => drop_ranges.insert(k, v), - }; - } + #[allow(dead_code)] + fn fork_drop_ranges(&self) -> HirIdMap { + self.drop_ranges.iter().map(|(k, v)| (*k, v.fork_at(self.expr_count))).collect() + } + + fn intersect_drop_ranges(&mut self, drops: HirIdMap) { + drops.into_iter().for_each(|(k, v)| match self.drop_ranges.get_mut(&k) { + Some(ranges) => *ranges = ranges.intersect(&v), + None => { + self.drop_ranges.insert(k, v); + } + }) + } + + #[allow(dead_code)] + fn merge_drop_ranges(&mut self, drops: HirIdMap) { + drops.into_iter().for_each(|(k, v)| { + if !self.drop_ranges.contains_key(&k) { + self.drop_ranges.insert(k, DropRange { events: vec![] }); + } + self.drop_ranges.get_mut(&k).unwrap().merge_with(&v, self.expr_count); + }); } /// ExprUseVisitor's consume callback doesn't go deep enough for our purposes in all @@ -751,7 +773,10 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for DropRangeVisitor<'tcx> { Some(parent) => parent, None => place_with_id.hir_id, }; - debug!("consume {:?}; diag_expr_id={:?}, using parent {:?}", place_with_id, diag_expr_id, parent); + debug!( + "consume {:?}; diag_expr_id={:?}, using parent {:?}", + place_with_id, diag_expr_id, parent + ); self.mark_consumed(parent, place_with_id.hir_id); place_hir_id(&place_with_id.place).map(|place| self.mark_consumed(parent, place)); } @@ -800,7 +825,7 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { self.visit_expr(lhs); self.visit_expr(rhs); - self.push_drop_scope(); + let old_drops = self.swap_drop_ranges(<_>::default()); std::mem::swap(&mut old_count, &mut self.expr_count); self.visit_expr(rhs); self.visit_expr(lhs); @@ -808,7 +833,39 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { // We should have visited the same number of expressions in either order. assert_eq!(old_count, self.expr_count); - self.pop_and_merge_drop_scope(); + self.intersect_drop_ranges(old_drops); + } + ExprKind::If(test, if_true, if_false) => { + self.visit_expr(test); + + match if_false { + Some(if_false) => { + let mut true_ranges = self.fork_drop_ranges(); + let mut false_ranges = self.fork_drop_ranges(); + + true_ranges = self.swap_drop_ranges(true_ranges); + self.visit_expr(if_true); + true_ranges = self.swap_drop_ranges(true_ranges); + + false_ranges = self.swap_drop_ranges(false_ranges); + self.visit_expr(if_false); + false_ranges = self.swap_drop_ranges(false_ranges); + + self.merge_drop_ranges(true_ranges); + self.merge_drop_ranges(false_ranges); + } + None => { + let mut true_ranges = self.fork_drop_ranges(); + debug!("true branch drop range fork: {:?}", true_ranges); + true_ranges = self.swap_drop_ranges(true_ranges); + self.visit_expr(if_true); + true_ranges = self.swap_drop_ranges(true_ranges); + debug!("true branch computed drop_ranges: {:?}", true_ranges); + debug!("drop ranges before merging: {:?}", self.drop_ranges); + self.merge_drop_ranges(true_ranges); + debug!("drop ranges after merging: {:?}", self.drop_ranges); + } + } } _ => intravisit::walk_expr(self, expr), } @@ -825,20 +882,131 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { } } -#[derive(Clone)] +#[derive(Clone, Debug, PartialEq, Eq)] +enum Event { + Drop(usize), + Reinit(usize), +} + +impl Event { + fn location(&self) -> usize { + match *self { + Event::Drop(i) | Event::Reinit(i) => i, + } + } +} + +impl PartialOrd for Event { + fn partial_cmp(&self, other: &Self) -> Option { + self.location().partial_cmp(&other.location()) + } +} + +#[derive(Clone, Debug, PartialEq, Eq)] struct DropRange { - /// The post-order id of the point where this expression is dropped. - /// - /// We can consider the value dropped at any post-order id greater than dropped_at. - dropped_at: usize, + events: Vec, } impl DropRange { + fn new(begin: usize) -> Self { + Self { events: vec![Event::Drop(begin)] } + } + fn intersect(&self, other: &Self) -> Self { - Self { dropped_at: self.dropped_at.max(other.dropped_at) } + let mut events = vec![]; + self.events + .iter() + .merge_join_by(other.events.iter(), |a, b| a.partial_cmp(b).unwrap()) + .fold((false, false), |(left, right), event| match event { + itertools::EitherOrBoth::Both(_, _) => todo!(), + itertools::EitherOrBoth::Left(e) => match e { + Event::Drop(i) => { + if !left && right { + events.push(Event::Drop(*i)); + } + (true, right) + } + Event::Reinit(i) => { + if left && !right { + events.push(Event::Reinit(*i)); + } + (false, right) + } + }, + itertools::EitherOrBoth::Right(e) => match e { + Event::Drop(i) => { + if left && !right { + events.push(Event::Drop(*i)); + } + (left, true) + } + Event::Reinit(i) => { + if !left && right { + events.push(Event::Reinit(*i)); + } + (left, false) + } + }, + }); + Self { events } + } + + fn is_dropped_at(&self, id: usize) -> bool { + match self.events.iter().try_fold(false, |is_dropped, event| { + if event.location() < id { + Ok(match event { + Event::Drop(_) => true, + Event::Reinit(_) => false, + }) + } else { + Err(is_dropped) + } + }) { + Ok(is_dropped) | Err(is_dropped) => is_dropped, + } + } + + #[allow(dead_code)] + fn drop(&mut self, location: usize) { + self.events.push(Event::Drop(location)) + } + + #[allow(dead_code)] + fn reinit(&mut self, location: usize) { + self.events.push(Event::Reinit(location)); + } + + /// Merges another range with this one. Meant to be used at control flow join points. + /// + /// After merging, the value will be dead at the end of the range only if it was dead + /// at the end of both self and other. + /// + /// Assumes that all locations in each range are less than joinpoint + #[allow(dead_code)] + fn merge_with(&mut self, other: &DropRange, join_point: usize) { + let mut events: Vec<_> = + self.events.iter().merge(other.events.iter()).dedup().cloned().collect(); + + events.push(if self.is_dropped_at(join_point) && other.is_dropped_at(join_point) { + Event::Drop(join_point) + } else { + Event::Reinit(join_point) + }); + + self.events = events; } - fn contains(&self, id: usize) -> bool { - id > self.dropped_at + /// Creates a new DropRange from this one at the split point. + /// + /// Used to model branching control flow. + #[allow(dead_code)] + fn fork_at(&self, split_point: usize) -> Self { + Self { + events: vec![if self.is_dropped_at(split_point) { + Event::Drop(split_point) + } else { + Event::Reinit(split_point) + }], + } } } diff --git a/src/test/ui/generator/drop-if.rs b/src/test/ui/generator/drop-if.rs new file mode 100644 index 00000000000..40f01f78662 --- /dev/null +++ b/src/test/ui/generator/drop-if.rs @@ -0,0 +1,22 @@ +// build-pass + +// This test case is reduced from src/test/ui/drop/dynamic-drop-async.rs + +#![feature(generators)] + +struct Ptr; +impl<'a> Drop for Ptr { + fn drop(&mut self) { + } +} + +fn main() { + let arg = true; + let _ = || { + let arr = [Ptr]; + if arg { + drop(arr); + } + yield + }; +} -- cgit 1.4.1-3-g733a5 From 96117701f94a2c08235a87fce9d362ca26997017 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 3 Nov 2021 16:28:07 -0700 Subject: Support reinitialization of variables --- .../rustc_typeck/src/check/generator_interior.rs | 45 +++++++++--- src/test/ui/generator/drop-control-flow.rs | 81 ++++++++++++++++++++++ src/test/ui/generator/drop-if.rs | 22 ------ 3 files changed, 116 insertions(+), 32 deletions(-) create mode 100644 src/test/ui/generator/drop-control-flow.rs delete mode 100644 src/test/ui/generator/drop-if.rs (limited to 'src') diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 6144cbbd8dd..65644a54c4f 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -691,13 +691,20 @@ impl DropRangeVisitor<'tcx> { self.consumed_places.get_mut(&consumer).map(|places| places.insert(target)); } + fn drop_range(&mut self, hir_id: &HirId) -> &mut DropRange { + if !self.drop_ranges.contains_key(hir_id) { + self.drop_ranges.insert(*hir_id, DropRange::empty()); + } + self.drop_ranges.get_mut(hir_id).unwrap() + } + fn record_drop(&mut self, hir_id: HirId) { - let drop_ranges = &mut self.drop_ranges; if self.borrowed_places.contains(&hir_id) { debug!("not marking {:?} as dropped because it is borrowed at some point", hir_id); } else { debug!("marking {:?} as dropped at {}", hir_id, self.expr_count); - drop_ranges.insert(hir_id, DropRange::new(self.expr_count)); + let count = self.expr_count; + self.drop_range(&hir_id).drop(count); } } @@ -706,7 +713,6 @@ impl DropRangeVisitor<'tcx> { other } - #[allow(dead_code)] fn fork_drop_ranges(&self) -> HirIdMap { self.drop_ranges.iter().map(|(k, v)| (*k, v.fork_at(self.expr_count))).collect() } @@ -720,7 +726,6 @@ impl DropRangeVisitor<'tcx> { }) } - #[allow(dead_code)] fn merge_drop_ranges(&mut self, drops: HirIdMap) { drops.into_iter().for_each(|(k, v)| { if !self.drop_ranges.contains_key(&k) { @@ -753,6 +758,20 @@ impl DropRangeVisitor<'tcx> { } } } + + fn reinit_expr(&mut self, expr: &hir::Expr<'_>) { + if let ExprKind::Path(hir::QPath::Resolved( + _, + hir::Path { res: hir::def::Res::Local(hir_id), .. }, + )) = expr.kind + { + let location = self.expr_count; + debug!("reinitializing {:?} at {}", hir_id, location); + self.drop_range(hir_id).reinit(location) + } else { + warn!("reinitializing {:?} is not supported", expr); + } + } } fn place_hir_id(place: &Place<'_>) -> Option { @@ -814,6 +833,7 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { } fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + let mut reinit = None; match expr.kind { ExprKind::AssignOp(_op, lhs, rhs) => { // These operations are weird because their order of evaluation depends on whether @@ -867,11 +887,20 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { } } } + ExprKind::Assign(lhs, rhs, _) => { + self.visit_expr(lhs); + self.visit_expr(rhs); + + reinit = Some(lhs); + } _ => intravisit::walk_expr(self, expr), } self.expr_count += 1; self.consume_expr(expr); + if let Some(expr) = reinit { + self.reinit_expr(expr); + } } fn visit_pat(&mut self, pat: &'tcx Pat<'tcx>) { @@ -908,8 +937,8 @@ struct DropRange { } impl DropRange { - fn new(begin: usize) -> Self { - Self { events: vec![Event::Drop(begin)] } + fn empty() -> Self { + Self { events: vec![] } } fn intersect(&self, other: &Self) -> Self { @@ -966,12 +995,10 @@ impl DropRange { } } - #[allow(dead_code)] fn drop(&mut self, location: usize) { self.events.push(Event::Drop(location)) } - #[allow(dead_code)] fn reinit(&mut self, location: usize) { self.events.push(Event::Reinit(location)); } @@ -982,7 +1009,6 @@ impl DropRange { /// at the end of both self and other. /// /// Assumes that all locations in each range are less than joinpoint - #[allow(dead_code)] fn merge_with(&mut self, other: &DropRange, join_point: usize) { let mut events: Vec<_> = self.events.iter().merge(other.events.iter()).dedup().cloned().collect(); @@ -999,7 +1025,6 @@ impl DropRange { /// Creates a new DropRange from this one at the split point. /// /// Used to model branching control flow. - #[allow(dead_code)] fn fork_at(&self, split_point: usize) -> Self { Self { events: vec![if self.is_dropped_at(split_point) { diff --git a/src/test/ui/generator/drop-control-flow.rs b/src/test/ui/generator/drop-control-flow.rs new file mode 100644 index 00000000000..b180a61b104 --- /dev/null +++ b/src/test/ui/generator/drop-control-flow.rs @@ -0,0 +1,81 @@ +// build-pass + +// A test to ensure generators capture values that were conditionally dropped, +// and also that values that are dropped along all paths to a yield do not get +// included in the generator type. + +#![feature(generators, negative_impls)] + +#![allow(unused_assignments, dead_code)] + +struct Ptr; +impl<'a> Drop for Ptr { + fn drop(&mut self) {} +} + +struct NonSend {} +impl !Send for NonSend {} + +fn assert_send(_: T) {} + +// This test case is reduced from src/test/ui/drop/dynamic-drop-async.rs +fn one_armed_if(arg: bool) { + let _ = || { + let arr = [Ptr]; + if arg { + drop(arr); + } + yield; + }; +} + +fn two_armed_if(arg: bool) { + assert_send(|| { + let arr = [Ptr]; + if arg { + drop(arr); + } else { + drop(arr); + } + yield; + }) +} + +fn if_let(arg: Option) { + let _ = || { + let arr = [Ptr]; + if let Some(_) = arg { + drop(arr); + } + yield; + }; +} + +fn reinit() { + let _ = || { + let mut arr = [Ptr]; + drop(arr); + arr = [Ptr]; + yield; + }; +} + +fn loop_uninit() { + let _ = || { + let mut arr = [Ptr]; + let mut count = 0; + drop(arr); + while count < 3 { + yield; + arr = [Ptr]; + count += 1; + } + }; +} + +fn main() { + one_armed_if(true); + if_let(Some(41)); + reinit(); + // loop_uninit(); +} diff --git a/src/test/ui/generator/drop-if.rs b/src/test/ui/generator/drop-if.rs deleted file mode 100644 index 40f01f78662..00000000000 --- a/src/test/ui/generator/drop-if.rs +++ /dev/null @@ -1,22 +0,0 @@ -// build-pass - -// This test case is reduced from src/test/ui/drop/dynamic-drop-async.rs - -#![feature(generators)] - -struct Ptr; -impl<'a> Drop for Ptr { - fn drop(&mut self) { - } -} - -fn main() { - let arg = true; - let _ = || { - let arr = [Ptr]; - if arg { - drop(arr); - } - yield - }; -} -- cgit 1.4.1-3-g733a5 From 298ca2f6799201152e2e75a781a7aabb29424aea Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 4 Nov 2021 10:56:07 -0700 Subject: Basic loop support --- .../rustc_typeck/src/check/generator_interior.rs | 36 ++++++++++++++++++---- src/test/ui/generator/drop-control-flow.rs | 2 +- 2 files changed, 31 insertions(+), 7 deletions(-) (limited to 'src') diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 65644a54c4f..d7ad84684d1 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -726,15 +726,19 @@ impl DropRangeVisitor<'tcx> { }) } - fn merge_drop_ranges(&mut self, drops: HirIdMap) { + fn merge_drop_ranges_at(&mut self, drops: HirIdMap, join_point: usize) { drops.into_iter().for_each(|(k, v)| { if !self.drop_ranges.contains_key(&k) { self.drop_ranges.insert(k, DropRange { events: vec![] }); } - self.drop_ranges.get_mut(&k).unwrap().merge_with(&v, self.expr_count); + self.drop_ranges.get_mut(&k).unwrap().merge_with(&v, join_point); }); } + fn merge_drop_ranges(&mut self, drops: HirIdMap) { + self.merge_drop_ranges_at(drops, self.expr_count); + } + /// ExprUseVisitor's consume callback doesn't go deep enough for our purposes in all /// expressions. This method consumes a little deeper into the expression when needed. fn consume_expr(&mut self, expr: &hir::Expr<'_>) { @@ -893,6 +897,17 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { reinit = Some(lhs); } + ExprKind::Loop(body, ..) => { + let body_drop_ranges = self.fork_drop_ranges(); + let old_drop_ranges = self.swap_drop_ranges(body_drop_ranges); + + let join_point = self.expr_count; + + self.visit_block(body); + + let body_drop_ranges = self.swap_drop_ranges(old_drop_ranges); + self.merge_drop_ranges_at(body_drop_ranges, join_point); + } _ => intravisit::walk_expr(self, expr), } @@ -1007,11 +1022,20 @@ impl DropRange { /// /// After merging, the value will be dead at the end of the range only if it was dead /// at the end of both self and other. - /// - /// Assumes that all locations in each range are less than joinpoint fn merge_with(&mut self, other: &DropRange, join_point: usize) { - let mut events: Vec<_> = - self.events.iter().merge(other.events.iter()).dedup().cloned().collect(); + let join_event = if self.is_dropped_at(join_point) && other.is_dropped_at(join_point) { + Event::Drop(join_point) + } else { + Event::Reinit(join_point) + }; + let mut events: Vec<_> = self + .events + .iter() + .merge([join_event].iter()) + .merge(other.events.iter()) + .dedup() + .cloned() + .collect(); events.push(if self.is_dropped_at(join_point) && other.is_dropped_at(join_point) { Event::Drop(join_point) diff --git a/src/test/ui/generator/drop-control-flow.rs b/src/test/ui/generator/drop-control-flow.rs index b180a61b104..6587e54df60 100644 --- a/src/test/ui/generator/drop-control-flow.rs +++ b/src/test/ui/generator/drop-control-flow.rs @@ -77,5 +77,5 @@ fn main() { one_armed_if(true); if_let(Some(41)); reinit(); - // loop_uninit(); + loop_uninit(); } -- cgit 1.4.1-3-g733a5 From 457415294c57dff4b07cc06165eb284d9a14a24a Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 4 Nov 2021 16:38:47 -0700 Subject: Handle more cases with conditionally initialized/dropped values --- .../rustc_typeck/src/check/generator_interior.rs | 48 +++++++++++++++++++++- src/test/ui/generator/drop-control-flow.rs | 44 +++++++++++++++++++- 2 files changed, 89 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index d7ad84684d1..e7794b199e7 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -871,7 +871,8 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { self.visit_expr(if_true); true_ranges = self.swap_drop_ranges(true_ranges); - false_ranges = self.swap_drop_ranges(false_ranges); + false_ranges = + self.swap_drop_ranges(trim_drop_ranges(&false_ranges, self.expr_count)); self.visit_expr(if_false); false_ranges = self.swap_drop_ranges(false_ranges); @@ -908,6 +909,31 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { let body_drop_ranges = self.swap_drop_ranges(old_drop_ranges); self.merge_drop_ranges_at(body_drop_ranges, join_point); } + ExprKind::Match(scrutinee, arms, ..) => { + self.visit_expr(scrutinee); + + let forked_ranges = self.fork_drop_ranges(); + let arm_drops = arms + .iter() + .map(|Arm { hir_id, pat, body, guard, .. }| { + debug!("match arm {:?} starts at {}", hir_id, self.expr_count); + let old_ranges = self + .swap_drop_ranges(trim_drop_ranges(&forked_ranges, self.expr_count)); + self.visit_pat(pat); + match guard { + Some(Guard::If(expr)) => self.visit_expr(expr), + Some(Guard::IfLet(pat, expr)) => { + self.visit_pat(pat); + self.visit_expr(expr); + } + None => (), + } + self.visit_expr(body); + self.swap_drop_ranges(old_ranges) + }) + .collect::>(); + arm_drops.into_iter().for_each(|drops| self.merge_drop_ranges(drops)); + } _ => intravisit::walk_expr(self, expr), } @@ -926,6 +952,10 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { } } +fn trim_drop_ranges(drop_ranges: &HirIdMap, trim_from: usize) -> HirIdMap { + drop_ranges.iter().map(|(k, v)| (*k, v.trimmed(trim_from))).collect() +} + #[derive(Clone, Debug, PartialEq, Eq)] enum Event { Drop(usize), @@ -1058,4 +1088,20 @@ impl DropRange { }], } } + + fn trimmed(&self, trim_from: usize) -> Self { + let start = if self.is_dropped_at(trim_from) { + Event::Drop(trim_from) + } else { + Event::Reinit(trim_from) + }; + + Self { + events: [start] + .iter() + .chain(self.events.iter().skip_while(|event| event.location() <= trim_from)) + .cloned() + .collect(), + } + } } diff --git a/src/test/ui/generator/drop-control-flow.rs b/src/test/ui/generator/drop-control-flow.rs index 6587e54df60..6319a29f5b7 100644 --- a/src/test/ui/generator/drop-control-flow.rs +++ b/src/test/ui/generator/drop-control-flow.rs @@ -5,7 +5,6 @@ // included in the generator type. #![feature(generators, negative_impls)] - #![allow(unused_assignments, dead_code)] struct Ptr; @@ -13,7 +12,7 @@ impl<'a> Drop for Ptr { fn drop(&mut self) {} } -struct NonSend {} +struct NonSend; impl !Send for NonSend {} fn assert_send(_: T) {} @@ -51,6 +50,29 @@ fn if_let(arg: Option) { }; } +fn init_in_if(arg: bool) { + assert_send(|| { + let mut x = NonSend; + drop(x); + if arg { + x = NonSend; + } else { + yield; + } + }) +} + +fn init_in_match_arm(arg: Option) { + assert_send(|| { + let mut x = NonSend; + drop(x); + match arg { + Some(_) => x = NonSend, + None => yield, + } + }) +} + fn reinit() { let _ = || { let mut arr = [Ptr]; @@ -73,9 +95,27 @@ fn loop_uninit() { }; } +fn nested_loop() { + let _ = || { + let mut arr = [Ptr]; + let mut count = 0; + drop(arr); + while count < 3 { + for _ in 0..3 { + yield; + } + arr = [Ptr]; + count += 1; + } + }; +} + fn main() { one_armed_if(true); if_let(Some(41)); + init_in_if(true); + init_in_match_arm(Some(41)); reinit(); loop_uninit(); + nested_loop(); } -- cgit 1.4.1-3-g733a5 From 46760b4e673e95a4775775fe7da028a33d0f50ae Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Mon, 22 Nov 2021 14:53:02 -0800 Subject: Update async-fn-nonsend.stderr --- src/test/ui/async-await/async-fn-nonsend.stderr | 35 +++++-------------------- 1 file changed, 6 insertions(+), 29 deletions(-) (limited to 'src') diff --git a/src/test/ui/async-await/async-fn-nonsend.stderr b/src/test/ui/async-await/async-fn-nonsend.stderr index bff28208573..abba5585c62 100644 --- a/src/test/ui/async-await/async-fn-nonsend.stderr +++ b/src/test/ui/async-await/async-fn-nonsend.stderr @@ -1,28 +1,5 @@ error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:49:17 - | -LL | assert_send(local_dropped_before_await()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `local_dropped_before_await` is not `Send` - | - = help: within `impl Future`, the trait `Send` is not implemented for `Rc<()>` -note: future is not `Send` as this value is used across an await - --> $DIR/async-fn-nonsend.rs:24:10 - | -LL | let x = non_send(); - | - has type `impl Debug` which is not `Send` -LL | drop(x); -LL | fut().await; - | ^^^^^^ await occurs here, with `x` maybe used later -LL | } - | - `x` is later dropped here -note: required by a bound in `assert_send` - --> $DIR/async-fn-nonsend.rs:46:24 - | -LL | fn assert_send(_: impl Send) {} - | ^^^^ required by this bound in `assert_send` - -error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:51:17 + --> $DIR/async-fn-nonsend.rs:50:17 | LL | assert_send(non_send_temporary_in_match()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_send_temporary_in_match` is not `Send` @@ -32,12 +9,12 @@ note: future is not `Send` as this value is used across an await --> $DIR/async-fn-nonsend.rs:33:25 | LL | match Some(non_send()) { - | ---------- has type `impl Debug` which is not `Send` + | ---------------- has type `Option` which is not `Send` LL | Some(_) => fut().await, - | ^^^^^^ await occurs here, with `non_send()` maybe used later + | ^^^^^^ await occurs here, with `Some(non_send())` maybe used later ... LL | } - | - `non_send()` is later dropped here + | - `Some(non_send())` is later dropped here note: required by a bound in `assert_send` --> $DIR/async-fn-nonsend.rs:46:24 | @@ -45,7 +22,7 @@ LL | fn assert_send(_: impl Send) {} | ^^^^ required by this bound in `assert_send` error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:53:17 + --> $DIR/async-fn-nonsend.rs:52:17 | LL | assert_send(non_sync_with_method_call()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_sync_with_method_call` is not `Send` @@ -68,5 +45,5 @@ note: required by a bound in `assert_send` LL | fn assert_send(_: impl Send) {} | ^^^^ required by this bound in `assert_send` -error: aborting due to 3 previous errors +error: aborting due to 2 previous errors -- cgit 1.4.1-3-g733a5 From 30e1b1e92e3a685567417f812e3a079a9d51422c Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Mon, 13 Dec 2021 10:47:28 -0800 Subject: Address code review comments 1. Add test case for partial drops 2. Simplify code in `propagate_to_fixpoint` and remove most clones 3. Clean up PostOrderIndex creation --- .../src/check/generator_interior/drop_ranges.rs | 44 +++++++++++----------- src/test/ui/generator/partial-drop.rs | 21 +++++++++++ 2 files changed, 43 insertions(+), 22 deletions(-) create mode 100644 src/test/ui/generator/partial-drop.rs (limited to 'src') diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs index eb95c4e0b64..7b76ff5e02b 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs @@ -34,13 +34,13 @@ use crate::expr_use_visitor; /// record the parent expression, which is the point where the drop actually takes place. pub struct ExprUseDelegate<'tcx> { hir: Map<'tcx>, - /// Records the point at which an expression or local variable is dropped. + /// Records the variables/expressions that are dropped by a given expression. /// /// The key is the hir-id of the expression, and the value is a set or hir-ids for variables /// or values that are consumed by that expression. /// /// Note that this set excludes "partial drops" -- for example, a statement like `drop(x.y)` is - /// not considered a drop of `x`. + /// not considered a drop of `x`, although it would be a drop of `x.y`. consumed_places: HirIdMap, /// A set of hir-ids of values or variables that are borrowed at some point within the body. borrowed_places: HirIdSet, @@ -437,29 +437,29 @@ impl DropRanges { let mut changed = false; for id in self.nodes.indices() { let old_state = self.nodes[id].drop_state.clone(); - if preds[id].len() != 0 { - self.nodes[id].drop_state = self.nodes[preds[id][0]].drop_state.clone(); - for pred in &preds[id][1..] { - let state = self.nodes[*pred].drop_state.clone(); - self.nodes[id].drop_state.intersect(&state); - } + let mut new_state = if id.index() == 0 { + BitSet::new_empty(self.num_values()) } else { - self.nodes[id].drop_state = if id.index() == 0 { - BitSet::new_empty(self.num_values()) - } else { - // If we are not the start node and we have no predecessors, treat - // everything as dropped because there's no way to get here anyway. - BitSet::new_filled(self.num_values()) - }; + // If we are not the start node and we have no predecessors, treat + // everything as dropped because there's no way to get here anyway. + BitSet::new_filled(self.num_values()) }; - for drop in &self.nodes[id].drops.clone() { - self.nodes[id].drop_state.insert(*drop); + + for pred in &preds[id] { + let state = &self.nodes[*pred].drop_state; + new_state.intersect(state); + } + + for drop in &self.nodes[id].drops { + new_state.insert(*drop); } - for reinit in &self.nodes[id].reinits.clone() { - self.nodes[id].drop_state.remove(*reinit); + + for reinit in &self.nodes[id].reinits { + new_state.remove(*reinit); } - changed |= old_state != self.nodes[id].drop_state; + changed |= old_state != new_state; + self.nodes[id].drop_state = new_state; } changed @@ -476,7 +476,7 @@ impl DropRanges { let mut preds = IndexVec::from_fn_n(|_| vec![], self.nodes.len()); for (id, node) in self.nodes.iter_enumerated() { if node.successors.len() == 0 && id.index() != self.nodes.len() - 1 { - preds[<_>::from(id.index() + 1)].push(id); + preds[id + 1].push(id); } else { for succ in &node.successors { preds[*succ].push(id); @@ -501,7 +501,7 @@ impl<'a> dot::GraphWalk<'a> for DropRanges { .iter_enumerated() .flat_map(|(i, node)| { if node.successors.len() == 0 { - vec![(i, PostOrderId::from_usize(i.index() + 1))] + vec![(i, i + 1)] } else { node.successors.iter().map(move |&s| (i, s)).collect() } diff --git a/src/test/ui/generator/partial-drop.rs b/src/test/ui/generator/partial-drop.rs new file mode 100644 index 00000000000..a2f616aa313 --- /dev/null +++ b/src/test/ui/generator/partial-drop.rs @@ -0,0 +1,21 @@ +// check-pass + +#![feature(negative_impls, generators)] + +struct Foo; +impl !Send for Foo {} + +struct Bar { + foo: Foo, + x: i32, +} + +fn main() { + assert_send(|| { + let guard = Bar { foo: Foo, x: 42 }; + drop(guard.foo); + yield; + }) +} + +fn assert_send(_: T) {} -- cgit 1.4.1-3-g733a5 From 7d82e4f7642a3675e7dc87a483d79cf02681d930 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 15 Dec 2021 12:35:34 -0800 Subject: Update stderr files --- src/test/ui/async-await/unresolved_type_param.stderr | 8 ++++---- src/test/ui/lint/must_not_suspend/dedup.stderr | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/test/ui/async-await/unresolved_type_param.stderr b/src/test/ui/async-await/unresolved_type_param.stderr index 6a268bcda62..d19a3226ef9 100644 --- a/src/test/ui/async-await/unresolved_type_param.stderr +++ b/src/test/ui/async-await/unresolved_type_param.stderr @@ -17,10 +17,10 @@ LL | bar().await; | ^^^ cannot infer type for type parameter `T` declared on the function `bar` | note: the type is part of the `async fn` body because of this `await` - --> $DIR/unresolved_type_param.rs:9:5 + --> $DIR/unresolved_type_param.rs:9:10 | LL | bar().await; - | ^^^^^^^^^^^ + | ^^^^^^ error[E0698]: type inside `async fn` body must be known in this context --> $DIR/unresolved_type_param.rs:9:5 @@ -29,10 +29,10 @@ LL | bar().await; | ^^^ cannot infer type for type parameter `T` declared on the function `bar` | note: the type is part of the `async fn` body because of this `await` - --> $DIR/unresolved_type_param.rs:9:5 + --> $DIR/unresolved_type_param.rs:9:10 | LL | bar().await; - | ^^^^^^^^^^^ + | ^^^^^^ error: aborting due to 3 previous errors diff --git a/src/test/ui/lint/must_not_suspend/dedup.stderr b/src/test/ui/lint/must_not_suspend/dedup.stderr index d1513747452..13fa3ae3008 100644 --- a/src/test/ui/lint/must_not_suspend/dedup.stderr +++ b/src/test/ui/lint/must_not_suspend/dedup.stderr @@ -2,7 +2,7 @@ error: `No` held across a suspend point, but should not be --> $DIR/dedup.rs:16:13 | LL | wheeee(&No {}).await; - | --------^^^^^------- the value is held across this suspend point + | ^^^^^ ------ the value is held across this suspend point | note: the lint level is defined here --> $DIR/dedup.rs:3:9 -- cgit 1.4.1-3-g733a5 From 4a70de79321552fa8c254b9998562d7814f3f72e Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 16 Dec 2021 12:07:36 -0800 Subject: Handle reinits in match guards --- .../generator_interior/drop_ranges/cfg_build.rs | 21 +++++++++++------- .../drop_ranges/cfg_visualize.rs | 9 ++++++++ src/test/ui/generator/reinit-in-match-guard.rs | 25 ++++++++++++++++++++++ 3 files changed, 47 insertions(+), 8 deletions(-) create mode 100644 src/test/ui/generator/reinit-in-match-guard.rs (limited to 'src') diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs index 32f423f3bfe..0520931d5f6 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs @@ -133,11 +133,10 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { ExprKind::Match(scrutinee, arms, ..) => { self.visit_expr(scrutinee); - let fork = self.expr_index; - let arm_end_ids = arms - .iter() - .map(|hir::Arm { pat, body, guard, .. }| { - self.drop_ranges.add_control_edge(fork, self.expr_index + 1); + let (guard_exit, arm_end_ids) = arms.iter().fold( + (self.expr_index, vec![]), + |(incoming_edge, mut arm_end_ids), hir::Arm { pat, body, guard, .. }| { + self.drop_ranges.add_control_edge(incoming_edge, self.expr_index + 1); self.visit_pat(pat); match guard { Some(Guard::If(expr)) => self.visit_expr(expr), @@ -147,10 +146,16 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { } None => (), } + let to_next_arm = self.expr_index; + // The default edge does not get added since we also have an explicit edge, + // so we also need to add an edge to the next node as well. + self.drop_ranges.add_control_edge(self.expr_index, self.expr_index + 1); self.visit_expr(body); - self.expr_index - }) - .collect::>(); + arm_end_ids.push(self.expr_index); + (to_next_arm, arm_end_ids) + }, + ); + self.drop_ranges.add_control_edge(guard_exit, self.expr_index + 1); arm_end_ids.into_iter().for_each(|arm_end| { self.drop_ranges.add_control_edge(arm_end, self.expr_index + 1) }); diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_visualize.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_visualize.rs index b87b3dd9a5f..20aad7aedf7 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_visualize.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_visualize.rs @@ -5,6 +5,15 @@ use rustc_graphviz as dot; use super::{DropRangesBuilder, PostOrderId}; +/// Writes the CFG for DropRangesBuilder to a .dot file for visualization. +/// +/// It is not normally called, but is kept around to easily add debugging +/// code when needed. +#[allow(dead_code)] +pub(super) fn write_graph_to_file(drop_ranges: &DropRangesBuilder, filename: &str) { + dot::render(drop_ranges, &mut std::fs::File::create(filename).unwrap()).unwrap(); +} + impl<'a> dot::GraphWalk<'a> for DropRangesBuilder { type Node = PostOrderId; diff --git a/src/test/ui/generator/reinit-in-match-guard.rs b/src/test/ui/generator/reinit-in-match-guard.rs new file mode 100644 index 00000000000..260b341a525 --- /dev/null +++ b/src/test/ui/generator/reinit-in-match-guard.rs @@ -0,0 +1,25 @@ +// build-pass + +#![feature(generators)] + +#![allow(unused_assignments, dead_code)] + +fn main() { + let _ = || { + let mut x = vec![22_usize]; + std::mem::drop(x); + match y() { + true if { + x = vec![]; + false + } => {} + _ => { + yield; + } + } + }; +} + +fn y() -> bool { + true +} -- cgit 1.4.1-3-g733a5 From a7df4e8d2f89379fb0b620cb0267f97c05bc1598 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 16 Dec 2021 13:34:39 -0800 Subject: Handle empty loops better --- .../src/check/generator_interior/drop_ranges/cfg_build.rs | 11 +++++++++-- src/test/ui/async-await/async-fn-nonsend.rs | 8 ++++++++ src/test/ui/async-await/async-fn-nonsend.stderr | 8 ++++---- 3 files changed, 21 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs index 4656f56569e..b434e05db80 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs @@ -157,8 +157,15 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { } ExprKind::Loop(body, ..) => { let loop_begin = self.expr_index + 1; - self.visit_block(body); - self.drop_ranges.add_control_edge(self.expr_index, loop_begin); + if body.stmts.is_empty() && body.expr.is_none() { + // For empty loops we won't have updated self.expr_index after visiting the + // body, meaning we'd get an edge from expr_index to expr_index + 1, but + // instead we want an edge from expr_index + 1 to expr_index + 1. + self.drop_ranges.add_control_edge(loop_begin, loop_begin); + } else { + self.visit_block(body); + self.drop_ranges.add_control_edge(self.expr_index, loop_begin); + } } ExprKind::Break(hir::Destination { target_id: Ok(target), .. }, ..) | ExprKind::Continue(hir::Destination { target_id: Ok(target), .. }, ..) => { diff --git a/src/test/ui/async-await/async-fn-nonsend.rs b/src/test/ui/async-await/async-fn-nonsend.rs index 210d9ff3f2d..55629132e40 100644 --- a/src/test/ui/async-await/async-fn-nonsend.rs +++ b/src/test/ui/async-await/async-fn-nonsend.rs @@ -43,6 +43,13 @@ async fn non_sync_with_method_call() { } } +async fn non_sync_with_infinite_loop() { + let f: &mut std::fmt::Formatter = loop {}; + if non_sync().fmt(f).unwrap() == () { + fut().await; + } +} + fn assert_send(_: impl Send) {} pub fn pass_assert() { @@ -51,4 +58,5 @@ pub fn pass_assert() { //~^ ERROR future cannot be sent between threads safely assert_send(non_sync_with_method_call()); //~^ ERROR future cannot be sent between threads safely + assert_send(non_sync_with_infinite_loop()); } diff --git a/src/test/ui/async-await/async-fn-nonsend.stderr b/src/test/ui/async-await/async-fn-nonsend.stderr index abba5585c62..9c87067a4d3 100644 --- a/src/test/ui/async-await/async-fn-nonsend.stderr +++ b/src/test/ui/async-await/async-fn-nonsend.stderr @@ -1,5 +1,5 @@ error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:50:17 + --> $DIR/async-fn-nonsend.rs:57:17 | LL | assert_send(non_send_temporary_in_match()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_send_temporary_in_match` is not `Send` @@ -16,13 +16,13 @@ LL | Some(_) => fut().await, LL | } | - `Some(non_send())` is later dropped here note: required by a bound in `assert_send` - --> $DIR/async-fn-nonsend.rs:46:24 + --> $DIR/async-fn-nonsend.rs:53:24 | LL | fn assert_send(_: impl Send) {} | ^^^^ required by this bound in `assert_send` error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:52:17 + --> $DIR/async-fn-nonsend.rs:59:17 | LL | assert_send(non_sync_with_method_call()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_sync_with_method_call` is not `Send` @@ -40,7 +40,7 @@ LL | } LL | } | - `f` is later dropped here note: required by a bound in `assert_send` - --> $DIR/async-fn-nonsend.rs:46:24 + --> $DIR/async-fn-nonsend.rs:53:24 | LL | fn assert_send(_: impl Send) {} | ^^^^ required by this bound in `assert_send` -- cgit 1.4.1-3-g733a5 From 787f4cbd15b91e88d757bf3f1ac1dadfa0e8ec5a Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 17 Dec 2021 14:36:51 -0800 Subject: Handle uninhabited return types This changes drop range analysis to handle uninhabited return types such as `!`. Since these calls to these functions do not return, we model them as ending in an infinite loop. --- .../src/check/generator_interior/drop_ranges.rs | 10 +++- .../generator_interior/drop_ranges/cfg_build.rs | 62 ++++++++++++++++++---- src/test/ui/async-await/async-fn-nonsend.rs | 2 - src/test/ui/async-await/async-fn-nonsend.stderr | 30 ++--------- 4 files changed, 64 insertions(+), 40 deletions(-) (limited to 'src') diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs index 9fbefcfb088..cf463d0aeae 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs @@ -36,8 +36,14 @@ pub fn compute_drop_ranges<'a, 'tcx>( let consumed_borrowed_places = find_consumed_and_borrowed(fcx, def_id, body); let num_exprs = fcx.tcx.region_scope_tree(def_id).body_expr_count(body.id()).unwrap_or(0); - let mut drop_ranges = - build_control_flow_graph(fcx.tcx.hir(), consumed_borrowed_places, body, num_exprs); + let mut drop_ranges = build_control_flow_graph( + fcx.tcx.hir(), + fcx.tcx, + &fcx.typeck_results.borrow(), + consumed_borrowed_places, + body, + num_exprs, + ); drop_ranges.propagate_to_fixpoint(); diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs index b434e05db80..1591b144dc6 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs @@ -8,7 +8,10 @@ use hir::{ }; use rustc_hir as hir; use rustc_index::vec::IndexVec; -use rustc_middle::hir::map::Map; +use rustc_middle::{ + hir::map::Map, + ty::{TyCtxt, TypeckResults}, +}; use std::mem::swap; /// Traverses the body to find the control flow graph and locations for the @@ -18,11 +21,14 @@ use std::mem::swap; /// can be done with propagate_to_fixpoint in cfg_propagate. pub(super) fn build_control_flow_graph<'tcx>( hir: Map<'tcx>, + tcx: TyCtxt<'tcx>, + typeck_results: &TypeckResults<'tcx>, consumed_borrowed_places: ConsumedAndBorrowedPlaces, body: &'tcx Body<'tcx>, num_exprs: usize, ) -> DropRangesBuilder { - let mut drop_range_visitor = DropRangeVisitor::new(hir, consumed_borrowed_places, num_exprs); + let mut drop_range_visitor = + DropRangeVisitor::new(hir, tcx, typeck_results, consumed_borrowed_places, num_exprs); intravisit::walk_body(&mut drop_range_visitor, body); drop_range_visitor.drop_ranges.process_deferred_edges(); @@ -36,22 +42,30 @@ pub(super) fn build_control_flow_graph<'tcx>( /// We are interested in points where a variables is dropped or initialized, and the control flow /// of the code. We identify locations in code by their post-order traversal index, so it is /// important for this traversal to match that in `RegionResolutionVisitor` and `InteriorVisitor`. -struct DropRangeVisitor<'tcx> { +struct DropRangeVisitor<'a, 'tcx> { hir: Map<'tcx>, places: ConsumedAndBorrowedPlaces, drop_ranges: DropRangesBuilder, expr_index: PostOrderId, + tcx: TyCtxt<'tcx>, + typeck_results: &'a TypeckResults<'tcx>, } -impl<'tcx> DropRangeVisitor<'tcx> { - fn new(hir: Map<'tcx>, places: ConsumedAndBorrowedPlaces, num_exprs: usize) -> Self { +impl<'a, 'tcx> DropRangeVisitor<'a, 'tcx> { + fn new( + hir: Map<'tcx>, + tcx: TyCtxt<'tcx>, + typeck_results: &'a TypeckResults<'tcx>, + places: ConsumedAndBorrowedPlaces, + num_exprs: usize, + ) -> Self { debug!("consumed_places: {:?}", places.consumed); let drop_ranges = DropRangesBuilder::new( places.consumed.iter().flat_map(|(_, places)| places.iter().copied()), hir, num_exprs, ); - Self { hir, places, drop_ranges, expr_index: PostOrderId::from_u32(0) } + Self { hir, places, drop_ranges, expr_index: PostOrderId::from_u32(0), typeck_results, tcx } } fn record_drop(&mut self, hir_id: HirId) { @@ -91,9 +105,23 @@ impl<'tcx> DropRangeVisitor<'tcx> { debug!("reinitializing {:?} is not supported", expr); } } + + /// For an expression with an uninhabited return type (e.g. a function that returns !), + /// this adds a self edge to to the CFG to model the fact that the function does not + /// return. + fn handle_uninhabited_return(&mut self, expr: &Expr<'tcx>) { + let ty = self.typeck_results.expr_ty(expr); + let ty = self.tcx.erase_regions(ty); + let m = self.tcx.parent_module(expr.hir_id).to_def_id(); + let param_env = self.tcx.param_env(m.expect_local()); + if self.tcx.is_ty_uninhabited_from(m, ty, param_env) { + // This function will not return. We model this fact as an infinite loop. + self.drop_ranges.add_control_edge(self.expr_index + 1, self.expr_index + 1); + } + } } -impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { +impl<'a, 'tcx> Visitor<'tcx> for DropRangeVisitor<'a, 'tcx> { type Map = intravisit::ErasedMap<'tcx>; fn nested_visit_map(&mut self) -> NestedVisitorMap { @@ -109,6 +137,7 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { reinit = Some(lhs); } + ExprKind::If(test, if_true, if_false) => { self.visit_expr(test); @@ -155,6 +184,7 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { self.drop_ranges.add_control_edge(arm_end, self.expr_index + 1) }); } + ExprKind::Loop(body, ..) => { let loop_begin = self.expr_index + 1; if body.stmts.is_empty() && body.expr.is_none() { @@ -172,6 +202,22 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { self.drop_ranges.add_control_edge_hir_id(self.expr_index, target); } + ExprKind::Call(f, args) => { + self.visit_expr(f); + for arg in args { + self.visit_expr(arg); + } + + self.handle_uninhabited_return(expr); + } + ExprKind::MethodCall(_, _, exprs, _) => { + for expr in exprs { + self.visit_expr(expr); + } + + self.handle_uninhabited_return(expr); + } + ExprKind::AddrOf(..) | ExprKind::Array(..) | ExprKind::AssignOp(..) @@ -179,7 +225,6 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { | ExprKind::Block(..) | ExprKind::Box(..) | ExprKind::Break(..) - | ExprKind::Call(..) | ExprKind::Cast(..) | ExprKind::Closure(..) | ExprKind::ConstBlock(..) @@ -192,7 +237,6 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { | ExprKind::Let(..) | ExprKind::Lit(..) | ExprKind::LlvmInlineAsm(..) - | ExprKind::MethodCall(..) | ExprKind::Path(..) | ExprKind::Repeat(..) | ExprKind::Ret(..) diff --git a/src/test/ui/async-await/async-fn-nonsend.rs b/src/test/ui/async-await/async-fn-nonsend.rs index 55629132e40..123cadc2cbb 100644 --- a/src/test/ui/async-await/async-fn-nonsend.rs +++ b/src/test/ui/async-await/async-fn-nonsend.rs @@ -36,7 +36,6 @@ async fn non_send_temporary_in_match() { } async fn non_sync_with_method_call() { - // FIXME: it would be nice for this to work let f: &mut std::fmt::Formatter = panic!(); if non_sync().fmt(f).unwrap() == () { fut().await; @@ -57,6 +56,5 @@ pub fn pass_assert() { assert_send(non_send_temporary_in_match()); //~^ ERROR future cannot be sent between threads safely assert_send(non_sync_with_method_call()); - //~^ ERROR future cannot be sent between threads safely assert_send(non_sync_with_infinite_loop()); } diff --git a/src/test/ui/async-await/async-fn-nonsend.stderr b/src/test/ui/async-await/async-fn-nonsend.stderr index 9c87067a4d3..be42d46a906 100644 --- a/src/test/ui/async-await/async-fn-nonsend.stderr +++ b/src/test/ui/async-await/async-fn-nonsend.stderr @@ -1,5 +1,5 @@ error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:57:17 + --> $DIR/async-fn-nonsend.rs:56:17 | LL | assert_send(non_send_temporary_in_match()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_send_temporary_in_match` is not `Send` @@ -16,34 +16,10 @@ LL | Some(_) => fut().await, LL | } | - `Some(non_send())` is later dropped here note: required by a bound in `assert_send` - --> $DIR/async-fn-nonsend.rs:53:24 + --> $DIR/async-fn-nonsend.rs:52:24 | LL | fn assert_send(_: impl Send) {} | ^^^^ required by this bound in `assert_send` -error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:59:17 - | -LL | assert_send(non_sync_with_method_call()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_sync_with_method_call` is not `Send` - | - = help: the trait `Send` is not implemented for `dyn std::fmt::Write` -note: future is not `Send` as this value is used across an await - --> $DIR/async-fn-nonsend.rs:42:14 - | -LL | let f: &mut std::fmt::Formatter = panic!(); - | - has type `&mut Formatter<'_>` which is not `Send` -LL | if non_sync().fmt(f).unwrap() == () { -LL | fut().await; - | ^^^^^^ await occurs here, with `f` maybe used later -LL | } -LL | } - | - `f` is later dropped here -note: required by a bound in `assert_send` - --> $DIR/async-fn-nonsend.rs:53:24 - | -LL | fn assert_send(_: impl Send) {} - | ^^^^ required by this bound in `assert_send` - -error: aborting due to 2 previous errors +error: aborting due to previous error -- cgit 1.4.1-3-g733a5 From 887e843eeb35e9cc78884e9d5feacf914377f355 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 17 Dec 2021 15:05:38 -0800 Subject: Update async-fn-nonsend.rs The previous commit made the non_sync_with_method_call case pass due to the await being unreachable. Unfortunately, this isn't actually the behavior the test was verifying. This change lifts the panic into a helper function so that the generator analysis still thinks the await is reachable, and therefore we preserve the same testing behavior. --- src/test/ui/async-await/async-fn-nonsend.rs | 18 +++++++++++++-- src/test/ui/async-await/async-fn-nonsend.stderr | 30 ++++++++++++++++++++++--- 2 files changed, 43 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/test/ui/async-await/async-fn-nonsend.rs b/src/test/ui/async-await/async-fn-nonsend.rs index 123cadc2cbb..c5453b67ef5 100644 --- a/src/test/ui/async-await/async-fn-nonsend.rs +++ b/src/test/ui/async-await/async-fn-nonsend.rs @@ -35,14 +35,26 @@ async fn non_send_temporary_in_match() { } } +fn get_formatter() -> std::fmt::Formatter<'static> { + panic!() +} + async fn non_sync_with_method_call() { + let f: &mut std::fmt::Formatter = &mut get_formatter(); + // It would by nice for this to work. + if non_sync().fmt(f).unwrap() == () { + fut().await; + } +} + +async fn non_sync_with_method_call_panic() { let f: &mut std::fmt::Formatter = panic!(); if non_sync().fmt(f).unwrap() == () { fut().await; } } -async fn non_sync_with_infinite_loop() { +async fn non_sync_with_method_call_infinite_loop() { let f: &mut std::fmt::Formatter = loop {}; if non_sync().fmt(f).unwrap() == () { fut().await; @@ -56,5 +68,7 @@ pub fn pass_assert() { assert_send(non_send_temporary_in_match()); //~^ ERROR future cannot be sent between threads safely assert_send(non_sync_with_method_call()); - assert_send(non_sync_with_infinite_loop()); + //~^ ERROR future cannot be sent between threads safely + assert_send(non_sync_with_method_call_panic()); + assert_send(non_sync_with_method_call_infinite_loop()); } diff --git a/src/test/ui/async-await/async-fn-nonsend.stderr b/src/test/ui/async-await/async-fn-nonsend.stderr index be42d46a906..40ad46b4862 100644 --- a/src/test/ui/async-await/async-fn-nonsend.stderr +++ b/src/test/ui/async-await/async-fn-nonsend.stderr @@ -1,5 +1,5 @@ error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:56:17 + --> $DIR/async-fn-nonsend.rs:68:17 | LL | assert_send(non_send_temporary_in_match()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_send_temporary_in_match` is not `Send` @@ -16,10 +16,34 @@ LL | Some(_) => fut().await, LL | } | - `Some(non_send())` is later dropped here note: required by a bound in `assert_send` - --> $DIR/async-fn-nonsend.rs:52:24 + --> $DIR/async-fn-nonsend.rs:64:24 | LL | fn assert_send(_: impl Send) {} | ^^^^ required by this bound in `assert_send` -error: aborting due to previous error +error: future cannot be sent between threads safely + --> $DIR/async-fn-nonsend.rs:70:17 + | +LL | assert_send(non_sync_with_method_call()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_sync_with_method_call` is not `Send` + | + = help: the trait `Send` is not implemented for `dyn std::fmt::Write` +note: future is not `Send` as this value is used across an await + --> $DIR/async-fn-nonsend.rs:46:14 + | +LL | let f: &mut std::fmt::Formatter = &mut get_formatter(); + | --------------- has type `Formatter<'_>` which is not `Send` +... +LL | fut().await; + | ^^^^^^ await occurs here, with `get_formatter()` maybe used later +LL | } +LL | } + | - `get_formatter()` is later dropped here +note: required by a bound in `assert_send` + --> $DIR/async-fn-nonsend.rs:64:24 + | +LL | fn assert_send(_: impl Send) {} + | ^^^^ required by this bound in `assert_send` + +error: aborting due to 2 previous errors -- cgit 1.4.1-3-g733a5 From 32930d9ea7cc79239daa19a040cbae9867053af8 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 5 Jan 2022 14:11:37 -0800 Subject: Safely handle partial drops We previously weren't tracking partial re-inits while being too aggressive around partial drops. With this change, we simply ignore partial drops, which is the safer, more conservative choice. --- .../drop_ranges/record_consumed_borrow.rs | 6 ++++- .../ui/async-await/partial-drop-partial-reinit.rs | 29 ++++++++++++++++++++++ .../async-await/partial-drop-partial-reinit.stderr | 27 ++++++++++++++++++++ src/test/ui/generator/partial-drop.rs | 4 +-- src/test/ui/generator/partial-drop.stderr | 25 +++++++++++++++++++ 5 files changed, 88 insertions(+), 3 deletions(-) create mode 100644 src/test/ui/async-await/partial-drop-partial-reinit.rs create mode 100644 src/test/ui/async-await/partial-drop-partial-reinit.stderr create mode 100644 src/test/ui/generator/partial-drop.stderr (limited to 'src') diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs index 2548b608092..845cd01a44e 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs @@ -85,7 +85,11 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { "consume {:?}; diag_expr_id={:?}, using parent {:?}", place_with_id, diag_expr_id, parent ); - self.mark_consumed(parent, place_with_id.into()); + // We do not currently support partial drops or reinits, so just ignore + // any places with projections. + if place_with_id.place.projections.is_empty() { + self.mark_consumed(parent, place_with_id.into()); + } } fn borrow( diff --git a/src/test/ui/async-await/partial-drop-partial-reinit.rs b/src/test/ui/async-await/partial-drop-partial-reinit.rs new file mode 100644 index 00000000000..73f0ca8153c --- /dev/null +++ b/src/test/ui/async-await/partial-drop-partial-reinit.rs @@ -0,0 +1,29 @@ +// edition:2021 +#![feature(negative_impls)] +#![allow(unused)] + +fn main() { + gimme_send(foo()); + //~^ ERROR cannot be sent between threads safely +} + +fn gimme_send(t: T) { + drop(t); +} + +struct NotSend {} + +impl Drop for NotSend { + fn drop(&mut self) {} +} + +impl !Send for NotSend {} + +async fn foo() { + let mut x = (NotSend {},); + drop(x.0); + x.0 = NotSend {}; + bar().await; +} + +async fn bar() {} diff --git a/src/test/ui/async-await/partial-drop-partial-reinit.stderr b/src/test/ui/async-await/partial-drop-partial-reinit.stderr new file mode 100644 index 00000000000..2097642eb24 --- /dev/null +++ b/src/test/ui/async-await/partial-drop-partial-reinit.stderr @@ -0,0 +1,27 @@ +error[E0277]: `NotSend` cannot be sent between threads safely + --> $DIR/partial-drop-partial-reinit.rs:6:16 + | +LL | gimme_send(foo()); + | ---------- ^^^^^ `NotSend` cannot be sent between threads safely + | | + | required by a bound introduced by this call +... +LL | async fn foo() { + | - within this `impl Future` + | + = help: within `impl Future`, the trait `Send` is not implemented for `NotSend` + = note: required because it appears within the type `(NotSend,)` + = note: required because it appears within the type `{ResumeTy, (NotSend,), impl Future, ()}` + = note: required because it appears within the type `[static generator@$DIR/partial-drop-partial-reinit.rs:22:16: 27:2]` + = note: required because it appears within the type `from_generator::GenFuture<[static generator@$DIR/partial-drop-partial-reinit.rs:22:16: 27:2]>` + = note: required because it appears within the type `impl Future` + = note: required because it appears within the type `impl Future` +note: required by a bound in `gimme_send` + --> $DIR/partial-drop-partial-reinit.rs:10:18 + | +LL | fn gimme_send(t: T) { + | ^^^^ required by this bound in `gimme_send` + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0277`. diff --git a/src/test/ui/generator/partial-drop.rs b/src/test/ui/generator/partial-drop.rs index a2f616aa313..c8c07ba41c7 100644 --- a/src/test/ui/generator/partial-drop.rs +++ b/src/test/ui/generator/partial-drop.rs @@ -1,5 +1,3 @@ -// check-pass - #![feature(negative_impls, generators)] struct Foo; @@ -12,6 +10,8 @@ struct Bar { fn main() { assert_send(|| { + //~^ ERROR generator cannot be sent between threads safely + // FIXME: it would be nice to make this work. let guard = Bar { foo: Foo, x: 42 }; drop(guard.foo); yield; diff --git a/src/test/ui/generator/partial-drop.stderr b/src/test/ui/generator/partial-drop.stderr new file mode 100644 index 00000000000..93112f52208 --- /dev/null +++ b/src/test/ui/generator/partial-drop.stderr @@ -0,0 +1,25 @@ +error: generator cannot be sent between threads safely + --> $DIR/partial-drop.rs:12:5 + | +LL | assert_send(|| { + | ^^^^^^^^^^^ generator is not `Send` + | + = help: within `[generator@$DIR/partial-drop.rs:12:17: 18:6]`, the trait `Send` is not implemented for `Foo` +note: generator is not `Send` as this value is used across a yield + --> $DIR/partial-drop.rs:17:9 + | +LL | let guard = Bar { foo: Foo, x: 42 }; + | ----- has type `Bar` which is not `Send` +LL | drop(guard.foo); +LL | yield; + | ^^^^^ yield occurs here, with `guard` maybe used later +LL | }) + | - `guard` is later dropped here +note: required by a bound in `assert_send` + --> $DIR/partial-drop.rs:21:19 + | +LL | fn assert_send(_: T) {} + | ^^^^ required by this bound in `assert_send` + +error: aborting due to previous error + -- cgit 1.4.1-3-g733a5 From e0a5370ef00938db0e76f6d7845befb51be629ff Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 14 Jan 2022 17:45:00 -0800 Subject: Respond to code review comments --- .../src/check/generator_interior/drop_ranges.rs | 48 ++++-- .../generator_interior/drop_ranges/cfg_build.rs | 167 +++++++++++++++++++-- .../drop_ranges/record_consumed_borrow.rs | 12 +- src/test/ui/generator/partial-drop.rs | 21 ++- src/test/ui/generator/partial-drop.stderr | 52 ++++++- 5 files changed, 265 insertions(+), 35 deletions(-) (limited to 'src') diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs index 681cd7cf935..21a8d7b5634 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs @@ -21,6 +21,7 @@ use rustc_data_structures::fx::FxHashMap; use rustc_hir as hir; use rustc_index::bit_set::BitSet; use rustc_index::vec::IndexVec; +use rustc_middle::hir::map::Map; use rustc_middle::hir::place::{PlaceBase, PlaceWithHirId}; use rustc_middle::ty; use std::collections::BTreeMap; @@ -53,15 +54,18 @@ pub fn compute_drop_ranges<'a, 'tcx>( DropRanges { tracked_value_map: drop_ranges.tracked_value_map, nodes: drop_ranges.nodes } } -/// Applies `f` to consumable portion of a HIR node. +/// Applies `f` to consumable node in the HIR subtree pointed to by `place`. /// -/// The `node` parameter should be the result of calling `Map::find(place)`. -fn for_each_consumable( - place: TrackedValue, - node: Option>, - mut f: impl FnMut(TrackedValue), -) { +/// This includes the place itself, and if the place is a reference to a local +/// variable then `f` is also called on the HIR node for that variable as well. +/// +/// For example, if `place` points to `foo()`, then `f` is called once for the +/// result of `foo`. On the other hand, if `place` points to `x` then `f` will +/// be called both on the `ExprKind::Path` node that represents the expression +/// as well as the HirId of the local `x` itself. +fn for_each_consumable<'tcx>(hir: Map<'tcx>, place: TrackedValue, mut f: impl FnMut(TrackedValue)) { f(place); + let node = hir.find(place.hir_id()); if let Some(Node::Expr(expr)) = node { match expr.kind { hir::ExprKind::Path(hir::QPath::Resolved( @@ -108,15 +112,37 @@ impl TrackedValue { } } -impl From<&PlaceWithHirId<'_>> for TrackedValue { - fn from(place_with_id: &PlaceWithHirId<'_>) -> Self { +/// Represents a reason why we might not be able to convert a HirId or Place +/// into a tracked value. +#[derive(Debug)] +enum TrackedValueConversionError { + /// Place projects are not currently supported. + /// + /// The reasoning around these is kind of subtle, so we choose to be more + /// conservative around these for now. There is not reason in theory we + /// cannot support these, we just have not implemented it yet. + PlaceProjectionsNotSupported, +} + +impl TryFrom<&PlaceWithHirId<'_>> for TrackedValue { + type Error = TrackedValueConversionError; + + fn try_from(place_with_id: &PlaceWithHirId<'_>) -> Result { + if !place_with_id.place.projections.is_empty() { + debug!( + "TrackedValue from PlaceWithHirId: {:?} has projections, which are not supported.", + place_with_id + ); + return Err(TrackedValueConversionError::PlaceProjectionsNotSupported); + } + match place_with_id.place.base { PlaceBase::Rvalue | PlaceBase::StaticItem => { - TrackedValue::Temporary(place_with_id.hir_id) + Ok(TrackedValue::Temporary(place_with_id.hir_id)) } PlaceBase::Local(hir_id) | PlaceBase::Upvar(ty::UpvarId { var_path: ty::UpvarPath { hir_id }, .. }) => { - TrackedValue::Variable(hir_id) + Ok(TrackedValue::Variable(hir_id)) } } } diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs index dfe8ed54b21..d7305957f94 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs @@ -43,6 +43,41 @@ pub(super) fn build_control_flow_graph<'tcx>( /// We are interested in points where a variables is dropped or initialized, and the control flow /// of the code. We identify locations in code by their post-order traversal index, so it is /// important for this traversal to match that in `RegionResolutionVisitor` and `InteriorVisitor`. +/// +/// We make several simplifying assumptions, with the goal of being more conservative than +/// necessary rather than less conservative (since being less conservative is unsound, but more +/// conservative is still safe). These assumptions are: +/// +/// 1. Moving a variable `a` counts as a move of the whole variable. +/// 2. Moving a partial path like `a.b.c` is ignored. +/// 3. Reinitializing through a field (e.g. `a.b.c = 5`) counds as a reinitialization of all of +/// `a`. +/// +/// Some examples: +/// +/// Rule 1: +/// ```rust +/// let mut a = (vec![0], vec![0]); +/// drop(a); +/// // `a` is not considered initialized. +/// ``` +/// +/// Rule 2: +/// ```rust +/// let mut a = (vec![0], vec![0]); +/// drop(a.0); +/// drop(a.1); +/// // `a` is still considered initialized. +/// ``` +/// +/// Rule 3: +/// ```rust +/// let mut a = (vec![0], vec![0]); +/// drop(a); +/// a.1 = vec![1]; +/// // all of `a` is considered initialized +/// ``` + struct DropRangeVisitor<'a, 'tcx> { hir: Map<'tcx>, places: ConsumedAndBorrowedPlaces, @@ -89,23 +124,76 @@ impl<'a, 'tcx> DropRangeVisitor<'a, 'tcx> { .get(&expr.hir_id) .map_or(vec![], |places| places.iter().cloned().collect()); for place in places { - for_each_consumable(place, self.hir.find(place.hir_id()), |value| { - self.record_drop(value) - }); + for_each_consumable(self.hir, place, |value| self.record_drop(value)); } } + /// Marks an expression as being reinitialized. + /// + /// Note that we always approximated on the side of things being more + /// initialized than they actually are, as opposed to less. In cases such + /// as `x.y = ...`, we would consider all of `x` as being initialized + /// instead of just the `y` field. + /// + /// This is because it is always safe to consider something initialized + /// even when it is not, but the other way around will cause problems. + /// + /// In the future, we will hopefully tighten up these rules to be more + /// precise. fn reinit_expr(&mut self, expr: &hir::Expr<'_>) { - if let ExprKind::Path(hir::QPath::Resolved( - _, - hir::Path { res: hir::def::Res::Local(hir_id), .. }, - )) = expr.kind - { - let location = self.expr_index; - debug!("reinitializing {:?} at {:?}", hir_id, location); - self.drop_ranges.reinit_at(TrackedValue::Variable(*hir_id), location); - } else { - debug!("reinitializing {:?} is not supported", expr); + // Walk the expression to find the base. For example, in an expression + // like `*a[i].x`, we want to find the `a` and mark that as + // reinitialized. + match expr.kind { + ExprKind::Path(hir::QPath::Resolved( + _, + hir::Path { res: hir::def::Res::Local(hir_id), .. }, + )) => { + // This is the base case, where we have found an actual named variable. + + let location = self.expr_index; + debug!("reinitializing {:?} at {:?}", hir_id, location); + self.drop_ranges.reinit_at(TrackedValue::Variable(*hir_id), location); + } + + ExprKind::Field(base, _) => self.reinit_expr(base), + + // Most expressions do not refer to something where we need to track + // reinitializations. + // + // Some of these may be interesting in the future + ExprKind::Path(..) + | ExprKind::Box(_) + | ExprKind::ConstBlock(_) + | ExprKind::Array(_) + | ExprKind::Call(_, _) + | ExprKind::MethodCall(_, _, _, _) + | ExprKind::Tup(_) + | ExprKind::Binary(_, _, _) + | ExprKind::Unary(_, _) + | ExprKind::Lit(_) + | ExprKind::Cast(_, _) + | ExprKind::Type(_, _) + | ExprKind::DropTemps(_) + | ExprKind::Let(_, _, _) + | ExprKind::If(_, _, _) + | ExprKind::Loop(_, _, _, _) + | ExprKind::Match(_, _, _) + | ExprKind::Closure(_, _, _, _, _) + | ExprKind::Block(_, _) + | ExprKind::Assign(_, _, _) + | ExprKind::AssignOp(_, _, _) + | ExprKind::Index(_, _) + | ExprKind::AddrOf(_, _, _) + | ExprKind::Break(_, _) + | ExprKind::Continue(_) + | ExprKind::Ret(_) + | ExprKind::InlineAsm(_) + | ExprKind::LlvmInlineAsm(_) + | ExprKind::Struct(_, _, _) + | ExprKind::Repeat(_, _) + | ExprKind::Yield(_, _) + | ExprKind::Err => (), } } @@ -158,13 +246,47 @@ impl<'a, 'tcx> Visitor<'tcx> for DropRangeVisitor<'a, 'tcx> { self.drop_ranges.add_control_edge(true_end, self.expr_index + 1); } ExprKind::Match(scrutinee, arms, ..) => { + // We walk through the match expression almost like a chain of if expressions. + // Here's a diagram to follow along with: + // + // ┌─┐ + // match │A│ { + // ┌───┴─┘ + // │ + // ┌▼┌───►┌─┐ ┌─┐ + // │B│ if │C│ =>│D│, + // └─┘ ├─┴──►└─┴──────┐ + // ┌──┘ │ + // ┌──┘ │ + // │ │ + // ┌▼┌───►┌─┐ ┌─┐ │ + // │E│ if │F│ =>│G│, │ + // └─┘ ├─┴──►└─┴┐ │ + // │ │ │ + // } ▼ ▼ │ + // ┌─┐◄───────────────────┘ + // │H│ + // └─┘ + // + // The order we want is that the scrutinee (A) flows into the first pattern (B), + // which flows into the guard (C). Then the guard either flows into the arm body + // (D) or into the start of the next arm (E). Finally, the body flows to the end + // of the match block (H). + // + // The subsequent arms follow the same ordering. First we go to the pattern, then + // the guard (if present, otherwise it flows straight into the body), then into + // the body and then to the end of the match expression. + // + // The comments below show which edge is being added. self.visit_expr(scrutinee); let (guard_exit, arm_end_ids) = arms.iter().fold( (self.expr_index, vec![]), |(incoming_edge, mut arm_end_ids), hir::Arm { pat, body, guard, .. }| { + // A -> B, or C -> E self.drop_ranges.add_control_edge(incoming_edge, self.expr_index + 1); self.visit_pat(pat); + // B -> C and E -> F are added implicitly due to the traversal order. match guard { Some(Guard::If(expr)) => self.visit_expr(expr), Some(Guard::IfLet(pat, expr)) => { @@ -173,17 +295,34 @@ impl<'a, 'tcx> Visitor<'tcx> for DropRangeVisitor<'a, 'tcx> { } None => (), } + // Likewise, C -> D and F -> G are added implicitly. + + // Save C, F, so we can add the other outgoing edge. let to_next_arm = self.expr_index; + // The default edge does not get added since we also have an explicit edge, // so we also need to add an edge to the next node as well. + // + // This adds C -> D, F -> G self.drop_ranges.add_control_edge(self.expr_index, self.expr_index + 1); self.visit_expr(body); + + // Save the end of the body so we can add the exit edge once we know where + // the exit is. arm_end_ids.push(self.expr_index); + + // Pass C to the next iteration, as well as vec![D] + // + // On the last round through, we pass F and vec![D, G] so that we can + // add all the exit edges. (to_next_arm, arm_end_ids) }, ); + // F -> H self.drop_ranges.add_control_edge(guard_exit, self.expr_index + 1); + arm_end_ids.into_iter().for_each(|arm_end| { + // D -> H, G -> H self.drop_ranges.add_control_edge(arm_end, self.expr_index + 1) }); } @@ -275,7 +414,7 @@ impl DropRangesBuilder { let mut tracked_value_map = FxHashMap::<_, TrackedValueIndex>::default(); let mut next = <_>::from(0u32); for value in tracked_values { - for_each_consumable(value, hir.find(value.hir_id()), |value| { + for_each_consumable(hir, value, |value| { if !tracked_value_map.contains_key(&value) { tracked_value_map.insert(value, next); next = next + 1; diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs index 845cd01a44e..059a135a6fb 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs @@ -85,11 +85,9 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { "consume {:?}; diag_expr_id={:?}, using parent {:?}", place_with_id, diag_expr_id, parent ); - // We do not currently support partial drops or reinits, so just ignore - // any places with projections. - if place_with_id.place.projections.is_empty() { - self.mark_consumed(parent, place_with_id.into()); - } + place_with_id + .try_into() + .map_or((), |tracked_value| self.mark_consumed(parent, tracked_value)); } fn borrow( @@ -98,7 +96,9 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { _diag_expr_id: HirId, _bk: rustc_middle::ty::BorrowKind, ) { - self.places.borrowed.insert(place_with_id.into()); + place_with_id + .try_into() + .map_or(false, |tracked_value| self.places.borrowed.insert(tracked_value)); } fn mutate( diff --git a/src/test/ui/generator/partial-drop.rs b/src/test/ui/generator/partial-drop.rs index c8c07ba41c7..36f6e78cb3b 100644 --- a/src/test/ui/generator/partial-drop.rs +++ b/src/test/ui/generator/partial-drop.rs @@ -15,7 +15,26 @@ fn main() { let guard = Bar { foo: Foo, x: 42 }; drop(guard.foo); yield; - }) + }); + + assert_send(|| { + //~^ ERROR generator cannot be sent between threads safely + // FIXME: it would be nice to make this work. + let guard = Bar { foo: Foo, x: 42 }; + drop(guard); + guard.foo = Foo; + guard.x = 23; + yield; + }); + + assert_send(|| { + //~^ ERROR generator cannot be sent between threads safely + // FIXME: it would be nice to make this work. + let guard = Bar { foo: Foo, x: 42 }; + let Bar { foo, x } = guard; + drop(foo); + yield; + }); } fn assert_send(_: T) {} diff --git a/src/test/ui/generator/partial-drop.stderr b/src/test/ui/generator/partial-drop.stderr index 93112f52208..9a1b0734d8c 100644 --- a/src/test/ui/generator/partial-drop.stderr +++ b/src/test/ui/generator/partial-drop.stderr @@ -13,13 +13,59 @@ LL | let guard = Bar { foo: Foo, x: 42 }; LL | drop(guard.foo); LL | yield; | ^^^^^ yield occurs here, with `guard` maybe used later -LL | }) +LL | }); | - `guard` is later dropped here note: required by a bound in `assert_send` - --> $DIR/partial-drop.rs:21:19 + --> $DIR/partial-drop.rs:40:19 | LL | fn assert_send(_: T) {} | ^^^^ required by this bound in `assert_send` -error: aborting due to previous error +error: generator cannot be sent between threads safely + --> $DIR/partial-drop.rs:20:5 + | +LL | assert_send(|| { + | ^^^^^^^^^^^ generator is not `Send` + | + = help: within `[generator@$DIR/partial-drop.rs:20:17: 28:6]`, the trait `Send` is not implemented for `Foo` +note: generator is not `Send` as this value is used across a yield + --> $DIR/partial-drop.rs:27:9 + | +LL | let guard = Bar { foo: Foo, x: 42 }; + | ----- has type `Bar` which is not `Send` +... +LL | yield; + | ^^^^^ yield occurs here, with `guard` maybe used later +LL | }); + | - `guard` is later dropped here +note: required by a bound in `assert_send` + --> $DIR/partial-drop.rs:40:19 + | +LL | fn assert_send(_: T) {} + | ^^^^ required by this bound in `assert_send` + +error: generator cannot be sent between threads safely + --> $DIR/partial-drop.rs:30:5 + | +LL | assert_send(|| { + | ^^^^^^^^^^^ generator is not `Send` + | + = help: within `[generator@$DIR/partial-drop.rs:30:17: 37:6]`, the trait `Send` is not implemented for `Foo` +note: generator is not `Send` as this value is used across a yield + --> $DIR/partial-drop.rs:36:9 + | +LL | let guard = Bar { foo: Foo, x: 42 }; + | ----- has type `Bar` which is not `Send` +... +LL | yield; + | ^^^^^ yield occurs here, with `guard` maybe used later +LL | }); + | - `guard` is later dropped here +note: required by a bound in `assert_send` + --> $DIR/partial-drop.rs:40:19 + | +LL | fn assert_send(_: T) {} + | ^^^^ required by this bound in `assert_send` + +error: aborting due to 3 previous errors -- cgit 1.4.1-3-g733a5 From 017747fa5afb1e5396582498303c8437792966d7 Mon Sep 17 00:00:00 2001 From: Esteban Kuber Date: Wed, 19 Jan 2022 02:27:15 +0000 Subject: Only suggest adding `!` to expressions that can be macro invocation --- compiler/rustc_resolve/src/late.rs | 4 ++ compiler/rustc_resolve/src/late/diagnostics.rs | 11 +++++- .../ui/hygiene/rustc-macro-transparency.stderr | 28 ++++---------- src/test/ui/resolve/resolve-hint-macro.fixed | 11 ++++++ src/test/ui/resolve/resolve-hint-macro.rs | 7 ++++ src/test/ui/resolve/resolve-hint-macro.stderr | 45 ++++++++++++++++++++-- 6 files changed, 82 insertions(+), 24 deletions(-) create mode 100644 src/test/ui/resolve/resolve-hint-macro.fixed (limited to 'src') diff --git a/compiler/rustc_resolve/src/late.rs b/compiler/rustc_resolve/src/late.rs index ccaaa2eaf46..2c678e71ae1 100644 --- a/compiler/rustc_resolve/src/late.rs +++ b/compiler/rustc_resolve/src/late.rs @@ -2517,6 +2517,10 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> { self.visit_expr(elem); self.resolve_anon_const(ct, IsRepeatExpr::Yes); } + ExprKind::Index(ref elem, ref idx) => { + self.resolve_expr(elem, Some(expr)); + self.visit_expr(idx); + } _ => { visit::walk_expr(self, expr); } diff --git a/compiler/rustc_resolve/src/late/diagnostics.rs b/compiler/rustc_resolve/src/late/diagnostics.rs index 4cd1b34bedc..7b4fe6f0e07 100644 --- a/compiler/rustc_resolve/src/late/diagnostics.rs +++ b/compiler/rustc_resolve/src/late/diagnostics.rs @@ -970,7 +970,13 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> { }; match (res, source) { - (Res::Def(DefKind::Macro(MacroKind::Bang), _), _) => { + ( + Res::Def(DefKind::Macro(MacroKind::Bang), _), + PathSource::Expr(Some(Expr { + kind: ExprKind::Index(..) | ExprKind::Call(..), .. + })) + | PathSource::Struct, + ) => { err.span_label(span, fallback_label); err.span_suggestion_verbose( span.shrink_to_hi(), @@ -982,6 +988,9 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> { err.note("if you want the `try` keyword, you need Rust 2018 or later"); } } + (Res::Def(DefKind::Macro(MacroKind::Bang), _), _) => { + err.span_label(span, fallback_label); + } (Res::Def(DefKind::TyAlias, def_id), PathSource::Trait(_)) => { err.span_label(span, "type aliases cannot be used as traits"); if self.r.session.is_nightly_build() { diff --git a/src/test/ui/hygiene/rustc-macro-transparency.stderr b/src/test/ui/hygiene/rustc-macro-transparency.stderr index e4c1c8ad293..17d05dd0963 100644 --- a/src/test/ui/hygiene/rustc-macro-transparency.stderr +++ b/src/test/ui/hygiene/rustc-macro-transparency.stderr @@ -11,16 +11,10 @@ LL | struct SemiTransparent; | ----------------------- similarly named unit struct `SemiTransparent` defined here ... LL | semitransparent; - | ^^^^^^^^^^^^^^^ not a value - | -help: use `!` to invoke the macro - | -LL | semitransparent!; - | + -help: a unit struct with a similar name exists - | -LL | SemiTransparent; - | ~~~~~~~~~~~~~~~ + | ^^^^^^^^^^^^^^^ + | | + | not a value + | help: a unit struct with a similar name exists: `SemiTransparent` error[E0423]: expected value, found macro `opaque` --> $DIR/rustc-macro-transparency.rs:30:5 @@ -29,16 +23,10 @@ LL | struct Opaque; | -------------- similarly named unit struct `Opaque` defined here ... LL | opaque; - | ^^^^^^ not a value - | -help: use `!` to invoke the macro - | -LL | opaque!; - | + -help: a unit struct with a similar name exists - | -LL | Opaque; - | ~~~~~~ + | ^^^^^^ + | | + | not a value + | help: a unit struct with a similar name exists (notice the capitalization): `Opaque` error: aborting due to 3 previous errors diff --git a/src/test/ui/resolve/resolve-hint-macro.fixed b/src/test/ui/resolve/resolve-hint-macro.fixed new file mode 100644 index 00000000000..54e01608498 --- /dev/null +++ b/src/test/ui/resolve/resolve-hint-macro.fixed @@ -0,0 +1,11 @@ +// run-rustfix +fn main() { + assert_eq!(1, 1); + //~^ ERROR expected function, found macro `assert_eq` + assert_eq! { 1, 1 }; + //~^ ERROR expected struct, variant or union type, found macro `assert_eq` + //~| ERROR expected identifier, found `1` + //~| ERROR expected identifier, found `1` + assert![true]; + //~^ ERROR expected value, found macro `assert` +} diff --git a/src/test/ui/resolve/resolve-hint-macro.rs b/src/test/ui/resolve/resolve-hint-macro.rs index 5532c8b90e3..f16e8c07553 100644 --- a/src/test/ui/resolve/resolve-hint-macro.rs +++ b/src/test/ui/resolve/resolve-hint-macro.rs @@ -1,4 +1,11 @@ +// run-rustfix fn main() { assert_eq(1, 1); //~^ ERROR expected function, found macro `assert_eq` + assert_eq { 1, 1 }; + //~^ ERROR expected struct, variant or union type, found macro `assert_eq` + //~| ERROR expected identifier, found `1` + //~| ERROR expected identifier, found `1` + assert[true]; + //~^ ERROR expected value, found macro `assert` } diff --git a/src/test/ui/resolve/resolve-hint-macro.stderr b/src/test/ui/resolve/resolve-hint-macro.stderr index 78d77678083..bc69ddd8ffe 100644 --- a/src/test/ui/resolve/resolve-hint-macro.stderr +++ b/src/test/ui/resolve/resolve-hint-macro.stderr @@ -1,5 +1,21 @@ +error: expected identifier, found `1` + --> $DIR/resolve-hint-macro.rs:5:17 + | +LL | assert_eq { 1, 1 }; + | --------- ^ expected identifier + | | + | while parsing this struct + +error: expected identifier, found `1` + --> $DIR/resolve-hint-macro.rs:5:20 + | +LL | assert_eq { 1, 1 }; + | --------- ^ expected identifier + | | + | while parsing this struct + error[E0423]: expected function, found macro `assert_eq` - --> $DIR/resolve-hint-macro.rs:2:5 + --> $DIR/resolve-hint-macro.rs:3:5 | LL | assert_eq(1, 1); | ^^^^^^^^^ not a function @@ -9,6 +25,29 @@ help: use `!` to invoke the macro LL | assert_eq!(1, 1); | + -error: aborting due to previous error +error[E0574]: expected struct, variant or union type, found macro `assert_eq` + --> $DIR/resolve-hint-macro.rs:5:5 + | +LL | assert_eq { 1, 1 }; + | ^^^^^^^^^ not a struct, variant or union type + | +help: use `!` to invoke the macro + | +LL | assert_eq! { 1, 1 }; + | + + +error[E0423]: expected value, found macro `assert` + --> $DIR/resolve-hint-macro.rs:9:5 + | +LL | assert[true]; + | ^^^^^^ not a value + | +help: use `!` to invoke the macro + | +LL | assert![true]; + | + + +error: aborting due to 5 previous errors -For more information about this error, try `rustc --explain E0423`. +Some errors have detailed explanations: E0423, E0574. +For more information about an error, try `rustc --explain E0423`. -- cgit 1.4.1-3-g733a5 From 10858d28af13351e8d959a8a577427b1580a7185 Mon Sep 17 00:00:00 2001 From: Richard Cobbe Date: Mon, 29 Nov 2021 11:28:41 -0800 Subject: Fix test directives; comment out calls broken on windows-gnu --- src/test/run-make/raw-dylib-alt-calling-convention/Makefile | 7 ++++++- src/test/run-make/raw-dylib-alt-calling-convention/lib.rs | 7 +++++-- src/test/run-make/raw-dylib-alt-calling-convention/output.txt | 2 -- 3 files changed, 11 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/test/run-make/raw-dylib-alt-calling-convention/Makefile b/src/test/run-make/raw-dylib-alt-calling-convention/Makefile index 0f874333fa0..4af8b43ea84 100644 --- a/src/test/run-make/raw-dylib-alt-calling-convention/Makefile +++ b/src/test/run-make/raw-dylib-alt-calling-convention/Makefile @@ -1,12 +1,17 @@ # Test the behavior of #[link(.., kind = "raw-dylib")] with alternative calling conventions. -# only-i686-pc-windows-msvc +# only-x86 +# only-windows -include ../../run-make-fulldeps/tools.mk all: $(call COMPILE_OBJ,"$(TMPDIR)"/extern.obj,extern.c) +ifdef IS_MSVC $(CC) "$(TMPDIR)"/extern.obj -link -dll -out:"$(TMPDIR)"/extern.dll +else + $(CC) "$(TMPDIR)"/extern.obj -shared -o "$(TMPDIR)"/extern.dll +endif $(RUSTC) --crate-type lib --crate-name raw_dylib_alt_calling_convention_test lib.rs $(RUSTC) --crate-type bin driver.rs -L "$(TMPDIR)" "$(TMPDIR)"/driver > "$(TMPDIR)"/output.txt diff --git a/src/test/run-make/raw-dylib-alt-calling-convention/lib.rs b/src/test/run-make/raw-dylib-alt-calling-convention/lib.rs index ba0f1418aba..165792b0490 100644 --- a/src/test/run-make/raw-dylib-alt-calling-convention/lib.rs +++ b/src/test/run-make/raw-dylib-alt-calling-convention/lib.rs @@ -62,9 +62,12 @@ pub fn library_function() { fastcall_fn_2(16, 3.5); fastcall_fn_3(3.5); fastcall_fn_4(1, 2, 3.0); - fastcall_fn_5(S { x: 1, y: 2 }, 16); + // FIXME: 91167 + // rustc generates incorrect code for the calls to fastcall_fn_5 and fastcall_fn_7 + // on i686-pc-windows-gnu; commenting these out until the indicated issue is fixed. + //fastcall_fn_5(S { x: 1, y: 2 }, 16); fastcall_fn_6(Some(&S { x: 10, y: 12 })); - fastcall_fn_7(S2 { x: 15, y: 16 }, 3); + //fastcall_fn_7(S2 { x: 15, y: 16 }, 3); fastcall_fn_8(S3 { x: [1, 2, 3, 4, 5] }, S3 { x: [6, 7, 8, 9, 10] }); fastcall_fn_9(1, 3.0); } diff --git a/src/test/run-make/raw-dylib-alt-calling-convention/output.txt b/src/test/run-make/raw-dylib-alt-calling-convention/output.txt index be598a22027..348bad63ed0 100644 --- a/src/test/run-make/raw-dylib-alt-calling-convention/output.txt +++ b/src/test/run-make/raw-dylib-alt-calling-convention/output.txt @@ -11,8 +11,6 @@ fastcall_fn_1(14) fastcall_fn_2(16, 3.5) fastcall_fn_3(3.5) fastcall_fn_4(1, 2, 3.0) -fastcall_fn_5(S { x: 1, y: 2 }, 16) fastcall_fn_6(S { x: 10, y: 12 }) -fastcall_fn_7(S2 { x: 15, y: 16 }, 3) fastcall_fn_8(S3 { x: [1, 2, 3, 4, 5] }, S3 { x: [6, 7, 8, 9, 10] }) fastcall_fn_9(1, 3.0) -- cgit 1.4.1-3-g733a5 From f491a9f601a8ad0efe7f38de5b807a2c07774ae1 Mon Sep 17 00:00:00 2001 From: Caio Date: Wed, 19 Jan 2022 16:23:44 -0300 Subject: Add tests to ensure that let_chains works with if_let_guard --- src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.rs | 12 +++++++++++- src/test/ui/rfc-2497-if-let-chains/then-else-blocks.rs | 16 +++++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.rs b/src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.rs index 5915cb9df26..945c665e35d 100644 --- a/src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.rs +++ b/src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.rs @@ -1,6 +1,6 @@ // check-pass -#![feature(let_chains)] +#![feature(if_let_guard, let_chains)] use std::ops::Range; @@ -16,6 +16,16 @@ fn main() { && let None = local_start { } + match opt { + Some(ref first) if let second = first && let _third = second => {}, + _ => {} + } + match opt { + Some(ref first) if let Range { start: local_start, end: _ } = first + && let None = local_start => {}, + _ => {} + } + while let first = &opt && let Some(ref second) = first && let None = second.start { } while let Some(ref first) = opt && let second = first && let _third = second { diff --git a/src/test/ui/rfc-2497-if-let-chains/then-else-blocks.rs b/src/test/ui/rfc-2497-if-let-chains/then-else-blocks.rs index 0856a105206..e061174f667 100644 --- a/src/test/ui/rfc-2497-if-let-chains/then-else-blocks.rs +++ b/src/test/ui/rfc-2497-if-let-chains/then-else-blocks.rs @@ -1,6 +1,6 @@ // run-pass -#![feature(let_chains)] +#![feature(if_let_guard, let_chains)] fn check_if_let(opt: Option>>, value: i32) -> bool { if let Some(first) = opt @@ -15,6 +15,17 @@ fn check_if_let(opt: Option>>, value: i32) -> bool { } } +fn check_let_guard(opt: Option>>, value: i32) -> bool { + match opt { + Some(first) if let Some(second) = first && let Some(third) = second && third == value => { + true + } + _ => { + false + } + } +} + fn check_while_let(opt: Option>>, value: i32) -> bool { while let Some(first) = opt && let Some(second) = first @@ -30,6 +41,9 @@ fn main() { assert_eq!(check_if_let(Some(Some(Some(1))), 1), true); assert_eq!(check_if_let(Some(Some(Some(1))), 9), false); + assert_eq!(check_let_guard(Some(Some(Some(1))), 1), true); + assert_eq!(check_let_guard(Some(Some(Some(1))), 9), false); + assert_eq!(check_while_let(Some(Some(Some(1))), 1), true); assert_eq!(check_while_let(Some(Some(Some(1))), 9), false); } -- cgit 1.4.1-3-g733a5 From 801ac0e24ff6269fe71d22d29b8482e2117b2f82 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Tue, 18 Jan 2022 22:05:17 -0800 Subject: Fix scroll offset when jumping to internal id --- src/librustdoc/html/static/css/rustdoc.css | 9 ++++++++- src/test/rustdoc-gui/sidebar-mobile.goml | 6 ++++++ 2 files changed, 14 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/librustdoc/html/static/css/rustdoc.css b/src/librustdoc/html/static/css/rustdoc.css index dbc068ce6b1..947eb5a37f2 100644 --- a/src/librustdoc/html/static/css/rustdoc.css +++ b/src/librustdoc/html/static/css/rustdoc.css @@ -1738,12 +1738,19 @@ details.rustdoc-toggle[open] > summary.hideme::after { } @media (max-width: 700px) { + /* When linking to an item with an `id` (for instance, by clicking a link in the sidebar, + or visiting a URL with a fragment like `#method.new`, we don't want the item to be obscured + by the topbar. Anything with an `id` gets scroll-margin-top equal to .mobile-topbar's size. + */ + *[id] { + scroll-margin-top: 45px; + } + .rustdoc { padding-top: 0px; /* Sidebar should overlay main content, rather than pushing main content to the right. Turn off `display: flex` on the body element. */ display: block; - scroll-margin-top: 45px; } main { diff --git a/src/test/rustdoc-gui/sidebar-mobile.goml b/src/test/rustdoc-gui/sidebar-mobile.goml index 547eb3fd1b3..680822b6ecb 100644 --- a/src/test/rustdoc-gui/sidebar-mobile.goml +++ b/src/test/rustdoc-gui/sidebar-mobile.goml @@ -29,3 +29,9 @@ assert-css: (".sidebar", {"display": "block", "left": "-1000px"}) // Check that the topbar is visible assert-property: (".mobile-topbar", {"clientHeight": "45"}) + +// Check that clicking an element from the sidebar scrolls to the right place +// so the target is not obscured by the topbar. +click: ".sidebar-menu-toggle" +click: ".sidebar-links a" +assert-position: ("#method\.must_use", {"y": 45}) -- cgit 1.4.1-3-g733a5 From ab239cc7499c137cc6d4b3b6e9a01b78f7bd3b07 Mon Sep 17 00:00:00 2001 From: Martin Nordholts Date: Wed, 19 Jan 2022 22:03:08 +0100 Subject: src/test/rustdoc-json: Check for `struct_field`s in `variant_tuple_struct.rs` The presence of `struct_field`s is being checked for already in `variant_struct.rs`. We should also check for them in `variant_tuple_struct.rs`. --- src/test/rustdoc-json/enums/variant_struct.rs | 4 ++-- src/test/rustdoc-json/enums/variant_tuple_struct.rs | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/test/rustdoc-json/enums/variant_struct.rs b/src/test/rustdoc-json/enums/variant_struct.rs index 246e6a09007..fcd92887c0b 100644 --- a/src/test/rustdoc-json/enums/variant_struct.rs +++ b/src/test/rustdoc-json/enums/variant_struct.rs @@ -2,8 +2,8 @@ // @has - "$.index[*][?(@.name=='EnumStruct')].kind" \"enum\" pub enum EnumStruct { // @has - "$.index[*][?(@.name=='VariantS')].inner.variant_kind" \"struct\" - // @has - "$.index[*][?(@.name=='x')]" - // @has - "$.index[*][?(@.name=='y')]" + // @has - "$.index[*][?(@.name=='x')].kind" \"struct_field\" + // @has - "$.index[*][?(@.name=='y')].kind" \"struct_field\" VariantS { x: u32, y: String, diff --git a/src/test/rustdoc-json/enums/variant_tuple_struct.rs b/src/test/rustdoc-json/enums/variant_tuple_struct.rs index d948dc552cd..ac3e72e2905 100644 --- a/src/test/rustdoc-json/enums/variant_tuple_struct.rs +++ b/src/test/rustdoc-json/enums/variant_tuple_struct.rs @@ -2,5 +2,7 @@ // @has - "$.index[*][?(@.name=='EnumTupleStruct')].kind" \"enum\" pub enum EnumTupleStruct { // @has - "$.index[*][?(@.name=='VariantA')].inner.variant_kind" \"tuple\" + // @has - "$.index[*][?(@.name=='0')].kind" \"struct_field\" + // @has - "$.index[*][?(@.name=='1')].kind" \"struct_field\" VariantA(u32, String), } -- cgit 1.4.1-3-g733a5 From dcb0721a29f63cd2fb3a30487c81cfb657f0a7ee Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Wed, 19 Jan 2022 20:12:31 -0800 Subject: Support --bless for pp-exact pretty printer tests --- src/tools/compiletest/src/runtest.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index f039ba59d23..22785013085 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -500,7 +500,19 @@ impl<'test> TestCx<'test> { expected = expected.replace(&cr, ""); } - self.compare_source(&expected, &actual); + if !self.config.bless { + self.compare_source(&expected, &actual); + } else if expected != actual { + let filepath_buf; + let filepath = match &self.props.pp_exact { + Some(file) => { + filepath_buf = self.testpaths.file.parent().unwrap().join(file); + &filepath_buf + } + None => &self.testpaths.file, + }; + fs::write(filepath, &actual).unwrap(); + } // If we're only making sure that the output matches then just stop here if self.props.pretty_compare_only { -- cgit 1.4.1-3-g733a5 From 4e17170c54c3e8bbff9cee1937dd6ab3b2001857 Mon Sep 17 00:00:00 2001 From: Artem Kryvokrysenko Date: Wed, 19 Jan 2022 00:59:44 -0800 Subject: rustdoc: auto create output directory when "--output-format json" This PR allows rustdoc to automatically create output directory in case it does not exist (when run with `--output-format json`). This fixes rustdoc crash: ```` $ rustdoc --output-format json -Z unstable-options src/main.rs error: couldn't generate documentation: No such file or directory (os error 2) | = note: failed to create or modify "doc/main.json" error: aborting due to previous error ```` With this fix behavior of `rustdoc --output-format json` becomes consistent with `rustdoc --output-format html` (which already auto-creates output directory if it's missing) --- src/librustdoc/json/mod.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/librustdoc/json/mod.rs b/src/librustdoc/json/mod.rs index 81fbfd9fdbd..f9e9fe0d3cf 100644 --- a/src/librustdoc/json/mod.rs +++ b/src/librustdoc/json/mod.rs @@ -7,7 +7,7 @@ mod conversions; use std::cell::RefCell; -use std::fs::File; +use std::fs::{create_dir_all, File}; use std::path::PathBuf; use std::rc::Rc; @@ -18,13 +18,14 @@ use rustc_session::Session; use rustdoc_json_types as types; -use crate::clean; use crate::clean::types::{ExternalCrate, ExternalLocation}; use crate::config::RenderOptions; +use crate::docfs::PathError; use crate::error::Error; use crate::formats::cache::Cache; use crate::formats::FormatRenderer; use crate::json::conversions::{from_item_id, IntoWithTcx}; +use crate::{clean, try_err}; #[derive(Clone)] crate struct JsonRenderer<'tcx> { @@ -256,10 +257,13 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> { .collect(), format_version: types::FORMAT_VERSION, }; - let mut p = self.out_path.clone(); + let out_dir = self.out_path.clone(); + try_err!(create_dir_all(&out_dir), out_dir); + + let mut p = out_dir; p.push(output.index.get(&output.root).unwrap().name.clone().unwrap()); p.set_extension("json"); - let file = File::create(&p).map_err(|error| Error { error: error.to_string(), file: p })?; + let file = try_err!(File::create(&p), p); serde_json::ser::to_writer(&file, &output).unwrap(); Ok(()) } -- cgit 1.4.1-3-g733a5 From 855c17643ab0ae64d38c336f14b122689efdc8d4 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 20 Jan 2022 21:18:04 +0100 Subject: add script to prevent point releases with same number as existing ones --- .github/workflows/ci.yml | 9 ++++++++ src/ci/github-actions/ci.yml | 4 ++++ src/ci/scripts/verify-stable-version-number.sh | 30 ++++++++++++++++++++++++++ 3 files changed, 43 insertions(+) create mode 100755 src/ci/scripts/verify-stable-version-number.sh (limited to 'src') diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fe5dedb6ba4..1f872569ab1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -128,6 +128,9 @@ jobs: - name: ensure backported commits are in upstream branches run: src/ci/scripts/verify-backported-commits.sh if: success() && !env.SKIP_JOB + - name: ensure the stable version number is correct + run: src/ci/scripts/verify-stable-version-number.sh + if: success() && !env.SKIP_JOB - name: run the build run: src/ci/scripts/run-build-from-ci.sh env: @@ -502,6 +505,9 @@ jobs: - name: ensure backported commits are in upstream branches run: src/ci/scripts/verify-backported-commits.sh if: success() && !env.SKIP_JOB + - name: ensure the stable version number is correct + run: src/ci/scripts/verify-stable-version-number.sh + if: success() && !env.SKIP_JOB - name: run the build run: src/ci/scripts/run-build-from-ci.sh env: @@ -612,6 +618,9 @@ jobs: - name: ensure backported commits are in upstream branches run: src/ci/scripts/verify-backported-commits.sh if: success() && !env.SKIP_JOB + - name: ensure the stable version number is correct + run: src/ci/scripts/verify-stable-version-number.sh + if: success() && !env.SKIP_JOB - name: run the build run: src/ci/scripts/run-build-from-ci.sh env: diff --git a/src/ci/github-actions/ci.yml b/src/ci/github-actions/ci.yml index a70cdc4b519..cc282fc2f0e 100644 --- a/src/ci/github-actions/ci.yml +++ b/src/ci/github-actions/ci.yml @@ -206,6 +206,10 @@ x--expand-yaml-anchors--remove: run: src/ci/scripts/verify-backported-commits.sh <<: *step + - name: ensure the stable version number is correct + run: src/ci/scripts/verify-stable-version-number.sh + <<: *step + - name: run the build run: src/ci/scripts/run-build-from-ci.sh env: diff --git a/src/ci/scripts/verify-stable-version-number.sh b/src/ci/scripts/verify-stable-version-number.sh new file mode 100755 index 00000000000..82eb3833ccf --- /dev/null +++ b/src/ci/scripts/verify-stable-version-number.sh @@ -0,0 +1,30 @@ +#!/bin/bash +# On the stable channel, check whether we're trying to build artifacts with the +# same version number of a release that's already been published, and fail the +# build if that's the case. +# +# It's a mistake whenever that happens: the release process won't start if it +# detects a duplicate version number, and the artifacts would have to be +# rebuilt anyway. + +set -euo pipefail +IFS=$'\n\t' + +if [[ "$(cat src/ci/channel)" != "stable" ]]; then + echo "This script only works on the stable channel. Skipping the check." + exit 0 +fi + +version="$(cat src/version)" +url="https://static.rust-lang.org/dist/channel-rust-${version}.toml" + +if curl --silent --fail "${url}" >/dev/null; then + echo "The version number ${version} matches an existing release." + echo + echo "If you're trying to prepare a point release, remember to change the" + echo "version number in the src/version file." + exit 1 +else + echo "The version number ${version} does not match any released version!" + exit 0 +fi -- cgit 1.4.1-3-g733a5 From 682ef4db8060a0b26266cdcf5631c5776cd45e0a Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 13 Jan 2022 15:50:11 +0100 Subject: Exclude "test" from doc_auto_cfg rendering --- src/librustdoc/clean/cfg.rs | 61 ++++++++++++++++++++++++++++++++----------- src/librustdoc/clean/types.rs | 5 +++- 2 files changed, 50 insertions(+), 16 deletions(-) (limited to 'src') diff --git a/src/librustdoc/clean/cfg.rs b/src/librustdoc/clean/cfg.rs index dfee2b702c1..2c1dcad1afc 100644 --- a/src/librustdoc/clean/cfg.rs +++ b/src/librustdoc/clean/cfg.rs @@ -43,23 +43,22 @@ crate struct InvalidCfgError { impl Cfg { /// Parses a `NestedMetaItem` into a `Cfg`. - fn parse_nested(nested_cfg: &NestedMetaItem) -> Result { + fn parse_nested( + nested_cfg: &NestedMetaItem, + exclude: &[Symbol], + ) -> Result, InvalidCfgError> { match nested_cfg { - NestedMetaItem::MetaItem(ref cfg) => Cfg::parse(cfg), + NestedMetaItem::MetaItem(ref cfg) => Cfg::parse_without(cfg, exclude), NestedMetaItem::Literal(ref lit) => { Err(InvalidCfgError { msg: "unexpected literal", span: lit.span }) } } } - /// Parses a `MetaItem` into a `Cfg`. - /// - /// The `MetaItem` should be the content of the `#[cfg(...)]`, e.g., `unix` or - /// `target_os = "redox"`. - /// - /// If the content is not properly formatted, it will return an error indicating what and where - /// the error is. - crate fn parse(cfg: &MetaItem) -> Result { + crate fn parse_without( + cfg: &MetaItem, + exclude: &[Symbol], + ) -> Result, InvalidCfgError> { let name = match cfg.ident() { Some(ident) => ident.name, None => { @@ -70,9 +69,21 @@ impl Cfg { } }; match cfg.kind { - MetaItemKind::Word => Ok(Cfg::Cfg(name, None)), + MetaItemKind::Word => { + if exclude.contains(&name) { + Ok(None) + } else { + Ok(Some(Cfg::Cfg(name, None))) + } + } MetaItemKind::NameValue(ref lit) => match lit.kind { - LitKind::Str(value, _) => Ok(Cfg::Cfg(name, Some(value))), + LitKind::Str(value, _) => { + if exclude.contains(&name) { + Ok(None) + } else { + Ok(Some(Cfg::Cfg(name, Some(value)))) + } + } _ => Err(InvalidCfgError { // FIXME: if the main #[cfg] syntax decided to support non-string literals, // this should be changed as well. @@ -81,23 +92,43 @@ impl Cfg { }), }, MetaItemKind::List(ref items) => { - let mut sub_cfgs = items.iter().map(Cfg::parse_nested); - match name { + let sub_cfgs = items.iter().filter_map(|i| match Cfg::parse_nested(i, exclude) { + Ok(Some(c)) => Some(Ok(c)), + Err(e) => Some(Err(e)), + _ => None, + }); + let ret = match name { sym::all => sub_cfgs.fold(Ok(Cfg::True), |x, y| Ok(x? & y?)), sym::any => sub_cfgs.fold(Ok(Cfg::False), |x, y| Ok(x? | y?)), sym::not => { + let mut sub_cfgs = sub_cfgs.collect::>(); if sub_cfgs.len() == 1 { - Ok(!sub_cfgs.next().unwrap()?) + Ok(!sub_cfgs.pop().unwrap()?) } else { Err(InvalidCfgError { msg: "expected 1 cfg-pattern", span: cfg.span }) } } _ => Err(InvalidCfgError { msg: "invalid predicate", span: cfg.span }), + }; + match ret { + Ok(c) => Ok(Some(c)), + Err(e) => Err(e), } } } } + /// Parses a `MetaItem` into a `Cfg`. + /// + /// The `MetaItem` should be the content of the `#[cfg(...)]`, e.g., `unix` or + /// `target_os = "redox"`. + /// + /// If the content is not properly formatted, it will return an error indicating what and where + /// the error is. + crate fn parse(cfg: &MetaItem) -> Result { + Self::parse_without(cfg, &[]).map(|ret| ret.unwrap()) + } + /// Checks whether the given configuration can be matched in the current session. /// /// Equivalent to `attr::cfg_matches`. diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index fac1a0817e0..347f9d0a47c 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -831,7 +831,10 @@ impl AttributesExt for [ast::Attribute] { self.iter() .filter(|attr| attr.has_name(sym::cfg)) .filter_map(|attr| single(attr.meta_item_list()?)) - .filter_map(|attr| Cfg::parse(attr.meta_item()?).ok()) + .filter_map(|attr| match Cfg::parse_without(attr.meta_item()?, &[sym::test]) { + Ok(Some(c)) => Some(c), + _ => None, + }) .filter(|cfg| !hidden_cfg.contains(cfg)) .fold(Cfg::True, |cfg, new_cfg| cfg & new_cfg) } else { -- cgit 1.4.1-3-g733a5 From fd005f53c204136732c446dc476afc1266de730b Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 13 Jan 2022 15:50:21 +0100 Subject: Update doc_auto_cfg test --- src/test/rustdoc/doc-auto-cfg.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/test/rustdoc/doc-auto-cfg.rs b/src/test/rustdoc/doc-auto-cfg.rs index fcdd8354569..57dd0529535 100644 --- a/src/test/rustdoc/doc-auto-cfg.rs +++ b/src/test/rustdoc/doc-auto-cfg.rs @@ -3,6 +3,12 @@ #![crate_name = "foo"] // @has foo/fn.foo.html -// @has - '//*[@class="item-info"]/*[@class="stab portability"]' 'non-test' -#[cfg(not(test))] +// @has - '//*[@class="item-info"]/*[@class="stab portability"]' 'non-doctest' +#[cfg(not(doctest))] pub fn foo() {} + +// @has foo/fn.bar.html +// @has - '//*[@class="item-info"]/*[@class="stab portability"]' 'doc' +// @!has - '//*[@class="item-info"]/*[@class="stab portability"]' 'test' +#[cfg(any(test, doc))] +pub fn bar() {} -- cgit 1.4.1-3-g733a5 From caec4a23f201ff4e2bbfc3f8fd8b97e5e64fc20d Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sun, 16 Jan 2022 21:03:16 +0100 Subject: Extra cfg_hide a bit to handle inner cfgs --- src/librustdoc/clean/cfg.rs | 21 ++++++++------------- src/librustdoc/clean/types.rs | 3 +-- src/librustdoc/visit_ast.rs | 1 + src/test/rustdoc/doc-cfg-hide.rs | 2 +- 4 files changed, 11 insertions(+), 16 deletions(-) (limited to 'src') diff --git a/src/librustdoc/clean/cfg.rs b/src/librustdoc/clean/cfg.rs index 2c1dcad1afc..afa02f1e5c7 100644 --- a/src/librustdoc/clean/cfg.rs +++ b/src/librustdoc/clean/cfg.rs @@ -8,6 +8,7 @@ use std::mem; use std::ops; use rustc_ast::{LitKind, MetaItem, MetaItemKind, NestedMetaItem}; +use rustc_data_structures::fx::FxHashSet; use rustc_feature::Features; use rustc_session::parse::ParseSess; use rustc_span::symbol::{sym, Symbol}; @@ -45,7 +46,7 @@ impl Cfg { /// Parses a `NestedMetaItem` into a `Cfg`. fn parse_nested( nested_cfg: &NestedMetaItem, - exclude: &[Symbol], + exclude: &FxHashSet, ) -> Result, InvalidCfgError> { match nested_cfg { NestedMetaItem::MetaItem(ref cfg) => Cfg::parse_without(cfg, exclude), @@ -57,7 +58,7 @@ impl Cfg { crate fn parse_without( cfg: &MetaItem, - exclude: &[Symbol], + exclude: &FxHashSet, ) -> Result, InvalidCfgError> { let name = match cfg.ident() { Some(ident) => ident.name, @@ -70,19 +71,13 @@ impl Cfg { }; match cfg.kind { MetaItemKind::Word => { - if exclude.contains(&name) { - Ok(None) - } else { - Ok(Some(Cfg::Cfg(name, None))) - } + let cfg = Cfg::Cfg(name, None); + if exclude.contains(&cfg) { Ok(None) } else { Ok(Some(cfg)) } } MetaItemKind::NameValue(ref lit) => match lit.kind { LitKind::Str(value, _) => { - if exclude.contains(&name) { - Ok(None) - } else { - Ok(Some(Cfg::Cfg(name, Some(value)))) - } + let cfg = Cfg::Cfg(name, Some(value)); + if exclude.contains(&cfg) { Ok(None) } else { Ok(Some(cfg)) } } _ => Err(InvalidCfgError { // FIXME: if the main #[cfg] syntax decided to support non-string literals, @@ -126,7 +121,7 @@ impl Cfg { /// If the content is not properly formatted, it will return an error indicating what and where /// the error is. crate fn parse(cfg: &MetaItem) -> Result { - Self::parse_without(cfg, &[]).map(|ret| ret.unwrap()) + Self::parse_without(cfg, &FxHashSet::default()).map(|ret| ret.unwrap()) } /// Checks whether the given configuration can be matched in the current session. diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 347f9d0a47c..92e8b893115 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -831,11 +831,10 @@ impl AttributesExt for [ast::Attribute] { self.iter() .filter(|attr| attr.has_name(sym::cfg)) .filter_map(|attr| single(attr.meta_item_list()?)) - .filter_map(|attr| match Cfg::parse_without(attr.meta_item()?, &[sym::test]) { + .filter_map(|attr| match Cfg::parse_without(attr.meta_item()?, hidden_cfg) { Ok(Some(c)) => Some(c), _ => None, }) - .filter(|cfg| !hidden_cfg.contains(cfg)) .fold(Cfg::True, |cfg, new_cfg| cfg & new_cfg) } else { Cfg::True diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index 90cb5d586c2..2cbb3324a5e 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -141,6 +141,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { }) .collect::>() }) + .chain([Cfg::Cfg(sym::test, None)].into_iter()) .collect(); self.cx.cache.exact_paths = self.exact_paths; diff --git a/src/test/rustdoc/doc-cfg-hide.rs b/src/test/rustdoc/doc-cfg-hide.rs index 424fa6d6a91..636957fe998 100644 --- a/src/test/rustdoc/doc-cfg-hide.rs +++ b/src/test/rustdoc/doc-cfg-hide.rs @@ -26,7 +26,7 @@ pub struct Hyperdulia; // @has 'oud/struct.Oystercatcher.html' // @count - '//*[@class="stab portability"]' 1 -// @matches - '//*[@class="stab portability"]' 'crate features solecism and oystercatcher' +// @matches - '//*[@class="stab portability"]' 'crate feature oystercatcher only' // compile-flags:--cfg feature="oystercatcher" #[cfg(all(feature = "solecism", feature = "oystercatcher"))] pub struct Oystercatcher; -- cgit 1.4.1-3-g733a5 From b0df7653d0cc7256e9bb34a0bc3c7e00f3afaea7 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 20 Jan 2022 22:13:32 +0100 Subject: More clean up --- src/librustdoc/clean/cfg.rs | 7 ++----- src/librustdoc/clean/types.rs | 5 ++--- 2 files changed, 4 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/librustdoc/clean/cfg.rs b/src/librustdoc/clean/cfg.rs index afa02f1e5c7..b72d2624177 100644 --- a/src/librustdoc/clean/cfg.rs +++ b/src/librustdoc/clean/cfg.rs @@ -87,11 +87,8 @@ impl Cfg { }), }, MetaItemKind::List(ref items) => { - let sub_cfgs = items.iter().filter_map(|i| match Cfg::parse_nested(i, exclude) { - Ok(Some(c)) => Some(Ok(c)), - Err(e) => Some(Err(e)), - _ => None, - }); + let sub_cfgs = + items.iter().filter_map(|i| Cfg::parse_nested(i, exclude).transpose()); let ret = match name { sym::all => sub_cfgs.fold(Ok(Cfg::True), |x, y| Ok(x? & y?)), sym::any => sub_cfgs.fold(Ok(Cfg::False), |x, y| Ok(x? | y?)), diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 92e8b893115..7ae7b940f26 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -831,9 +831,8 @@ impl AttributesExt for [ast::Attribute] { self.iter() .filter(|attr| attr.has_name(sym::cfg)) .filter_map(|attr| single(attr.meta_item_list()?)) - .filter_map(|attr| match Cfg::parse_without(attr.meta_item()?, hidden_cfg) { - Ok(Some(c)) => Some(c), - _ => None, + .filter_map(|attr| { + Cfg::parse_without(attr.meta_item()?, hidden_cfg).ok().flatten() }) .fold(Cfg::True, |cfg, new_cfg| cfg & new_cfg) } else { -- cgit 1.4.1-3-g733a5