Add another case of fallback to () avoid breakage

This adds src/test/ui/never_type/fallback-closure-ret.rs as a test case which
showcases the failure mode fixed by this commit.

Author: Mark-Simulacrum

Diff for: compiler/rustc_infer/src/traits/engine.rs

@@ -1,5 +1,6 @@
 use crate::infer::InferCtxt;
 use crate::traits::Obligation;
+use rustc_data_structures::fx::FxHashMap;
 use rustc_hir as hir;
 use rustc_hir::def_id::DefId;
 use rustc_middle::ty::{self, ToPredicate, Ty, WithConstness};
@@ -73,6 +74,8 @@ pub trait TraitEngine<'tcx>: 'tcx {
     }
 
     fn pending_obligations(&self) -> Vec<PredicateObligation<'tcx>>;
+
+    fn relationships(&mut self) -> &mut FxHashMap<ty::TyVid, ty::FoundRelationships>;
 }
 
 pub trait TraitEngineExt<'tcx> {

Diff for: compiler/rustc_middle/src/ty/mod.rs

@@ -2090,3 +2090,16 @@ impl<'tcx> fmt::Debug for SymbolName<'tcx> {
         fmt::Display::fmt(&self.name, fmt)
     }
 }
+
+#[derive(Debug, Default, Copy, Clone)]
+pub struct FoundRelationships {
+    /// This is true if we identified that this Ty (`?T`) is found in a `?T: Foo`
+    /// obligation, where:
+    ///
+    ///  * `Foo` is not `Sized`
+    ///  * `(): Foo` may be satisfied
+    pub self_in_trait: bool,
+    /// This is true if we identified that this Ty (`?T`) is found in a `<_ as
+    /// _>::AssocType = ?T`
+    pub output: bool,
+}

Diff for: compiler/rustc_trait_selection/src/traits/chalk_fulfill.rs

@@ -7,16 +7,21 @@ use crate::traits::{
     ChalkEnvironmentAndGoal, FulfillmentError, FulfillmentErrorCode, ObligationCause,
     PredicateObligation, SelectionError, TraitEngine,
 };
-use rustc_data_structures::fx::FxIndexSet;
+use rustc_data_structures::fx::{FxHashMap, FxIndexSet};
 use rustc_middle::ty::{self, Ty};
 
 pub struct FulfillmentContext<'tcx> {
     obligations: FxIndexSet<PredicateObligation<'tcx>>,
+
+    relationships: FxHashMap<ty::TyVid, ty::FoundRelationships>,
 }
 
 impl FulfillmentContext<'tcx> {
     crate fn new() -> Self {
-        FulfillmentContext { obligations: FxIndexSet::default() }
+        FulfillmentContext {
+            obligations: FxIndexSet::default(),
+            relationships: FxHashMap::default(),
+        }
     }
 }
 
@@ -39,6 +44,8 @@ impl TraitEngine<'tcx> for FulfillmentContext<'tcx> {
         assert!(!infcx.is_in_snapshot());
         let obligation = infcx.resolve_vars_if_possible(obligation);
 
+        super::relationships::update(self, infcx, &obligation);
+
         self.obligations.insert(obligation);
     }
 
@@ -146,4 +153,8 @@ impl TraitEngine<'tcx> for FulfillmentContext<'tcx> {
     fn pending_obligations(&self) -> Vec<PredicateObligation<'tcx>> {
         self.obligations.iter().cloned().collect()
     }
+
+    fn relationships(&mut self) -> &mut FxHashMap<ty::TyVid, ty::FoundRelationships> {
+        &mut self.relationships
+    }
 }

Diff for: compiler/rustc_trait_selection/src/traits/fulfill.rs

@@ -1,4 +1,5 @@
 use crate::infer::{InferCtxt, TyOrConstInferVar};
+use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::obligation_forest::ProcessResult;
 use rustc_data_structures::obligation_forest::{Error, ForestObligation, Outcome};
 use rustc_data_structures::obligation_forest::{ObligationForest, ObligationProcessor};
@@ -53,6 +54,9 @@ pub struct FulfillmentContext<'tcx> {
     // A list of all obligations that have been registered with this
     // fulfillment context.
     predicates: ObligationForest<PendingPredicateObligation<'tcx>>,
+
+    relationships: FxHashMap<ty::TyVid, ty::FoundRelationships>,
+
     // Should this fulfillment context register type-lives-for-region
     // obligations on its parent infcx? In some cases, region
     // obligations are either already known to hold (normalization) or
