From e41380a407002b2fa31cfb93d715d66059aa43b5 Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 11 Apr 2023 12:08:23 +0200 Subject: [PATCH 1/2] support fulfillment with non-fatal overflow --- .../src/traits/engine.rs | 27 ++++++++ .../src/traits/fulfill.rs | 69 +++++++++++-------- 2 files changed, 68 insertions(+), 28 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/engine.rs b/compiler/rustc_trait_selection/src/traits/engine.rs index 8acc31cd410bd..2676f35bc3c51 100644 --- a/compiler/rustc_trait_selection/src/traits/engine.rs +++ b/compiler/rustc_trait_selection/src/traits/engine.rs @@ -27,6 +27,13 @@ use rustc_span::Span; pub trait TraitEngineExt<'tcx> { fn new(tcx: TyCtxt<'tcx>) -> Box; fn new_in_snapshot(tcx: TyCtxt<'tcx>) -> Box; + /// Creates a new fulfillment context which does not abort compilation + /// on overflow. + /// + /// WARNING: Overflow will be returned as an error in `select_where_possible` + /// even though the overflow may not be the final result for the given obligation, + /// so you have to be incredibly careful when using `select_where_possible. + fn with_query_mode_canonical(tcx: TyCtxt<'tcx>) -> Box; } impl<'tcx> TraitEngineExt<'tcx> for dyn TraitEngine<'tcx> { @@ -45,6 +52,14 @@ impl<'tcx> TraitEngineExt<'tcx> for dyn TraitEngine<'tcx> { TraitSolver::Next => Box::new(NextFulfillmentCtxt::new()), } } + + fn with_query_mode_canonical(tcx: TyCtxt<'tcx>) -> Box { + match tcx.sess.opts.unstable_opts.trait_solver { + TraitSolver::Classic => Box::new(FulfillmentContext::with_query_mode_canonical()), + TraitSolver::Chalk => Box::new(ChalkFulfillmentContext::new()), + TraitSolver::Next => Box::new(NextFulfillmentCtxt::new()), + } + } } /// Used if you want to have pleasant experience when dealing @@ -63,6 +78,18 @@ impl<'a, 'tcx> ObligationCtxt<'a, 'tcx> { Self { infcx, engine: RefCell::new(>::new_in_snapshot(infcx.tcx)) } } + /// You have to be incredibly careful when using this method. + /// + /// Used in places where we have to deal with overflow in a non-fatal way. + /// Note that by using this the trait solver ends up being incomplete in a + /// may way because overflow is returned as a hard error instead of ambiguity. + pub fn with_query_mode_canonical(infcx: &'a InferCtxt<'tcx>) -> Self { + Self { + infcx, + engine: RefCell::new(>::with_query_mode_canonical(infcx.tcx)), + } + } + pub fn register_obligation(&self, obligation: PredicateObligation<'tcx>) { self.engine.borrow_mut().register_predicate_obligation(self.infcx, obligation); } diff --git a/compiler/rustc_trait_selection/src/traits/fulfill.rs b/compiler/rustc_trait_selection/src/traits/fulfill.rs index 07e31e87bfb46..8153cf3770308 100644 --- a/compiler/rustc_trait_selection/src/traits/fulfill.rs +++ b/compiler/rustc_trait_selection/src/traits/fulfill.rs @@ -12,7 +12,6 @@ use rustc_middle::ty::subst::SubstsRef; use rustc_middle::ty::{self, Binder, Const, TypeVisitableExt}; use std::marker::PhantomData; -use super::const_evaluatable; use super::project::{self, ProjectAndUnifyResult}; use super::select::SelectionContext; use super::wf; @@ -22,6 +21,7 @@ use super::CodeSelectionError; use super::EvaluationResult; use super::PredicateObligation; use super::Unimplemented; +use super::{const_evaluatable, TraitQueryMode}; use super::{FulfillmentError, FulfillmentErrorCode}; use crate::traits::project::PolyProjectionObligation; @@ -64,6 +64,8 @@ pub struct FulfillmentContext<'tcx> { // a snapshot (they don't *straddle* a snapshot, so there // is no trouble there). usable_in_snapshot: bool, + + query_mode: TraitQueryMode, } #[derive(Clone, Debug)] @@ -80,38 +82,30 @@ pub struct PendingPredicateObligation<'tcx> { #[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))] static_assert_size!(PendingPredicateObligation<'_>, 72); -impl<'a, 'tcx> FulfillmentContext<'tcx> { +impl<'tcx> FulfillmentContext<'tcx> { /// Creates a new fulfillment context. pub(super) fn new() -> FulfillmentContext<'tcx> { - FulfillmentContext { predicates: ObligationForest::new(), usable_in_snapshot: false } + FulfillmentContext { + predicates: ObligationForest::new(), + usable_in_snapshot: false, + query_mode: TraitQueryMode::Standard, + } } pub(super) fn new_in_snapshot() -> FulfillmentContext<'tcx> { - FulfillmentContext { predicates: ObligationForest::new(), usable_in_snapshot: true } + FulfillmentContext { + predicates: ObligationForest::new(), + usable_in_snapshot: true, + query_mode: TraitQueryMode::Standard, + } } - /// Attempts to select obligations using `selcx`. - fn select(&mut self, selcx: SelectionContext<'a, 'tcx>) -> Vec> { - let span = debug_span!("select", obligation_forest_size = ?self.predicates.len()); - let _enter = span.enter(); - - // Process pending obligations. - let outcome: Outcome<_, _> = - self.predicates.process_obligations(&mut FulfillProcessor { selcx }); - - // FIXME: if we kept the original cache key, we could mark projection - // obligations as complete for the projection cache here. - - let errors: Vec> = - outcome.errors.into_iter().map(to_fulfillment_error).collect(); - - debug!( - "select({} predicates remaining, {} errors) done", - self.predicates.len(), - errors.len() - ); - - errors + pub(super) fn with_query_mode_canonical() -> FulfillmentContext<'tcx> { + FulfillmentContext { + predicates: ObligationForest::new(), + usable_in_snapshot: false, + query_mode: TraitQueryMode::Canonical, + } } } @@ -138,8 +132,27 @@ impl<'tcx> TraitEngine<'tcx> for FulfillmentContext<'tcx> { } fn select_where_possible(&mut self, infcx: &InferCtxt<'tcx>) -> Vec> { - let selcx = SelectionContext::new(infcx); - self.select(selcx) + let span = debug_span!("select", obligation_forest_size = ?self.predicates.len()); + let selcx = SelectionContext::with_query_mode(infcx, self.query_mode); + let _enter = span.enter(); + + // Process pending obligations. + let outcome: Outcome<_, _> = + self.predicates.process_obligations(&mut FulfillProcessor { selcx }); + + // FIXME: if we kept the original cache key, we could mark projection + // obligations as complete for the projection cache here. + + let errors: Vec> = + outcome.errors.into_iter().map(to_fulfillment_error).collect(); + + debug!( + "select({} predicates remaining, {} errors) done", + self.predicates.len(), + errors.len() + ); + + errors } fn drain_unstalled_obligations( From 97c5793e1c8590b79f38c748e397d504855ced18 Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 11 Apr 2023 12:11:03 +0200 Subject: [PATCH 2/2] filter trivial predicates when checking methods --- .../src/check/compare_impl_item.rs | 90 +++++++++++++++++-- tests/ui/implied-bounds/issue-108544-1.rs | 21 +++++ tests/ui/implied-bounds/issue-108544-2.rs | 19 ++++ .../trait-where-clause-implied.rs | 15 ++++ 4 files changed, 139 insertions(+), 6 deletions(-) create mode 100644 tests/ui/implied-bounds/issue-108544-1.rs create mode 100644 tests/ui/implied-bounds/issue-108544-2.rs create mode 100644 tests/ui/implied-bounds/trait-where-clause-implied.rs diff --git a/compiler/rustc_hir_analysis/src/check/compare_impl_item.rs b/compiler/rustc_hir_analysis/src/check/compare_impl_item.rs index f6c2004c4a672..268617535dd98 100644 --- a/compiler/rustc_hir_analysis/src/check/compare_impl_item.rs +++ b/compiler/rustc_hir_analysis/src/check/compare_impl_item.rs @@ -12,7 +12,7 @@ use rustc_hir::{GenericParamKind, ImplItemKind}; use rustc_infer::infer::outlives::env::OutlivesEnvironment; use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind}; use rustc_infer::infer::{self, InferCtxt, TyCtxtInferExt}; -use rustc_infer::traits::util; +use rustc_infer::traits::{util, Obligation}; use rustc_middle::ty::error::{ExpectedFound, TypeError}; use rustc_middle::ty::util::ExplicitSelf; use rustc_middle::ty::{ @@ -190,15 +190,13 @@ fn compare_method_predicate_entailment<'tcx>( .map(|(predicate, _)| predicate), ); + let caller_bounds = filter_trivial_predicates(tcx, hybrid_preds.predicates); + // Construct trait parameter environment and then shift it into the placeholder viewpoint. // The key step here is to update the caller_bounds's predicates to be // the new hybrid bounds we computed. let normalize_cause = traits::ObligationCause::misc(impl_m_span, impl_m_def_id); - let param_env = ty::ParamEnv::new( - tcx.mk_predicates(&hybrid_preds.predicates), - Reveal::UserFacing, - hir::Constness::NotConst, - ); + let param_env = ty::ParamEnv::new(caller_bounds, Reveal::UserFacing, hir::Constness::NotConst); let param_env = traits::normalize_param_env_or_error(tcx, param_env, normalize_cause); let infcx = &tcx.infer_ctxt().build(); @@ -2080,3 +2078,83 @@ fn assoc_item_kind_str(impl_item: &ty::AssocItem) -> &'static str { ty::AssocKind::Type => "type", } } + +// FIXME(-Ztrait-solver=next): This hack should be unnecessary with the new trait +// solver as it is better at dealing with ambiguity. +// +// Even if this code isn't completely trivial, it only removes predicates so it +// should always remain sound. +#[instrument(level = "debug", skip(tcx, predicates))] +fn filter_trivial_predicates<'tcx>( + tcx: TyCtxt<'tcx>, + mut predicates: Vec>, +) -> &'tcx ty::List> { + // We start with a bad approximation of whether a predicate is trivial and put all + // non-trivial predicates into the environment used when checking whether the + // remaining ones are trivial. + let mut non_trivial_predicates = Vec::new(); + for &predicate in predicates.iter() { + if !may_be_trivial_predicate(predicate) { + non_trivial_predicates.push(predicate); + } + } + + let non_trivial_predicates = tcx.mk_predicates(&non_trivial_predicates); + if non_trivial_predicates.len() == predicates.len() { + non_trivial_predicates + } else { + let param_env = + ty::ParamEnv::new(non_trivial_predicates, Reveal::UserFacing, hir::Constness::NotConst); + predicates.retain(|&p| !is_trivial_predicate(tcx, param_env, p)); + tcx.mk_predicates(&predicates) + } +} + +// A bad approximation of whether a predicate is trivial. Used to put all non-trivial +// predicates into the environment while checking whether the remaining ones are trivial. +fn may_be_trivial_predicate<'tcx>(predicate: ty::Predicate<'tcx>) -> bool { + // We only consider trait and projection predicates which don't have a parameter + // as a self type as potentially non-trivial. + match predicate.kind().skip_binder() { + ty::PredicateKind::Clause(ty::Clause::Trait(predicate)) => { + !matches!(predicate.self_ty().kind(), ty::Param(_)) + } + ty::PredicateKind::Clause(ty::Clause::Projection(predicate)) => { + !matches!(predicate.self_ty().kind(), ty::Param(_)) + } + _ => false, + } +} + +/// Returns whether `predicate` is trivially provable in the empty environment. +/// +/// While it's definitely trivial if we return `Yes`, this function is incomplete, +/// so it may incorrectly return `No` even though the `predicate` is actually trivial. +#[instrument(level = "debug", skip(tcx), ret)] +fn is_trivial_predicate<'tcx>( + tcx: TyCtxt<'tcx>, + param_env: ty::ParamEnv<'tcx>, + predicate: ty::Predicate<'tcx>, +) -> bool { + if !may_be_trivial_predicate(predicate) { + return false; + } + + let infcx = tcx.infer_ctxt().build(); + // HACK: This can overflow and we must not abort here as that would break existing + // crates, most notably `typenum`. + // + // To deal with this we change overflow to only abort trait solving without + // aborting compilation. This means that this code isn't complete and may + // incorrectly error which is acceptable as this is just a best effort. + let ocx = ObligationCtxt::with_query_mode_canonical(&infcx); + let obligation = Obligation::new(tcx, ObligationCause::dummy(), param_env, predicate); + ocx.register_obligation(obligation); + if ocx.select_all_or_error().is_empty() { + let outlives_env = OutlivesEnvironment::new(param_env); + infcx.process_registered_region_obligations(outlives_env.region_bound_pairs(), param_env); + infcx.resolve_regions(&outlives_env).is_empty() + } else { + false + } +} diff --git a/tests/ui/implied-bounds/issue-108544-1.rs b/tests/ui/implied-bounds/issue-108544-1.rs new file mode 100644 index 0000000000000..955209e5d7659 --- /dev/null +++ b/tests/ui/implied-bounds/issue-108544-1.rs @@ -0,0 +1,21 @@ + +// check-pass + +use std::borrow::Cow; + +pub trait Trait { + fn method(self) -> Option> + where + Self: Sized; +} + +impl<'a> Trait for Cow<'a, str> { + // We have to check `WF(return-type)` which requires `Cow<'static, str>: Sized`. + // If we use the `Self: Sized` bound from the trait method we end up equating + // `Cow<'a, str>` with `Cow<'static, str>`, causing an error. + fn method(self) -> Option> { + None + } +} + +fn main() {} diff --git a/tests/ui/implied-bounds/issue-108544-2.rs b/tests/ui/implied-bounds/issue-108544-2.rs new file mode 100644 index 0000000000000..68a554bb7b379 --- /dev/null +++ b/tests/ui/implied-bounds/issue-108544-2.rs @@ -0,0 +1,19 @@ +// check-pass + +// Similar to issue-108544.rs except that we have a generic `T` which +// previously caused an overeager fast-path to trigger. +use std::borrow::Cow; + +pub trait Trait { + fn method(self) -> Option> + where + Self: Sized; +} + +impl<'a, T: Clone> Trait for Cow<'a, T> { + fn method(self) -> Option> { + None + } +} + +fn main() {} diff --git a/tests/ui/implied-bounds/trait-where-clause-implied.rs b/tests/ui/implied-bounds/trait-where-clause-implied.rs new file mode 100644 index 0000000000000..5f9ab66d3c896 --- /dev/null +++ b/tests/ui/implied-bounds/trait-where-clause-implied.rs @@ -0,0 +1,15 @@ +// check-pass + +pub trait Trait<'a, 'b> { + fn method(self, _: &'static &'static ()) + where + 'a: 'b; +} + +impl<'a> Trait<'a, 'static> for () { + // On first glance, this seems like we have the extra implied bound that + // `'a: 'static`, but we know this from the trait method where clause. + fn method(self, _: &'static &'a ()) {} +} + +fn main() {}