From 48081065f4badf0544ec02d6ecde673716fd6695 Mon Sep 17 00:00:00 2001 From: Alex Zatelepin Date: Mon, 16 Sep 2019 17:16:17 +0300 Subject: [PATCH 1/6] fix typo --- src/librustc_typeck/check/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index be5723959fb22..064908de48f30 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -472,7 +472,7 @@ pub enum Diverges { WarnedAlways } -// Convenience impls for combinig `Diverges`. +// Convenience impls for combining `Diverges`. impl ops::BitAnd for Diverges { type Output = Self; From 057569e2c2864ce907d780fc022831064c61e984 Mon Sep 17 00:00:00 2001 From: Alex Zatelepin Date: Wed, 18 Sep 2019 18:08:56 +0300 Subject: [PATCH 2/6] fix spurious unreachable_code lints for try{} block ok-wrapping --- src/librustc/hir/lowering/expr.rs | 43 ++++++++++++++++++++++--------- src/librustc/hir/mod.rs | 2 +- src/librustc_typeck/check/expr.rs | 33 ++++++++++++++++++------ 3 files changed, 57 insertions(+), 21 deletions(-) diff --git a/src/librustc/hir/lowering/expr.rs b/src/librustc/hir/lowering/expr.rs index 9dcecedd97cae..0d2c7c13a85b9 100644 --- a/src/librustc/hir/lowering/expr.rs +++ b/src/librustc/hir/lowering/expr.rs @@ -394,17 +394,35 @@ impl LoweringContext<'_> { fn lower_expr_try_block(&mut self, body: &Block) -> hir::ExprKind { self.with_catch_scope(body.id, |this| { - let unstable_span = this.mark_span_with_reason( + 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| x.into_inner(), + ); + + let from_ok_span = this.mark_span_with_reason( DesugaringKind::TryBlock, - body.span, + tail_expr.span, this.allow_try_trait.clone(), ); - let mut block = this.lower_block(body, true).into_inner(); - let tail = block.expr.take().map_or_else( - || this.expr_unit(this.sess.source_map().end_point(unstable_span)), - |x: P| x.into_inner(), + + let ok_wrapped_span = this.mark_span_with_reason( + DesugaringKind::TryBlock, + tail_expr.span, + None ); - block.expr = Some(this.wrap_in_try_constructor(sym::from_ok, tail, unstable_span)); + + block.expr = Some(this.wrap_in_try_constructor( + sym::from_ok, from_ok_span, tail_expr, ok_wrapped_span)); + hir::ExprKind::Block(P(block), None) }) } @@ -412,12 +430,13 @@ impl LoweringContext<'_> { fn wrap_in_try_constructor( &mut self, method: Symbol, - e: hir::Expr, - unstable_span: Span, + method_span: Span, + expr: hir::Expr, + overall_span: Span, ) -> P { let path = &[sym::ops, sym::Try, method]; - let from_err = P(self.expr_std_path(unstable_span, path, None, ThinVec::new())); - P(self.expr_call(e.span, from_err, hir_vec![e])) + let constructor = P(self.expr_std_path(method_span, path, None, ThinVec::new())); + P(self.expr_call(overall_span, constructor, hir_vec![expr])) } fn lower_arm(&mut self, arm: &Arm) -> hir::Arm { @@ -1244,7 +1263,7 @@ impl LoweringContext<'_> { self.expr_call_std_path(try_span, from_path, hir_vec![err_expr]) }; let from_err_expr = - self.wrap_in_try_constructor(sym::from_error, from_expr, unstable_span); + self.wrap_in_try_constructor(sym::from_error, unstable_span, from_expr, try_span); let thin_attrs = ThinVec::from(attrs); let catch_scope = self.catch_scopes.last().map(|x| *x); let ret_expr = if let Some(catch_node) = catch_scope { diff --git a/src/librustc/hir/mod.rs b/src/librustc/hir/mod.rs index 9ae661f0952a4..9b4d88a5a0967 100644 --- a/src/librustc/hir/mod.rs +++ b/src/librustc/hir/mod.rs @@ -861,7 +861,7 @@ pub struct Block { pub span: Span, /// If true, then there may exist `break 'a` values that aim to /// break out of this block early. - /// Used by `'label: {}` blocks and by `catch` statements. + /// Used by `'label: {}` blocks and by `try {}` blocks. pub targeted_by_break: bool, } diff --git a/src/librustc_typeck/check/expr.rs b/src/librustc_typeck/check/expr.rs index 04c8536de8dfe..a93ea0cc00467 100644 --- a/src/librustc_typeck/check/expr.rs +++ b/src/librustc_typeck/check/expr.rs @@ -18,6 +18,7 @@ use crate::util::nodemap::FxHashMap; use crate::astconv::AstConv as _; use errors::{Applicability, DiagnosticBuilder, pluralise}; +use syntax_pos::hygiene::DesugaringKind; use syntax::ast; use syntax::symbol::{Symbol, kw, sym}; use syntax::source_map::Span; @@ -150,8 +151,22 @@ 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), + }; + // Warn for expressions after diverging siblings. - self.warn_if_unreachable(expr.hir_id, expr.span, "expression"); + if !is_try_block_generated_expr { + self.warn_if_unreachable(expr.hir_id, expr.span, "expression"); + } // Hide the outer diverging and has_errors flags. let old_diverges = self.diverges.get(); @@ -162,13 +177,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let ty = self.check_expr_kind(expr, expected, needs); // Warn for non-block expressions with diverging children. - match expr.kind { - 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"), + if !is_try_block_ok_wrapped_expr { + match expr.kind { + 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"), + } } // Any expression that produces a value of type `!` must have diverged From c474c6e8253fe30684aa908ebef6e2923fb7146a Mon Sep 17 00:00:00 2001 From: Alex Zatelepin Date: Wed, 18 Sep 2019 18:10:19 +0300 Subject: [PATCH 3/6] add tests --- .../ui/try-block/try-block-bad-type.stderr | 8 +- .../try-block-unreachable-code-lint.rs | 76 +++++++++++++++++++ .../try-block-unreachable-code-lint.stderr | 28 +++++++ 3 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/try-block/try-block-unreachable-code-lint.rs create mode 100644 src/test/ui/try-block/try-block-unreachable-code-lint.stderr diff --git a/src/test/ui/try-block/try-block-bad-type.stderr b/src/test/ui/try-block/try-block-bad-type.stderr index e1c2c6b675e9b..0e993cb4e5885 100644 --- 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 = try { }; found type `()` error[E0277]: the trait bound `(): std::ops::Try` is not satisfied - --> $DIR/try-block-bad-type.rs:17:23 + --> $DIR/try-block-bad-type.rs:17:25 | 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:24 + --> $DIR/try-block-bad-type.rs:19:26 | 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` diff --git a/src/test/ui/try-block/try-block-unreachable-code-lint.rs b/src/test/ui/try-block/try-block-unreachable-code-lint.rs new file mode 100644 index 0000000000000..5a9f662d229b2 --- /dev/null +++ b/src/test/ui/try-block/try-block-unreachable-code-lint.rs @@ -0,0 +1,76 @@ +// Test unreachable_code lint for `try {}` block ok-wrapping. See issues #54165, #63324. + +// compile-flags: --edition 2018 +// check-pass +#![feature(try_blocks)] +#![warn(unreachable_code)] + +fn err() -> Result { + Err(()) +} + +// In the following cases unreachable code is autogenerated and should not be reported. + +fn test_ok_wrapped_divergent_expr_1() { + let res: Result = try { + loop { + err()?; + } + }; + println!("res: {:?}", res); +} + +fn test_ok_wrapped_divergent_expr_2() { + let _: Result = try { + return + }; +} + +fn test_autogenerated_unit_after_divergent_expr() { + let _: Result<(), ()> = try { + return; + }; +} + +// In the following cases unreachable code should be reported. + +fn test_try_block_after_divergent_stmt() { + let _: Result = { + return; + + try { + loop { + err()?; + } + } + // ~^^^^^ WARNING unreachable expression + }; +} + +fn test_wrapped_divergent_expr() { + let _: Result = { + Err(return) + // ~^ WARNING unreachable call + }; +} + +fn test_expr_after_divergent_stmt_in_try_block() { + let res: Result = try { + loop { + err()?; + } + + 42 + // ~^ WARNING unreachable expression + }; + println!("res: {:?}", res); +} + +fn main() { + test_ok_wrapped_divergent_expr_1(); + test_ok_wrapped_divergent_expr_2(); + test_autogenerated_unit_after_divergent_expr(); + test_try_block_after_divergent_stmt(); + test_wrapped_divergent_expr(); + test_expr_after_divergent_stmt_in_try_block(); +} diff --git a/src/test/ui/try-block/try-block-unreachable-code-lint.stderr b/src/test/ui/try-block/try-block-unreachable-code-lint.stderr new file mode 100644 index 0000000000000..621d882aa9fb9 --- /dev/null +++ b/src/test/ui/try-block/try-block-unreachable-code-lint.stderr @@ -0,0 +1,28 @@ +warning: unreachable expression + --> $DIR/try-block-unreachable-code-lint.rs:41:9 + | +LL | / try { +LL | | loop { +LL | | err()?; +LL | | } +LL | | } + | |_________^ + | +note: lint level defined here + --> $DIR/try-block-unreachable-code-lint.rs:6:9 + | +LL | #![warn(unreachable_code)] + | ^^^^^^^^^^^^^^^^ + +warning: unreachable call + --> $DIR/try-block-unreachable-code-lint.rs:52:9 + | +LL | Err(return) + | ^^^ + +warning: unreachable expression + --> $DIR/try-block-unreachable-code-lint.rs:63:9 + | +LL | 42 + | ^^ + From ffa526937e54a22219a265df867e42f848a78a81 Mon Sep 17 00:00:00 2001 From: Alex Zatelepin Date: Thu, 19 Sep 2019 01:20:18 +0300 Subject: [PATCH 4/6] address review comments --- src/librustc/hir/lowering/expr.rs | 28 ++++++------- src/librustc_typeck/check/expr.rs | 40 +++++++++---------- .../ui/try-block/try-block-bad-type.stderr | 8 ++-- 3 files changed, 37 insertions(+), 39 deletions(-) diff --git a/src/librustc/hir/lowering/expr.rs b/src/librustc/hir/lowering/expr.rs index 0d2c7c13a85b9..db5b197c5d673 100644 --- a/src/librustc/hir/lowering/expr.rs +++ b/src/librustc/hir/lowering/expr.rs @@ -392,36 +392,34 @@ impl LoweringContext<'_> { ) } + /// Desugar `try { ; }` into `{ ; ::std::ops::Try::from_ok() }`, + /// `try { ; }` into `{ ; ::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| 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| 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 index a93ea0cc00467..9152eee1a2c5a 100644 --- a/src/librustc_typeck/check/expr.rs +++ b/src/librustc_typeck/check/expr.rs @@ -151,20 +151,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"); } @@ -177,15 +175,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.kind { - 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.kind { + 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_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"), } // 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 index 0e993cb4e5885..e1c2c6b675e9b 100644 --- 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 = 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` From cb4ed52fd0d732c9a464937434651810fb666246 Mon Sep 17 00:00:00 2001 From: Alex Zatelepin Date: Sun, 22 Sep 2019 17:09:10 +0300 Subject: [PATCH 5/6] fix test after rebase --- .../try-block-unreachable-code-lint.stderr | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/test/ui/try-block/try-block-unreachable-code-lint.stderr b/src/test/ui/try-block/try-block-unreachable-code-lint.stderr index 621d882aa9fb9..54fed04d400e9 100644 --- a/src/test/ui/try-block/try-block-unreachable-code-lint.stderr +++ b/src/test/ui/try-block/try-block-unreachable-code-lint.stderr @@ -1,12 +1,15 @@ warning: unreachable expression --> $DIR/try-block-unreachable-code-lint.rs:41:9 | +LL | return; + | ------ any code following this expression is unreachable +LL | LL | / try { LL | | loop { LL | | err()?; LL | | } LL | | } - | |_________^ + | |_________^ unreachable expression | note: lint level defined here --> $DIR/try-block-unreachable-code-lint.rs:6:9 @@ -18,11 +21,18 @@ warning: unreachable call --> $DIR/try-block-unreachable-code-lint.rs:52:9 | LL | Err(return) - | ^^^ + | ^^^ ------ any code following this expression is unreachable + | | + | unreachable call warning: unreachable expression --> $DIR/try-block-unreachable-code-lint.rs:63:9 | -LL | 42 - | ^^ +LL | / loop { +LL | | err()?; +LL | | } + | |_________- any code following this expression is unreachable +LL | +LL | 42 + | ^^ unreachable expression From 75fdb959321dc04acb70bed73fe8cc84e85ae135 Mon Sep 17 00:00:00 2001 From: Alex Zatelepin Date: Tue, 1 Oct 2019 20:04:41 +0300 Subject: [PATCH 6/6] change .node -> .kind after rebase --- src/librustc_typeck/check/expr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_typeck/check/expr.rs b/src/librustc_typeck/check/expr.rs index 9152eee1a2c5a..7a6fe9560fbff 100644 --- a/src/librustc_typeck/check/expr.rs +++ b/src/librustc_typeck/check/expr.rs @@ -154,7 +154,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // 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 { + let is_try_block_generated_unit_expr = match expr.kind { ExprKind::Call(_, ref args) if expr.span.is_desugaring(DesugaringKind::TryBlock) => args.len() == 1 && args[0].span.is_desugaring(DesugaringKind::TryBlock),