Auto merge of #85556 - FabianWolff:issue-85071, r=estebank,jackh726

Warn about unreachable code following an expression with an uninhabited type

This pull request fixes #85071. The issue is that liveness analysis currently is "smarter" than reachability analysis when it comes to detecting uninhabited types: Unreachable code is detected during type checking, where full type information is not yet available. Therefore, the check for type inhabitedness is quite crude:
https://github.com/rust-lang/rust/blob/fc81ad22c453776de16acf9938976930cf8c9401/compiler/rustc_typeck/src/check/expr.rs#L202-L205

i.e. it only checks for `!`, but not other, non-trivially uninhabited types, such as empty enums, structs containing an uninhabited type, etc. By contrast, liveness analysis, which runs after type checking, can benefit from the more sophisticated `tcx.is_ty_uninhabited_from()`:
https://github.com/rust-lang/rust/blob/fc81ad22c453776de16acf9938976930cf8c9401/compiler/rustc_passes/src/liveness.rs#L981
https://github.com/rust-lang/rust/blob/fc81ad22c453776de16acf9938976930cf8c9401/compiler/rustc_passes/src/liveness.rs#L996

This can lead to confusing warnings when a variable is reported as unused, but the use of the variable is not reported as unreachable. For instance:
```rust
enum Foo {}
fn f() -> Foo {todo!()}

fn main() {
    let x = f();
    let _ = x;
}
```
currently leads to
```
warning: unused variable: `x`
 --> t1.rs:5:9
  |
5 |     let x = f();
  |         ^ help: if this is intentional, prefix it with an underscore: `_x`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: 1 warning emitted
```
which is confusing, because `x` _appears_ to be used in line 6. With my changes, I get:
```
warning: unreachable expression
 --> t1.rs:6:13
  |
5 |     let x = f();
  |             --- any code following this expression is unreachable
6 |     let _ = x;
  |             ^ unreachable expression
  |
  = note: `#[warn(unreachable_code)]` on by default
note: this expression has type `Foo`, which is uninhabited
 --> t1.rs:5:13
  |
5 |     let x = f();
  |             ^^^

warning: unused variable: `x`
 --> t1.rs:5:9
  |
5 |     let x = f();
  |         ^ help: if this is intentional, prefix it with an underscore: `_x`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: 2 warnings emitted
```
My implementation is slightly inelegant because unreachable code warnings can now be issued in two different places (during type checking and during liveness analysis), but I think it is the solution with the least amount of unnecessary code duplication, given that the new warning integrates nicely with liveness analysis, where unreachable code is already implicitly detected for the purpose of finding unused variables.

Author: bors

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,
@@ -396,14 +396,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
@@ -436,11 +436,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);
             }
 
@@ -992,32 +992,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 succ = if self.ir.tcx.is_ty_uninhabited_from(
-                    m,
-                    self.typeck_results.expr_ty(expr),
-                    self.param_env,
-                ) {
-                    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 succ = if self.ir.tcx.is_ty_uninhabited_from(
-                    m,
-                    self.typeck_results.expr_ty(expr),
-                    self.param_env,
-                ) {
-                    self.exit_ln
-                } else {
-                    succ
-                };
-
+                let succ = self.check_is_ty_uninhabited(expr, succ);
                 self.propagate_through_exprs(args, succ)
             }
 
@@ -1289,6 +1270,66 @@ 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,
+        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();
+                },
+            );
+        }
+    }
 }
 
 // _______________________________________________________________________

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
+}

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
+

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
+}

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
+