stop special-casing 'static in evaluate

lcnr · lcnr · commit 73c79cd80631 · 2023-03-22T11:37:57.000+01:00
diff --git a/compiler/rustc_infer/src/infer/freshen.rs b/compiler/rustc_infer/src/infer/freshen.rs
@@ -43,18 +43,16 @@ pub struct TypeFreshener<'a, 'tcx> {
     const_freshen_count: u32,
     ty_freshen_map: FxHashMap<ty::InferTy, Ty<'tcx>>,
     const_freshen_map: FxHashMap<ty::InferConst<'tcx>, ty::Const<'tcx>>,
-    keep_static: bool,
 }
 
 impl<'a, 'tcx> TypeFreshener<'a, 'tcx> {
-    pub fn new(infcx: &'a InferCtxt<'tcx>, keep_static: bool) -> TypeFreshener<'a, 'tcx> {
+    pub fn new(infcx: &'a InferCtxt<'tcx>) -> TypeFreshener<'a, 'tcx> {
         TypeFreshener {
             infcx,
             ty_freshen_count: 0,
             const_freshen_count: 0,
             ty_freshen_map: Default::default(),
             const_freshen_map: Default::default(),
-            keep_static,
         }
     }
 
@@ -121,18 +119,9 @@ impl<'a, 'tcx> TypeFolder<TyCtxt<'tcx>> for TypeFreshener<'a, 'tcx> {
             | ty::ReFree(_)
             | ty::ReVar(_)
             | ty::RePlaceholder(..)
+            | ty::ReStatic
             | ty::ReError(_)
-            | ty::ReErased => {
-                // replace all free regions with 'erased
-                self.interner().lifetimes.re_erased
-            }
-            ty::ReStatic => {
-                if self.keep_static {
-                    r
-                } else {
-                    self.interner().lifetimes.re_erased
-                }
-            }
+            | ty::ReErased => self.interner().lifetimes.re_erased,
         }
     }
 
diff --git a/compiler/rustc_infer/src/infer/mod.rs b/compiler/rustc_infer/src/infer/mod.rs
@@ -706,12 +706,7 @@ impl<'tcx> InferCtxt<'tcx> {
     }
 
     pub fn freshener<'b>(&'b self) -> TypeFreshener<'b, 'tcx> {
-        freshen::TypeFreshener::new(self, false)
-    }
-
-    /// Like `freshener`, but does not replace `'static` regions.
-    pub fn freshener_keep_static<'b>(&'b self) -> TypeFreshener<'b, 'tcx> {
-        freshen::TypeFreshener::new(self, true)
+        freshen::TypeFreshener::new(self)
     }
 
     pub fn unsolved_variables(&self) -> Vec<Ty<'tcx>> {
diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs
@@ -211,7 +211,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
     pub fn new(infcx: &'cx InferCtxt<'tcx>) -> SelectionContext<'cx, 'tcx> {
         SelectionContext {
             infcx,
-            freshener: infcx.freshener_keep_static(),
+            freshener: infcx.freshener(),
             intercrate_ambiguity_causes: None,
             query_mode: TraitQueryMode::Standard,
         }
@@ -769,14 +769,16 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                 }
 
                 ty::PredicateKind::Clause(ty::Clause::TypeOutlives(pred)) => {
-                    // A global type with no late-bound regions can only
-                    // contain the "'static" lifetime (any other lifetime
-                    // would either be late-bound or local), so it is guaranteed
-                    // to outlive any other lifetime
-                    if pred.0.is_global() && !pred.0.has_late_bound_vars() {
-                        Ok(EvaluatedToOk)
-                    } else {
+                    // A global type with no free lifetimes or generic parameters
+                    // outlives anything.
+                    if pred.0.has_free_regions()
+                        || pred.0.has_late_bound_regions()
+                        || pred.0.has_non_region_infer()
+                        || pred.0.has_non_region_infer()
+                    {
                         Ok(EvaluatedToOkModuloRegions)
+                    } else {
+                        Ok(EvaluatedToOk)
                     }
                 }
 
@@ -1824,6 +1826,12 @@ enum DropVictim {
     No,
 }
 
+impl DropVictim {
+    fn drop_if(should_drop: bool) -> DropVictim {
+        if should_drop { DropVictim::Yes } else { DropVictim::No }
+    }
+}
+
 /// ## Winnowing
 ///
 /// Winnowing is the process of attempting to resolve ambiguity by
@@ -1889,11 +1897,7 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
                     // or the current one if tied (they should both evaluate to the same answer). This is
                     // probably best characterized as a "hack", since we might prefer to just do our
                     // best to *not* create essentially duplicate candidates in the first place.
-                    if other.bound_vars().len() <= victim.bound_vars().len() {
-                        DropVictim::Yes
-                    } else {
-                        DropVictim::No
-                    }
+                    DropVictim::drop_if(other.bound_vars().len() <= victim.bound_vars().len())
                 } else if other.skip_binder().trait_ref == victim.skip_binder().trait_ref
                     && victim.skip_binder().constness == ty::BoundConstness::NotConst
                     && other.skip_binder().polarity == victim.skip_binder().polarity
