Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove interior mutability from TraitDef by turning fields into queries #41911

Merged
merged 10 commits into from
May 18, 2017
Merged
7 changes: 7 additions & 0 deletions src/librustc/dep_graph/dep_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ pub enum DepNode<D: Clone + Debug> {
UsedTraitImports(D),
ConstEval(D),
SymbolName(D),
SpecializationGraph(D),
ObjectSafety(D),

// The set of impls for a given trait. Ultimately, it would be
// nice to get more fine-grained here (e.g., to include a
Expand All @@ -116,6 +118,8 @@ pub enum DepNode<D: Clone + Debug> {
// than changes in the impl body.
TraitImpls(D),

AllLocalTraitImpls,

// Nodes representing caches. To properly handle a true cache, we
// don't use a DepTrackingMap, but rather we push a task node.
// Otherwise the write into the map would be incorrectly
Expand Down Expand Up @@ -262,7 +266,10 @@ impl<D: Clone + Debug> DepNode<D> {
UsedTraitImports(ref d) => op(d).map(UsedTraitImports),
ConstEval(ref d) => op(d).map(ConstEval),
SymbolName(ref d) => op(d).map(SymbolName),
SpecializationGraph(ref d) => op(d).map(SpecializationGraph),
ObjectSafety(ref d) => op(d).map(ObjectSafety),
TraitImpls(ref d) => op(d).map(TraitImpls),
AllLocalTraitImpls => Some(AllLocalTraitImpls),
TraitItems(ref d) => op(d).map(TraitItems),
ReprHints(ref d) => op(d).map(ReprHints),
TraitSelect { ref trait_def_id, ref input_def_id } => {
Expand Down
61 changes: 61 additions & 0 deletions src/librustc/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,67 @@ RFC. It is, however, [currently unimplemented][iss15872].
[iss15872]: https://github.com/rust-lang/rust/issues/15872
"##,

E0119: r##"
There are conflicting trait implementations for the same type.
Example of erroneous code:

```compile_fail,E0119
trait MyTrait {
fn get(&self) -> usize;
}

impl<T> MyTrait for T {
fn get(&self) -> usize { 0 }
}

struct Foo {
value: usize
}

impl MyTrait for Foo { // error: conflicting implementations of trait
// `MyTrait` for type `Foo`
fn get(&self) -> usize { self.value }
}
```

When looking for the implementation for the trait, the compiler finds
both the `impl<T> MyTrait for T` where T is all types and the `impl
MyTrait for Foo`. Since a trait cannot be implemented multiple times,
this is an error. So, when you write:

```
trait MyTrait {
fn get(&self) -> usize;
}

impl<T> MyTrait for T {
fn get(&self) -> usize { 0 }
}
```

This makes the trait implemented on all types in the scope. So if you
try to implement it on another one after that, the implementations will
conflict. Example:

```
trait MyTrait {
fn get(&self) -> usize;
}

impl<T> MyTrait for T {
fn get(&self) -> usize { 0 }
}

struct Foo;

fn main() {
let f = Foo;

f.get(); // the trait is implemented so we can use it
}
```
"##,

E0133: r##"
Unsafe code was used outside of an unsafe function or block.

Expand Down
4 changes: 2 additions & 2 deletions src/librustc/hir/map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -497,15 +497,15 @@ impl<'hir> Map<'hir> {
}

pub fn trait_impls(&self, trait_did: DefId) -> &'hir [NodeId] {
self.dep_graph.read(DepNode::TraitImpls(trait_did));
self.dep_graph.read(DepNode::AllLocalTraitImpls);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why you converted this into a single dep-node, rather than keeping it per-trait. It seems like this is losing quite a bit of precision, no?

(That is, if any trait adds an impl, we will consider them all to have changed.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly for ease of implementation and symmetry with the situation in metadata. DepNode::TraitImpls has been re-purposed to represent local and remote impls and, with red-green, will take care of stopping invalidation short.

Without red-green though you are right, it's losing precision. Want me to make it per-item?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not a big deal.


// NB: intentionally bypass `self.forest.krate()` so that we
// do not trigger a read of the whole krate here
self.forest.krate.trait_impls.get(&trait_did).map_or(&[], |xs| &xs[..])
}

pub fn trait_default_impl(&self, trait_did: DefId) -> Option<NodeId> {
self.dep_graph.read(DepNode::TraitImpls(trait_did));
self.dep_graph.read(DepNode::AllLocalTraitImpls);

// NB: intentionally bypass `self.forest.krate()` so that we
// do not trigger a read of the whole krate here
Expand Down
8 changes: 8 additions & 0 deletions src/librustc/ich/fingerprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,11 @@ impl stable_hasher::StableHasherResult for Fingerprint {
fingerprint
}
}

impl<CTX> stable_hasher::HashStable<CTX> for Fingerprint {
fn hash_stable<W: stable_hasher::StableHasherResult>(&self,
_: &mut CTX,
hasher: &mut stable_hasher::StableHasher<W>) {
::std::hash::Hash::hash(&self.0, hasher);
}
}
24 changes: 23 additions & 1 deletion src/librustc/ich/hcx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use ty;
use util::nodemap::NodeMap;

use std::hash as std_hash;
use std::collections::{HashMap, HashSet};
use std::collections::{HashMap, HashSet, BTreeMap};

use syntax::ast;
use syntax::attr;
Expand Down Expand Up @@ -348,3 +348,25 @@ pub fn hash_stable_nodemap<'a, 'tcx, V, W>(hcx: &mut StableHashingContext<'a, 't
hcx.tcx.hir.definitions().node_to_hir_id(*node_id).local_id
});
}


pub fn hash_stable_btreemap<'a, 'tcx, K, V, SK, F, W>(hcx: &mut StableHashingContext<'a, 'tcx>,
hasher: &mut StableHasher<W>,
map: &BTreeMap<K, V>,
extract_stable_key: F)
where K: Eq + Ord,
V: HashStable<StableHashingContext<'a, 'tcx>>,
SK: HashStable<StableHashingContext<'a, 'tcx>> + Ord + Clone,
F: Fn(&mut StableHashingContext<'a, 'tcx>, &K) -> SK,
W: StableHasherResult,
{
let mut keys: Vec<_> = map.keys()
.map(|k| (extract_stable_key(hcx, k), k))
.collect();
keys.sort_unstable_by_key(|&(ref stable_key, _)| stable_key.clone());
keys.len().hash_stable(hcx, hasher);
for (stable_key, key) in keys {
stable_key.hash_stable(hcx, hasher);
map[key].hash_stable(hcx, hasher);
}
}
3 changes: 2 additions & 1 deletion src/librustc/ich/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
pub use self::fingerprint::Fingerprint;
pub use self::caching_codemap_view::CachingCodemapView;
pub use self::hcx::{StableHashingContext, NodeIdHashingMode, hash_stable_hashmap,
hash_stable_hashset, hash_stable_nodemap};
hash_stable_hashset, hash_stable_nodemap,
hash_stable_btreemap};
mod fingerprint;
mod caching_codemap_view;
mod hcx;
Expand Down
18 changes: 16 additions & 2 deletions src/librustc/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -619,8 +619,6 @@ pub fn get_vtable_methods<'a, 'tcx>(
debug!("get_vtable_methods({:?})", trait_ref);

supertraits(tcx, trait_ref).flat_map(move |trait_ref| {
tcx.populate_implementations_for_trait_if_necessary(trait_ref.def_id());

let trait_methods = tcx.associated_items(trait_ref.def_id())
.filter(|item| item.kind == ty::AssociatedKind::Method);

Expand Down Expand Up @@ -782,3 +780,19 @@ impl<'tcx> TraitObligation<'tcx> {
ty::Binder(self.predicate.skip_binder().self_ty())
}
}

pub fn provide(providers: &mut ty::maps::Providers) {
*providers = ty::maps::Providers {
is_object_safe: object_safety::is_object_safe_provider,
specialization_graph_of: specialize::specialization_graph_provider,
..*providers
};
}

pub fn provide_extern(providers: &mut ty::maps::Providers) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since they're identical I'd use the same function twice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You think so? It's just a coincidence that they are identical at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe have provide() call provide_extern()? I imagine that anything we provide for extern crates, we would also provide for locals, but maybe not the other way around

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense.

*providers = ty::maps::Providers {
is_object_safe: object_safety::is_object_safe_provider,
specialization_graph_of: specialize::specialization_graph_provider,
..*providers
};
}
25 changes: 6 additions & 19 deletions src/librustc/traits/object_safety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,25 +77,6 @@ pub enum MethodViolationCode {
}

impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
pub fn is_object_safe(self, trait_def_id: DefId) -> bool {
// Because we query yes/no results frequently, we keep a cache:
let def = self.trait_def(trait_def_id);

let result = def.object_safety().unwrap_or_else(|| {
let result = self.object_safety_violations(trait_def_id).is_empty();

// Record just a yes/no result in the cache; this is what is
// queried most frequently. Note that this may overwrite a
// previous result, but always with the same thing.
def.set_object_safety(result);

result
});

debug!("is_object_safe({:?}) = {}", trait_def_id, result);

result
}

/// Returns the object safety violations that affect
/// astconv - currently, Self in supertraits. This is needed
Expand Down Expand Up @@ -391,3 +372,9 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
error
}
}

