Rollup merge of #68297 - Aaron1011:fix/new-const-prop-bounds, r=oli-obk

Filter and test predicates using `normalize_and_test_predicates` for const-prop

Fixes #68264

Previously, I attempted to use
`substitute_normalize_and_test_predicates` to detect unsatisfiable
bounds. Unfortunately, since const-prop runs in a generic environment
(we don't have any of the function's generic parameters substituted),
this could lead to cycle errors when attempting to normalize predicates.

This check is replaced with a more precise check. We now only call
`normalize_and_test_predicates` on predicates that have the possibility
of being proved unsatisfiable - that is, predicates that don't depend
on anything local to the function (e.g. generic parameters). This
ensures that we don't hit cycle errors when we normalize said
predicates, while still ensuring that we detect unsatisfiable
predicates.

I haven't been able to come up with a minimization of the Diesel issue - however, I've verified that it compiles successfully.

src/librustc/mir/mono.rs

@@ -1,7 +1,6 @@
 use crate::dep_graph::{DepConstructor, DepNode, WorkProduct, WorkProductId};
 use crate::ich::{Fingerprint, NodeIdHashingMode, StableHashingContext};
 use crate::session::config::OptLevel;
-use crate::traits::TraitQueryMode;
 use crate::ty::print::obsolete::DefPathBasedNames;
 use crate::ty::{subst::InternalSubsts, Instance, InstanceDef, SymbolName, TyCtxt};
 use rustc_data_structures::base_n;
@@ -168,9 +167,7 @@ impl<'tcx> MonoItem<'tcx> {
             MonoItem::GlobalAsm(..) => return true,
         };
 
-        // We shouldn't encounter any overflow here, so we use TraitQueryMode::Standard\
-        // to report an error if overflow somehow occurs.
-        tcx.substitute_normalize_and_test_predicates((def_id, &substs, TraitQueryMode::Standard))
+        tcx.substitute_normalize_and_test_predicates((def_id, &substs))
     }
 
     pub fn to_string(&self, tcx: TyCtxt<'tcx>, debug: bool) -> String {

src/librustc/query/mod.rs

@@ -1156,11 +1156,11 @@ rustc_queries! {
             desc { "normalizing `{:?}`", goal }
         }
 
-        query substitute_normalize_and_test_predicates(key: (DefId, SubstsRef<'tcx>, traits::TraitQueryMode)) -> bool {
+        query substitute_normalize_and_test_predicates(key: (DefId, SubstsRef<'tcx>)) -> bool {
             no_force
             desc { |tcx|
-                "testing substituted normalized predicates in mode {:?}:`{}`",
-                key.2, tcx.def_path_str(key.0)
+                "testing substituted normalized predicates:`{}`",
+                tcx.def_path_str(key.0)
             }
         }
 

src/librustc/traits/fulfill.rs

@@ -16,7 +16,6 @@ use super::CodeSelectionError;
 use super::{ConstEvalFailure, Unimplemented};
 use super::{FulfillmentError, FulfillmentErrorCode};
 use super::{ObligationCause, PredicateObligation};
-use crate::traits::TraitQueryMode;
 
 impl<'tcx> ForestObligation for PendingPredicateObligation<'tcx> {
     type Predicate = ty::Predicate<'tcx>;
@@ -63,9 +62,6 @@ pub struct FulfillmentContext<'tcx> {
     // a snapshot (they don't *straddle* a snapshot, so there
     // is no trouble there).
     usable_in_snapshot: bool,
-
-    // The `TraitQueryMode` used when constructing a `SelectionContext`
-    query_mode: TraitQueryMode,
 }
 
 #[derive(Clone, Debug)]
@@ -79,26 +75,12 @@ pub struct PendingPredicateObligation<'tcx> {
 static_assert_size!(PendingPredicateObligation<'_>, 136);
 
 impl<'a, 'tcx> FulfillmentContext<'tcx> {
-    /// Creates a new fulfillment context with `TraitQueryMode::Standard`
-    /// You almost always want to use this instead of `with_query_mode`
+    /// Creates a new fulfillment context.
     pub fn new() -> FulfillmentContext<'tcx> {
         FulfillmentContext {
             predicates: ObligationForest::new(),
             register_region_obligations: true,
             usable_in_snapshot: false,
-            query_mode: TraitQueryMode::Standard,
-        }
-    }
-
-    /// Creates a new fulfillment context with the specified query mode.
-    /// This should only be used when you want to ignore overflow,
-    /// rather than reporting it as an error.
-    pub fn with_query_mode(query_mode: TraitQueryMode) -> FulfillmentContext<'tcx> {
-        FulfillmentContext {
-            predicates: ObligationForest::new(),
-            register_region_obligations: true,
-            usable_in_snapshot: false,
-            query_mode,
         }
     }
 
@@ -107,7 +89,6 @@ impl<'a, 'tcx> FulfillmentContext<'tcx> {
             predicates: ObligationForest::new(),
             register_region_obligations: true,
             usable_in_snapshot: true,
-            query_mode: TraitQueryMode::Standard,
         }
     }
 
@@ -116,7 +97,6 @@ impl<'a, 'tcx> FulfillmentContext<'tcx> {
             predicates: ObligationForest::new(),
             register_region_obligations: false,
             usable_in_snapshot: false,
-            query_mode: TraitQueryMode::Standard,
         }
     }
 
