Fix 'type annotations needed' error with opaque types

Related: rust-lang#66426

This commit adds handling for opaque types during inference variable
fallback. Type variables generated from the instantiatino of opaque
types now fallback to the opque type itself.

Normally, the type variable for an instantiated opaque type is either
unified with the concrete type, or with the opaque type itself (e.g when
a function returns an opaque type by calling another function).

However, it's possible for the type variable to be left completely
unconstrained. This can occur in code like this:

```rust
pub type Foo = impl Copy;
fn produce() -> Option<Foo> {
    None
}
```

Here, we'll instantatiate the `Foo` in `Option<Foo>` to a fresh type
variable, but we will never unify it with anything due to the fact
that we return a `None`.

This results in the error message:

`type annotations needed: cannot resolve `_: std::marker::Copy``

pointing at `pub type Foo = impl Copy`.

This message is not only confusing, it's incorrect. When an opaque type
inference variable is completely unconstrained, we can always fall back
to using the opaque type itself. This effectively turns that particular
use of the opaque type into a non-defining use, even if it appears in a
defining scope.

Author: Aaron1011

src/librustc/infer/opaque_types/mod.rs

@@ -24,6 +24,10 @@ pub type OpaqueTypeMap<'tcx> = DefIdMap<OpaqueTypeDecl<'tcx>>;
 /// appear in the return type).
 #[derive(Copy, Clone, Debug)]
 pub struct OpaqueTypeDecl<'tcx> {
+
+    /// The opaque type (`ty::Opaque`) for this declaration
+    pub opaque_type: Ty<'tcx>,
+
     /// The substitutions that we apply to the opaque type that this
     /// `impl Trait` desugars to. e.g., if:
     ///
