Warn about unreachable code following an expression with an uninhabited type

FabianWolff · FabianWolff · commit 2bcc7591142c · 2021-05-21T18:30:36.000+02:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
+