@@ -97,6 +101,7 @@ impl<'a, 'tcx> FulfillmentContext<'tcx> {
     pub fn new() -> FulfillmentContext<'tcx> {
         FulfillmentContext {
             predicates: ObligationForest::new(),
+            relationships: FxHashMap::default(),
             register_region_obligations: true,
             usable_in_snapshot: false,
         }
@@ -105,6 +110,7 @@ impl<'a, 'tcx> FulfillmentContext<'tcx> {
     pub fn new_in_snapshot() -> FulfillmentContext<'tcx> {
         FulfillmentContext {
             predicates: ObligationForest::new(),
+            relationships: FxHashMap::default(),
             register_region_obligations: true,
             usable_in_snapshot: true,
         }
@@ -113,6 +119,7 @@ impl<'a, 'tcx> FulfillmentContext<'tcx> {
     pub fn new_ignoring_regions() -> FulfillmentContext<'tcx> {
         FulfillmentContext {
             predicates: ObligationForest::new(),
+            relationships: FxHashMap::default(),
             register_region_obligations: false,
             usable_in_snapshot: false,
         }
@@ -210,6 +217,8 @@ impl<'tcx> TraitEngine<'tcx> for FulfillmentContext<'tcx> {
 
         assert!(!infcx.is_in_snapshot() || self.usable_in_snapshot);
 
+        super::relationships::update(self, infcx, &obligation);
+
         self.predicates
             .register_obligation(PendingPredicateObligation { obligation, stalled_on: vec![] });
     }
@@ -265,6 +274,10 @@ impl<'tcx> TraitEngine<'tcx> for FulfillmentContext<'tcx> {
     fn pending_obligations(&self) -> Vec<PredicateObligation<'tcx>> {
         self.predicates.map_pending_obligations(|o| o.obligation.clone())
     }
+
+    fn relationships(&mut self) -> &mut FxHashMap<ty::TyVid, ty::FoundRelationships> {
+        &mut self.relationships
+    }
 }
 
 struct FulfillProcessor<'a, 'b, 'tcx> {

Diff for: compiler/rustc_trait_selection/src/traits/mod.rs

@@ -15,6 +15,7 @@ mod object_safety;
 mod on_unimplemented;
 mod project;
 pub mod query;
+pub(crate) mod relationships;
 mod select;
 mod specialize;
 mod structural_match;

Diff for: compiler/rustc_trait_selection/src/traits/relationships.rs

@@ -0,0 +1,69 @@
+use crate::infer::InferCtxt;
+use crate::traits::query::evaluate_obligation::InferCtxtExt;
+use crate::traits::{ObligationCause, PredicateObligation};
+use rustc_infer::traits::TraitEngine;
+use rustc_middle::ty::{self, ToPredicate};
+
+pub(crate) fn update<'tcx, T>(
+    engine: &mut T,
+    infcx: &InferCtxt<'_, 'tcx>,
+    obligation: &PredicateObligation<'tcx>,
+) where
+    T: TraitEngine<'tcx>,
+{
+    // (*) binder skipped
+    if let ty::PredicateKind::Trait(predicate) = obligation.predicate.kind().skip_binder() {
+        if let Some(ty) =
+            infcx.shallow_resolve(predicate.self_ty()).ty_vid().map(|t| infcx.root_var(t))
+        {
+            if infcx
+                .tcx
+                .lang_items()
+                .sized_trait()
+                .map_or(false, |st| st != predicate.trait_ref.def_id)
+            {
+                let new_self_ty = infcx.tcx.types.unit;
+
+                let trait_ref = ty::TraitRef {
+                    substs: infcx
+                        .tcx
+                        .mk_substs_trait(new_self_ty, &predicate.trait_ref.substs[1..]),
+                    ..predicate.trait_ref
+                };
+
+                // Then contstruct a new obligation with Self = () added
+                // to the ParamEnv, and see if it holds.
+                let o = rustc_infer::traits::Obligation::new(
+                    ObligationCause::dummy(),
+                    obligation.param_env,
+                    obligation
+                        .predicate
+                        .kind()
+                        .map_bound(|_| {
+                            // (*) binder moved here
+                            ty::PredicateKind::Trait(ty::TraitPredicate {
+                                trait_ref,
+                                constness: predicate.constness,
+                            })
+                        })
+                        .to_predicate(infcx.tcx),
+                );
+                // Don't report overflow errors. Otherwise equivalent to may_hold.
+                if let Ok(result) = infcx.probe(|_| infcx.evaluate_obligation(&o)) {
+                    if result.may_apply() {
+                        engine.relationships().entry(ty).or_default().self_in_trait = true;
+                    }
+                }
+            }
+        }
+    }
+
+    if let ty::PredicateKind::Projection(predicate) = obligation.predicate.kind().skip_binder() {
+        // If the projection predicate (Foo::Bar == X) has X as a non-TyVid,
+        // we need to make it into one.
+        if let Some(vid) = predicate.ty.ty_vid() {
+            debug!("relationship: {:?}.output = true", vid);
+            engine.relationships().entry(vid).or_default().output = true;
+        }
+    }
+}