pub(super) fn is_object_safe_provider<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
trait_def_id: DefId)
-> bool {
tcx.object_safety_violations(trait_def_id).is_empty()
}
40 changes: 22 additions & 18 deletions src/librustc/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1317,26 +1317,30 @@ fn assoc_ty_def<'cx, 'gcx, 'tcx>(
assoc_ty_name: ast::Name)
-> Option<specialization_graph::NodeItem<ty::AssociatedItem>>
{
let trait_def_id = selcx.tcx().impl_trait_ref(impl_def_id).unwrap().def_id;
let trait_def = selcx.tcx().trait_def(trait_def_id);

if !trait_def.is_complete(selcx.tcx()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work? Do we produce cycle errors in more cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean exactly. I don't see a potential cycle here.

Copy link
Member

@eddyb eddyb May 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The !is_complete case is a fallback for cycles, that is, when normalization of e.g. <T as Foo>::Assoc is needed to compute Foo's specialization graph.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @aturon We may be lacking tests here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so assoc_ty_def() might be called while still building the specialization graph?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to be used in the specialization graph of the same trait.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to be used in the specialization graph of the same trait.

Yes, that makes sense. And I thought the following example might trigger the condition, but it doesn't :(

#![feature(specialization)]

trait Assoc {
    type Output;
}

impl<T> Assoc for T {
    default type Output = u8;
}

impl Assoc for u32 {
    type Output = u32;
}

impl Assoc for u64 {
    // Shouldn't this cause a reentrant call?
    type Output = <u32 as Assoc>::Output;
}

fn main() {
    let _x: <u32 as Assoc>::Output = 0;
}

Copy link
Member

@eddyb eddyb May 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the impl header - only Self/params are checked by coherence/specialization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the following gives me the error overflow evaluating the requirement <u32 as Assoc>::Output regardless of whether cycle detection is enabled:

#![feature(specialization)]

trait Assoc {
    type Output;
}

impl Assoc for u32 {
    type Output = u64;
}

impl Assoc for <u32 as Assoc>::Output {
    type Output = u64;
}

fn main() {
    let _x: <u64 as Assoc>::Output = 0;
}

So that's not really what we are looking for, I guess?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't really expect you to be able to project <u32 as Assoc>::Output in an impl for Assoc. What I expect could work is if there is a DAG between the traits, sort of like this:

#![feature(specialization)]

use std::vec;

trait Assoc {
    type Output;
}

impl Assoc for u32 {
    type Output = u64;
}

impl Assoc for <vec::IntoIter<u64> as Iterator>::Item {
    type Output = u64;
}

fn main() {
}

Interestingly, coherence rejects this example, presumably because of RFC 1214 concerns. I'm not sure that's right, but this example does work today:

#![feature(specialization)]

use std::vec;

trait Assoc {
    type Output;
}

impl Assoc for u32 {
    type Output = u64;
}

impl Assoc for <u32 as Assoc2>::Output {
    type Output = u64;
}

trait Assoc2 {
    type Output;
}

impl Assoc2 for u32 {
    type Output = u64;
}

fn main() {
}

That said, I think that would also work in @michaelwoerister's branch, no?

let impl_node = specialization_graph::Node::Impl(impl_def_id);
for item in impl_node.items(selcx.tcx()) {
if item.kind == ty::AssociatedKind::Type && item.name == assoc_ty_name {
return Some(specialization_graph::NodeItem {
node: specialization_graph::Node::Impl(impl_def_id),
item: item,
});
}
let tcx = selcx.tcx();
let trait_def_id = tcx.impl_trait_ref(impl_def_id).unwrap().def_id;
let trait_def = tcx.trait_def(trait_def_id);

// This function may be called while we are still building the
// specialization graph that is queried below (via TraidDef::ancestors()),
// so, in order to avoid unnecessary infinite recursion, we manually look
// for the associated item at the given impl.
// If there is no such item in that impl, this function will fail with a
// cycle error if the specialization graph is currently being built.
let impl_node = specialization_graph::Node::Impl(impl_def_id);
for item in impl_node.items(tcx) {
if item.kind == ty::AssociatedKind::Type && item.name == assoc_ty_name {
return Some(specialization_graph::NodeItem {
node: specialization_graph::Node::Impl(impl_def_id),
item: item,
});
}
None
} else {
trait_def
.ancestors(impl_def_id)
.defs(selcx.tcx(), assoc_ty_name, ty::AssociatedKind::Type)
.next()
}

trait_def
.ancestors(tcx, impl_def_id)
.defs(tcx, assoc_ty_name, ty::AssociatedKind::Type)
.next()
}

// # Cache
Expand Down
62 changes: 61 additions & 1 deletion src/librustc/traits/specialize/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use ty::subst::{Subst, Substs};
use traits::{self, Reveal, ObligationCause};
use ty::{self, TyCtxt, TypeFoldable};
use syntax_pos::DUMMY_SP;
use std::rc::Rc;

pub mod specialization_graph;

Expand Down Expand Up @@ -118,7 +119,7 @@ pub fn find_associated_item<'a, 'tcx>(
let trait_def_id = tcx.trait_id_of_impl(impl_data.impl_def_id).unwrap();
let trait_def = tcx.trait_def(trait_def_id);

let ancestors = trait_def.ancestors(impl_data.impl_def_id);
let ancestors = trait_def.ancestors(tcx, impl_data.impl_def_id);
match ancestors.defs(tcx, item.name, item.kind).next() {
Some(node_item) => {
let substs = tcx.infer_ctxt((), Reveal::All).enter(|infcx| {
Expand Down Expand Up @@ -285,3 +286,62 @@ impl SpecializesCache {
self.map.insert((a, b), result);
}
}

// Query provider for `specialization_graph_of`.
pub(super) fn specialization_graph_provider<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
trait_id: DefId)
-> Rc<specialization_graph::Graph> {
let mut sg = specialization_graph::Graph::new();

let mut trait_impls: Vec<DefId> = tcx.trait_impls_of(trait_id).iter().collect();

// The coherence checking implementation seems to rely on impls being
// iterated over (roughly) in definition order, so we are sorting by
// negated CrateNum (so remote definitions are visited first) and then
// by a flattend version of the DefIndex.
trait_impls.sort_unstable_by_key(|def_id| {
(-(def_id.krate.as_u32() as i64),
def_id.index.address_space().index(),
def_id.index.as_array_index())
});

for impl_def_id in trait_impls {
if impl_def_id.is_local() {
// This is where impl overlap checking happens:
let insert_result = sg.insert(tcx, impl_def_id);
// Report error if there was one.
if let Err(overlap) = insert_result {
let mut err = struct_span_err!(tcx.sess,
tcx.span_of_impl(impl_def_id).unwrap(),
E0119,
"conflicting implementations of trait `{}`{}:",
overlap.trait_desc,
overlap.self_desc.clone().map_or(String::new(),
|ty| {
format!(" for type `{}`", ty)
}));

match tcx.span_of_impl(overlap.with_impl) {
Ok(span) => {
err.span_label(span, format!("first implementation here"));
err.span_label(tcx.span_of_impl(impl_def_id).unwrap(),
format!("conflicting implementation{}",
overlap.self_desc
.map_or(String::new(),
|ty| format!(" for `{}`", ty))));
}
Err(cname) => {
err.note(&format!("conflicting implementation in crate `{}`", cname));
}
}

err.emit();
}
} else {
let parent = tcx.impl_parent(impl_def_id).unwrap_or(trait_id);
sg.record_impl_from_cstore(tcx, parent, impl_def_id)
}
}

Rc::new(sg)
}
Loading