user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Auto merge of #53255 - orium:fix-bug-overflow-send, r=arielb1

Browse source 
Add a per-tree error cache to the obligation forest

This implements part of what @nikomatsakis mentioned in  https://github.com/rust-lang/rust/pull/30533#issuecomment-170705871:

> 1. If you find that a new obligation is a duplicate of one already in the tree, the proper processing is:
>      * if that other location is your parent, you should abort with a cycle error (or accept it, if coinductive)
>      * if that other location is not an ancestor, you can safely ignore the new obligation

In particular it implements the "if that other location is your parent accept it, if coinductive" part.  This fixes #40827.

I have to say that I'm not 100% confident that this is rock solid.  This is my first pull request 🎉, and I didn't know anything about the trait resolver before this.  In particular I'm not totally sure that comparing predicates is enough (for instance, do we need to compare `param_env` as well?).  Also, I'm not sure what @nikomatsakis mentions [here](https://github.com/rust-lang/rust/issues/30977#issue-127091096), but it might be something that affects this PR:

> In particular, I am wary of getting things wrong around inference variables! We can always add things to the set in their current state, and if unifications occur then the obligation is just kind of out-of-date, but I want to be sure we don't accidentally fail to notice that something is our ancestor. I decided this was subtle enough to merit its own PR.

Anyway, go ahead and review 🙂.

Ref #30977.

# Performance

We are now copying vectors around, so I decided to do some benchmarking.  A simple benchmark shows that this does not seem to affect performance in a measurable way:

I ran `cargo clean && cargo build` 20 times on actix-web (84b27db) and these are the results:

```text
rustc master:

            Mean        Std.Dev.    Min         Median      Max
real        66.637      2.996       57.220      67.714      69.314
user        307.293     14.741      258.093     312.209     320.702
sys         12.524      0.653       10.499      12.726      13.193

rustc fix-bug-overflow-send:

            Mean        Std.Dev.    Min         Median      Max
real        66.297      4.310       53.532      67.516      70.348
user        306.812     22.371      236.917     314.748     326.229
sys         12.757      0.952       9.671       13.125      13.544
```

I will do a more comprehensive benchmark (compiling rustc stage1) and post the results.

r? @nikomatsakis, @nnethercote

PS: It is better to review this commit-by-commit.
This commit is contained in:
bors 2018-09-30 19:41:07 +00:00
commit fc403ad987
10 changed files with 183 additions and 49 deletions
Download patch file Download diff file Expand all files Collapse all files

8
src/librustc/traits/auto_trait.rs
View file

@ -73,16 +73,16 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
/// ```
/// struct Foo<T> { data: Box<T> }
/// ```
///
/// then this might return that Foo<T>: 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<T>: Send if Box<T>: 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<A>(
@ -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<T: Copy>', 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

3
src/librustc/traits/fulfill.rs
View file

@ -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<FulfillmentError<'tcx>>> {
-> Result<(), Vec<FulfillmentError<'tcx>>> {
debug!("select(obligation-forest-size={})", self.predicates.len());
let mut errors = Vec::new();

2
src/librustc/traits/mod.rs
View file

@ -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

20
src/librustc/traits/select.rs
View file

@ -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<T> { data: T, next: Option<Box<List<T>>> {
// struct List<T> { data: T, next: Option<Box<List<T>>> }
//
// `Box<List<T>>` will be `Send` if `T` is `Send` and
// `Option<Box<List<T>>>` is `Send`, and in turn
@ -1407,7 +1407,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
stack: &TraitObligationStack<'o, 'tcx>)
-> Result<SelectionCandidateSet<'tcx>, 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<Selection<'tcx>,SelectionError<'tcx>>
-> Result<Selection<'tcx>, 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<PredicateObligation<'_>> = 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);

93
src/librustc_data_structures/obligation_forest/mod.rs
View file

@ -65,6 +65,12 @@ pub enum ProcessResult<O, E> {
Error(E),
}
#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)]
struct ObligationTreeId(usize);
type ObligationTreeIdGenerator =
::std::iter::Map<::std::ops::RangeFrom<usize>, fn(usize) -> ObligationTreeId>;
pub struct ObligationForest<O: ForestObligation> {
/// The list of obligations. In between calls to
/// `process_obligations`, this list only contains nodes in the
@ -79,11 +85,25 @@ pub struct ObligationForest<O: ForestObligation> {
/// at a higher index than its parent. This is needed by the
/// backtrace iterator (which uses `split_at`).
nodes: Vec<Node<O>>,
/// A cache of predicates that have been successfully completed.
done_cache: FxHashSet<O::Predicate>,
/// An cache of the nodes in `nodes`, indexed by predicate.
waiting_cache: FxHashMap<O::Predicate, NodeIndex>,
scratch: Option<Vec<usize>>,
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<ObligationTreeId, FxHashSet<O::Predicate>>,
}
#[derive(Debug)]
@ -99,6 +119,9 @@ struct Node<O> {
/// Obligations that depend on this obligation for their
/// completion. They must all be in a non-pending state.
dependents: Vec<NodeIndex>,
/// 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<O: ForestObligation> ObligationForest<O> {
done_cache: FxHashSet(),
waiting_cache: FxHashMap(),
scratch: Some(vec![]),
obligation_tree_id_generator: (0..).map(|i| ObligationTreeId(i)),
error_cache: FxHashMap(),
}
}
@ -187,7 +212,7 @@ impl<O: ForestObligation> ObligationForest<O> {
-> Result<(), ()>
{
if self.done_cache.contains(obligation.as_predicate()) {
return Ok(())
return Ok(());
}
match self.waiting_cache.entry(obligation.as_predicate().clone()) {
@ -214,9 +239,29 @@ impl<O: ForestObligation> ObligationForest<O> {
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<O: ForestObligation> ObligationForest<O> {
.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<O: ForestObligation> ObligationForest<O> {
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 { state: 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<O: ForestObligation> ObligationForest<O> {
}
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<O: ForestObligation> ObligationForest<O> {
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<O: ForestObligation> ObligationForest<O> {
}
impl<O> Node<O> {
fn new(parent: Option<NodeIndex>, obligation: O) -> Node<O> {
fn new(
parent: Option<NodeIndex>,
obligation: O,
obligation_tree_id: ObligationTreeId
) -> Node<O> {
Node {
obligation,
state: Cell::new(NodeState::Pending),
parent,
dependents: vec![],
obligation_tree_id,
}
}
}

8
src/librustc_data_structures/obligation_forest/test.rs
View file

@ -59,8 +59,9 @@ impl<OF, BF, O, E> ObligationProcessor for ClosureObligationProcessor<OF, BF, O,
fn process_backedge<'c, I>(&mut self, _cycle: I,
_marker: PhantomData<&'c Self::Obligation>)
where I: Clone + Iterator<Item=&'c Self::Obligation> {
}
where I: Clone + Iterator<Item=&'c Self::Obligation>
{
}
}
@ -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

27
src/test/ui/issue-40827.rs Normal file
View file

@ -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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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<Bar>);
enum Bar {
A(Rc<Foo>),
B(Option<Foo>),
}
fn f<T: Send>(_: T) {}
fn main() {
f(Foo(Arc::new(Bar::B(None))));
//~^ ERROR E0277
//~| ERROR E0277
}

35
src/test/ui/issue-40827.stderr Normal file
View file

@ -0,0 +1,35 @@
error[E0277]: `std::rc::Rc<Foo>` cannot be sent between threads safely
--> $DIR/issue-40827.rs:24:5
|
LL | f(Foo(Arc::new(Bar::B(None))));
| ^ `std::rc::Rc<Foo>` cannot be sent between threads safely
|
= help: within `Bar`, the trait `std::marker::Send` is not implemented for `std::rc::Rc<Foo>`
= 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<Bar>`
= note: required because it appears within the type `Foo`
note: required by `f`
--> $DIR/issue-40827.rs:21:1
|
LL | fn f<T: Send>(_: T) {}
| ^^^^^^^^^^^^^^^^^^^
error[E0277]: `std::rc::Rc<Foo>` cannot be shared between threads safely
--> $DIR/issue-40827.rs:24:5
|
LL | f(Foo(Arc::new(Bar::B(None))));
| ^ `std::rc::Rc<Foo>` cannot be shared between threads safely
|
= help: within `Bar`, the trait `std::marker::Sync` is not implemented for `std::rc::Rc<Foo>`
= 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<Bar>`
= note: required because it appears within the type `Foo`
note: required by `f`
--> $DIR/issue-40827.rs:21:1
|
LL | fn f<T: Send>(_: T) {}
| ^^^^^^^^^^^^^^^^^^^
error: aborting due to 2 previous errors
For more information about this error, try `rustc --explain E0277`.

4
src/test/ui/recursion/recursive-requirements.rs
View file

@ -23,5 +23,7 @@ pub struct Bar {
}
fn main() {
let _: AssertSync<Foo> = unimplemented!(); //~ ERROR E0275
let _: AssertSync<Foo> = unimplemented!();
//~^ ERROR E0277
//~| ERROR E0277
}

32
src/test/ui/recursion/recursive-requirements.stderr
View file

@ -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<Foo> = unimplemented!(); //~ ERROR E0275
| ^^^^^^^^^^^^^^^
LL | let _: AssertSync<Foo> = 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<Foo>`
= 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<T: Sync>(PhantomData<T>);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error[E0277]: `*const Foo` cannot be shared between threads safely
--> $DIR/recursive-requirements.rs:26:12
|
LL | let _: AssertSync<Foo> = 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<Bar>`
= note: required because it appears within the type `Foo`
note: required by `AssertSync`
--> $DIR/recursive-requirements.rs:13:1
|
LL | struct AssertSync<T: Sync>(PhantomData<T>);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
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`.