@@ -1150,6 +1154,7 @@ impl<'a, 'tcx> Instantiator<'a, 'tcx> {
         self.opaque_types.insert(
             def_id,
             OpaqueTypeDecl {
+                opaque_type: ty,
                 substs,
                 definition_span,
                 concrete_ty: ty_var,

src/librustc_typeck/check/mod.rs

@@ -147,7 +147,7 @@ use crate::TypeAndSubsts;
 use crate::lint;
 use crate::util::captures::Captures;
 use crate::util::common::{ErrorReported, indenter};
-use crate::util::nodemap::{DefIdMap, DefIdSet, FxHashSet, HirIdMap};
+use crate::util::nodemap::{DefIdMap, DefIdSet, FxHashMap, FxHashSet, HirIdMap};
 
 pub use self::Expectation::*;
 use self::autoderef::Autoderef;
@@ -231,6 +231,13 @@ pub struct Inherited<'a, 'tcx> {
     // 'de-opaque' OpaqueTypeDecl, after typeck is done with all functions.
     opaque_types: RefCell<DefIdMap<OpaqueTypeDecl<'tcx>>>,
 
+    /// A map from inference variables created from opaque
+    /// type instantiations (ty::Infer) to the actual opaque
+    /// type (`ty::Opaque`). Used during fallback to map unconstrained
+    /// opaque type inference variables to their corresponding
+    /// opaque type.
+    opaque_types_vars: RefCell<FxHashMap<Ty<'tcx>, Ty<'tcx>>>,
+
     /// Each type parameter has an implicit region bound that
     /// indicates it must outlive at least the function body (the user
     /// may specify stronger requirements). This field indicates the
@@ -696,6 +703,7 @@ impl Inherited<'a, 'tcx> {
             deferred_cast_checks: RefCell::new(Vec::new()),
             deferred_generator_interiors: RefCell::new(Vec::new()),
             opaque_types: RefCell::new(Default::default()),
+            opaque_types_vars: RefCell::new(Default::default()),
             implicit_region_bound,
             body_id,
         }
@@ -937,9 +945,46 @@ fn typeck_tables_of(tcx: TyCtxt<'_>, def_id: DefId) -> &ty::TypeckTables<'_> {
         // All type checking constraints were added, try to fallback unsolved variables.
         fcx.select_obligations_where_possible(false, |_| {});
         let mut fallback_has_occurred = false;
+
+        // We do fallback in two passes, to try to generate
+        // better error messages.
+        // The first time, we do *not* replace opaque types.
+        for ty in &fcx.unsolved_variables() {
+            fallback_has_occurred |= fcx.fallback_if_possible(ty, false /* opaque_fallback */);
+        }
+        // We now see if we can make progress. This might
+        // cause us to unify inference variables for opaque types,
+        // since we may have unified some other type variables
+        // during the first phase of fallback.
+        // This means that we only replace inference variables with their underlying
+        // opaque types as a last resort.
+        //
+        // In code like this:
+        //
+        // ```rust
+        // type MyType = impl Copy;
+        // fn produce() -> MyType { true }
+        // fn bad_produce() -> MyType { panic!() }
+        // ```
+        //
+        // we want to unify the opaque inference variable in `bad_produce`
+        // with the diverging fallback for `panic!` (e.g. `()` or `!`),
+        // This will produce a nice error message about conflicting concrete
+        // types for `MyType`.
+        //
+        // If we had tried to fallback the opaque inference variable to `MyType`,
+        // we will generate a confusing type-check error that does not explicitly
+        // refer to opaque types.
+        fcx.select_obligations_where_possible(fallback_has_occurred, |_| {});
+
+        // We now run fallback again, but this time we allow it to replace
+        // unconstrained opaque type variables, in addition to performing
+        // other kinds of fallback.
         for ty in &fcx.unsolved_variables() {
-            fallback_has_occurred |= fcx.fallback_if_possible(ty);
+            fallback_has_occurred |= fcx.fallback_if_possible(ty, true /* opaque_fallback */);
         }
+
+        // See if we can make any more progress
         fcx.select_obligations_where_possible(fallback_has_occurred, |_| {});
 
         // Even though coercion casts provide type hints, we check casts after fallback for
@@ -2864,8 +2909,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         );
 
         let mut opaque_types = self.opaque_types.borrow_mut();
+        let mut opaque_types_vars = self.opaque_types_vars.borrow_mut();
         for (ty, decl) in opaque_type_map {
             let _ = opaque_types.insert(ty, decl);
+            let _ = opaque_types_vars.insert(decl.concrete_ty, decl.opaque_type);
         }
 
         value
@@ -3078,7 +3125,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     // Fallback becomes very dubious if we have encountered type-checking errors.
     // In that case, fallback to Error.
     // The return value indicates whether fallback has occurred.
-    fn fallback_if_possible(&self, ty: Ty<'tcx>) -> bool {
+    fn fallback_if_possible(&self, ty: Ty<'tcx>, opaque_fallback: bool) -> bool {
         use rustc::ty::error::UnconstrainedNumeric::Neither;
         use rustc::ty::error::UnconstrainedNumeric::{UnconstrainedInt, UnconstrainedFloat};
 
@@ -3088,7 +3135,53 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             UnconstrainedInt => self.tcx.types.i32,
             UnconstrainedFloat => self.tcx.types.f64,
             Neither if self.type_var_diverges(ty) => self.tcx.mk_diverging_default(),
-            Neither => return false,
+            Neither => {
+                // This type variable was created from the instantiation of an opaque
+                // type. The fact that we're attempting to perform fallback for it
+                // means that the function neither constrained it to a concrete
+                // type, nor to the opaque type itself.
+                //
+                // For example, in this code:
+                //
+                //```
+                // type MyType = impl Copy;
+                // fn defining_use() -> MyType { true }
+                // fn other_use() -> MyType { defining_use() }
+                // ```
+                //
+                // `defining_use` will constrain the instantiated inference
+                // variable to `bool`, while `other_use` will constrain
+                // the instantiated inference variable to `MyType`.
+                //
+                // When we process opaque types during writeback, we
+                // will handle cases like `other_use`, and not count
+                // them as defining usages
+                //
+                // However, we also need to handle cases like this:
+                //
+                // ```rust
+                // pub type Foo = impl Copy;
+                // fn produce() -> Option<Foo> {
+                //     None
+                //  }
+                //  ```
+                //
+                // In the above snippet, the inference varaible created by
+                // instantiating `Option<Foo>` will be completely unconstrained.
+                // We treat this as a non-defining use by making the inference
+                // variable fall back to the opaque type itself.
+                if opaque_fallback {
+                    if let Some(opaque_ty) = self.opaque_types_vars.borrow().get(ty) {
+                        debug!("fallback_if_possible: falling back opaque type var {:?} to {:?}",
+                               ty, opaque_ty);
+                        *opaque_ty
+                    } else {
+                        return false;
+                    }
+                } else {
+                    return false;
+                }
+            },
         };
         debug!("fallback_if_possible: defaulting `{:?}` to `{:?}`", ty, fallback);
         self.demand_eqtype(syntax_pos::DUMMY_SP, ty, fallback);

src/test/ui/impl-trait/where-allowed-2.rs

@@ -0,0 +1,9 @@
+//! Ideally, these tests would go in `where-allowed.rs`, but we bail out
+//! too early to display them.
+use std::fmt::Debug;
+
+// Disallowed
+fn in_adt_in_return() -> Vec<impl Debug> { panic!() }
+//~^ ERROR opaque type expands to a recursive type
+
+fn main() {}

src/test/ui/impl-trait/where-allowed-2.stderr

@@ -0,0 +1,11 @@
+error[E0720]: opaque type expands to a recursive type
+  --> $DIR/where-allowed-2.rs:6:30
+   |
+LL | fn in_adt_in_return() -> Vec<impl Debug> { panic!() }
+   |                              ^^^^^^^^^^ expands to a recursive type
+   |
+   = note: type resolves to itself
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0720`.

src/test/ui/impl-trait/where-allowed.rs

@@ -11,10 +11,6 @@ fn in_return() -> impl Debug { panic!() }
 // Allowed
 fn in_adt_in_parameters(_: Vec<impl Debug>) { panic!() }
 
-// Disallowed
-fn in_adt_in_return() -> Vec<impl Debug> { panic!() }
-//~^ ERROR type annotations needed
-
 // Disallowed
 fn in_fn_parameter_in_parameters(_: fn(impl Debug)) { panic!() }
 //~^ ERROR `impl Trait` not allowed outside of function and inherent method return types
@@ -60,7 +56,6 @@ fn in_impl_Fn_return_in_parameters(_: &impl Fn() -> impl Debug) { panic!() }
 fn in_impl_Fn_parameter_in_return() -> &'static impl Fn(impl Debug) { panic!() }
 //~^ ERROR `impl Trait` not allowed outside of function and inherent method return types
 //~| ERROR nested `impl Trait` is not allowed
-//~| ERROR type annotations needed
 
 // Disallowed
 fn in_impl_Fn_return_in_return() -> &'static impl Fn() -> impl Debug { panic!() }

src/test/ui/type-alias-impl-trait/fallback.rs

@@ -0,0 +1,22 @@
+// Tests that we correctly handle the instantiated
+// inference variable being completely unconstrained.
+//
+// check-pass
+#![feature(type_alias_impl_trait)]
+
+type Foo = impl Copy;
+
+enum Wrapper<T> {
+    First(T),
+    Second
+}
+
+fn _make_iter() -> Foo {
+    true
+}
+
+fn _produce() -> Wrapper<Foo> {
+    Wrapper::Second
+}
+
+fn main() {}