From 0026d9678ce2ed580817c9070697d15ec9972840 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Thu, 30 Mar 2023 16:55:52 +0000 Subject: [PATCH] Revert "stop special-casing `'static` in evaluate" This reverts commit 73c79cd80631bf4cb4a20914d02aa08d0f80ba7f. --- compiler/rustc_infer/src/infer/freshen.rs | 17 +- compiler/rustc_infer/src/infer/mod.rs | 7 +- .../src/traits/select/mod.rs | 164 +++++++++--------- ...erlap-marker-trait-with-static-lifetime.rs | 6 +- ...p-marker-trait-with-static-lifetime.stderr | 31 ---- .../overlapping-impl-1-modulo-regions.rs | 14 +- .../overlapping-impl-1-modulo-regions.stderr | 14 -- 7 files changed, 102 insertions(+), 151 deletions(-) delete mode 100644 tests/ui/marker_trait_attr/overlap-marker-trait-with-static-lifetime.stderr delete mode 100644 tests/ui/marker_trait_attr/overlapping-impl-1-modulo-regions.stderr diff --git a/compiler/rustc_infer/src/infer/freshen.rs b/compiler/rustc_infer/src/infer/freshen.rs index d89f63e5c53e9..f09f93abf45d6 100644 --- a/compiler/rustc_infer/src/infer/freshen.rs +++ b/compiler/rustc_infer/src/infer/freshen.rs @@ -43,16 +43,18 @@ pub struct TypeFreshener<'a, 'tcx> { const_freshen_count: u32, ty_freshen_map: FxHashMap>, const_freshen_map: FxHashMap, ty::Const<'tcx>>, + keep_static: bool, } impl<'a, 'tcx> TypeFreshener<'a, 'tcx> { - pub fn new(infcx: &'a InferCtxt<'tcx>) -> TypeFreshener<'a, 'tcx> { + pub fn new(infcx: &'a InferCtxt<'tcx>, keep_static: bool) -> 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, } } @@ -119,9 +121,18 @@ impl<'a, 'tcx> TypeFolder> for TypeFreshener<'a, 'tcx> { | ty::ReFree(_) | ty::ReVar(_) | ty::RePlaceholder(..) - | ty::ReStatic | ty::ReError(_) - | ty::ReErased => self.interner().lifetimes.re_erased, + | 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 + } + } } } diff --git a/compiler/rustc_infer/src/infer/mod.rs b/compiler/rustc_infer/src/infer/mod.rs index 9903ffa90bae1..2ddca3c8b8c0e 100644 --- a/compiler/rustc_infer/src/infer/mod.rs +++ b/compiler/rustc_infer/src/infer/mod.rs @@ -713,7 +713,12 @@ impl<'tcx> InferCtxt<'tcx> { } pub fn freshener<'b>(&'b self) -> TypeFreshener<'b, 'tcx> { - freshen::TypeFreshener::new(self) + 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) } pub fn unsolved_variables(&self) -> Vec> { diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 3ed3dd2d20d84..d1d986af296a5 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -210,7 +210,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { pub fn new(infcx: &'cx InferCtxt<'tcx>) -> SelectionContext<'cx, 'tcx> { SelectionContext { infcx, - freshener: infcx.freshener(), + freshener: infcx.freshener_keep_static(), intercrate_ambiguity_causes: None, query_mode: TraitQueryMode::Standard, } @@ -769,16 +769,14 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } ty::PredicateKind::Clause(ty::Clause::TypeOutlives(pred)) => { - // 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 { + // 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 { + Ok(EvaluatedToOkModuloRegions) } } @@ -1826,12 +1824,6 @@ 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 @@ -1897,7 +1889,11 @@ 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. - DropVictim::drop_if(other.bound_vars().len() <= victim.bound_vars().len()) + if other.bound_vars().len() <= victim.bound_vars().len() { + DropVictim::Yes + } else { + DropVictim::No + } } 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 @@ -1927,13 +1923,17 @@ impl<'tcx> SelectionContext<'_, 'tcx> { | ObjectCandidate(_) | ProjectionCandidate(..), ) => { - // 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)) + 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 + } } (ObjectCandidate(_) | ProjectionCandidate(..), ParamCandidate(ref victim_cand)) => { // Prefer these to a global where-clause bound @@ -1955,16 +1955,18 @@ impl<'tcx> SelectionContext<'_, 'tcx> { ) => { // Prefer these to a global where-clause bound // (see issue #50825). - DropVictim::drop_if( - is_global(victim_cand) && other.evaluation.must_apply_modulo_regions(), - ) + if is_global(victim_cand) && other.evaluation.must_apply_modulo_regions() { + DropVictim::Yes + } else { + DropVictim::No + } } (ProjectionCandidate(i, _), ProjectionCandidate(j, _)) | (ObjectCandidate(i), ObjectCandidate(j)) => { // Arbitrarily pick the lower numbered candidate for backwards // compatibility reasons. Don't let this affect inference. - DropVictim::drop_if(i < j && !needs_infer) + if i < j && !needs_infer { DropVictim::Yes } else { DropVictim::No } } (ObjectCandidate(_), ProjectionCandidate(..)) | (ProjectionCandidate(..), ObjectCandidate(_)) => { @@ -2015,65 +2017,55 @@ impl<'tcx> SelectionContext<'_, 'tcx> { } } - 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 - } - // 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(), - ) + 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, } - None => DropVictim::No, + } else { + 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 index b9f1de7ec13a5..62aa22d41ed8e 100644 --- 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,8 +1,4 @@ -// 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 +// check-pass #![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 deleted file mode 100644 index fe4de540b513a..0000000000000 --- a/tests/ui/marker_trait_attr/overlap-marker-trait-with-static-lifetime.stderr +++ /dev/null @@ -1,31 +0,0 @@ -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 index 97a814f51eec5..a8f3db5f5b25b 100644 --- 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,17 +1,9 @@ -// known-bug: #109481 -// -// While the `T: Copy` is always applicable when checking -// that the impl `impl 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. +// check-pass #![feature(marker_trait_attr)] #[marker] pub trait F {} -impl F for T {} -impl F for T {} +impl F for T where T: Copy {} +impl F for T where T: 'static {} 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 deleted file mode 100644 index e713d1451cfd2..0000000000000 --- a/tests/ui/marker_trait_attr/overlapping-impl-1-modulo-regions.stderr +++ /dev/null @@ -1,14 +0,0 @@ -error[E0310]: the parameter type `T` may not live long enough - --> $DIR/overlapping-impl-1-modulo-regions.rs:14:21 - | -LL | impl F for T {} - | ^ ...so that the type `T` will meet its required lifetime bounds - | -help: consider adding an explicit lifetime bound... - | -LL | impl F for T {} - | +++++++++ - -error: aborting due to previous error - -For more information about this error, try `rustc --explain E0310`.