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()
}
Loading