From 2bcc7591142c4bad5cbe290df28ded0e80e6b870 Mon Sep 17 00:00:00 2001 From: Fabian Wolff Date: Fri, 21 May 2021 16:56:45 +0200 Subject: [PATCH 1/2] Warn about unreachable code following an expression with an uninhabited type --- compiler/rustc_passes/src/liveness.rs | 111 ++++++++++++++---- src/test/ui/lint/dead-code/issue-85071-2.rs | 22 ++++ .../ui/lint/dead-code/issue-85071-2.stderr | 34 ++++++ src/test/ui/lint/dead-code/issue-85071.rs | 19 +++ src/test/ui/lint/dead-code/issue-85071.stderr | 34 ++++++ 5 files changed, 200 insertions(+), 20 deletions(-) create mode 100644 src/test/ui/lint/dead-code/issue-85071-2.rs create mode 100644 src/test/ui/lint/dead-code/issue-85071-2.stderr create mode 100644 src/test/ui/lint/dead-code/issue-85071.rs create mode 100644 src/test/ui/lint/dead-code/issue-85071.stderr diff --git a/compiler/rustc_passes/src/liveness.rs b/compiler/rustc_passes/src/liveness.rs index 4ceefa17bcf3d..80c1bd7a5f5f3 100644 --- a/compiler/rustc_passes/src/liveness.rs +++ b/compiler/rustc_passes/src/liveness.rs @@ -95,7 +95,7 @@ use rustc_hir::{Expr, HirId, HirIdMap, HirIdSet}; use rustc_index::vec::IndexVec; use rustc_middle::hir::map::Map; use rustc_middle::ty::query::Providers; -use rustc_middle::ty::{self, DefIdTree, RootVariableMinCaptureList, TyCtxt}; +use rustc_middle::ty::{self, DefIdTree, RootVariableMinCaptureList, Ty, TyCtxt}; use rustc_session::lint; use rustc_span::symbol::{kw, sym, Symbol}; use rustc_span::Span; @@ -123,8 +123,8 @@ rustc_index::newtype_index! { #[derive(Copy, Clone, PartialEq, Debug)] enum LiveNodeKind { UpvarNode(Span), - ExprNode(Span), - VarDefNode(Span), + ExprNode(Span, HirId), + VarDefNode(Span, HirId), ClosureNode, ExitNode, } @@ -133,8 +133,8 @@ fn live_node_kind_to_string(lnk: LiveNodeKind, tcx: TyCtxt<'_>) -> String { let sm = tcx.sess.source_map(); match lnk { UpvarNode(s) => format!("Upvar node [{}]", sm.span_to_diagnostic_string(s)), - ExprNode(s) => format!("Expr node [{}]", sm.span_to_diagnostic_string(s)), - VarDefNode(s) => format!("Var def node [{}]", sm.span_to_diagnostic_string(s)), + ExprNode(s, _) => format!("Expr node [{}]", sm.span_to_diagnostic_string(s)), + VarDefNode(s, _) => format!("Var def node [{}]", sm.span_to_diagnostic_string(s)), ClosureNode => "Closure node".to_owned(), ExitNode => "Exit node".to_owned(), } @@ -297,7 +297,7 @@ impl IrMaps<'tcx> { } pat.each_binding(|_, hir_id, _, ident| { - self.add_live_node_for_node(hir_id, VarDefNode(ident.span)); + self.add_live_node_for_node(hir_id, VarDefNode(ident.span, hir_id)); self.add_variable(Local(LocalInfo { id: hir_id, name: ident.name, @@ -391,14 +391,14 @@ impl<'tcx> Visitor<'tcx> for IrMaps<'tcx> { hir::ExprKind::Path(hir::QPath::Resolved(_, ref path)) => { debug!("expr {}: path that leads to {:?}", expr.hir_id, path.res); if let Res::Local(_var_hir_id) = path.res { - self.add_live_node_for_node(expr.hir_id, ExprNode(expr.span)); + self.add_live_node_for_node(expr.hir_id, ExprNode(expr.span, expr.hir_id)); } intravisit::walk_expr(self, expr); } hir::ExprKind::Closure(..) => { // Interesting control flow (for loops can contain labeled // breaks or continues) - self.add_live_node_for_node(expr.hir_id, ExprNode(expr.span)); + self.add_live_node_for_node(expr.hir_id, ExprNode(expr.span, expr.hir_id)); // Make a live_node for each captured variable, with the span // being the location that the variable is used. This results @@ -426,11 +426,11 @@ impl<'tcx> Visitor<'tcx> for IrMaps<'tcx> { // live nodes required for interesting control flow: hir::ExprKind::If(..) | hir::ExprKind::Match(..) | hir::ExprKind::Loop(..) => { - self.add_live_node_for_node(expr.hir_id, ExprNode(expr.span)); + self.add_live_node_for_node(expr.hir_id, ExprNode(expr.span, expr.hir_id)); intravisit::walk_expr(self, expr); } hir::ExprKind::Binary(op, ..) if op.node.is_lazy() => { - self.add_live_node_for_node(expr.hir_id, ExprNode(expr.span)); + self.add_live_node_for_node(expr.hir_id, ExprNode(expr.span, expr.hir_id)); intravisit::walk_expr(self, expr); } @@ -978,11 +978,26 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { hir::ExprKind::Call(ref f, ref args) => { let m = self.ir.tcx.parent_module(expr.hir_id).to_def_id(); - let succ = if self.ir.tcx.is_ty_uninhabited_from( - m, - self.typeck_results.expr_ty(expr), - self.param_env, - ) { + let ty = self.typeck_results.expr_ty(expr); + let succ = if self.ir.tcx.is_ty_uninhabited_from(m, ty, self.param_env) { + if let LiveNodeKind::ExprNode(succ_span, succ_id) = self.ir.lnks[succ] { + self.warn_about_unreachable( + expr.span, + ty, + succ_span, + succ_id, + "expression", + ); + } else if let LiveNodeKind::VarDefNode(succ_span, succ_id) = self.ir.lnks[succ] + { + self.warn_about_unreachable( + expr.span, + ty, + succ_span, + succ_id, + "definition", + ); + } self.exit_ln } else { succ @@ -993,11 +1008,26 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { hir::ExprKind::MethodCall(.., ref args, _) => { let m = self.ir.tcx.parent_module(expr.hir_id).to_def_id(); - let succ = if self.ir.tcx.is_ty_uninhabited_from( - m, - self.typeck_results.expr_ty(expr), - self.param_env, - ) { + let ty = self.typeck_results.expr_ty(expr); + let succ = if self.ir.tcx.is_ty_uninhabited_from(m, ty, self.param_env) { + if let LiveNodeKind::ExprNode(succ_span, succ_id) = self.ir.lnks[succ] { + self.warn_about_unreachable( + expr.span, + ty, + succ_span, + succ_id, + "expression", + ); + } else if let LiveNodeKind::VarDefNode(succ_span, succ_id) = self.ir.lnks[succ] + { + self.warn_about_unreachable( + expr.span, + ty, + succ_span, + succ_id, + "definition", + ); + } self.exit_ln } else { succ @@ -1274,6 +1304,47 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { ln } + + fn warn_about_unreachable( + &mut self, + orig_span: Span, + orig_ty: Ty<'tcx>, + expr_span: Span, + expr_id: HirId, + descr: &str, + ) { + if !orig_ty.is_never() { + // Unreachable code warnings are already emitted during type checking. + // However, during type checking, full type information is being + // calculated but not yet available, so the check for diverging + // expressions due to uninhabited result types is pretty crude and + // only checks whether ty.is_never(). Here, we have full type + // information available and can issue warnings for less obviously + // uninhabited types (e.g. empty enums). The check above is used so + // that we do not emit the same warning twice if the uninhabited type + // is indeed `!`. + + self.ir.tcx.struct_span_lint_hir( + lint::builtin::UNREACHABLE_CODE, + expr_id, + expr_span, + |lint| { + let msg = format!("unreachable {}", descr); + lint.build(&msg) + .span_label(expr_span, &msg) + .span_label(orig_span, "any code following this expression is unreachable") + .span_note( + orig_span, + &format!( + "this expression has type `{}`, which is uninhabited", + orig_ty + ), + ) + .emit(); + }, + ); + } + } } // _______________________________________________________________________ diff --git a/src/test/ui/lint/dead-code/issue-85071-2.rs b/src/test/ui/lint/dead-code/issue-85071-2.rs new file mode 100644 index 0000000000000..f0639931c84f3 --- /dev/null +++ b/src/test/ui/lint/dead-code/issue-85071-2.rs @@ -0,0 +1,22 @@ +// A slight variation of issue-85071.rs. Here, a method is called instead +// of a function, and the warning is about an unreachable definition +// instead of an unreachable expression. + +// check-pass + +#![warn(unused_variables,unreachable_code)] + +enum Foo {} + +struct S; +impl S { + fn f(&self) -> Foo {todo!()} +} + +fn main() { + let s = S; + let x = s.f(); + //~^ WARNING: unused variable: `x` + let _y = x; + //~^ WARNING: unreachable definition +} diff --git a/src/test/ui/lint/dead-code/issue-85071-2.stderr b/src/test/ui/lint/dead-code/issue-85071-2.stderr new file mode 100644 index 0000000000000..86fbd1d75e89f --- /dev/null +++ b/src/test/ui/lint/dead-code/issue-85071-2.stderr @@ -0,0 +1,34 @@ +warning: unreachable definition + --> $DIR/issue-85071-2.rs:20:9 + | +LL | let x = s.f(); + | ----- any code following this expression is unreachable +LL | +LL | let _y = x; + | ^^ unreachable definition + | +note: the lint level is defined here + --> $DIR/issue-85071-2.rs:7:26 + | +LL | #![warn(unused_variables,unreachable_code)] + | ^^^^^^^^^^^^^^^^ +note: this expression has type `Foo`, which is uninhabited + --> $DIR/issue-85071-2.rs:18:13 + | +LL | let x = s.f(); + | ^^^^^ + +warning: unused variable: `x` + --> $DIR/issue-85071-2.rs:18:9 + | +LL | let x = s.f(); + | ^ help: if this is intentional, prefix it with an underscore: `_x` + | +note: the lint level is defined here + --> $DIR/issue-85071-2.rs:7:9 + | +LL | #![warn(unused_variables,unreachable_code)] + | ^^^^^^^^^^^^^^^^ + +warning: 2 warnings emitted + diff --git a/src/test/ui/lint/dead-code/issue-85071.rs b/src/test/ui/lint/dead-code/issue-85071.rs new file mode 100644 index 0000000000000..d6969321cad4b --- /dev/null +++ b/src/test/ui/lint/dead-code/issue-85071.rs @@ -0,0 +1,19 @@ +// Checks that an unreachable code warning is emitted when an expression is +// preceded by an expression with an uninhabited type. Previously, the +// variable liveness analysis was "smarter" than the reachability analysis +// in this regard, which led to confusing "unused variable" warnings +// without an accompanying explanatory "unreachable expression" warning. + +// check-pass + +#![warn(unused_variables,unreachable_code)] + +enum Foo {} +fn f() -> Foo {todo!()} + +fn main() { + let x = f(); + //~^ WARNING: unused variable: `x` + let _ = x; + //~^ WARNING: unreachable expression +} diff --git a/src/test/ui/lint/dead-code/issue-85071.stderr b/src/test/ui/lint/dead-code/issue-85071.stderr new file mode 100644 index 0000000000000..49555fdaa3595 --- /dev/null +++ b/src/test/ui/lint/dead-code/issue-85071.stderr @@ -0,0 +1,34 @@ +warning: unreachable expression + --> $DIR/issue-85071.rs:17:13 + | +LL | let x = f(); + | --- any code following this expression is unreachable +LL | +LL | let _ = x; + | ^ unreachable expression + | +note: the lint level is defined here + --> $DIR/issue-85071.rs:9:26 + | +LL | #![warn(unused_variables,unreachable_code)] + | ^^^^^^^^^^^^^^^^ +note: this expression has type `Foo`, which is uninhabited + --> $DIR/issue-85071.rs:15:13 + | +LL | let x = f(); + | ^^^ + +warning: unused variable: `x` + --> $DIR/issue-85071.rs:15:9 + | +LL | let x = f(); + | ^ help: if this is intentional, prefix it with an underscore: `_x` + | +note: the lint level is defined here + --> $DIR/issue-85071.rs:9:9 + | +LL | #![warn(unused_variables,unreachable_code)] + | ^^^^^^^^^^^^^^^^ + +warning: 2 warnings emitted + From 7a98fd412425ed39e8737e63a35da96202e43e56 Mon Sep 17 00:00:00 2001 From: Fabian Wolff Date: Sat, 22 May 2021 01:25:16 +0200 Subject: [PATCH 2/2] Small refactoring in `liveness.rs` --- compiler/rustc_passes/src/liveness.rs | 72 ++++++++------------------- 1 file changed, 21 insertions(+), 51 deletions(-) diff --git a/compiler/rustc_passes/src/liveness.rs b/compiler/rustc_passes/src/liveness.rs index 80c1bd7a5f5f3..413fb32b70de3 100644 --- a/compiler/rustc_passes/src/liveness.rs +++ b/compiler/rustc_passes/src/liveness.rs @@ -977,62 +977,13 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { } hir::ExprKind::Call(ref f, ref args) => { - let m = self.ir.tcx.parent_module(expr.hir_id).to_def_id(); - let ty = self.typeck_results.expr_ty(expr); - let succ = if self.ir.tcx.is_ty_uninhabited_from(m, ty, self.param_env) { - if let LiveNodeKind::ExprNode(succ_span, succ_id) = self.ir.lnks[succ] { - self.warn_about_unreachable( - expr.span, - ty, - succ_span, - succ_id, - "expression", - ); - } else if let LiveNodeKind::VarDefNode(succ_span, succ_id) = self.ir.lnks[succ] - { - self.warn_about_unreachable( - expr.span, - ty, - succ_span, - succ_id, - "definition", - ); - } - self.exit_ln - } else { - succ - }; + let succ = self.check_is_ty_uninhabited(expr, succ); let succ = self.propagate_through_exprs(args, succ); self.propagate_through_expr(&f, succ) } hir::ExprKind::MethodCall(.., ref args, _) => { - let m = self.ir.tcx.parent_module(expr.hir_id).to_def_id(); - let ty = self.typeck_results.expr_ty(expr); - let succ = if self.ir.tcx.is_ty_uninhabited_from(m, ty, self.param_env) { - if let LiveNodeKind::ExprNode(succ_span, succ_id) = self.ir.lnks[succ] { - self.warn_about_unreachable( - expr.span, - ty, - succ_span, - succ_id, - "expression", - ); - } else if let LiveNodeKind::VarDefNode(succ_span, succ_id) = self.ir.lnks[succ] - { - self.warn_about_unreachable( - expr.span, - ty, - succ_span, - succ_id, - "definition", - ); - } - self.exit_ln - } else { - succ - }; - + let succ = self.check_is_ty_uninhabited(expr, succ); self.propagate_through_exprs(args, succ) } @@ -1305,6 +1256,25 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { ln } + fn check_is_ty_uninhabited(&mut self, expr: &Expr<'_>, succ: LiveNode) -> LiveNode { + let ty = self.typeck_results.expr_ty(expr); + let m = self.ir.tcx.parent_module(expr.hir_id).to_def_id(); + if self.ir.tcx.is_ty_uninhabited_from(m, ty, self.param_env) { + match self.ir.lnks[succ] { + LiveNodeKind::ExprNode(succ_span, succ_id) => { + self.warn_about_unreachable(expr.span, ty, succ_span, succ_id, "expression"); + } + LiveNodeKind::VarDefNode(succ_span, succ_id) => { + self.warn_about_unreachable(expr.span, ty, succ_span, succ_id, "definition"); + } + _ => {} + }; + self.exit_ln + } else { + succ + } + } + fn warn_about_unreachable( &mut self, orig_span: Span,