Auto merge of #46192 - arielb1:locally-coherent, r=<try>

coherence: fix is_knowable logic

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.

cc #43355.

FIXME: make this a soft error. I suppose we'll crater first.

r? @nikomatsakis

Needs a crater run

src/librustc/lint/builtin.rs

@@ -198,6 +198,12 @@ declare_lint! {
     "detects generic lifetime arguments in path segments with late bound lifetime parameters"
 }
 
+declare_lint! {
+    pub INCOHERENT_FUNDAMENTAL_IMPLS,
+    Deny,
+    "potentially-conflicting impls were erroneously allowed"
+}
+
 declare_lint! {
     pub DEPRECATED,
     Warn,
@@ -254,6 +260,7 @@ impl LintPass for HardwiredLints {
             MISSING_FRAGMENT_SPECIFIER,
             PARENTHESIZED_PARAMS_IN_TYPES_AND_MODULES,
             LATE_BOUND_LIFETIME_ARGUMENTS,
+            INCOHERENT_FUNDAMENTAL_IMPLS,
             DEPRECATED,
             UNUSED_UNSAFE,
             UNUSED_MUT

src/librustc/traits/coherence.rs

@@ -13,14 +13,26 @@
 use hir::def_id::{DefId, LOCAL_CRATE};
 use syntax_pos::DUMMY_SP;
 use traits::{self, Normalized, SelectionContext, Obligation, ObligationCause, Reveal};
+use traits::IntercrateMode;
 use traits::select::IntercrateAmbiguityCause;
 use ty::{self, Ty, TyCtxt};
 use ty::subst::Subst;
 
 use infer::{InferCtxt, InferOk};
 
-#[derive(Copy, Clone)]
-struct InferIsLocal(bool);
+#[derive(Copy, Clone, Debug)]
+/// Whether we do the orphan check relative to this crate or
+/// to some remote crate.
+enum InCrate {
+    Local,
+    Remote
+}
+
+#[derive(Debug, Copy, Clone)]
+pub enum Conflict {
+    Upstream,
+    Downstream { used_to_be_broken: bool }
+}
 
 pub struct OverlapResult<'tcx> {
     pub impl_header: ty::ImplHeader<'tcx>,
@@ -31,16 +43,19 @@ pub struct OverlapResult<'tcx> {
 /// `ImplHeader` with those types substituted
 pub fn overlapping_impls<'cx, 'gcx, 'tcx>(infcx: &InferCtxt<'cx, 'gcx, 'tcx>,
                                           impl1_def_id: DefId,
-                                          impl2_def_id: DefId)
+                                          impl2_def_id: DefId,
+                                          intercrate_mode: IntercrateMode)
                                           -> Option<OverlapResult<'tcx>>
 {
     debug!("impl_can_satisfy(\
            impl1_def_id={:?}, \
-           impl2_def_id={:?})",
+           impl2_def_id={:?},
+           intercrate_mode={:?})",
            impl1_def_id,
-           impl2_def_id);
+           impl2_def_id,
+           intercrate_mode);
 
-    let selcx = &mut SelectionContext::intercrate(infcx);
+    let selcx = &mut SelectionContext::intercrate(infcx, intercrate_mode);
     overlap(selcx, impl1_def_id, impl2_def_id)
 }
 
@@ -126,32 +141,49 @@ fn overlap<'cx, 'gcx, 'tcx>(selcx: &mut SelectionContext<'cx, 'gcx, 'tcx>,
 }
 
 pub fn trait_ref_is_knowable<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
