Auto merge of #42850 - estebank:unwanted-return-rotj, r=nikomatsakis

Detect missing `;` on methods with return type `()`

 - Point out the origin of a type requirement when it is the return type
   of a method
 - Point out possibly missing semicolon when the return type is `()` and
   the implicit return makes sense as a statement
 - Suggest changing the return type of methods with default return type
 - Don't suggest changing the return type on `fn main()`
 - Don't suggest changing the return type on impl fn
 - Suggest removal of semicolon (instead of being help)

Author: bors

src/librustc/hir/map/mod.rs

@@ -594,8 +594,12 @@ impl<'hir> Map<'hir> {
     /// last good node id we found. Note that reaching the crate root (id == 0),
     /// is not an error, since items in the crate module have the crate root as
     /// parent.
-    fn walk_parent_nodes<F>(&self, start_id: NodeId, found: F) -> Result<NodeId, NodeId>
-        where F: Fn(&Node<'hir>) -> bool
+    fn walk_parent_nodes<F, F2>(&self,
+                                start_id: NodeId,
+                                found: F,
+                                bail_early: F2)
+        -> Result<NodeId, NodeId>
+        where F: Fn(&Node<'hir>) -> bool, F2: Fn(&Node<'hir>) -> bool
     {
         let mut id = start_id;
         loop {
@@ -616,6 +620,8 @@ impl<'hir> Map<'hir> {
                 Some(ref node) => {
                     if found(node) {
                         return Ok(parent_node);
+                    } else if bail_early(node) {
+                        return Err(parent_node);
                     }
                 }
                 None => {
@@ -626,6 +632,56 @@ impl<'hir> Map<'hir> {
         }
     }
 
+    /// Retrieve the NodeId for `id`'s enclosing method, unless there's a
+    /// `while` or `loop` before reacing it, as block tail returns are not
+    /// available in them.
+    ///
+    /// ```
+    /// fn foo(x: usize) -> bool {
+    ///     if x == 1 {
+    ///         true  // `get_return_block` gets passed the `id` corresponding
+    ///     } else {  // to this, it will return `foo`'s `NodeId`.
+    ///         false
+    ///     }
+    /// }
+    /// ```
+    ///
+    /// ```
+    /// fn foo(x: usize) -> bool {
+    ///     loop {
+    ///         true  // `get_return_block` gets passed the `id` corresponding
+    ///     }         // to this, it will return `None`.
+    ///     false
+    /// }
+    /// ```
+    pub fn get_return_block(&self, id: NodeId) -> Option<NodeId> {
+        let match_fn = |node: &Node| {
+            match *node {
+                NodeItem(_) |
+                NodeForeignItem(_) |
+                NodeTraitItem(_) |
+                NodeImplItem(_) => true,
+                _ => false,
+            }
+        };
+        let match_non_returning_block = |node: &Node| {
+            match *node {
+                NodeExpr(ref expr) => {
+                    match expr.node {
+                        ExprWhile(..) | ExprLoop(..) => true,
+                        _ => false,
+                    }
+                }
+                _ => false,
+            }
+        };
+
+        match self.walk_parent_nodes(id, match_fn, match_non_returning_block) {
+            Ok(id) => Some(id),
+            Err(_) => None,
+        }
+    }
+
     /// Retrieve the NodeId for `id`'s parent item, or `id` itself if no
     /// parent item is in this map. The "parent item" is the closest parent node
     /// in the AST which is recorded by the map and is an item, either an item
@@ -637,7 +693,7 @@ impl<'hir> Map<'hir> {
             NodeTraitItem(_) |
             NodeImplItem(_) => true,
             _ => false,
-        }) {
+        }, |_| false) {
             Ok(id) => id,
             Err(id) => id,
         }
@@ -649,7 +705,7 @@ impl<'hir> Map<'hir> {
         let id = match self.walk_parent_nodes(id, |node| match *node {
             NodeItem(&Item { node: Item_::ItemMod(_), .. }) => true,
             _ => false,
-        }) {
+        }, |_| false) {
             Ok(id) => id,
             Err(id) => id,
         };
@@ -668,7 +724,7 @@ impl<'hir> Map<'hir> {
             NodeImplItem(_) |
             NodeBlock(_) => true,
             _ => false,
-        }) {
+        }, |_| false) {
             Ok(id) => Some(id),
             Err(_) => None,
         }

src/librustc/traits/error_reporting.rs

@@ -1088,7 +1088,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
             ObligationCauseCode::VariableType(_) => {
                 err.note("all local variables must have a statically known size");
             }
-            ObligationCauseCode::ReturnType => {
+            ObligationCauseCode::SizedReturnType => {
                 err.note("the return type of a function must have a \
                           statically known size");
             }
@@ -1133,6 +1133,8 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                               but not on the corresponding trait method",
                              predicate));
             }
