address review comments

rust-lang · Centril · Sep 16, 2019 · Sep 18, 2019 · Sep 18, 2019 · Sep 18, 2019
commit 65251ea73ab5d19e70a6ff4b64fb218a03504b6b
diff --git a/src/librustc/hir/lowering/expr.rs b/src/librustc/hir/lowering/expr.rs
@@ -392,36 +392,34 @@ impl LoweringContext<'_> {
         )
     }
 
+    /// Desugar `try { <stmts>; <expr> }` into `{ <stmts>; ::std::ops::Try::from_ok(<expr>) }`,
+    /// `try { <stmts>; }` into `{ <stmts>; ::std::ops::Try::from_ok(()) }`
+    /// and save the block id to use it as a break target for desugaring of the `?` operator.
     fn lower_expr_try_block(&mut self, body: &Block) -> hir::ExprKind {
         self.with_catch_scope(body.id, |this| {
             let mut block = this.lower_block(body, true).into_inner();
 
-            let tail_expr = block.expr.take().map_or_else(
-                || {
-                    let unit_span = this.mark_span_with_reason(
-                        DesugaringKind::TryBlock,
-                        this.sess.source_map().end_point(body.span),
-                        None
-                    );
-                    this.expr_unit(unit_span)
-                },
-                |x: P<hir::Expr>| x.into_inner(),
-            );
-
-            let from_ok_span = this.mark_span_with_reason(
+            let try_span = this.mark_span_with_reason(
                 DesugaringKind::TryBlock,
-                tail_expr.span,
+                body.span,
                 this.allow_try_trait.clone(),
             );
 
+            // Final expression of the block (if present) or `()` with span at the end of block
+            let tail_expr = block.expr.take().map_or_else(
+                || this.expr_unit(this.sess.source_map().end_point(try_span)),
+                |x: P<hir::Expr>| x.into_inner(),
+            );
+
             let ok_wrapped_span = this.mark_span_with_reason(
                 DesugaringKind::TryBlock,
                 tail_expr.span,
                 None
             );
 
+            // `::std::ops::Try::from_ok($tail_expr)`
             block.expr = Some(this.wrap_in_try_constructor(
-                sym::from_ok, from_ok_span, tail_expr, ok_wrapped_span));
+                sym::from_ok, try_span, tail_expr, ok_wrapped_span));
 
             hir::ExprKind::Block(P(block), None)
         })

diff --git a/src/librustc_typeck/check/expr.rs b/src/librustc_typeck/check/expr.rs
@@ -148,20 +148,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         debug!(">> type-checking: expr={:?} expected={:?}",
                expr, expected);
 
-        // If when desugaring the try block we ok-wrapped an expression that diverges
-        // (e.g. `try { return }`) then technically the ok-wrapping expression is unreachable.
-        // But since it is autogenerated code the resulting warning is confusing for the user
-        // so we want avoid generating it.
-        // Ditto for the autogenerated `Try::from_ok(())` at the end of e.g. `try { return; }`.
-        let (is_try_block_ok_wrapped_expr, is_try_block_generated_expr) = match expr.node {
-            ExprKind::Call(_, ref args) if expr.span.is_desugaring(DesugaringKind::TryBlock) => {
-                (true, args.len() == 1 && args[0].span.is_desugaring(DesugaringKind::TryBlock))
-            }
-            _ => (false, false),
+        // True if `expr` is a `Try::from_ok(())` that is a result of desugaring a try block
+        // without the final expr (e.g. `try { return; }`). We don't want to generate an
+        // unreachable_code lint for it since warnings for autogenerated code are confusing.
+        let is_try_block_generated_unit_expr = match expr.node {
+            ExprKind::Call(_, ref args) if expr.span.is_desugaring(DesugaringKind::TryBlock) =>
+                args.len() == 1 && args[0].span.is_desugaring(DesugaringKind::TryBlock),
+
+            _ => false,
         };
 
         // Warn for expressions after diverging siblings.
-        if !is_try_block_generated_expr {
+        if !is_try_block_generated_unit_expr {
             self.warn_if_unreachable(expr.hir_id, expr.span, "expression");
         }
 
@@ -174,15 +172,17 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         let ty = self.check_expr_kind(expr, expected, needs);
 
         // Warn for non-block expressions with diverging children.
-        if !is_try_block_ok_wrapped_expr {
-            match expr.node {
-                ExprKind::Block(..) | ExprKind::Loop(..) | ExprKind::Match(..) => {},
-                ExprKind::Call(ref callee, _) =>
-                    self.warn_if_unreachable(expr.hir_id, callee.span, "call"),
-                ExprKind::MethodCall(_, ref span, _) =>
-                    self.warn_if_unreachable(expr.hir_id, *span, "call"),
-                _ => self.warn_if_unreachable(expr.hir_id, expr.span, "expression"),
-            }
+        match expr.node {
+            ExprKind::Block(..) | ExprKind::Loop(..) | ExprKind::Match(..) => {},
+            // If `expr` is a result of desugaring the try block and is an ok-wrapped
+            // diverging expression (e.g. it arose from desugaring of `try { return }`),
+            // we skip issuing a warning because it is autogenerated code.
+            ExprKind::Call(..) if expr.span.is_desugaring(DesugaringKind::TryBlock) => {},
+            ExprKind::Call(ref callee, _) =>
+                self.warn_if_un
CAD6
reachable(expr.hir_id, callee.span, "call"),
+            ExprKind::MethodCall(_, ref span, _) =>
+                self.warn_if_unreachable(expr.hir_id, *span, "call"),
+            _ => self.warn_if_unreachable(expr.hir_id, expr.span, "expression"),
         }
 
         // Any expression that produces a value of type `!` must have diverged

diff --git a/src/test/ui/try-block/try-block-bad-type.stderr b/src/test/ui/try-block/try-block-bad-type.stderr
@@ -32,18 +32,18 @@ LL |     let res: Result<i32, i32> = try { };
               found type `()`
 
 error[E0277]: the trait bound `(): std::ops::Try` is not satisfied
-  --> $DIR/try-block-bad-type.rs:17:25
+  --> $DIR/try-block-bad-type.rs:17:23
    |
 LL |     let res: () = try { };
-   |                         ^ the trait `std::ops::Try` is not implemented for `()`
+   |                       ^^^ the trait `std::ops::Try` is not implemented for `()`
    |
    = note: required by `std::ops::Try::from_ok`
 
 error[E0277]: the trait bound `i32: std::ops::Try` is not satisfied
-  --> $DIR/try-block-bad-type.rs:19:26
+  --> $DIR/try-block-bad-type.rs:19:24
    |
 LL |     let res: i32 = try { 5 };
-   |                          ^ the trait `std::ops::Try` is not implemented for `i32`
+   |                        ^^^^^ the trait `std::ops::Try` is not implemented for `i32`
    |
    = note: required by `std::ops::Try::from_ok`