From d2ff5d696cfa6003b10a124910259fe5a5885271 Mon Sep 17 00:00:00 2001 From: Diogo Sousa Date: Fri, 10 Aug 2018 18:13:43 +0100 Subject: [PATCH 1/2] Typos and style fixes. --- src/librustc/traits/auto_trait.rs | 8 ++++---- src/librustc/traits/fulfill.rs | 3 +-- src/librustc/traits/mod.rs | 2 +- src/librustc/traits/select.rs | 20 ++++++++++--------- .../obligation_forest/mod.rs | 6 +++--- .../obligation_forest/test.rs | 8 +++----- 6 files changed, 23 insertions(+), 24 deletions(-) diff --git a/src/librustc/traits/auto_trait.rs b/src/librustc/traits/auto_trait.rs index 8f106a0812538..79fc9b449238d 100644 --- a/src/librustc/traits/auto_trait.rs +++ b/src/librustc/traits/auto_trait.rs @@ -73,16 +73,16 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> { /// ``` /// struct Foo { data: Box } /// ``` - + /// /// then this might return that Foo: Send if T: Send (encoded in the AutoTraitResult type). /// The analysis attempts to account for custom impls as well as other complex cases. This /// result is intended for use by rustdoc and other such consumers. - + /// /// (Note that due to the coinductive nature of Send, the full and correct result is actually /// quite simple to generate. That is, when a type has no custom impl, it is Send iff its field /// types are all Send. So, in our example, we might have that Foo: Send if Box: Send. /// But this is often not the best way to present to the user.) - + /// /// Warning: The API should be considered highly unstable, and it may be refactored or removed /// in the future. pub fn find_auto_trait_generics( @@ -288,7 +288,7 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> { // hold. // // One additional consideration is supertrait bounds. Normally, a ParamEnv is only ever - // consutrcted once for a given type. As part of the construction process, the ParamEnv will + // constructed once for a given type. As part of the construction process, the ParamEnv will // have any supertrait bounds normalized - e.g. if we have a type 'struct Foo', the // ParamEnv will contain 'T: Copy' and 'T: Clone', since 'Copy: Clone'. When we construct our // own ParamEnv, we need to do this ourselves, through traits::elaborate_predicates, or else diff --git a/src/librustc/traits/fulfill.rs b/src/librustc/traits/fulfill.rs index 19ee2c1aabfa4..0f330504334a6 100644 --- a/src/librustc/traits/fulfill.rs +++ b/src/librustc/traits/fulfill.rs @@ -45,7 +45,6 @@ impl<'tcx> ForestObligation for PendingPredicateObligation<'tcx> { /// along. Once all type inference constraints have been generated, the /// method `select_all_or_error` can be used to report any remaining /// ambiguous cases as errors. - pub struct FulfillmentContext<'tcx> { // A list of all obligations that have been registered with this // fulfillment context. @@ -89,7 +88,7 @@ impl<'a, 'gcx, 'tcx> FulfillmentContext<'tcx> { /// Attempts to select obligations using `selcx`. fn select(&mut self, selcx: &mut SelectionContext<'a, 'gcx, 'tcx>) - -> Result<(),Vec>> { + -> Result<(), Vec>> { debug!("select(obligation-forest-size={})", self.predicates.len()); let mut errors = Vec::new(); diff --git a/src/librustc/traits/mod.rs b/src/librustc/traits/mod.rs index 6b1092814a404..f394fbd9c7b4c 100644 --- a/src/librustc/traits/mod.rs +++ b/src/librustc/traits/mod.rs @@ -644,7 +644,7 @@ pub fn normalize_param_env_or_error<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, // have the errors get reported at a defined place (e.g., // during typeck). Instead I have all parameter // environments, in effect, going through this function - // and hence potentially reporting errors. This ensurse of + // and hence potentially reporting errors. This ensures of // course that we never forget to normalize (the // alternative seemed like it would involve a lot of // manual invocations of this fn -- and then we'd have to diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index ab71d13ab0686..7e504094ad86f 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -582,7 +582,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { match self.confirm_candidate(obligation, candidate) { Err(SelectionError::Overflow) => { assert!(self.query_mode == TraitQueryMode::Canonical); - return Err(SelectionError::Overflow); + Err(SelectionError::Overflow) }, Err(e) => Err(e), Ok(candidate) => Ok(Some(candidate)) @@ -879,7 +879,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { // must be met of course). One obvious case this comes up is // marker traits like `Send`. Think of a linked list: // - // struct List { data: T, next: Option>> { + // struct List { data: T, next: Option>> } // // `Box>` will be `Send` if `T` is `Send` and // `Option>>` is `Send`, and in turn @@ -1407,7 +1407,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { stack: &TraitObligationStack<'o, 'tcx>) -> Result, SelectionError<'tcx>> { - let TraitObligationStack { obligation, .. } = *stack; + let obligation = stack.obligation; let ref obligation = Obligation { param_env: obligation.param_env, cause: obligation.cause.clone(), @@ -1788,9 +1788,9 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { } fn assemble_candidates_from_auto_impls(&mut self, - obligation: &TraitObligation<'tcx>, - candidates: &mut SelectionCandidateSet<'tcx>) - -> Result<(), SelectionError<'tcx>> + obligation: &TraitObligation<'tcx>, + candidates: &mut SelectionCandidateSet<'tcx>) + -> Result<(), SelectionError<'tcx>> { // OK to skip binder here because the tests we do below do not involve bound regions let self_ty = *obligation.self_ty().skip_binder(); @@ -2433,7 +2433,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { fn confirm_candidate(&mut self, obligation: &TraitObligation<'tcx>, candidate: SelectionCandidate<'tcx>) - -> Result,SelectionError<'tcx>> + -> Result, SelectionError<'tcx>> { debug!("confirm_candidate({:?}, {:?})", obligation, @@ -2612,11 +2612,11 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { let mut obligations = self.collect_predicates_for_types( obligation.param_env, cause, - obligation.recursion_depth+1, + obligation.recursion_depth + 1, trait_def_id, nested); - let trait_obligations = self.in_snapshot(|this, snapshot| { + let trait_obligations: Vec> = self.in_snapshot(|this, snapshot| { let poly_trait_ref = obligation.predicate.to_poly_trait_ref(); let (trait_ref, skol_map) = this.infcx().skolemize_late_bound_regions(&poly_trait_ref); @@ -2630,6 +2630,8 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { snapshot) }); + // Adds the predicates from the trait. Note that this contains a `Self: Trait` + // predicate as usual. It won't have any effect since auto traits are coinductive. obligations.extend(trait_obligations); debug!("vtable_auto_impl: obligations={:?}", obligations); diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 7ef88852685d5..094c1cec31acc 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -187,7 +187,7 @@ impl ObligationForest { -> Result<(), ()> { if self.done_cache.contains(obligation.as_predicate()) { - return Ok(()) + return Ok(()); } match self.waiting_cache.entry(obligation.as_predicate().clone()) { @@ -269,8 +269,8 @@ impl ObligationForest { self.nodes[index]); let result = match self.nodes[index] { - Node { state: ref _state, ref mut obligation, .. } - if _state.get() == NodeState::Pending => + Node { ref state, ref mut obligation, .. } + if state.get() == NodeState::Pending => { processor.process_obligation(obligation) } diff --git a/src/librustc_data_structures/obligation_forest/test.rs b/src/librustc_data_structures/obligation_forest/test.rs index 527a1ef0ec441..c27a65e34310f 100644 --- a/src/librustc_data_structures/obligation_forest/test.rs +++ b/src/librustc_data_structures/obligation_forest/test.rs @@ -59,8 +59,9 @@ impl ObligationProcessor for ClosureObligationProcessor(&mut self, _cycle: I, _marker: PhantomData<&'c Self::Obligation>) - where I: Clone + Iterator { - } + where I: Clone + Iterator + { + } } @@ -350,11 +351,8 @@ fn done_dependency() { }, |_|{})); assert_eq!(ok, vec!["(A,B,C): Sized"]); assert_eq!(err.len(), 0); - - } - #[test] fn orphan() { // check that orphaned nodes are handled correctly From 6bfa6aa87255cf8291333139ad2d383950b5a0f7 Mon Sep 17 00:00:00 2001 From: Diogo Sousa Date: Sun, 30 Sep 2018 02:41:49 +0100 Subject: [PATCH 2/2] Deduplicate errors in the obligation forest. Fixes #40827. --- .../obligation_forest/mod.rs | 91 +++++++++++++++---- src/test/ui/issue-40827.rs | 27 ++++++ src/test/ui/issue-40827.stderr | 35 +++++++ .../ui/recursion/recursive-requirements.rs | 4 +- .../recursion/recursive-requirements.stderr | 32 +++++-- 5 files changed, 162 insertions(+), 27 deletions(-) create mode 100644 src/test/ui/issue-40827.rs create mode 100644 src/test/ui/issue-40827.stderr diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 094c1cec31acc..f159857e74467 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -65,6 +65,12 @@ pub enum ProcessResult { Error(E), } +#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)] +struct ObligationTreeId(usize); + +type ObligationTreeIdGenerator = + ::std::iter::Map<::std::ops::RangeFrom, fn(usize) -> ObligationTreeId>; + pub struct ObligationForest { /// The list of obligations. In between calls to /// `process_obligations`, this list only contains nodes in the @@ -79,11 +85,25 @@ pub struct ObligationForest { /// at a higher index than its parent. This is needed by the /// backtrace iterator (which uses `split_at`). nodes: Vec>, + /// A cache of predicates that have been successfully completed. done_cache: FxHashSet, + /// An cache of the nodes in `nodes`, indexed by predicate. waiting_cache: FxHashMap, + scratch: Option>, + + obligation_tree_id_generator: ObligationTreeIdGenerator, + + /// Per tree error cache. This is used to deduplicate errors, + /// which is necessary to avoid trait resolution overflow in + /// some cases. + /// + /// See [this][details] for details. + /// + /// [details]: https://github.com/rust-lang/rust/pull/53255#issuecomment-421184780 + error_cache: FxHashMap>, } #[derive(Debug)] @@ -99,6 +119,9 @@ struct Node { /// Obligations that depend on this obligation for their /// completion. They must all be in a non-pending state. dependents: Vec, + + /// Identifier of the obligation tree to which this node belongs. + obligation_tree_id: ObligationTreeId, } /// The state of one node in some tree within the forest. This @@ -165,6 +188,8 @@ impl ObligationForest { done_cache: FxHashSet(), waiting_cache: FxHashMap(), scratch: Some(vec![]), + obligation_tree_id_generator: (0..).map(|i| ObligationTreeId(i)), + error_cache: FxHashMap(), } } @@ -214,9 +239,29 @@ impl ObligationForest { Entry::Vacant(v) => { debug!("register_obligation_at({:?}, {:?}) - ok, new index is {}", obligation, parent, self.nodes.len()); - v.insert(NodeIndex::new(self.nodes.len())); - self.nodes.push(Node::new(parent, obligation)); - Ok(()) + + let obligation_tree_id = match parent { + Some(p) => { + let parent_node = &self.nodes[p.get()]; + parent_node.obligation_tree_id + } + None => self.obligation_tree_id_generator.next().unwrap() + }; + + let already_failed = + parent.is_some() + && self.error_cache + .get(&obligation_tree_id) + .map(|errors| errors.contains(obligation.as_predicate())) + .unwrap_or(false); + + if already_failed { + Err(()) + } else { + v.insert(NodeIndex::new(self.nodes.len())); + self.nodes.push(Node::new(parent, obligation, obligation_tree_id)); + Ok(()) + } } } } @@ -251,6 +296,15 @@ impl ObligationForest { .collect() } + fn insert_into_error_cache(&mut self, node_index: usize) { + let node = &self.nodes[node_index]; + + self.error_cache + .entry(node.obligation_tree_id) + .or_insert_with(|| FxHashSet()) + .insert(node.obligation.as_predicate().clone()); + } + /// Perform a pass through the obligation list. This must /// be called in a loop until `outcome.stalled` is false. /// @@ -264,22 +318,15 @@ impl ObligationForest { let mut stalled = true; for index in 0..self.nodes.len() { - debug!("process_obligations: node {} == {:?}", - index, - self.nodes[index]); + debug!("process_obligations: node {} == {:?}", index, self.nodes[index]); let result = match self.nodes[index] { - Node { ref state, ref mut obligation, .. } - if state.get() == NodeState::Pending => - { - processor.process_obligation(obligation) - } + Node { ref state, ref mut obligation, .. } if state.get() == NodeState::Pending => + processor.process_obligation(obligation), _ => continue }; - debug!("process_obligations: node {} got result {:?}", - index, - result); + debug!("process_obligations: node {} got result {:?}", index, result); match result { ProcessResult::Unchanged => { @@ -420,13 +467,13 @@ impl ObligationForest { } while let Some(i) = error_stack.pop() { - let node = &self.nodes[i]; - - match node.state.get() { + match self.nodes[i].state.get() { NodeState::Error => continue, - _ => node.state.set(NodeState::Error) + _ => self.nodes[i].state.set(NodeState::Error), } + let node = &self.nodes[i]; + error_stack.extend( node.parent.iter().chain(node.dependents.iter()).map(|x| x.get()) ); @@ -514,6 +561,7 @@ impl ObligationForest { self.waiting_cache.remove(self.nodes[i].obligation.as_predicate()); node_rewrites[i] = nodes_len; dead_nodes += 1; + self.insert_into_error_cache(i); } NodeState::OnDfsStack | NodeState::Success => unreachable!() } @@ -587,12 +635,17 @@ impl ObligationForest { } impl Node { - fn new(parent: Option, obligation: O) -> Node { + fn new( + parent: Option, + obligation: O, + obligation_tree_id: ObligationTreeId + ) -> Node { Node { obligation, state: Cell::new(NodeState::Pending), parent, dependents: vec![], + obligation_tree_id, } } } diff --git a/src/test/ui/issue-40827.rs b/src/test/ui/issue-40827.rs new file mode 100644 index 0000000000000..4b079ace3ca32 --- /dev/null +++ b/src/test/ui/issue-40827.rs @@ -0,0 +1,27 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use std::rc::Rc; +use std::sync::Arc; + +struct Foo(Arc); + +enum Bar { + A(Rc), + B(Option), +} + +fn f(_: T) {} + +fn main() { + f(Foo(Arc::new(Bar::B(None)))); + //~^ ERROR E0277 + //~| ERROR E0277 +} diff --git a/src/test/ui/issue-40827.stderr b/src/test/ui/issue-40827.stderr new file mode 100644 index 0000000000000..dd0ebf96d19e4 --- /dev/null +++ b/src/test/ui/issue-40827.stderr @@ -0,0 +1,35 @@ +error[E0277]: `std::rc::Rc` cannot be sent between threads safely + --> $DIR/issue-40827.rs:24:5 + | +LL | f(Foo(Arc::new(Bar::B(None)))); + | ^ `std::rc::Rc` cannot be sent between threads safely + | + = help: within `Bar`, the trait `std::marker::Send` is not implemented for `std::rc::Rc` + = note: required because it appears within the type `Bar` + = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc` + = note: required because it appears within the type `Foo` +note: required by `f` + --> $DIR/issue-40827.rs:21:1 + | +LL | fn f(_: T) {} + | ^^^^^^^^^^^^^^^^^^^ + +error[E0277]: `std::rc::Rc` cannot be shared between threads safely + --> $DIR/issue-40827.rs:24:5 + | +LL | f(Foo(Arc::new(Bar::B(None)))); + | ^ `std::rc::Rc` cannot be shared between threads safely + | + = help: within `Bar`, the trait `std::marker::Sync` is not implemented for `std::rc::Rc` + = note: required because it appears within the type `Bar` + = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc` + = note: required because it appears within the type `Foo` +note: required by `f` + --> $DIR/issue-40827.rs:21:1 + | +LL | fn f(_: T) {} + | ^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0277`. diff --git a/src/test/ui/recursion/recursive-requirements.rs b/src/test/ui/recursion/recursive-requirements.rs index 2c0f0338b2d15..dd6d2f6a2256a 100644 --- a/src/test/ui/recursion/recursive-requirements.rs +++ b/src/test/ui/recursion/recursive-requirements.rs @@ -23,5 +23,7 @@ pub struct Bar { } fn main() { - let _: AssertSync = unimplemented!(); //~ ERROR E0275 + let _: AssertSync = unimplemented!(); + //~^ ERROR E0277 + //~| ERROR E0277 } diff --git a/src/test/ui/recursion/recursive-requirements.stderr b/src/test/ui/recursion/recursive-requirements.stderr index d9e08102ab6a6..8fe282505e907 100644 --- a/src/test/ui/recursion/recursive-requirements.stderr +++ b/src/test/ui/recursion/recursive-requirements.stderr @@ -1,15 +1,33 @@ -error[E0275]: overflow evaluating the requirement `Foo: std::marker::Sync` +error[E0277]: `*const Bar` cannot be shared between threads safely --> $DIR/recursive-requirements.rs:26:12 | -LL | let _: AssertSync = unimplemented!(); //~ ERROR E0275 - | ^^^^^^^^^^^^^^^ +LL | let _: AssertSync = unimplemented!(); + | ^^^^^^^^^^^^^^^ `*const Bar` cannot be shared between threads safely | - = help: consider adding a `#![recursion_limit="128"]` attribute to your crate - = note: required because it appears within the type `std::marker::PhantomData` + = help: within `Foo`, the trait `std::marker::Sync` is not implemented for `*const Bar` + = note: required because it appears within the type `Foo` +note: required by `AssertSync` + --> $DIR/recursive-requirements.rs:13:1 + | +LL | struct AssertSync(PhantomData); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error[E0277]: `*const Foo` cannot be shared between threads safely + --> $DIR/recursive-requirements.rs:26:12 + | +LL | let _: AssertSync = unimplemented!(); + | ^^^^^^^^^^^^^^^ `*const Foo` cannot be shared between threads safely + | + = help: within `Foo`, the trait `std::marker::Sync` is not implemented for `*const Foo` = note: required because it appears within the type `Bar` = note: required because it appears within the type `std::marker::PhantomData` = note: required because it appears within the type `Foo` +note: required by `AssertSync` + --> $DIR/recursive-requirements.rs:13:1 + | +LL | struct AssertSync(PhantomData); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to previous error +error: aborting due to 2 previous errors -For more information about this error, try `rustc --explain E0275`. +For more information about this error, try `rustc --explain E0277`.