+            ObligationCauseCode::ReturnType(_) |
+            ObligationCauseCode::BlockTailExpression(_) => (),
         }
     }
 

src/librustc/traits/mod.rs

@@ -118,65 +118,76 @@ pub enum ObligationCauseCode<'tcx> {
     /// Obligation incurred due to an object cast.
     ObjectCastObligation(/* Object type */ Ty<'tcx>),
 
-    /// Various cases where expressions must be sized/copy/etc:
-    AssignmentLhsSized,        // L = X implies that L is Sized
-    StructInitializerSized,    // S { ... } must be Sized
-    VariableType(ast::NodeId), // Type of each variable must be Sized
-    ReturnType,                // Return type must be Sized
-    RepeatVec,                 // [T,..n] --> T must be Copy
-
-    // Types of fields (other than the last) in a struct must be sized.
+    // Various cases where expressions must be sized/copy/etc:
+    /// L = X implies that L is Sized
+    AssignmentLhsSized,
+    /// S { ... } must be Sized
+    StructInitializerSized,
+    /// Type of each variable must be Sized
+    VariableType(ast::NodeId),
+    /// Return type must be Sized
+    SizedReturnType,
+    /// [T,..n] --> T must be Copy
+    RepeatVec,
+
+    /// Types of fields (other than the last) in a struct must be sized.
     FieldSized,
 
-    // Constant expressions must be sized.
+    /// Constant expressions must be sized.
     ConstSized,
 
-    // static items must have `Sync` type
+    /// static items must have `Sync` type
     SharedStatic,
 
     BuiltinDerivedObligation(DerivedObligationCause<'tcx>),
 
     ImplDerivedObligation(DerivedObligationCause<'tcx>),
 
-    // error derived when matching traits/impls; see ObligationCause for more details
+    /// error derived when matching traits/impls; see ObligationCause for more details
     CompareImplMethodObligation {
         item_name: ast::Name,
         impl_item_def_id: DefId,
         trait_item_def_id: DefId,
         lint_id: Option<ast::NodeId>,
     },
 
-    // Checking that this expression can be assigned where it needs to be
+    /// Checking that this expression can be assigned where it needs to be
     // FIXME(eddyb) #11161 is the original Expr required?
     ExprAssignable,
 
-    // Computing common supertype in the arms of a match expression
+    /// Computing common supertype in the arms of a match expression
     MatchExpressionArm { arm_span: Span,
                          source: hir::MatchSource },
 
-    // Computing common supertype in an if expression
+    /// Computing common supertype in an if expression
     IfExpression,
 
-    // Computing common supertype of an if expression with no else counter-part
+    /// Computing common supertype of an if expression with no else counter-part
     IfExpressionWithNoElse,
 
-    // `where a == b`
+    /// `where a == b`
     EquatePredicate,
 
-    // `main` has wrong type
+    /// `main` has wrong type
     MainFunctionType,
 
-    // `start` has wrong type
+    /// `start` has wrong type
     StartFunctionType,
 
-    // intrinsic has wrong type
+    /// intrinsic has wrong type
     IntrinsicType,
 
-    // method receiver
+    /// method receiver
     MethodReceiver,
 
-    // `return` with no expression
+    /// `return` with no expression
     ReturnNoExpression,
+
+    /// `return` with an expression
+    ReturnType(ast::NodeId),
+
+    /// Block implicit return
+    BlockTailExpression(ast::NodeId),
 }
 
 #[derive(Clone, Debug, PartialEq, Eq)]

src/librustc/traits/structural_impls.rs

@@ -191,7 +191,8 @@ impl<'a, 'tcx> Lift<'tcx> for traits::ObligationCauseCode<'a> {
             super::AssignmentLhsSized => Some(super::AssignmentLhsSized),
             super::StructInitializerSized => Some(super::StructInitializerSized),
             super::VariableType(id) => Some(super::VariableType(id)),
-            super::ReturnType => Some(super::ReturnType),
+            super::ReturnType(id) => Some(super::ReturnType(id)),
+            super::SizedReturnType => Some(super::SizedReturnType),
             super::RepeatVec => Some(super::RepeatVec),
             super::FieldSized => Some(super::FieldSized),
             super::ConstSized => Some(super::ConstSized),
@@ -213,34 +214,19 @@ impl<'a, 'tcx> Lift<'tcx> for traits::ObligationCauseCode<'a> {
                     lint_id: lint_id,
                 })
             }
-            super::ExprAssignable => {
-                Some(super::ExprAssignable)
-            }
+            super::ExprAssignable => Some(super::ExprAssignable),
             super::MatchExpressionArm { arm_span, source } => {
                 Some(super::MatchExpressionArm { arm_span: arm_span,
                                                  source: source })
             }
-            super::IfExpression => {
-                Some(super::IfExpression)
-            }
-            super::IfExpressionWithNoElse => {
-                Some(super::IfExpressionWithNoElse)
-            }
-            super::EquatePredicate => {
-                Some(super::EquatePredicate)
-            }
-            super::MainFunctionType => {
-                Some(super::MainFunctionType)
-            }
-            super::StartFunctionType => {
-                Some(super::StartFunctionType)
-            }
-            super::IntrinsicType => {
-                Some(super::IntrinsicType)
-            }
-            super::MethodReceiver => {
-                Some(super::MethodReceiver)
-            }
+            super::IfExpression => Some(super::IfExpression),
+            super::IfExpressionWithNoElse => Some(super::IfExpressionWithNoElse),
+            super::EquatePredicate => Some(super::EquatePredicate),
+            super::MainFunctionType => Some(super::MainFunctionType),
+            super::StartFunctionType => Some(super::StartFunctionType),
+            super::IntrinsicType => Some(super::IntrinsicType),
+            super::MethodReceiver => Some(super::MethodReceiver),
+            super::BlockTailExpression(id) => Some(super::BlockTailExpression(id)),
         }
     }
 }
