Rollup merge of #67914 - Aaron1011:fix/const-prop-impossible, r=matthewjasper,oli-obk

Don't run const propagation on items with inconsistent bounds

Fixes #67696

Using `#![feature(trivial_bounds)]`, it's possible to write functions
with unsatisfiable 'where' clauses, making them uncallable. However, the
user can act as if these 'where' clauses are true inside the body of the
function, leading to code that would normally be impossible to write.

Since const propgation can run even without any user-written calls to a
function, we need to explcitly check for these uncallable functions.

8 files changed, 115 insertions, 14 deletions

diff --git a/src/librustc/mir/mono.rs b/src/librustc/mir/mono.rs
index 51ce575e51f..e7a4c5b5921 100644
--- a/src/librustc/mir/mono.rs
+++ b/src/librustc/mir/mono.rs
@@ -1,6 +1,7 @@
 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;
@@ -167,7 +168,9 @@ impl<'tcx> MonoItem<'tcx> {
             MonoItem::GlobalAsm(..) => return true,
         };
 
-        tcx.substitute_normalize_and_test_predicates((def_id, &substs))
+        // 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))
     }
 
     pub fn to_string(&self, tcx: TyCtxt<'tcx>, debug: bool) -> String {
diff --git a/src/librustc/query/mod.rs b/src/librustc/query/mod.rs
index f4c262fbac1..a20e011b91a 100644
--- a/src/librustc/query/mod.rs
+++ b/src/librustc/query/mod.rs
@@ -1148,11 +1148,11 @@ rustc_queries! {
             desc { "normalizing `{:?}`", goal }
         }
 
-        query substitute_normalize_and_test_predicates(key: (DefId, SubstsRef<'tcx>)) -> bool {
+        query substitute_normalize_and_test_predicates(key: (DefId, SubstsRef<'tcx>, traits::TraitQueryMode)) -> bool {
             no_force
             desc { |tcx|
-                "testing substituted normalized predicates:`{}`",
-                tcx.def_path_str(key.0)
+                "testing substituted normalized predicates in mode {:?}:`{}`",
+                key.2, tcx.def_path_str(key.0)
             }
         }
 
diff --git a/src/librustc/traits/fulfill.rs b/src/librustc/traits/fulfill.rs
index 46ece6fc405..9e5abc80822 100644
--- a/src/librustc/traits/fulfill.rs
+++ b/src/librustc/traits/fulfill.rs
@@ -16,6 +16,7 @@ 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>;
@@ -62,6 +63,9 @@ 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)]
@@ -75,12 +79,26 @@ pub struct PendingPredicateObligation<'tcx> {
 static_assert_size!(PendingPredicateObligation<'_>, 136);
 
 impl<'a, 'tcx> FulfillmentContext<'tcx> {
-    /// Creates a new fulfillment context.
+    /// Creates a new fulfillment context with `TraitQueryMode::Standard`
+    /// You almost always want to use this instead of `with_query_mode`
     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,
         }
     }
 
@@ -89,6 +107,7 @@ impl<'a, 'tcx> FulfillmentContext<'tcx> {
             predicates: ObligationForest::new(),
             register_region_obligations: true,
             usable_in_snapshot: true,
+            query_mode: TraitQueryMode::Standard,
         }
     }
 
@@ -97,6 +116,7 @@ impl<'a, 'tcx> FulfillmentContext<'tcx> {
             predicates: ObligationForest::new(),
             register_region_obligations: false,
             usable_in_snapshot: false,
+            query_mode: TraitQueryMode::Standard,
         }
     }
 
@@ -217,7 +237,7 @@ impl<'tcx> TraitEngine<'tcx> for FulfillmentContext<'tcx> {
         &mut self,
         infcx: &InferCtxt<'_, 'tcx>,
     ) -> Result<(), Vec<FulfillmentError<'tcx>>> {
-        let mut selcx = SelectionContext::new(infcx);
+        let mut selcx = SelectionContext::with_query_mode(infcx, self.query_mode);
         self.select(&mut selcx)
     }
 