@@ -1923,17 +1927,13 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
                 | ObjectCandidate(_)
                 | ProjectionCandidate(..),
             ) => {
-                if is_global(other_cand) {
-                    DropVictim::No
-                } else {
-                    // We have a where clause so don't go around looking
-                    // for impls. Arbitrarily give param candidates priority
-                    // over projection and object candidates.
-                    //
-                    // Global bounds from the where clause should be ignored
-                    // here (see issue #50825).
-                    DropVictim::Yes
-                }
+                // We have a where clause so don't go around looking
+                // for impls. Arbitrarily give param candidates priority
+                // over projection and object candidates.
+                //
+                // Global bounds from the where clause should be ignored
+                // here (see issue #50825).
+                DropVictim::drop_if(!is_global(other_cand))
             }
             (ObjectCandidate(_) | ProjectionCandidate(..), ParamCandidate(ref victim_cand)) => {
                 // Prefer these to a global where-clause bound
@@ -1955,18 +1955,16 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
             ) => {
                 // Prefer these to a global where-clause bound
                 // (see issue #50825).
-                if is_global(victim_cand) && other.evaluation.must_apply_modulo_regions() {
-                    DropVictim::Yes
-                } else {
-                    DropVictim::No
-                }
+                DropVictim::drop_if(
+                    is_global(victim_cand) && other.evaluation.must_apply_modulo_regions(),
+                )
             }
 
             (ProjectionCandidate(i, _), ProjectionCandidate(j, _))
             | (ObjectCandidate(i), ObjectCandidate(j)) => {
                 // Arbitrarily pick the lower numbered candidate for backwards
                 // compatibility reasons. Don't let this affect inference.
-                if i < j && !needs_infer { DropVictim::Yes } else { DropVictim::No }
+                DropVictim::drop_if(i < j && !needs_infer)
             }
             (ObjectCandidate(_), ProjectionCandidate(..))
             | (ProjectionCandidate(..), ObjectCandidate(_)) => {
@@ -2017,55 +2015,65 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
                     }
                 }
 
