lint impls that will become incoherent when leak-check is removed

nikomatsakis · nikomatsakis · commit 363fabaf1fbd · 2020-02-06T15:58:01.000-05:00
diff --git a/src/librustc/infer/mod.rs b/src/librustc/infer/mod.rs
@@ -836,14 +836,15 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
         r
     }
 
-    /// Execute `f` then unroll any bindings it creates.
-    pub fn skip_leak_check<R, F>(&self, f: F) -> R
+    /// If `should_skip` is true, then execute `f` then unroll any bindings it creates.
+    pub fn probe_maybe_skip_leak_check<R, F>(&self, should_skip: bool, f: F) -> R
     where
         F: FnOnce(&CombinedSnapshot<'a, 'tcx>) -> R,
     {
         debug!("probe()");
         let snapshot = self.start_snapshot();
-        self.skip_leak_check.set(true);
+        let skip_leak_check = should_skip || self.skip_leak_check.get();
+        self.skip_leak_check.set(skip_leak_check);
         let r = f(&snapshot);
         self.rollback_to("probe", snapshot);
         r
diff --git a/src/librustc/traits/coherence.rs b/src/librustc/traits/coherence.rs
@@ -7,6 +7,7 @@
 use crate::infer::{CombinedSnapshot, InferOk};
 use crate::traits::select::IntercrateAmbiguityCause;
 use crate::traits::IntercrateMode;
+use crate::traits::SkipLeakCheck;
 use crate::traits::{self, Normalized, Obligation, ObligationCause, SelectionContext};
 use crate::ty::fold::TypeFoldable;
 use crate::ty::subst::Subst;
@@ -53,6 +54,7 @@ pub fn overlapping_impls<F1, F2, R>(
     impl1_def_id: DefId,
     impl2_def_id: DefId,
     intercrate_mode: IntercrateMode,
+    skip_leak_check: SkipLeakCheck,
     on_overlap: F1,
     no_overlap: F2,
 ) -> R
@@ -70,7 +72,7 @@ where
 
     let overlaps = tcx.infer_ctxt().enter(|infcx| {
         let selcx = &mut SelectionContext::intercrate(&infcx, intercrate_mode);
-        overlap(selcx, impl1_def_id, impl2_def_id).is_some()
+        overlap(selcx, skip_leak_check, impl1_def_id, impl2_def_id).is_some()
     });
 
     if !overlaps {
@@ -83,7 +85,7 @@ where
     tcx.infer_ctxt().enter(|infcx| {
         let selcx = &mut SelectionContext::intercrate(&infcx, intercrate_mode);
         selcx.enable_tracking_intercrate_ambiguity_causes();
-        on_overlap(overlap(selcx, impl1_def_id, impl2_def_id).unwrap())
+        on_overlap(overlap(selcx, skip_leak_check, impl1_def_id, impl2_def_id).unwrap())
     })
 }
 
@@ -113,12 +115,15 @@ fn with_fresh_ty_vars<'cx, 'tcx>(
 /// where-clauses)? If so, returns an `ImplHeader` that unifies the two impls.
 fn overlap<'cx, 'tcx>(
     selcx: &mut SelectionContext<'cx, 'tcx>,
+    skip_leak_check: SkipLeakCheck,
     a_def_id: DefId,
     b_def_id: DefId,
 ) -> Option<OverlapResult<'tcx>> {
     debug!("overlap(a_def_id={:?}, b_def_id={:?})", a_def_id, b_def_id);
 
-    selcx.infcx().probe(|snapshot| overlap_within_probe(selcx, a_def_id, b_def_id, snapshot))
+    selcx.infcx().probe_maybe_skip_leak_check(skip_leak_check.is_yes(), |snapshot| {
+        overlap_within_probe(selcx, a_def_id, b_def_id, snapshot)
+    })
 }
 
 fn overlap_within_probe(
@@ -146,7 +151,9 @@ fn overlap_within_probe(
         .eq_impl_headers(&a_impl_header, &b_impl_header)
     {
         Ok(InferOk { obligations, value: () }) => obligations,
-        Err(_) => return None,
+        Err(_) => {
+            return None;
+        }
     };
 
     debug!("overlap: unification check succeeded");
diff --git a/src/librustc/traits/mod.rs b/src/librustc/traits/mod.rs
@@ -83,6 +83,28 @@ pub enum IntercrateMode {
     Fixed,
 }
 
+/// Whether to skip the leak check, as part of a future compatibility warning step.
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+pub enum SkipLeakCheck {
+    Yes,
+    No,
+}
+
+impl SkipLeakCheck {
+    fn is_yes(self) -> bool {
+        self == SkipLeakCheck::Yes
+    }
+}
+
+/// The "default" for skip-leak-check corresponds to the current
+/// behavior (do not skip the leak check) -- not the behavior we are
+/// transitioning into.
+impl Default for SkipLeakCheck {
+    fn default() -> Self {
+        SkipLeakCheck::No
+    }
+}
+
 /// The mode that trait queries run in.
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 pub enum TraitQueryMode {
diff --git a/src/librustc/traits/specialize/mod.rs b/src/librustc/traits/specialize/mod.rs
@@ -19,6 +19,7 @@ use crate::ty::{self, TyCtxt, TypeFoldable};
 use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::struct_span_err;
 use rustc_hir::def_id::DefId;
+use rustc_session::lint::builtin::COHERENCE_LEAK_CHECK;
 use rustc_session::lint::builtin::ORDER_DEPENDENT_TRAIT_OBJECTS;
 use rustc_span::DUMMY_SP;
 
@@ -97,7 +98,7 @@ pub fn translate_substs<'a, 'tcx>(
                 |_| {
                     bug!(
                         "When translating substitutions for specialization, the expected \
-                          specialization failed to hold"
+                         specialization failed to hold"
                     )
                 },
             )
@@ -268,7 +269,7 @@ fn fulfill_implication<'a, 'tcx>(
                 // no dice!
                 debug!(
                     "fulfill_implication: for impls on {:?} and {:?}, \
-                        could not fulfill: {:?} given {:?}",
+                     could not fulfill: {:?} given {:?}",
                     source_trait_ref, target_trait_ref, errors, param_env.caller_bounds
                 );
                 Err(())
@@ -342,6 +343,7 @@ pub(super) fn specialization_graph_provider(
                             FutureCompatOverlapErrorKind::Issue33140 => {
                                 ORDER_DEPENDENT_TRAIT_OBJECTS
                             }
+                            FutureCompatOverlapErrorKind::LeakCheck => COHERENCE_LEAK_CHECK,
                         };
                         tcx.struct_span_lint_hir(
                             lint,
diff --git a/src/librustc/traits/specialize/specialization_graph.rs b/src/librustc/traits/specialize/specialization_graph.rs
@@ -11,6 +11,7 @@ pub use rustc::traits::types::specialization_graph::*;
 pub enum FutureCompatOverlapErrorKind {
     Issue43355,
     Issue33140,
+    LeakCheck,
 }
 
 #[derive(Debug)]
@@ -111,6 +112,7 @@ impl<'tcx> Children {
                 possible_sibling,
                 impl_def_id,
                 traits::IntercrateMode::Issue43355,
+                traits::SkipLeakCheck::default(),
                 |overlap| {
                     if let Some(overlap_kind) =
                         tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling)
@@ -161,6 +163,7 @@ impl<'tcx> Children {
                         possible_sibling,
                         impl_def_id,
                         traits::IntercrateMode::Fixed,
+                        traits::SkipLeakCheck::default(),
                         |overlap| {
                             last_lint = Some(FutureCompatOverlapError {
                                 error: overlap_error(overlap),
@@ -169,6 +172,23 @@ impl<'tcx> Children {
                         },
                         || (),
                     );
+
+                    if last_lint.is_none() {
+                        traits::overlapping_impls(
+                            tcx,
+                            possible_sibling,
+                            impl_def_id,
+                            traits::IntercrateMode::Fixed,
+                            traits::SkipLeakCheck::Yes,
+                            |overlap| {
+                                last_lint = Some(FutureCompatOverlapError {
+                                    error: overlap_error(overlap),
+                                    kind: FutureCompatOverlapErrorKind::LeakCheck,
+                                });
+                            },
+                            || (),
+                        );
+                    }
                 }
 
                 // no overlap (error bailed already via ?)
@@ -247,7 +267,7 @@ impl<'tcx> Graph {
         if trait_ref.references_error() {
             debug!(
                 "insert: inserting dummy node for erroneous TraitRef {:?}, \
-                    impl_def_id={:?}, trait_def_id={:?}",
+                 impl_def_id={:?}, trait_def_id={:?}",
                 trait_ref, impl_def_id, trait_def_id
             );
 
@@ -326,7 +346,7 @@ impl<'tcx> Graph {
         if self.parent.insert(child, parent).is_some() {
             bug!(
                 "When recording an impl from the crate store, information about its parent \
-                  was already present."
+                 was already present."
             );
         }
 
diff --git a/src/librustc_session/lint/builtin.rs b/src/librustc_session/lint/builtin.rs
@@ -260,6 +260,16 @@ declare_lint! {
     };
 }
 
+declare_lint! {
+    pub COHERENCE_LEAK_CHECK,
+    Deny,
+    "distinct impls distinguished only by the leak-check code",
+    @future_incompatible = FutureIncompatibleInfo {
+        reference: "issue #56105 <https://github.com/rust-lang/rust/issues/56105>",
+        edition: None,
+    };
+}
+
 declare_lint! {
     pub DEPRECATED,
     Warn,
@@ -509,6 +519,7 @@ declare_lint_pass! {
         MISSING_FRAGMENT_SPECIFIER,
         LATE_BOUND_LIFETIME_ARGUMENTS,
         ORDER_DEPENDENT_TRAIT_OBJECTS,
+        COHERENCE_LEAK_CHECK,
         DEPRECATED,
         UNUSED_UNSAFE,
         UNUSED_MUT,
diff --git a/src/librustc_typeck/coherence/inherent_impls_overlap.rs b/src/librustc_typeck/coherence/inherent_impls_overlap.rs
@@ -1,5 +1,5 @@
 use crate::namespace::Namespace;
-use rustc::traits::{self, IntercrateMode};
+use rustc::traits::{self, IntercrateMode, SkipLeakCheck};
 use rustc::ty::TyCtxt;
 use rustc_errors::struct_span_err;
 use rustc_hir as hir;
@@ -76,6 +76,9 @@ impl InherentOverlapChecker<'tcx> {
                     impl1_def_id,
                     impl2_def_id,
                     IntercrateMode::Issue43355,
+                    // We go ahead and just skip the leak check for
+                    // inherent impls without warning.
+                    SkipLeakCheck::Yes,
                     |overlap| {
                         self.check_for_common_items_in_impls(impl1_def_id, impl2_def_id, overlap);
                         false
diff --git a/src/test/ui/coherence/coherence-inherited-subtyping.old.stderr b/src/test/ui/coherence/coherence-inherited-subtyping.old.stderr
@@ -0,0 +1,14 @@
+error[E0592]: duplicate definitions with name `method1`
+  --> $DIR/coherence-inherited-subtyping.rs:14:5
+   |
+LL |     fn method1(&self) {}
+   |     ^^^^^^^^^^^^^^^^^^^^ duplicate definitions for `method1`
+...
+LL |     fn method1(&self) {}
+   |     -------------------- other definition for `method1`
+   |
+   = note: this behavior recently changed as a result of a bug fix; see rust-lang/rust#56105 for details
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0592`.
diff --git a/src/test/ui/coherence/coherence-inherited-subtyping.re.stderr b/src/test/ui/coherence/coherence-inherited-subtyping.re.stderr
@@ -0,0 +1,14 @@
+error[E0592]: duplicate definitions with name `method1`
+  --> $DIR/coherence-inherited-subtyping.rs:14:5
+   |
+LL |     fn method1(&self) {}
+   |     ^^^^^^^^^^^^^^^^^^^^ duplicate definitions for `method1`
+...
+LL |     fn method1(&self) {}
+   |     -------------------- other definition for `method1`
+   |
+   = note: this behavior recently changed as a result of a bug fix; see rust-lang/rust#56105 for details
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0592`.
diff --git a/src/test/ui/coherence/coherence-inherited-subtyping.rs b/src/test/ui/coherence/coherence-inherited-subtyping.rs
@@ -0,0 +1,21 @@
+// Test that two distinct impls which match subtypes of one another
+// yield coherence errors (or not) depending on the variance.
+//
+// Note: This scenario is currently accepted, but as part of the
+// universe transition (#56105) may eventually become an error.
+
+// revisions: old re
+
+struct Foo<T> {
+    t: T,
+}
+
+impl Foo<for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8> {
+    fn method1(&self) {} //~ ERROR duplicate definitions with name `method1`
+}
+
+impl Foo<for<'a> fn(&'a u8, &'a u8) -> &'a u8> {
+    fn method1(&self) {}
+}
+
+fn main() {}
diff --git a/src/test/ui/coherence/coherence-subtyping.old.stderr b/src/test/ui/coherence/coherence-subtyping.old.stderr
@@ -0,0 +1,16 @@
+error: conflicting implementations of trait `TheTrait` for type `for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8`:
+  --> $DIR/coherence-subtyping.rs:15:1
+   |
+LL | impl TheTrait for for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8 {}
+   | ---------------------------------------------------------- first implementation here
+LL | 
+LL | impl TheTrait for for<'a> fn(&'a u8, &'a u8) -> &'a u8 {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8`
+   |
+   = note: `#[deny(coherence_leak_check)]` on by default
+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+   = note: for more information, see issue #56105 <https://github.com/rust-lang/rust/issues/56105>
+   = note: this behavior recently changed as a result of a bug fix; see rust-lang/rust#56105 for details
+
+error: aborting due to previous error
+
diff --git a/src/test/ui/coherence/coherence-subtyping.re.stderr b/src/test/ui/coherence/coherence-subtyping.re.stderr
@@ -0,0 +1,16 @@
+error: conflicting implementations of trait `TheTrait` for type `for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8`:
+  --> $DIR/coherence-subtyping.rs:15:1
+   |
+LL | impl TheTrait for for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8 {}
+   | ---------------------------------------------------------- first implementation here
+LL | 
+LL | impl TheTrait for for<'a> fn(&'a u8, &'a u8) -> &'a u8 {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8`
+   |
+   = note: `#[deny(coherence_leak_check)]` on by default
+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+   = note: for more information, see issue #56105 <https://github.com/rust-lang/rust/issues/56105>
+   = note: this behavior recently changed as a result of a bug fix; see rust-lang/rust#56105 for details
+
+error: aborting due to previous error
+
diff --git a/src/test/ui/coherence/coherence-subtyping.rs b/src/test/ui/coherence/coherence-subtyping.rs
@@ -5,16 +5,18 @@
 // universe transition (#56105) may eventually become an error.
 
 // revisions: old re
-// build-pass (FIXME(62277): could be check-pass?)
 
 trait TheTrait {
-    fn foo(&self) { }
+    fn foo(&self) {}
 }
 
-impl TheTrait for for<'a,'b> fn(&'a u8, &'b u8) -> &'a u8 {
-}
+impl TheTrait for for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8 {}
 
 impl TheTrait for for<'a> fn(&'a u8, &'a u8) -> &'a u8 {
+    //[re]~^ ERROR conflicting implementation
+    //[re]~^^ WARNING this was previously accepted by the compiler but is being phased out
+    //[old]~^^^ ERROR conflicting implementation
+    //[old]~^^^^ WARNING this was previously accepted by the compiler but is being phased out
 }
 
-fn main() { }
+fn main() {}

Original file line number	Diff line number	Diff line change
`@@ -836,14 +836,15 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {`
`836`	`836`	`r`
`837`	`837`	`}`
`838`	`838`
`839`		- /// Execute `f` then unroll any bindings it creates.
`840`		`- pub fn skip_leak_check<R, F>(&self, f: F) -> R`
	`839`	+ /// If `should_skip` is true, then execute `f` then unroll any bindings it creates.
	`840`	`+ pub fn probe_maybe_skip_leak_check<R, F>(&self, should_skip: bool, f: F) -> R`
`841`	`841`	`where`
`842`	`842`	`F: FnOnce(&CombinedSnapshot<'a, 'tcx>) -> R,`
`843`	`843`	`{`
`844`	`844`	`debug!("probe()");`
`845`	`845`	`let snapshot = self.start_snapshot();`
`846`		`- self.skip_leak_check.set(true);`
	`846`	`+ let skip_leak_check = should_skip \|\| self.skip_leak_check.get();`
	`847`	`+ self.skip_leak_check.set(skip_leak_check);`
`847`	`848`	`let r = f(&snapshot);`
`848`	`849`	`self.rollback_to("probe", snapshot);`
`849`	`850`	`r`