@@ -492,12 +478,14 @@ impl<'tcx> TypeFoldable<'tcx> for traits::ObligationCauseCode<'tcx> {
             super::AssignmentLhsSized |
             super::StructInitializerSized |
             super::VariableType(_) |
-            super::ReturnType |
+            super::ReturnType(_) |
+            super::SizedReturnType |
             super::ReturnNoExpression |
             super::RepeatVec |
             super::FieldSized |
             super::ConstSized |
             super::SharedStatic |
+            super::BlockTailExpression(_) |
             super::CompareImplMethodObligation { .. } => self.clone(),
 
             super::ProjectionWf(proj) => super::ProjectionWf(proj.fold_with(folder)),
@@ -537,12 +525,14 @@ impl<'tcx> TypeFoldable<'tcx> for traits::ObligationCauseCode<'tcx> {
             super::AssignmentLhsSized |
             super::StructInitializerSized |
             super::VariableType(_) |
-            super::ReturnType |
+            super::ReturnType(_) |
+            super::SizedReturnType |
             super::ReturnNoExpression |
             super::RepeatVec |
             super::FieldSized |
             super::ConstSized |
             super::SharedStatic |
+            super::BlockTailExpression(_) |
             super::CompareImplMethodObligation { .. } => false,
 
             super::ProjectionWf(proj) => proj.visit_with(visitor),

