coherence: fix is_knowable logic

[arielb1]

A trait-ref that passes the orphan-check rules can still be implemented in a crate downstream from our crate (for example, LocalType for LocalTrait<_> might be matched by a LocalType for LocalTrait<TypeFromDownstreamCrate>), and this should be known by the is_knowable logic.

Trait selection had a hackfix for this, but it's an hacky fix that does not handle all cases. This patch removes it.

fixes #43355.

r? @nikomatsakis

Needs a crater run

[arielb1]

@bors try

[bors]

⌛ Trying commit 5360699 with merge faf75254806fa54615158a9058c5a50b81dc8006...

[arielb1]

@bors r- retry

[bors]

⌛ Trying commit 5360699 with merge 999ab8e65bc12abb057c3e7ba58a538ae3ab7c57...

[arielb1]

@bors untry

[arielb1]

@bors retry

[bors]

⌛ Trying commit 5360699 with merge 0bac3d526df71b0959316897d8d8b8d1baf6b4f1...

[arielb1]

@bors try retry

[bors]

⌛ Trying commit 4c08ed5 with merge 0c5e181...

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[aidanhs]

Crater run started.

[bors]

☔ The latest upstream changes (presumably #46430) made this pull request unmergeable. Please resolve the merge conflicts.

[aidanhs]

Hi @arielb1 (crater requester), @nikomatsakis (reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-46192/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

[arielb1]

Regressions:

crossbeam-epoch-0.1.0 & bow-1.0.3 (yay I broke Servo, but this is no longer a dependency)

impl<T> Into<Box<T>> for Owned<T>
 = note: conflicting implementation in crate `core`:
 - impl<T, U> core::convert::Into<U> for T
   where U: core::convert::From<T>;

This is a problem because any joker can write this (on nightly, but some variants are probably possible using #[fundamental] traits on stable - e.g. if Bow implemented FnOnce):

#![feature(optin_builtin_traits)]

pub struct Foo;
pub auto trait Xyz {}
impl !Xyz for Foo {}
impl !Xyz for Box<Foo> {}

impl<T: Xyz> From<T> for Box<Foo> {
    fn from(_: T) -> Self { Box::new(Foo) }
}

fn main() {
    let _foo = <Box<Foo>>::from(());
}

Which would cause a coherence conflict on linking for the trait-ref <Owned<Foo> as Into<Box<Foo>>>. This is a link-safety issue, so the impl must not be allowed.

I think the only fix is to not use Into but instead use an into_box function.

glib v0.1.3

impl<'a, T: ?Sized + SetValue> SetValue for &'a T {
impl<T: IsA<Object>> SetValue for T {

This causes a conflict because downstream crates can implement IsA<Object> for both T and &T, unless we check this impl in every downstream crate.

The crate author had actually encountered some other coherence problems with this approach, and removed this in a new version of their crate: gtk-rs/glib@b36de4e#diff-dd26c2766e65aca43e4228e64ebd0cf8

I don't see any crates depending on glib 0.1.3 itself.

ocl v0.15.0

rusqlite v0.10.3

rusqlcipher v0.14.6

This is basically the same issue as glib - attempt to implement a trait for both T: Trait and &T where T: Trait.

impl<'a, E> From<E> for EventArray where E: Into<Event>
impl<'a, E> From<&'a E> for EventArray where E: Into<Event> + Clone

impl<'a, T: ?Sized> From<&'a T> for ToSqlOutput<'a> where &'a T: Into<ValueRef<'a>>
impl<'a, T: Into<Value>> From<T> for ToSqlOutput<'a>

This is still an issue in the newest crate version, probably need to talk with the crate authors

• @jgallagher (rusqlite & rusqlcipher) - https://github.com/jgallagher/rusqlite/issues/313
• @cogciprocate (ocl) - ?

lru-disk-cache v0.1.0 (aka sccache)

impl<K, V> CountableMeter<K, V> for Count {
impl<K, V, T: Meter<K, V, Measure=usize>> CountableMeter<K, V> for T {

This is a conflict because coherence can't see that <Count as Meter<K, V>>::Measure = () can't coexist with <Count as Meter<K, V>>::Measure = usize.

This can be fixed by the crate author by breaking the associated type to a type parameter:

use std::borrow::Borrow;
/// A trait for measuring the size of a cache entry.
///
/// If you implement this trait, you should use `usize` as the `Measure` type, otherwise you will
/// also have to implement [`CountableMeter`][countablemeter].
///
/// [countablemeter]: trait.Meter.html
pub trait Meter<K, V> {
    /// The type used to store measurements.
    type Measure: Default + Copy;
    /// Calculate the size of `key` and `value`.
    fn measure<Q: ?Sized>(&self, key: &Q, value: &V) -> Self::Measure
        where K: Borrow<Q>;
}

/// Size limit based on a simple count of cache items.
pub struct Count;

impl<K, V> Meter<K, V> for Count {
   /// Don't store anything, the measurement can be derived from the map.
    type Measure = ();

    /// Don't actually count anything either.
    fn measure<Q: ?Sized>(&self, _: &Q, _: &V) -> ()
        where K: Borrow<Q>
    {}
}

/// A trait to allow the default `Count` measurement to not store an
/// extraneous counter.
pub trait CountableMeter<K, V>: Meter<K, V> {
    /// Add `amount` to `current` and return the sum.
    fn add(&self, current: Self::Measure, amount: Self::Measure) -> Self::Measure;
    /// Subtract `amount` from `current` and return the difference.
    fn sub(&self, current: Self::Measure, amount: Self::Measure) -> Self::Measure;
    /// Return `current` as a `usize` if possible, otherwise return `None`.
    ///
    /// If this method returns `None` the cache will use the number of cache entries as
    /// its size.
    fn size(&self, current: Self::Measure) -> Option<usize>;
}

/// `Count` is all no-ops, the number of entries in the map is the size.
impl<K, V, T: Meter<K, V>> CountableMeter<K, V> for T
    where T: CountableMeterWithMeasure<K, V, <T as Meter<K, V>>::Measure>
{

    fn add(&self, current: Self::Measure, amount: Self::Measure) -> Self::Measure {
        CountableMeterWithMeasure::add(self, current, amount)
    }
    
    fn sub(&self, current: Self::Measure, amount: Self::Measure) -> Self::Measure {
        CountableMeterWithMeasure::sub(self, current, amount)
    }

    fn size(&self, current: Self::Measure) -> Option<usize> {
        CountableMeterWithMeasure::size(self, current)
    }
}


pub trait CountableMeterWithMeasure<K, V, M> {
    /// Add `amount` to `current` and return the sum.
    fn add(&self, current: M, amount: M) -> M;
    /// Subtract `amount` from `current` and return the difference.
    fn sub(&self, current: M, amount: M) -> M;
    /// Return `current` as a `usize` if possible, otherwise return `None`.
    ///
    /// If this method returns `None` the cache will use the number of cache entries as
    /// its size.
    fn size(&self, current: M) -> Option<usize>;
}


/// For any other `Meter` with `Measure=usize`, just do the simple math.
impl<K, V, T> CountableMeterWithMeasure<K, V, usize> for T {
    fn add(&self, current: usize, amount: usize) -> usize {
        current + amount
    }
    fn sub(&self, current: usize, amount: usize) -> usize {
        current - amount
    }
    fn size(&self, current: usize) -> Option<usize> { Some(current) }
}

impl<K, V> CountableMeterWithMeasure<K, V, ()> for Count {
    fn add(&self, current: (), amount: ()) {}
    fn sub(&self, current: (), amount: ()) {}
    fn size(&self, current: ()) -> Option<usize> { None }
}
fn main() {}

[nikomatsakis]

Left a few documentation nits.

[nikomatsakis]

Nit: s/Option/Vec/, right?

[nikomatsakis]

This is wonderful.

[nikomatsakis]

@arielb1 this only issues warnings presently, right? Seems like we ought to land it, no? r=me, if you agree.

[arielb1]

Yea this only issues warnings. I'll land this.

[luser]

I pushed a fix to lru-disk-cache in sccache, thanks for providing working code!
mozilla/sccache@d148529

[arielb1]

@bors r=nikomatsakis

[bors]

📌 Commit 11f22ab has been approved by nikomatsakis

[arielb1]

@bors r=nikomatsakis

[bors]

📌 Commit 425c2c3 has been approved by nikomatsakis

[bors]

⌛ Testing commit 425c2c3 with merge 6536e05...

[bors]

💔 Test failed - status-travis

[kennytm]

@bors retry #44159

ERROR:  Could not find a valid gem 'aws-sdk-resources' (= 2.10.97) in any repository

[bors]

⌛ Testing commit 425c2c3 with merge 632ad19...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 632ad19 to master...