splitting methods into smaller ones, add docs, better variable naming

3 files changed, 109 insertions, 64 deletions

diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs
index 7eadc9efe6e..de9fb754935 100644
--- a/src/librustc_typeck/check/demand.rs
+++ b/src/librustc_typeck/check/demand.rs
@@ -61,22 +61,50 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
         }
     }
 
-    fn check_ref(&self, expr: &hir::Expr, checked_ty: Ty<'tcx>,
-                 expected: Ty<'tcx>) -> Option<String> {
-        match (&checked_ty.sty, &expected.sty) {
-            (&ty::TyRef(_, x_mutability), &ty::TyRef(_, y_mutability)) => {
+    /// This function is used to determine potential "simple" improvements or users' errors and
+    /// provide them useful help. For example:
+    ///
+    /// ```
+    /// fn some_fn(s: &str) {}
+    ///
+    /// let x = "hey!".to_owned();
+    /// some_fn(x); // error
+    /// ```
+    ///
+    /// No need to find every potential function which could make a coercion to transform a
+    /// `String` into a `&str` since a `&` would do the trick!
+    ///
+    /// In addition of this check, it also checks between references mutability state. If the
+    /// expected is mutable but the provided isn't, maybe we could just say "Hey, try with
+    /// `&mut`!".
+    fn check_ref(&self,
+                 expr: &hir::Expr,
+                 checked_ty: Ty<'tcx>,
+                 expected: Ty<'tcx>)
+                 -> Option<String> {
+        match (&expected.sty, &checked_ty.sty) {
+            (&ty::TyRef(_, expected_mutability),
+             &ty::TyRef(_, checked_mutability)) => {
                 // check if there is a mutability difference
-                if x_mutability.mutbl == hir::Mutability::MutImmutable &&
-                   x_mutability.mutbl != y_mutability.mutbl &&
-                   self.can_sub_types(&x_mutability.ty, y_mutability.ty).is_ok() {
+                if checked_mutability.mutbl == hir::Mutability::MutImmutable &&
+                   checked_mutability.mutbl != expected_mutability.mutbl &&
+                   self.can_sub_types(&checked_mutability.ty,
+                                      expected_mutability.ty).is_ok() {
                     if let Ok(src) = self.tcx.sess.codemap().span_to_snippet(expr.span) {
                         return Some(format!("try with `&mut {}`", &src.replace("&", "")));
                     }
                 }
                 None
             }
-            (_, &ty::TyRef(_, mutability)) => {
-                // check if it can work when put into a ref
+            (&ty::TyRef(_, mutability), _) => {
+                // Check if it can work when put into a ref. For example:
+                //
+                // ```
+                // fn bar(x: &mut i32) {}
+                //
+                // let x = 0u32;
+                // bar(&x); // error, expected &mut
+                // ```
                 let ref_ty = match mutability.mutbl {
                     hir::Mutability::MutMutable => self.tcx.mk_mut_ref(
                                                        self.tcx.mk_region(ty::ReStatic),
@@ -110,11 +138,11 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
             let mode = probe::Mode::MethodCall;
             let suggestions = if let Some(s) = self.check_ref(expr, checked_ty, expected) {
                 Some(s)
-            } else if let Ok(methods) = self.probe_return(syntax_pos::DUMMY_SP,
-                                                          mode,
-                                                          expected,
-                                                          checked_ty,
-                                                          ast::DUMMY_NODE_ID) {
+            } else if let Ok(methods) = self.probe_for_return_type(syntax_pos::DUMMY_SP,
+                                                                   mode,
+                                                                   expected,
+                                                                   checked_ty,
+                                                                   ast::DUMMY_NODE_ID) {
                 let suggestions: Vec<_> =
                     methods.iter()
                            .map(|ref x| {
@@ -143,29 +171,38 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
         }
     }
 
+    fn format_method_suggestion(&self, method: &ImplOrTraitItem<'tcx>) -> String {
+        format!(".{}({})",
+                method.name(),
+                if self.has_not_input_arg(method) {
+                    ""
+                } else {
+                    "..."
+                })
+    }
+
+    fn display_suggested_methods(&self, methods: &[Rc<ImplOrTraitItem<'tcx>>]) -> String {
+        methods.iter()
+               .take(5)
+               .map(|method| self.format_method_suggestion(&*method))
+               .collect::<Vec<String>>()
+               .join("\n - ")
+    }
+
     fn get_best_match(&self, methods: &[Rc<ImplOrTraitItem<'tcx>>]) -> String {
-        if methods.len() == 1 {
-            return format!(" - {}", methods[0].name());
-        }
-        let no_argument_methods: Vec<&Rc<ImplOrTraitItem<'tcx>>> =
+        let no_argument_methods: Vec<Rc<ImplOrTraitItem<'tcx>>> =
             methods.iter()
                    .filter(|ref x| self.has_not_input_arg(&*x))
+                   .map(|x| x.clone())
                    .collect();
         if no_argument_methods.len() > 0 {
-            no_argument_methods.iter()
-                               .take(5)
-                               .map(|method| format!(".{}()", method.name()))
-                               .collect::<Vec<String>>()
-                               .join("\n - ")
+            self.display_suggested_methods(&no_argument_methods)
         } else {
-            methods.iter()
-                   .take(5)
-                   .map(|method| format!(".{}()", method.name()))
-                   .collect::<Vec<String>>()
-                   .join("\n - ")
+            self.display_suggested_methods(&methods)
         }
     }
 
+    // This function checks if the method isn't static and takes other arguments than `self`.
     fn has_not_input_arg(&self, method: &ImplOrTraitItem<'tcx>) -> bool {
         match *method {
             ImplOrTraitItem::MethodTraitItem(ref x) => {
diff --git a/src/librustc_typeck/check/method/mod.rs b/src/librustc_typeck/check/method/mod.rs
index bd240c74133..968cc242c60 100644
--- a/src/librustc_typeck/check/method/mod.rs
+++ b/src/librustc_typeck/check/method/mod.rs
@@ -91,7 +91,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                          allow_private: bool)
                          -> bool {
         let mode = probe::Mode::MethodCall;
-        match self.probe_method(span, mode, method_name, self_ty, call_expr_id) {
+        match self.probe_for_name(span, mode, method_name, self_ty, call_expr_id) {
             Ok(..) => true,
             Err(NoMatch(..)) => false,
             Err(Ambiguity(..)) => true,
@@ -130,7 +130,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
 
         let mode = probe::Mode::MethodCall;
         let self_ty = self.resolve_type_vars_if_possible(&self_ty);
-        let pick = self.probe_method(span, mode, method_name, self_ty, call_expr.id)?.remove(0);
+        let pick = self.probe_for_name(span, mode, method_name, self_ty, call_expr.id)?.remove(0);
 
         if let Some(import_id) = pick.import_id {
             self.tcx.used_trait_imports.borrow_mut().insert(import_id);
@@ -328,7 +328,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                         expr_id: ast::NodeId)
                         -> Result<Def, MethodError<'tcx>> {
         let mode = probe::Mode::Path;
-        let picks = self.probe_method(span, mode, method_name, self_ty, expr_id)?;
+        let picks = self.probe_for_name(span, mode, method_name, self_ty, expr_id)?;
         let pick = &picks[0];
 
         if let Some(import_id) = pick.import_id {
diff --git a/src/librustc_typeck/check/method/probe.rs b/src/librustc_typeck/check/method/probe.rs
index 1961a6247ed..4558aa5b2b5 100644
--- a/src/librustc_typeck/check/method/probe.rs
+++ b/src/librustc_typeck/check/method/probe.rs
@@ -150,41 +150,41 @@ pub enum Mode {
 }
 
 impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
-    pub fn probe_return(&self,
-                        span: Span,
-                        mode: Mode,
-                        return_type: Ty<'tcx>,
-                        self_ty: Ty<'tcx>,
-                        scope_expr_id: ast::NodeId)
-                        -> PickResult<'tcx> {
+    pub fn probe_for_return_type(&self,
+                                 span: Span,
+                                 mode: Mode,
+                                 return_type: Ty<'tcx>,
+                                 self_ty: Ty<'tcx>,
+                                 scope_expr_id: ast::NodeId)
+                                 -> PickResult<'tcx> {
         debug!("probe(self_ty={:?}, return_type={}, scope_expr_id={})",
                self_ty,
                return_type,
                scope_expr_id);
-        self._probe(span, mode, LookingFor::ReturnType(return_type), self_ty, scope_expr_id)
+        self.probe_for(span, mode, LookingFor::ReturnType(return_type), self_ty, scope_expr_id)
     }
 
-    pub fn probe_method(&self,
-                        span: Span,
-                        mode: Mode,
-                        item_name: ast::Name,
-                        self_ty: Ty<'tcx>,
-                        scope_expr_id: ast::NodeId)
-                        -> PickResult<'tcx> {
+    pub fn probe_for_name(&self,
+                          span: Span,
+                          mode: Mode,
+                          item_name: ast::Name,
+                          self_ty: Ty<'tcx>,
+                          scope_expr_id: ast::NodeId)
+                          -> PickResult<'tcx> {
         debug!("probe(self_ty={:?}, item_name={}, scope_expr_id={})",
                self_ty,
                item_name,
                scope_expr_id);
-        self._probe(span, mode, LookingFor::MethodName(item_name), self_ty, scope_expr_id)
+        self.probe_for(span, mode, LookingFor::MethodName(item_name), self_ty, scope_expr_id)
     }
 
-    fn _probe(&self,
-              span: Span,
-              mode: Mode,
-              looking_for: LookingFor<'tcx>,
-              self_ty: Ty<'tcx>,
-              scope_expr_id: ast::NodeId)
-              -> PickResult<'tcx> {
+    fn probe_for(&self,
+                 span: Span,
+                 mode: Mode,
+                 looking_for: LookingFor<'tcx>,
+                 self_ty: Ty<'tcx>,
+                 scope_expr_id: ast::NodeId)
+                 -> PickResult<'tcx> {
 
         // FIXME(#18741) -- right now, creating the steps involves evaluating the
         // `*` operator, which registers obligations that then escape into
@@ -265,6 +265,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
 
         let final_ty = match looking_for {
             &LookingFor::MethodName(_) => autoderef.unambiguous_final_ty(),
+            // Since ReturnType case tries to coerce the returned type to the
+            // expected one, we need all the information!
             &LookingFor::ReturnType(_) => self_ty,
         };
         match final_ty.sty {
@@ -627,10 +629,10 @@ impl<'a, 'gcx, 'tcx> ProbeContext<'a, 'gcx, 'tcx> {
         match *method {
             ty::ImplOrTraitItem::MethodTraitItem(ref x) => {
                 self.probe(|_| {
-                    let output = self.replace_late_bound_regions_with_fresh_var(
-                        self.span, infer::FnCall, &x.fty.sig.output());
                     let substs = self.fresh_substs_for_item(self.span, method.def_id());
-                    let output = output.0.subst(self.tcx, substs);
+                    let output = x.fty.sig.output().subst(self.tcx, substs);
+                    let (output, _) = self.replace_late_bound_regions_with_fresh_var(
+                        self.span, infer::FnCall, &output);
                     self.can_sub_types(output, expected).is_ok()
                 })
             }
@@ -950,10 +952,17 @@ impl<'a, 'gcx, 'tcx> ProbeContext<'a, 'gcx, 'tcx> {
         let steps = self.steps.clone();
 
         match self.looking_for {
-            LookingFor::MethodName(_) => steps.iter()
-                                              .filter_map(|step| self.pick_step(step))
-                                              .next(),
+            LookingFor::MethodName(_) => {
+                // find the first step that works
+                steps.iter()
+                     .filter_map(|step| self.pick_step(step))
+                     .next()
+            }
             LookingFor::ReturnType(_) => {
+                // Normally, we stop at the first step where we find a positive match.
+                // But when we are scanning for methods with a suitable return type,
+                // these methods have distinct names and hence may not shadow one another
+                // (also, this is just for hints, so precision is less important).
                 let mut ret = Vec::new();
 
                 for step in steps.iter() {
@@ -1050,10 +1059,9 @@ impl<'a, 'gcx, 'tcx> ProbeContext<'a, 'gcx, 'tcx> {
         match self.looking_for {
             LookingFor::MethodName(_) => it.nth(0),
             LookingFor::ReturnType(_) => {
-                let mut ret = Vec::new();
-                it.filter_map(|entry| entry.ok())
-                  .map(|mut v| { ret.append(&mut v); })
-                  .all(|_| true);
+                let ret = it.filter_map(|entry| entry.ok())
+                            .flat_map(|v| v)
+                            .collect::<Vec<_>>();
 
                 if ret.len() < 1 {
                     None