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

Don't run const propagation on items with inconsistent bounds

Fixes rust-lang#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.

Author: JohnTitor

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 {

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)
             }
         }
 

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)
     }
 

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;
             }

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

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(

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() {}

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)]