detect wrong number of args when type-checking a closure

nikomatsakis · nikomatsakis · commit cc05561048f0 · 2018-02-12T16:49:21.000-05:00
Instead of creating inference variables for those argument types, use
the trait error-reporting code to give a nicer error.
diff --git a/src/librustc/traits/error_reporting.rs b/src/librustc/traits/error_reporting.rs
@@ -747,7 +747,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                     ty::TyTuple(ref tys, _) => tys.iter()
                         .map(|t| match t.sty {
                             ty::TypeVariants::TyTuple(ref tys, _) => ArgKind::Tuple(
-                                span,
+                                Some(span),
                                 tys.iter()
                                     .map(|ty| ("_".to_owned(), format!("{}", ty.sty)))
                                     .collect::<Vec<_>>()
@@ -815,7 +815,11 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
         }
     }
 
-    fn get_fn_like_arguments(&self, node: hir::map::Node) -> (Span, Vec<ArgKind>) {
+    /// Given some node representing a fn-like thing in the HIR map,
+    /// returns a span and `ArgKind` information that describes the
+    /// arguments it expects. This can be supplied to
+    /// `report_arg_count_mismatch`.
+    pub fn get_fn_like_arguments(&self, node: hir::map::Node) -> (Span, Vec<ArgKind>) {
         match node {
             hir::map::NodeExpr(&hir::Expr {
                 node: hir::ExprClosure(_, ref _decl, id, span, _),
@@ -829,7 +833,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                             ..
                         } = arg.pat.clone().into_inner() {
                             ArgKind::Tuple(
-                                span,
+                                Some(span),
                                 args.iter().map(|pat| {
                                     let snippet = self.tcx.sess.codemap()
                                         .span_to_snippet(pat.span).unwrap();
@@ -862,7 +866,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                 (self.tcx.sess.codemap().def_span(span), decl.inputs.iter()
                         .map(|arg| match arg.clone().into_inner().node {
                     hir::TyTup(ref tys) => ArgKind::Tuple(
-                        arg.span,
+                        Some(arg.span),
                         tys.iter()
                             .map(|_| ("_".to_owned(), "_".to_owned()))
                             .collect::<Vec<_>>(),
@@ -874,7 +878,10 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
         }
     }
 
-    fn report_arg_count_mismatch(
+    /// Reports an error when the number of arguments needed by a
+    /// trait match doesn't match the number that the expression
+    /// provides.
+    pub fn report_arg_count_mismatch(
         &self,
         span: Span,
         found_span: Option<Span>,
@@ -1363,13 +1370,34 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
     }
 }
 
-enum ArgKind {
+/// Summarizes information
+pub enum ArgKind {
+    /// An argument of non-tuple type. Parameters are (name, ty)
     Arg(String, String),
-    Tuple(Span, Vec<(String, String)>),
+
+    /// An argument of tuple type. For a "found" argument, the span is
+    /// the locationo in the source of the pattern. For a "expected"
+    /// argument, it will be None. The vector is a list of (name, ty)
+    /// strings for the components of the tuple.
+    Tuple(Option<Span>, Vec<(String, String)>),
 }
 
 impl ArgKind {
     fn empty() -> ArgKind {
         ArgKind::Arg("_".to_owned(), "_".to_owned())
     }
+
+    /// Creates an `ArgKind` from the expected type of an
+    /// argument. This has no name (`_`) and no source spans..
+    pub fn from_expected_ty(t: Ty<'_>) -> ArgKind {
+        match t.sty {
+            ty::TyTuple(ref tys, _) => ArgKind::Tuple(
+                None,
+                tys.iter()
+                   .map(|ty| ("_".to_owned(), format!("{}", ty.sty)))
+                   .collect::<Vec<_>>()
+            ),
+            _ => ArgKind::Arg("_".to_owned(), format!("{}", t.sty)),
+        }
+    }
 }
diff --git a/src/librustc/traits/mod.rs b/src/librustc/traits/mod.rs
@@ -49,7 +49,7 @@ pub use self::util::SupertraitDefIds;
 pub use self::util::transitive_bounds;
 
 mod coherence;
-mod error_reporting;
+pub mod error_reporting;
 mod fulfill;
 mod project;
 mod object_safety;
diff --git a/src/librustc_typeck/check/closure.rs b/src/librustc_typeck/check/closure.rs
@@ -17,14 +17,24 @@ use rustc::hir::def_id::DefId;
 use rustc::infer::{InferOk, InferResult};
 use rustc::infer::LateBoundRegionConversionTime;
 use rustc::infer::type_variable::TypeVariableOrigin;
+use rustc::traits::error_reporting::ArgKind;
 use rustc::ty::{self, ToPolyTraitRef, Ty};
 use rustc::ty::subst::Substs;
 use rustc::ty::TypeFoldable;
 use std::cmp;
 use std::iter;
 use syntax::abi::Abi;
+use syntax::codemap::Span;
 use rustc::hir;
 
+/// What signature do we *expect* the closure to have from context?
+#[derive(Debug)]
+struct ExpectedSig<'tcx> {
+    /// Span that gave us this expectation, if we know that.
+    cause_span: Option<Span>,
+    sig: ty::FnSig<'tcx>,
+}
+
 struct ClosureSignatures<'tcx> {
     bound_sig: ty::PolyFnSig<'tcx>,
     liberated_sig: ty::FnSig<'tcx>,
@@ -63,7 +73,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
         decl: &'gcx hir::FnDecl,
         body: &'gcx hir::Body,
         gen: Option<hir::GeneratorMovability>,
-        expected_sig: Option<ty::FnSig<'tcx>>,
+        expected_sig: Option<ExpectedSig<'tcx>>,
     ) -> Ty<'tcx> {
         debug!(
             "check_closure(opt_kind={:?}, expected_sig={:?})",
@@ -160,10 +170,12 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
         closure_type
     }
 
+    /// Given the expected type, figures out what it can about this closure we
+    /// are about to type check:
     fn deduce_expectations_from_expected_type(
         &self,
         expected_ty: Ty<'tcx>,
-    ) -> (Option<ty::FnSig<'tcx>>, Option<ty::ClosureKind>) {
+    ) -> (Option<ExpectedSig<'tcx>>, Option<ty::ClosureKind>) {
         debug!(
             "deduce_expectations_from_expected_type(expected_ty={:?})",
             expected_ty
@@ -175,7 +187,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                     .projection_bounds()
                     .filter_map(|pb| {
                         let pb = pb.with_self_ty(self.tcx, self.tcx.types.err);
-                        self.deduce_sig_from_projection(&pb)
+                        self.deduce_sig_from_projection(None, &pb)
                     })
                     .next();
                 let kind = object_type
@@ -184,15 +196,21 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                 (sig, kind)
             }
             ty::TyInfer(ty::TyVar(vid)) => self.deduce_expectations_from_obligations(vid),
-            ty::TyFnPtr(sig) => (Some(sig.skip_binder().clone()), Some(ty::ClosureKind::Fn)),
+            ty::TyFnPtr(sig) => {
+                let expected_sig = ExpectedSig {
+                    cause_span: None,
+                    sig: sig.skip_binder().clone(),
+                };
+                (Some(expected_sig), Some(ty::ClosureKind::Fn))
+            }
             _ => (None, None),
         }
     }
 
     fn deduce_expectations_from_obligations(
         &self,
         expected_vid: ty::TyVid,
-    ) -> (Option<ty::FnSig<'tcx>>, Option<ty::ClosureKind>) {
+    ) -> (Option<ExpectedSig<'tcx>>, Option<ty::ClosureKind>) {
         let fulfillment_cx = self.fulfillment_cx.borrow();
         // Here `expected_ty` is known to be a type inference variable.
 
@@ -212,7 +230,12 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                     ty::Predicate::Projection(ref proj_predicate) => {
                         let trait_ref = proj_predicate.to_poly_trait_ref(self.tcx);
                         self.self_type_matches_expected_vid(trait_ref, expected_vid)
-                            .and_then(|_| self.deduce_sig_from_projection(proj_predicate))
+                            .and_then(|_| {
+                                self.deduce_sig_from_projection(
+                                    Some(obligation.cause.span),
+                                    proj_predicate,
+                                )
+                            })
                     }
                     _ => None,
                 }
@@ -262,10 +285,15 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
 
     /// Given a projection like "<F as Fn(X)>::Result == Y", we can deduce
     /// everything we need to know about a closure.
+    ///
+    /// The `cause_span` should be the span that caused us to
+    /// have this expected signature, or `None` if we can't readily
+    /// know that.
     fn deduce_sig_from_projection(
         &self,
+        cause_span: Option<Span>,
         projection: &ty::PolyProjectionPredicate<'tcx>,
-    ) -> Option<ty::FnSig<'tcx>> {
+    ) -> Option<ExpectedSig<'tcx>> {
         let tcx = self.tcx;
 
         debug!("deduce_sig_from_projection({:?})", projection);
@@ -297,16 +325,16 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
             ret_param_ty
         );
 
-        let fn_sig = self.tcx.mk_fn_sig(
+        let sig = self.tcx.mk_fn_sig(
             input_tys.cloned(),
             ret_param_ty,
             false,
             hir::Unsafety::Normal,
             Abi::Rust,
         );
-        debug!("deduce_sig_from_projection: fn_sig {:?}", fn_sig);
+        debug!("deduce_sig_from_projection: sig {:?}", sig);
 
-        Some(fn_sig)
+        Some(ExpectedSig { cause_span, sig })
     }
 
     fn self_type_matches_expected_vid(
@@ -330,7 +358,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
         expr_def_id: DefId,
         decl: &hir::FnDecl,
         body: &hir::Body,
-        expected_sig: Option<ty::FnSig<'tcx>>,
+        expected_sig: Option<ExpectedSig<'tcx>>,
     ) -> ClosureSignatures<'tcx> {
         if let Some(e) = expected_sig {
             self.sig_of_closure_with_expectation(expr_def_id, decl, body, e)
@@ -406,7 +434,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
         expr_def_id: DefId,
         decl: &hir::FnDecl,
         body: &hir::Body,
-        expected_sig: ty::FnSig<'tcx>,
+        expected_sig: ExpectedSig<'tcx>,
     ) -> ClosureSignatures<'tcx> {
         debug!(
             "sig_of_closure_with_expectation(expected_sig={:?})",
@@ -416,20 +444,24 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
         // Watch out for some surprises and just ignore the
         // expectation if things don't see to match up with what we
         // expect.
-        if expected_sig.variadic != decl.variadic {
-            return self.sig_of_closure_no_expectation(expr_def_id, decl, body);
-        } else if expected_sig.inputs_and_output.len() != decl.inputs.len() + 1 {
-            // we could probably handle this case more gracefully
+        if expected_sig.sig.variadic != decl.variadic {
             return self.sig_of_closure_no_expectation(expr_def_id, decl, body);
+        } else if expected_sig.sig.inputs_and_output.len() != decl.inputs.len() + 1 {
+            return self.sig_of_closure_with_mismatched_number_of_arguments(
+                expr_def_id,
+                decl,
+                body,
+                expected_sig,
+            );
         }
 
         // Create a `PolyFnSig`. Note the oddity that late bound
         // regions appearing free in `expected_sig` are now bound up
         // in this binder we are creating.
-        assert!(!expected_sig.has_regions_escaping_depth(1));
+        assert!(!expected_sig.sig.has_regions_escaping_depth(1));
         let bound_sig = ty::Binder(self.tcx.mk_fn_sig(
-            expected_sig.inputs().iter().cloned(),
-            expected_sig.output(),
+            expected_sig.sig.inputs().iter().cloned(),
+            expected_sig.sig.output(),
             decl.variadic,
             hir::Unsafety::Normal,
             Abi::RustCall,
@@ -455,6 +487,35 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
         closure_sigs
     }
 
+    fn sig_of_closure_with_mismatched_number_of_arguments(
+        &self,
+        expr_def_id: DefId,
+        decl: &hir::FnDecl,
+        body: &hir::Body,
+        expected_sig: ExpectedSig<'tcx>,
+    ) -> ClosureSignatures<'tcx> {
+        let expr_map_node = self.tcx.hir.get_if_local(expr_def_id).unwrap();
+        let expected_args: Vec<_> = expected_sig
+            .sig
+            .inputs()
+            .iter()
+            .map(|ty| ArgKind::from_expected_ty(ty))
+            .collect();
+        let (closure_span, found_args) = self.get_fn_like_arguments(expr_map_node);
+        let expected_span = expected_sig.cause_span.unwrap_or(closure_span);
+        self.report_arg_count_mismatch(
+            expected_span,
+            Some(closure_span),
+            expected_args,
+            found_args,
+            true,
+        ).emit();
+
+        let error_sig = self.error_sig_of_closure(decl);
+
+        self.closure_sigs(expr_def_id, body, error_sig)
+    }
+
     /// Enforce the user's types against the expectation.  See
     /// `sig_of_closure_with_expectation` for details on the overall
     /// strategy.
@@ -560,6 +621,38 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
         result
     }
 
+    /// Converts the types that the user supplied, in case that doing
+    /// so should yield an error, but returns back a signature where
+    /// all parameters are of type `TyErr`.
+    fn error_sig_of_closure(&self, decl: &hir::FnDecl) -> ty::PolyFnSig<'tcx> {
+        let astconv: &AstConv = self;
+
+        let supplied_arguments = decl.inputs.iter().map(|a| {
+            // Convert the types that the user supplied (if any), but ignore them.
+            astconv.ast_ty_to_ty(a);
+            self.tcx.types.err
+        });
+
+        match decl.output {
+            hir::Return(ref output) => {
+                astconv.ast_ty_to_ty(&output);
+            }
+            hir::DefaultReturn(_) => {}
+        }
+
+        let result = ty::Binder(self.tcx.mk_fn_sig(
+            supplied_arguments,
+            self.tcx.types.err,
+            decl.variadic,
+            hir::Unsafety::Normal,
+            Abi::RustCall,
+        ));
+
+        debug!("supplied_sig_of_closure: result={:?}", result);
+
+        result
+    }
+
     fn closure_sigs(
         &self,
         expr_def_id: DefId,
diff --git a/src/test/ui/mismatched_types/closure-arg-count-expected-type-issue-47244.rs b/src/test/ui/mismatched_types/closure-arg-count-expected-type-issue-47244.rs
@@ -0,0 +1,29 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Regression test for #47244: in this specific scenario, when the
+// expected type indicated 1 argument but the closure takes two, we
+// would (early on) create type variables for the type of `b`. If the
+// user then attempts to invoke a method on `b`, we would get an error
+// saying that the type of `b` must be known, which was not very
+// helpful.
+
+use std::collections::HashMap;
+fn main() {
+
+    let m = HashMap::new();
+    m.insert( "foo", "bar" );
+
+    m.iter().map( |_, b| {
+        //~^ ERROR closure is expected to take a single 2-tuple
+
+        b.to_string()
+    });
+}
diff --git a/src/test/ui/mismatched_types/closure-arg-count-expected-type-issue-47244.stderr b/src/test/ui/mismatched_types/closure-arg-count-expected-type-issue-47244.stderr
@@ -0,0 +1,14 @@
+error[E0593]: closure is expected to take a single 2-tuple as argument, but it takes 2 distinct arguments
+  --> $DIR/closure-arg-count-expected-type-issue-47244.rs:24:14
+   |
+24 |     m.iter().map( |_, b| {
+   |              ^^^  ------ takes 2 distinct arguments
+   |              |
+   |              expected closure that takes a single 2-tuple as argument
+help: change the closure to accept a tuple instead of individual arguments
+   |
+24 |     m.iter().map( |(_, b)| {
+   |                   ^^^^^^^^
+
+error: aborting due to previous error
+