@@ -237,7 +217,7 @@ impl<'tcx> TraitEngine<'tcx> for FulfillmentContext<'tcx> {
         &mut self,
         infcx: &InferCtxt<'_, 'tcx>,
     ) -> Result<(), Vec<FulfillmentError<'tcx>>> {
-        let mut selcx = SelectionContext::with_query_mode(infcx, self.query_mode);
+        let mut selcx = SelectionContext::new(infcx);
         self.select(&mut selcx)
     }
 

src/librustc/traits/mod.rs

@@ -95,7 +95,7 @@ pub enum IntercrateMode {
 }
 
 /// The mode that trait queries run in.
-#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash, HashStable)]
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
 pub enum TraitQueryMode {
     // Standard/un-canonicalized queries get accurate
     // spans etc. passed in and hence can do reasonable
@@ -1014,17 +1014,16 @@ where
 /// environment. If this returns false, then either normalize
 /// encountered an error or one of the predicates did not hold. Used
 /// when creating vtables to check for unsatisfiable methods.
-fn normalize_and_test_predicates<'tcx>(
+pub fn normalize_and_test_predicates<'tcx>(
     tcx: TyCtxt<'tcx>,
     predicates: Vec<ty::Predicate<'tcx>>,
-    mode: TraitQueryMode,
 ) -> bool {
-    debug!("normalize_and_test_predicates(predicates={:?}, mode={:?})", predicates, mode);
+    debug!("normalize_and_test_predicates(predicates={:?})", predicates);
 
     let result = tcx.infer_ctxt().enter(|infcx| {
         let param_env = ty::ParamEnv::reveal_all();
-        let mut selcx = SelectionContext::with_query_mode(&infcx, mode);
-        let mut fulfill_cx = FulfillmentContext::with_query_mode(mode);
+        let mut selcx = SelectionContext::new(&infcx);
+        let mut fulfill_cx = FulfillmentContext::new();
         let cause = ObligationCause::dummy();
         let Normalized { value: predicates, obligations } =
             normalize(&mut selcx, param_env, cause.clone(), &predicates);
@@ -1044,12 +1043,12 @@ fn normalize_and_test_predicates<'tcx>(
 
 fn substitute_normalize_and_test_predicates<'tcx>(
     tcx: TyCtxt<'tcx>,
-    key: (DefId, SubstsRef<'tcx>, TraitQueryMode),
+    key: (DefId, SubstsRef<'tcx>),
 ) -> bool {
     debug!("substitute_normalize_and_test_predicates(key={:?})", key);
 
     let predicates = tcx.predicates_of(key.0).instantiate(tcx, key.1).predicates;
-    let result = normalize_and_test_predicates(tcx, predicates, key.2);
+    let result = normalize_and_test_predicates(tcx, predicates);
 
     debug!("substitute_normalize_and_test_predicates(key={:?}) = {:?}", key, result);
     result
@@ -1102,10 +1101,7 @@ fn vtable_methods<'tcx>(
             // Note that this method could then never be called, so we
             // do not want to try and codegen it, in that case (see #23435).
             let predicates = tcx.predicates_of(def_id).instantiate_own(tcx, substs);
-            // We don't expect overflow here, so report an error if it somehow ends
-            // up happening.
-            if !normalize_and_test_predicates(tcx, predicates.predicates, TraitQueryMode::Standard)
-            {
+            if !normalize_and_test_predicates(tcx, predicates.predicates) {
                 debug!("vtable_methods: predicates do not hold");
                 return None;
             }

src/librustc/ty/query/keys.rs

@@ -125,15 +125,6 @@ impl<'tcx> Key for (DefId, SubstsRef<'tcx>) {
     }
 }
 
-impl<'tcx> Key for (DefId, SubstsRef<'tcx>, traits::TraitQueryMode) {
-    fn query_crate(&self) -> CrateNum {
-        self.0.krate
-    }
-    fn default_span(&self, tcx: TyCtxt<'_>) -> Span {
-        self.0.default_span(tcx)
-    }
-}
-
 impl<'tcx> Key for (ty::ParamEnv<'tcx>, ty::PolyTraitRef<'tcx>) {
     fn query_crate(&self) -> CrateNum {
         self.1.def_id().krate

src/librustc_mir/transform/const_prop.rs

@@ -14,7 +14,7 @@ use rustc::mir::{
     SourceInfo, SourceScope, SourceScopeData, Statement, StatementKind, Terminator, TerminatorKind,
     UnOp, RETURN_PLACE,
 };
-use rustc::traits::TraitQueryMode;
+use rustc::traits;
 use rustc::ty::layout::{
     HasDataLayout, HasTyCtxt, LayoutError, LayoutOf, Size, TargetDataLayout, TyLayout,
 };
@@ -90,28 +90,28 @@ impl<'tcx> MirPass<'tcx> for ConstProp {
         // If there are unsatisfiable where clauses, then all bets are
         // off, and we just give up.
         //
-        // Note that we use TraitQueryMode::Canonical here, which causes
-        // us to treat overflow like any other error. This is because we
-        // are "speculatively" evaluating this item with the default substs.
-        // While this usually succeeds, it may fail with tricky impls
-        // (e.g. the typenum crate). Const-propagation is fundamentally
-        // "best-effort", and does not affect correctness in any way.
-        // Therefore, it's perfectly fine to just "give up" if we're
-        // unable to check the bounds with the default substs.
+        // We manually filter the predicates, skipping anything that's not
+        // "global". We are in a potentially generic context
+        // (e.g. we are evaluating a function without substituting generic
+        // parameters, so this filtering serves two purposes:
         //
-        // False negatives (failing to run const-prop on something when we actually
-        // could) are fine. However, false positives (running const-prop on
-        // an item with unsatisfiable bounds) can lead to us generating invalid
-        // MIR.
-        if !tcx.substitute_normalize_and_test_predicates((
-            source.def_id(),
-            InternalSubsts::identity_for_item(tcx, source.def_id()),
-            TraitQueryMode::Canonical,
-        )) {
-            trace!(
-                "ConstProp skipped for item with unsatisfiable predicates: {:?}",
-                source.def_id()
-            );
+        // 1. We skip evaluating any predicates that we would
+        // never be able prove are unsatisfiable (e.g. `<T as Foo>`
+        // 2. We avoid trying to normalize predicates involving generic
+        // parameters (e.g. `<T as Foo>::MyItem`). This can confuse
+        // the normalization code (leading to cycle errors), since
+        // it's usually never invoked in this way.
+        let predicates = tcx
+            .predicates_of(source.def_id())
+            .predicates
+            .iter()
+            .filter_map(|(p, _)| if p.is_global() { Some(*p) } else { None })
+            .collect();
+        if !traits::normalize_and_test_predicates(
+            tcx,
+            traits::elaborate_predicates(tcx, predicates).collect(),
+        ) {
+            trace!("ConstProp skipped for {:?}: found unsatisfiable predicates", source.def_id());
             return;
         }
 

src/test/ui/consts/issue-68264-overflow.rs

@@ -0,0 +1,43 @@
+// check-pass
+// compile-flags: --emit=mir,link
+// Regression test for issue #68264
+// Checks that we don't encounter overflow
+// when running const-prop on functions with
+// complicated bounds
+pub trait Query {}
+
+pub trait AsQuery {
+    type Query: Query;
+}
+pub trait Table: AsQuery + Sized {}
+
+pub trait LimitDsl {
+    type Output;
+}
+
+pub(crate) trait LoadQuery<Conn, U>: RunQueryDsl<Conn> {}
+
+impl<T: Query> AsQuery for T {
+    type Query = Self;
+}
+
+impl<T> LimitDsl for T
+where
+    T: Table,
+    T::Query: LimitDsl,
+{
+    type Output = <T::Query as LimitDsl>::Output;
+}
+
+pub(crate) trait RunQueryDsl<Conn>: Sized {
+    fn first<U>(self, _conn: &Conn) -> U
+    where
+        Self: LimitDsl,
+        Self::Output: LoadQuery<Conn, U>,
+    {
+        // Overflow is caused by this function body
+        unimplemented!()
+    }
+}
+
+fn main() {}