-                if other.evaluation.must_apply_considering_regions() {
-                    match tcx.impls_are_allowed_to_overlap(other_def, victim_def) {
-                        Some(ty::ImplOverlapKind::Permitted { marker: true }) => {
-                            // Subtle: If the predicate we are evaluating has inference
-                            // variables, do *not* allow discarding candidates due to
-                            // marker trait impls.
-                            //
-                            // Without this restriction, we could end up accidentally
-                            // constraining inference variables based on an arbitrarily
-                            // chosen trait impl.
-                            //
-                            // Imagine we have the following code:
-                            //
-                            // ```rust
-                            // #[marker] trait MyTrait {}
-                            // impl MyTrait for u8 {}
-                            // impl MyTrait for bool {}
-                            // ```
-                            //
-                            // And we are evaluating the predicate `<_#0t as MyTrait>`.
-                            //
-                            // During selection, we will end up with one candidate for each
-                            // impl of `MyTrait`. If we were to discard one impl in favor
-                            // of the other, we would be left with one candidate, causing
-                            // us to "successfully" select the predicate, unifying
-                            // _#0t with (for example) `u8`.
-                            //
-                            // However, we have no reason to believe that this unification
-                            // is correct - we've essentially just picked an arbitrary
-                            // *possibility* for _#0t, and required that this be the *only*
-                            // possibility.
-                            //
-                            // Eventually, we will either:
-                            // 1) Unify all inference variables in the predicate through
-                            // some other means (e.g. type-checking of a function). We will
-                            // then be in a position to drop marker trait candidates
-                            // without constraining inference variables (since there are
-                            // none left to constrain)
-                            // 2) Be left with some unconstrained inference variables. We
-                            // will then correctly report an inference error, since the
-                            // existence of multiple marker trait impls tells us nothing
-                            // about which one should actually apply.
-                            if needs_infer { DropVictim::No } else { DropVictim::Yes }
-                        }
-                        Some(_) => DropVictim::Yes,
-                        None => DropVictim::No,
+                match tcx.impls_are_allowed_to_overlap(other_def, victim_def) {
+                    // For #33140 the impl headers must be exactly equal, the trait must not have
+                    // any associated items and there are no where-clauses.
+                    //
+                    // We can just arbitrarily drop one of the impls.
+                    Some(ty::ImplOverlapKind::Issue33140) => {
+                        assert_eq!(other.evaluation, victim.evaluation);
+                        DropVictim::Yes
                     }
-                } else {
-                    DropVictim::No
+                    // For candidates which already reference errors it doesn't really
+                    // matter what we do 🤷
+                    Some(ty::ImplOverlapKind::Permitted { marker: false }) => {
+                        DropVictim::drop_if(other.evaluation.must_apply_considering_regions())
+                    }
+                    Some(ty::ImplOverlapKind::Permitted { marker: true }) => {
+                        // Subtle: If the predicate we are evaluating has inference
+                        // variables, do *not* allow discarding candidates due to
+                        // marker trait impls.
+                        //
+                        // Without this restriction, we could end up accidentally
+                        // constraining inference variables based on an arbitrarily
+                        // chosen trait impl.
+                        //
+                        // Imagine we have the following code:
+                        //
+                        // ```rust
+                        // #[marker] trait MyTrait {}
+                        // impl MyTrait for u8 {}
+                        // impl MyTrait for bool {}
+                        // ```
+                        //
+                        // And we are evaluating the predicate `<_#0t as MyTrait>`.
+                        //
+                        // During selection, we will end up with one candidate for each
+                        // impl of `MyTrait`. If we were to discard one impl in favor
+                        // of the other, we would be left with one candidate, causing
+                        // us to "successfully" select the predicate, unifying
+                        // _#0t with (for example) `u8`.
+                        //
+                        // However, we have no reason to believe that this unification
+                        // is correct - we've essentially just picked an arbitrary
+                        // *possibility* for _#0t, and required that this be the *only*
+                        // possibility.
+                        //
+                        // Eventually, we will either:
+                        // 1) Unify all inference variables in the predicate through
+                        // some other means (e.g. type-checking of a function). We will
+                        // then be in a position to drop marker trait candidates
+                        // without constraining inference variables (since there are
+                        // none left to constrain)
+                        // 2) Be left with some unconstrained inference variables. We
+                        // will then correctly report an inference error, since the
+                        // existence of multiple marker trait impls tells us nothing
+                        // about which one should actually apply.
+                        DropVictim::drop_if(
+                            !needs_infer && other.evaluation.must_apply_considering_regions(),
+                        )
+                    }
+                    None => DropVictim::No,
                 }
             }
 
diff --git a/tests/ui/marker_trait_attr/overlap-marker-trait-with-static-lifetime.rs b/tests/ui/marker_trait_attr/overlap-marker-trait-with-static-lifetime.rs
@@ -1,4 +1,8 @@
-// check-pass
+// known-bug: #89515
+//
+// The trait solver cannot deal with ambiguous marker trait impls
+// if there are lifetimes involved. As we must not special-case any
+// regions this does not work, even with 'static
 #![feature(marker_trait_attr)]
 
 #[marker]
diff --git a/tests/ui/marker_trait_attr/overlap-marker-trait-with-static-lifetime.stderr b/tests/ui/marker_trait_attr/overlap-marker-trait-with-static-lifetime.stderr
@@ -0,0 +1,31 @@
+error[E0283]: type annotations needed: cannot satisfy `&'static (): Marker`
+  --> $DIR/overlap-marker-trait-with-static-lifetime.rs:11:17
+   |
+LL | impl Marker for &'static () {}
+   |                 ^^^^^^^^^^^
+   |
+note: multiple `impl`s satisfying `&'static (): Marker` found
+  --> $DIR/overlap-marker-trait-with-static-lifetime.rs:11:1
+   |
+LL | impl Marker for &'static () {}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL | impl Marker for &'static () {}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error[E0283]: type annotations needed: cannot satisfy `&'static (): Marker`
+  --> $DIR/overlap-marker-trait-with-static-lifetime.rs:12:17
+   |
+LL | impl Marker for &'static () {}
+   |                 ^^^^^^^^^^^
+   |
+note: multiple `impl`s satisfying `&'static (): Marker` found
+  --> $DIR/overlap-marker-trait-with-static-lifetime.rs:11:1
+   |
+LL | impl Marker for &'static () {}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL | impl Marker for &'static () {}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0283`.
diff --git a/tests/ui/marker_trait_attr/overlapping-impl-1-modulo-regions.rs b/tests/ui/marker_trait_attr/overlapping-impl-1-modulo-regions.rs
@@ -1,9 +1,17 @@
-// check-pass
+// known-bug: #109481
+//
+// While the `T: Copy` is always applicable when checking
+// that the impl `impl<T: Copy> F for T {}` is well formed,
+// the old trait solver can only approximate this by checking
+// that there are no inference variables in the obligation and
+// no region constraints in the evaluation result.
+//
+// Because of this we end up with ambiguity here.
 #![feature(marker_trait_attr)]
 
 #[marker]
 pub trait F {}
-impl<T> F for T where T: Copy {}
-impl<T> F for T where T: 'static {}
+impl<T: Copy> F for T {}
+impl<T: 'static> F for T {}
 
 fn main() {}
diff --git a/tests/ui/marker_trait_attr/overlapping-impl-1-modulo-regions.stderr b/tests/ui/marker_trait_attr/overlapping-impl-1-modulo-regions.stderr
@@ -0,0 +1,14 @@
+error[E0310]: the parameter type `T` may not live long enough
+  --> $DIR/overlapping-impl-1-modulo-regions.rs:14:21
+   |
+LL | impl<T: Copy> F for T {}
+   |                     ^ ...so that the type `T` will meet its required lifetime bounds
+   |
+help: consider adding an explicit lifetime bound...
+   |
+LL | impl<T: Copy + 'static> F for T {}
+   |              +++++++++
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0310`.

Original file line number	Diff line number	Diff line change
`@@ -706,12 +706,7 @@ impl<'tcx> InferCtxt<'tcx> {`
`706`	`706`	`}`
`707`	`707`
`708`	`708`	`pub fn freshener<'b>(&'b self) -> TypeFreshener<'b, 'tcx> {`
`709`		`- freshen::TypeFreshener::new(self, false)`
`710`		`- }`
`711`		`-`
`712`		- /// Like `freshener`, but does not replace `'static` regions.
`713`		`- pub fn freshener_keep_static<'b>(&'b self) -> TypeFreshener<'b, 'tcx> {`
`714`		`- freshen::TypeFreshener::new(self, true)`
	`709`	`+ freshen::TypeFreshener::new(self)`
`715`	`710`	`}`
`716`	`711`
`717`	`712`	`pub fn unsolved_variables(&self) -> Vec<Ty<'tcx>> {`