-                                             trait_ref: ty::TraitRef<'tcx>) -> bool
+                                             trait_ref: ty::TraitRef<'tcx>)
+                                             -> Option<Conflict>
 {
     debug!("trait_ref_is_knowable(trait_ref={:?})", trait_ref);
-
-    // if the orphan rules pass, that means that no ancestor crate can
-    // impl this, so it's up to us.
-    if orphan_check_trait_ref(tcx, trait_ref, InferIsLocal(false)).is_ok() {
-        debug!("trait_ref_is_knowable: orphan check passed");
-        return true;
+    if orphan_check_trait_ref(tcx, trait_ref, InCrate::Remote).is_ok() {
+        // A downstream or cousin crate is allowed to implement some
+        // substitution of this trait-ref.
+
+        // A trait can be implementable for a trait ref by both the current
+        // crate and crates downstream of it. Older versions of rustc
+        // were not aware of this, causing incoherence (issue #43355).
+        let used_to_be_broken =
+            orphan_check_trait_ref(tcx, trait_ref, InCrate::Local).is_ok();
+        if used_to_be_broken {
+            debug!("trait_ref_is_knowable({:?}) - USED TO BE BROKEN", trait_ref);
+        }
+        return Some(Conflict::Downstream { used_to_be_broken });
     }
 
-    // if the trait is not marked fundamental, then it's always possible that
-    // an ancestor crate will impl this in the future, if they haven't
-    // already
-    if !trait_ref_is_local_or_fundamental(tcx, trait_ref) {
-        debug!("trait_ref_is_knowable: trait is neither local nor fundamental");
-        return false;
+    if trait_ref_is_local_or_fundamental(tcx, trait_ref) {
+        // This is a local or fundamental trait, so future-compatibility
+        // is no concern. We know that downstream/cousin crates are not
+        // allowed to implement a substitution of this trait ref, which
+        // means impls could only come from dependencies of this crate,
+        // which we already know about.
+        return None;
     }
 
-    // find out when some downstream (or cousin) crate could impl this
-    // trait-ref, presuming that all the parameters were instantiated
-    // with downstream types. If not, then it could only be
-    // implemented by an upstream crate, which means that the impl
-    // must be visible to us, and -- since the trait is fundamental
-    // -- we can test.
-    orphan_check_trait_ref(tcx, trait_ref, InferIsLocal(true)).is_err()
+    // This is a remote non-fundamental trait, so if another crate
+    // can be the "final owner" of a substitution of this trait-ref,
+    // they are allowed to implement it future-compatibly.
+    //
+    // However, if we are a final owner, then nobody else can be,
+    // and if we are an intermediate owner, then we don't care
+    // about future-compatibility, which means that we're OK if
+    // we are an owner.
+    if orphan_check_trait_ref(tcx, trait_ref, InCrate::Local).is_ok() {
+        debug!("trait_ref_is_knowable: orphan check passed");
+        return None;
+    } else {
+        debug!("trait_ref_is_knowable: nonlocal, nonfundamental, unowned");
+        return Some(Conflict::Upstream);
+    }
 }
 
 pub fn trait_ref_is_local_or_fundamental<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
@@ -189,30 +221,32 @@ pub fn orphan_check<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
         return Ok(());
     }
 
-    orphan_check_trait_ref(tcx, trait_ref, InferIsLocal(false))
+    orphan_check_trait_ref(tcx, trait_ref, InCrate::Local)
 }
 
 fn orphan_check_trait_ref<'tcx>(tcx: TyCtxt,
                                 trait_ref: ty::TraitRef<'tcx>,