diff --git a/src/librustc/traits/mod.rs b/src/librustc/traits/mod.rs
index 2d3160dc3e5..31de5409fc8 100644
--- a/src/librustc/traits/mod.rs
+++ b/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)]
+#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash, HashStable)]
 pub enum TraitQueryMode {
     // Standard/un-canonicalized queries get accurate
     // spans etc. passed in and hence can do reasonable
@@ -1017,13 +1017,14 @@ where
 fn normalize_and_test_predicates<'tcx>(
     tcx: TyCtxt<'tcx>,
     predicates: Vec<ty::Predicate<'tcx>>,
+    mode: TraitQueryMode,
 ) -> bool {
-    debug!("normalize_and_test_predicates(predicates={:?})", predicates);
+    debug!("normalize_and_test_predicates(predicates={:?}, mode={:?})", predicates, mode);
 
     let result = tcx.infer_ctxt().enter(|infcx| {
         let param_env = ty::ParamEnv::reveal_all();
-        let mut selcx = SelectionContext::new(&infcx);
-        let mut fulfill_cx = FulfillmentContext::new();
+        let mut selcx = SelectionContext::with_query_mode(&infcx, mode);
+        let mut fulfill_cx = FulfillmentContext::with_query_mode(mode);
         let cause = ObligationCause::dummy();
         let Normalized { value: predicates, obligations } =
             normalize(&mut selcx, param_env, cause.clone(), &predicates);
@@ -1043,12 +1044,12 @@ fn normalize_and_test_predicates<'tcx>(
 
 fn substitute_normalize_and_test_predicates<'tcx>(
     tcx: TyCtxt<'tcx>,
-    key: (DefId, SubstsRef<'tcx>),
+    key: (DefId, SubstsRef<'tcx>, TraitQueryMode),
 ) -> 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);
+    let result = normalize_and_test_predicates(tcx, predicates, key.2);
 
     debug!("substitute_normalize_and_test_predicates(key={:?}) = {:?}", key, result);
     result
@@ -1101,7 +1102,10 @@ 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);
-            if !normalize_and_test_predicates(tcx, predicates.predicates) {
+            // 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)
+            {
                 debug!("vtable_methods: predicates do not hold");
                 return None;
             }
diff --git a/src/librustc/ty/query/keys.rs b/src/librustc/ty/query/keys.rs
index cbf335ad607..3fb3720a563 100644
--- a/src/librustc/ty/query/keys.rs
+++ b/src/librustc/ty/query/keys.rs
@@ -125,6 +125,15 @@ 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
diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs
index 1d5a643484a..90c97480c75 100644
--- a/src/librustc_mir/transform/const_prop.rs
+++ b/src/librustc_mir/transform/const_prop.rs
@@ -14,6 +14,7 @@ use rustc::mir::{
     SourceInfo, SourceScope, SourceScopeData, Statement, StatementKind, Terminator, TerminatorKind,
     UnOp, RETURN_PLACE,
 };
+use rustc::traits::TraitQueryMode;
 use rustc::ty::layout::{
     HasDataLayout, HasTyCtxt, LayoutError, LayoutOf, Size, TargetDataLayout, TyLayout,
 };
@@ -74,6 +75,46 @@ impl<'tcx> MirPass<'tcx> for ConstProp {
             return;
         }
 
+        // Check if it's even possible to satisfy the 'where' clauses
+        // for this item.
+        // This branch will never be taken for any normal function.
+        // However, it's possible to `#!feature(trivial_bounds)]` to write
+        // a function with impossible to satisfy clauses, e.g.:
+        // `fn foo() where String: Copy {}`
+        //
+        // We don't usually need to worry about this kind of case,
+        // since we would get a compilation error if the user tried
+        // to call it. However, since we can do const propagation
+        // even without any calls to the function, we need to make
+        // sure that it even makes sense to try to evaluate the body.
+        // 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.
+        //
+        // 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()
+            );
+            return;
+        }
+
         trace!("ConstProp starting for {:?}", source.def_id());
 
         let dummy_body = &Body::new(
diff --git a/src/test/ui/consts/issue-67696-const-prop-ice.rs b/src/test/ui/consts/issue-67696-const-prop-ice.rs
new file mode 100644
index 00000000000..ad52608b3f4
--- /dev/null
+++ b/src/test/ui/consts/issue-67696-const-prop-ice.rs
@@ -0,0 +1,20 @@
+// check-pass
+// compile-flags: --emit=mir,link
+// Checks that we don't ICE due to attempting to run const prop
+// on a function with unsatisifable 'where' clauses
+
+#![allow(unused)]
+
+trait A {
+    fn foo(&self) -> Self where Self: Copy;
+}
+
+impl A for [fn(&())] {
+    fn foo(&self) -> Self where Self: Copy { *(&[] as &[_]) }
+}
+
+impl A for i32 {
+    fn foo(&self) -> Self { 3 }
+}
+
+fn main() {}
diff --git a/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs b/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs
index 6450ddd1b67..69eee66e64d 100644
--- a/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs
+++ b/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs
@@ -1,4 +1,8 @@
-// run-pass
+// check-pass
+// compile-flags: --emit=mir,link
+// Force mir to be emitted, to ensure that const
+// propagation doesn't ICE on a function
+// with an 'impossible' body. See issue #67696
 // Inconsistent bounds with trait implementations
 
 #![feature(trivial_bounds)]