src/librustc/ty/mod.rs

@@ -481,6 +481,18 @@ impl<'tcx> TyS<'tcx> {
             _ => false,
         }
     }
+
+    pub fn is_suggestable(&self) -> bool {
+        match self.sty {
+            TypeVariants::TyAnon(..) |
+            TypeVariants::TyFnDef(..) |
+            TypeVariants::TyFnPtr(..) |
+            TypeVariants::TyDynamic(..) |
+            TypeVariants::TyClosure(..) |
+            TypeVariants::TyProjection(..) => false,
+            _ => true,
+        }
+    }
 }
 
 impl<'a, 'gcx, 'tcx> HashStable<StableHashingContext<'a, 'gcx, 'tcx>> for ty::TyS<'tcx> {

src/librustc_errors/emitter.rs

@@ -47,7 +47,12 @@ impl Emitter for EmitterWriter {
                // don't display multiline suggestions as labels
                sugg.substitution_parts[0].substitutions[0].find('\n').is_none() {
                 let substitution = &sugg.substitution_parts[0].substitutions[0];
-                let msg = format!("help: {} `{}`", sugg.msg, substitution);
+                let msg = if substitution.len() == 0 {
+                    // This substitution is only removal, don't show it
+                    format!("help: {}", sugg.msg)
+                } else {
+                    format!("help: {} `{}`", sugg.msg, substitution)
+                };
                 primary_span.push_span_label(sugg.substitution_spans().next().unwrap(), msg);
             } else {
                 // if there are multiple suggestions, print them all in full

src/librustc_typeck/check/coercion.rs

@@ -1168,6 +1168,18 @@ impl<'gcx, 'tcx, 'exprs, E> CoerceMany<'gcx, 'tcx, 'exprs, E>
                             "`return;` in a function whose return type is not `()`");
                         db.span_label(cause.span, "return type is not ()");
                     }
+                    ObligationCauseCode::BlockTailExpression(blk_id) => {
+                        db = fcx.report_mismatched_types(cause, expected, found, err);
+
+                        let expr = expression.unwrap_or_else(|| {
+                            span_bug!(cause.span,
+                                      "supposed to be part of a block tail expression, but the \
+                                       expression is empty");
+                        });
+                        fcx.suggest_mismatched_types_on_tail(&mut db, expr,
+                                                             expected, found,
+                                                             cause.span, blk_id);
+                    }
                     _ => {
                         db = fcx.report_mismatched_types(cause, expected, found, err);
                     }

src/librustc_typeck/check/demand.rs

@@ -73,15 +73,21 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
         }
     }
 
+    pub fn demand_coerce(&self, expr: &hir::Expr, checked_ty: Ty<'tcx>, expected: Ty<'tcx>) {
+        if let Some(mut err) = self.demand_coerce_diag(expr, checked_ty, expected) {
+            err.emit();
+        }
+    }
+
     // Checks that the type of `expr` can be coerced to `expected`.
     //
     // NB: This code relies on `self.diverges` to be accurate.  In
     // particular, assignments to `!` will be permitted if the
     // diverges flag is currently "always".
-    pub fn demand_coerce(&self,
-                         expr: &hir::Expr,
-                         checked_ty: Ty<'tcx>,
-                         expected: Ty<'tcx>) {
+    pub fn demand_coerce_diag(&self,
+                              expr: &hir::Expr,
+                              checked_ty: Ty<'tcx>,
+                              expected: Ty<'tcx>) -> Option<DiagnosticBuilder<'tcx>> {
         let expected = self.resolve_type_vars_with_obligations(expected);
 
         if let Err(e) = self.try_coerce(expr, checked_ty, self.diverges.get(), expected) {
@@ -105,8 +111,9 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                                       self.get_best_match(&suggestions).join("\n")));
                 }
             }
-            err.emit();
+            return Some(err);
         }
+        None
     }
 
     fn format_method_suggestion(&self, method: &AssociatedItem) -> String {