-                                infer_is_local: InferIsLocal)
+                                in_crate: InCrate)
                                 -> Result<(), OrphanCheckErr<'tcx>>
 {
-    debug!("orphan_check_trait_ref(trait_ref={:?}, infer_is_local={})",
-           trait_ref, infer_is_local.0);
+    debug!("orphan_check_trait_ref(trait_ref={:?}, in_crate={:?})",
+           trait_ref, in_crate);
 
     // First, create an ordered iterator over all the type parameters to the trait, with the self
     // type appearing first.
     // Find the first input type that either references a type parameter OR
     // some local type.
     for input_ty in trait_ref.input_types() {
-        if ty_is_local(tcx, input_ty, infer_is_local) {
+        if ty_is_local(tcx, input_ty, in_crate) {
             debug!("orphan_check_trait_ref: ty_is_local `{:?}`", input_ty);
 
             // First local input type. Check that there are no
             // uncovered type parameters.
-            let uncovered_tys = uncovered_tys(tcx, input_ty, infer_is_local);
+            let uncovered_tys = uncovered_tys(tcx, input_ty, in_crate);
             for uncovered_ty in uncovered_tys {
-                if let Some(param) = uncovered_ty.walk().find(|t| is_type_parameter(t)) {
+                if let Some(param) = uncovered_ty.walk()
+                    .find(|t| is_possibly_remote_type(t, in_crate))
+                {
                     debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
                     return Err(OrphanCheckErr::UncoveredTy(param));
                 }
@@ -224,11 +258,11 @@ fn orphan_check_trait_ref<'tcx>(tcx: TyCtxt,
 
         // Otherwise, enforce invariant that there are no type
         // parameters reachable.
-        if !infer_is_local.0 {
-            if let Some(param) = input_ty.walk().find(|t| is_type_parameter(t)) {
-                debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
-                return Err(OrphanCheckErr::UncoveredTy(param));
-            }
+        if let Some(param) = input_ty.walk()
+            .find(|t| is_possibly_remote_type(t, in_crate))
+        {
+            debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
+            return Err(OrphanCheckErr::UncoveredTy(param));
         }
     }
 
@@ -237,29 +271,29 @@ fn orphan_check_trait_ref<'tcx>(tcx: TyCtxt,
     return Err(OrphanCheckErr::NoLocalInputType);
 }
 
-fn uncovered_tys<'tcx>(tcx: TyCtxt, ty: Ty<'tcx>, infer_is_local: InferIsLocal)
+fn uncovered_tys<'tcx>(tcx: TyCtxt, ty: Ty<'tcx>, in_crate: InCrate)
                        -> Vec<Ty<'tcx>> {
-    if ty_is_local_constructor(ty, infer_is_local) {
+    if ty_is_local_constructor(ty, in_crate) {
         vec![]
     } else if fundamental_ty(tcx, ty) {
         ty.walk_shallow()
-          .flat_map(|t| uncovered_tys(tcx, t, infer_is_local))
+          .flat_map(|t| uncovered_tys(tcx, t, in_crate))
           .collect()
     } else {
         vec![ty]
     }
 }
 
-fn is_type_parameter(ty: Ty) -> bool {
+fn is_possibly_remote_type(ty: Ty, _in_crate: InCrate) -> bool {
     match ty.sty {
         ty::TyProjection(..) | ty::TyParam(..) => true,
         _ => false,
     }
 }
 
-fn ty_is_local(tcx: TyCtxt, ty: Ty, infer_is_local: InferIsLocal) -> bool {
-    ty_is_local_constructor(ty, infer_is_local) ||
-        fundamental_ty(tcx, ty) && ty.walk_shallow().any(|t| ty_is_local(tcx, t, infer_is_local))
+fn ty_is_local(tcx: TyCtxt, ty: Ty, in_crate: InCrate) -> bool {
+    ty_is_local_constructor(ty, in_crate) ||
+        fundamental_ty(tcx, ty) && ty.walk_shallow().any(|t| ty_is_local(tcx, t, in_crate))
 }
 
 fn fundamental_ty(tcx: TyCtxt, ty: Ty) -> bool {
@@ -273,7 +307,16 @@ fn fundamental_ty(tcx: TyCtxt, ty: Ty) -> bool {
     }
 }
 
-fn ty_is_local_constructor(ty: Ty, infer_is_local: InferIsLocal)-> bool {
+fn def_id_is_local(def_id: DefId, in_crate: InCrate) -> bool {
+    match in_crate {
+        // The type is local to *this* crate - it will not be
+        // local in any other crate.
+        InCrate::Remote => false,
+        InCrate::Local => def_id.is_local()
+    }
+}
+
+fn ty_is_local_constructor(ty: Ty, in_crate: InCrate) -> bool {
     debug!("ty_is_local_constructor({:?})", ty);
 
     match ty.sty {
@@ -296,20 +339,20 @@ fn ty_is_local_constructor(ty: Ty, infer_is_local: InferIsLocal)-> bool {
             false
         }
 
-        ty::TyInfer(..) => {
-            infer_is_local.0
-        }
-
-        ty::TyAdt(def, _) => {
-            def.did.is_local()
-        }
+        ty::TyInfer(..) => match in_crate {
+            InCrate::Local => false,
+            // The inference variable might be unified with a local
+            // type in that remote crate.
+            InCrate::Remote => true,
+        },
 
-        ty::TyForeign(did) => {
-            did.is_local()
-        }
+        ty::TyAdt(def, _) => def_id_is_local(def.did, in_crate),
+        ty::TyForeign(did) => def_id_is_local(did, in_crate),
 
         ty::TyDynamic(ref tt, ..) => {
-            tt.principal().map_or(false, |p| p.def_id().is_local())
+            tt.principal().map_or(false, |p| {
+                def_id_is_local(p.def_id(), in_crate)
+            })
         }
 
         ty::TyError => {

src/librustc/traits/mod.rs

@@ -60,6 +60,13 @@ mod structural_impls;
 pub mod trans;
 mod util;
 
+// Whether to enable bug compatibility with issue #43355
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+pub enum IntercrateMode {
+    Issue43355,
+    Fixed
+}
+
 /// An `Obligation` represents some trait reference (e.g. `int:Eq`) for
 /// which the vtable must be found.  The process of finding a vtable is
 /// called "resolving" the `Obligation`. This process consists of