Diff for: compiler/rustc_typeck/src/check/fallback.rs

@@ -11,9 +11,19 @@ impl<'tcx> FnCtxt<'_, 'tcx> {
     /// Performs type inference fallback, returning true if any fallback
     /// occurs.
     pub(super) fn type_inference_fallback(&self) -> bool {
+        debug!(
+            "type-inference-fallback start obligations: {:#?}",
+            self.fulfillment_cx.borrow_mut().pending_obligations()
+        );
+
         // All type checking constraints were added, try to fallback unsolved variables.
         self.select_obligations_where_possible(false, |_| {});
 
+        debug!(
+            "type-inference-fallback post selection obligations: {:#?}",
+            self.fulfillment_cx.borrow_mut().pending_obligations()
+        );
+
         // Check if we have any unsolved varibales. If not, no need for fallback.
         let unsolved_variables = self.unsolved_variables();
         if unsolved_variables.is_empty() {
@@ -251,6 +261,8 @@ impl<'tcx> FnCtxt<'_, 'tcx> {
     ) -> FxHashMap<Ty<'tcx>, Ty<'tcx>> {
         debug!("calculate_diverging_fallback({:?})", unsolved_variables);
 
+        let relationships = self.fulfillment_cx.borrow_mut().relationships().clone();
+
         // Construct a coercion graph where an edge `A -> B` indicates
         // a type variable is that is coerced
         let coercion_graph = self.create_coercion_graph();
@@ -334,21 +346,63 @@ impl<'tcx> FnCtxt<'_, 'tcx> {
             roots_reachable_from_non_diverging,
         );
 
+        debug!("inherited: {:#?}", self.inh.fulfillment_cx.borrow_mut().pending_obligations());
+        debug!("obligations: {:#?}", self.fulfillment_cx.borrow_mut().pending_obligations());
+        debug!("relationships: {:#?}", relationships);
+
         // For each diverging variable, figure out whether it can
         // reach a member of N. If so, it falls back to `()`. Else
         // `!`.
         let mut diverging_fallback = FxHashMap::default();
+        diverging_fallback.reserve(diverging_vids.len());
         for &diverging_vid in &diverging_vids {
             let diverging_ty = self.tcx.mk_ty_var(diverging_vid);
             let root_vid = self.infcx.root_var(diverging_vid);
             let can_reach_non_diverging = coercion_graph
                 .depth_first_search(root_vid)
                 .any(|n| roots_reachable_from_non_diverging.visited(n));
-            if can_reach_non_diverging {
-                debug!("fallback to (): {:?}", diverging_vid);
+
+            let mut relationship = ty::FoundRelationships { self_in_trait: false, output: false };
+
+            for (vid, rel) in relationships.iter() {
+                if self.infcx.root_var(*vid) == root_vid {
+                    relationship.self_in_trait |= rel.self_in_trait;
+                    relationship.output |= rel.output;
+                }
+            }
+
+            if relationship.self_in_trait && relationship.output {
+                // This case falls back to () to ensure that the code pattern in
+                // src/test/ui/never_type/fallback-closure-ret.rs continues to
+                // compile when never_type_fallback is enabled.
+                //
+                // This rule is not readily explainable from first principles,
+                // but is rather intended as a patchwork fix to ensure code
+                // which compiles before the stabilization of never type
+                // fallback continues to work.
+                //
+                // Typically this pattern is encountered in a function taking a
+                // closure as a parameter, where the return type of that closure
+                // (checked by `relationship.output`) is expected to implement
+                // some trait (checked by `relationship.self_in_trait`). This
+                // can come up in non-closure cases too, so we do not limit this
+                // rule to specifically `FnOnce`.
+                //
+                // When the closure's body is something like `panic!()`, the
+                // return type would normally be inferred to `!`. However, it
+                // needs to fall back to `()` in order to still compile, as the
+                // trait is specifically implemented for `()` but not `!`.
+                //
+                // For details on the requirements for these relationships to be
+                // set, see the relationship finding module in
+                // compiler/rustc_trait_selection/src/traits/relationships.rs.
+                debug!("fallback to () - found trait and projection: {:?}", diverging_vid);
+                diverging_fallback.insert(diverging_ty, self.tcx.types.unit);
+            } else if can_reach_non_diverging {
+                debug!("fallback to () - reached non-diverging: {:?}", diverging_vid);
                 diverging_fallback.insert(diverging_ty, self.tcx.types.unit);
             } else {
-                debug!("fallback to !: {:?}", diverging_vid);
+                debug!("fallback to ! - all diverging: {:?}", diverging_vid);
                 diverging_fallback.insert(diverging_ty, self.tcx.mk_diverging_default());
             }
         }
@@ -369,10 +423,23 @@ impl<'tcx> FnCtxt<'_, 'tcx> {
                 obligation.predicate.kind().no_bound_vars()
             })
             .filter_map(|atom| {
-                if let ty::PredicateKind::Coerce(ty::CoercePredicate { a, b }) = atom {
-                    let a_vid = self.root_vid(a)?;
-                    let b_vid = self.root_vid(b)?;
-                    Some((a_vid, b_vid))
+                // We consider both subtyping and coercion to imply 'flow' from
+                // some position in the code `a` to a different position `b`.
+                // This is then used to determine which variables interact with
+                // live code, and as such must fall back to `()` to preserve
+                // soundness.
+                //
+                // In practice currently the two ways that this happens is
+                // coercion and subtyping.
+                let (a, b) = if let ty::PredicateKind::Coerce(ty::CoercePredicate { a, b }) = atom {
+                    (a, b)
+                } else if let ty::PredicateKind::Subtype(ty::SubtypePredicate {
+                    a_is_expected: _,
+                    a,
+                    b,
+                }) = atom
+                {
+                    (a, b)
                 } else {
                     return None;
                 };

Diff for: src/test/ui/never_type/fallback-closure-ret.rs

@@ -0,0 +1,23 @@
+// This test verifies that never type fallback preserves the following code in a
+// compiling state. This pattern is fairly common in the wild, notably seen in
+// wasmtime v0.16. Typically this is some closure wrapper that expects a
+// collection of 'known' signatures, and -> ! is not included in that set.
+//
+// This test is specifically targeted by the unit type fallback when
+// encountering a set of obligations like `?T: Foo` and `Trait::Projection =
+// ?T`. In the code below, these are `R: Bar` and `Fn::Output = R`.
+//
+// revisions: nofallback fallback
+// check-pass
+
+#![cfg_attr(fallback, feature(never_type_fallback))]
+
+trait Bar { }
+impl Bar for () {  }
+impl Bar for u32 {  }
+
+fn foo<R: Bar>(_: impl Fn() -> R) {}
+
+fn main() {
+    foo(|| panic!());
+}

Diff for: src/test/ui/never_type/fallback-closure-wrap.rs

@@ -0,0 +1,30 @@
+// This is a minified example from Crater breakage observed when attempting to
+// stabilize never type, nstoddard/webgl-gui @ 22f0169f.
+//
+// This particular test case currently fails as the inference to `()` rather
+// than `!` happens as a result of an `as` cast, which is not currently tracked.
+// Crater did not find many cases of this occuring, but it is included for
+// awareness.
+//
+// revisions: nofallback fallback
+//[nofallback] check-pass
+//[fallback] check-fail
+
+#![cfg_attr(fallback, feature(never_type_fallback))]
+
+use std::marker::PhantomData;
+
+fn main() {
+    let error = Closure::wrap(Box::new(move || {
+        //[fallback]~^ ERROR type mismatch resolving
+        panic!("Can't connect to server.");
+    }) as Box<dyn FnMut()>);
+}
+
+struct Closure<T: ?Sized>(PhantomData<T>);
+
+impl<T: ?Sized> Closure<T> {
+    fn wrap(data: Box<T>) -> Closure<T> {
+        todo!()
+    }
+}