Be far more strict about what we consider to be a read of never

compiler-errors · compiler-errors · commit a0179f516f90 · 2024-09-06T11:50:47.000-04:00
diff --git a/compiler/rustc_hir_typeck/src/coercion.rs b/compiler/rustc_hir_typeck/src/coercion.rs
@@ -1007,7 +1007,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
         let cause =
             cause.unwrap_or_else(|| self.cause(expr.span, ObligationCauseCode::ExprAssignable));
-        let coerce = Coerce::new(self, cause, allow_two_phase, self.expr_constitutes_read(expr));
+        let coerce = Coerce::new(
+            self,
+            cause,
+            allow_two_phase,
+            self.expr_guaranteed_to_constitute_read_for_never(expr),
+        );
         let ok = self.commit_if_ok(|_| coerce.coerce(source, target))?;
 
         let (adjustments, _) = self.register_infer_ok_obligations(ok);
@@ -1209,6 +1214,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         // probably aren't processing function arguments here and even if we were,
         // they're going to get autorefed again anyway and we can apply 2-phase borrows
         // at that time.
+        //
+        // NOTE: we set `coerce_never` to `true` here because coercion LUBs only
+        // operate on values and not places, so a never coercion is valid.
         let mut coerce = Coerce::new(self, cause.clone(), AllowTwoPhase::No, true);
         coerce.use_lub = true;
 
diff --git a/compiler/rustc_hir_typeck/src/expr.rs b/compiler/rustc_hir_typeck/src/expr.rs
@@ -65,7 +65,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
         // While we don't allow *arbitrary* coercions here, we *do* allow
         // coercions from ! to `expected`.
-        if ty.is_never() && self.expr_constitutes_read(expr) {
+        if ty.is_never() && self.expr_guaranteed_to_constitute_read_for_never(expr) {
             if let Some(_) = self.typeck_results.borrow().adjustments().get(expr.hir_id) {
                 let reported = self.dcx().span_delayed_bug(
                     expr.span,
@@ -245,7 +245,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         // unless it's a place expression that isn't being read from, in which case
         // diverging would be unsound since we may never actually read the `!`.
         // e.g. `let _ = *never_ptr;` with `never_ptr: *const !`.
-        if ty.is_never() && self.expr_constitutes_read(expr) {
+        if ty.is_never() && self.expr_guaranteed_to_constitute_read_for_never(expr) {
             self.diverges.set(self.diverges.get() | Diverges::always(expr.span));
         }
 
@@ -274,10 +274,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     /// expression and the *parent* expression is the scrutinee of a match or
     /// the pointee of an `&` addr-of expression, since both of those parent
     /// expressions take a *place* and not a value.
-    ///
-    /// This function is unfortunately a bit heuristical, though it is certainly
-    /// far sounder than the prior status quo: <https://github.com/rust-lang/rust/issues/117288>.
-    pub(super) fn expr_constitutes_read(&self, expr: &'tcx hir::Expr<'tcx>) -> bool {
+    pub(super) fn expr_guaranteed_to_constitute_read_for_never(
+        &self,
+        expr: &'tcx hir::Expr<'tcx>,
+    ) -> bool {
         // We only care about place exprs. Anything else returns an immediate
         // which would constitute a read. We don't care about distinguishing
         // "syntactic" place exprs since if the base of a field projection is
@@ -300,15 +300,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                         expr.hir_id != lhs.hir_id
                     }
 
-                    // If we have a subpattern that performs a read, we want to consider this
-                    // to diverge for compatibility to support something like `let x: () = *never_ptr;`.
+                    // See note on `PatKind::Or` below for why this is `all`.
                     ExprKind::Match(scrutinee, arms, _) => {
                         assert_eq!(scrutinee.hir_id, expr.hir_id);
-                        arms.iter().any(|arm| self.pat_constitutes_read(arm.pat))
+                        arms.iter()
+                            .all(|arm| self.pat_guaranteed_to_constitute_read_for_never(arm.pat))
                     }
                     ExprKind::Let(hir::LetExpr { init, pat, .. }) => {
                         assert_eq!(init.hir_id, expr.hir_id);
-                        self.pat_constitutes_read(*pat)
+                        self.pat_guaranteed_to_constitute_read_for_never(*pat)
                     }
 
                     // Any expression child of these expressions constitute reads.
@@ -349,7 +349,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             // to diverge for compatibility to support something like `let x: () = *never_ptr;`.
             hir::Node::LetStmt(hir::LetStmt { init: Some(target), pat, .. }) => {
                 assert_eq!(target.hir_id, expr.hir_id);
-                self.pat_constitutes_read(*pat)
+                self.pat_guaranteed_to_constitute_read_for_never(*pat)
             }
 
             // These nodes (if they have a sub-expr) do constitute a read.
@@ -400,36 +400,45 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     }
 
     /// Whether this pattern constitutes a read of value of the scrutinee that
-    /// it is matching against.
+    /// it is matching against. This is used to determine whether we should
+    /// perform `NeverToAny` coercions
     ///
     /// See above for the nuances of what happens when this returns true.
-    pub(super) fn pat_constitutes_read(&self, pat: &hir::Pat<'_>) -> bool {
-        let mut does_read = false;
-        pat.walk(|pat| {
-            match pat.kind {
-                hir::PatKind::Wild | hir::PatKind::Never | hir::PatKind::Or(_) => {
-                    // Recurse
-                    true
-                }
-                hir::PatKind::Binding(_, _, _, _)
-                | hir::PatKind::Struct(_, _, _)
-                | hir::PatKind::TupleStruct(_, _, _)
-                | hir::PatKind::Path(_)
-                | hir::PatKind::Tuple(_, _)
-                | hir::PatKind::Box(_)
-                | hir::PatKind::Ref(_, _)
-                | hir::PatKind::Deref(_)
-                | hir::PatKind::Lit(_)
-                | hir::PatKind::Range(_, _, _)
-                | hir::PatKind::Slice(_, _, _)
-                | hir::PatKind::Err(_) => {
-                    does_read = true;
-                    // No need to continue.
-                    false
-                }
-            }
-        });
-        does_read
+    pub(super) fn pat_guaranteed_to_constitute_read_for_never(&self, pat: &hir::Pat<'_>) -> bool {
+        match pat.kind {
+            // Does not constitute a read.
+            hir::PatKind::Wild => false,
+
+            // This is unnecessarily restrictive when the pattern that doesn't
+            // constitute a read is unreachable.
+            //
+            // For example `match *never_ptr { value => {}, _ => {} }` or
+            // `match *never_ptr { _ if false => {}, value => {} }`.
+            //
+            // It is however fine to be restrictive here; only returning `true`
+            // can lead to unsoundness.
+            hir::PatKind::Or(subpats) => {
+                subpats.iter().all(|pat| self.pat_guaranteed_to_constitute_read_for_never(pat))
+            }
+
+            // Does constitute a read, since it is equivalent to a discriminant read.
+            hir::PatKind::Never => true,
+
+            // All of these constitute a read, or match on something that isn't `!`,
+            // which would require a `NeverToAny` coercion.
+            hir::PatKind::Binding(_, _, _, _)
+            | hir::PatKind::Struct(_, _, _)
+            | hir::PatKind::TupleStruct(_, _, _)
+            | hir::PatKind::Path(_)
+            | hir::PatKind::Tuple(_, _)
+            | hir::PatKind::Box(_)
+            | hir::PatKind::Ref(_, _)
+            | hir::PatKind::Deref(_)
+            | hir::PatKind::Lit(_)
+            | hir::PatKind::Range(_, _, _)
+            | hir::PatKind::Slice(_, _, _)
+            | hir::PatKind::Err(_) => true,
+        }
     }
 
     #[instrument(skip(self, expr), level = "debug")]
diff --git a/compiler/rustc_hir_typeck/src/pat.rs b/compiler/rustc_hir_typeck/src/pat.rs
@@ -28,7 +28,6 @@ use tracing::{debug, instrument, trace};
 use ty::VariantDef;
 
 use super::report_unexpected_variant_res;
-use crate::diverges::Diverges;
 use crate::gather_locals::DeclOrigin;
 use crate::{errors, FnCtxt, LoweredTy};
 
@@ -278,12 +277,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             }
         };
 
-        // All other patterns constitute a read, which causes us to diverge
-        // if the type is never.
-        if ty.is_never() && self.pat_constitutes_read(pat) {
-            self.diverges.set(self.diverges.get() | Diverges::always(pat.span));
-        }
-
         self.write_ty(pat.hir_id, ty);
 
         // (note_1): In most of the cases where (note_1) is referenced
diff --git a/tests/mir-opt/uninhabited_enum.process_never.SimplifyLocals-final.after.mir b/tests/mir-opt/uninhabited_enum.process_never.SimplifyLocals-final.after.mir
@@ -4,10 +4,10 @@ fn process_never(_1: *const !) -> () {
     debug input => _1;
     let mut _0: ();
     scope 1 {
-        debug _input => _1;
+        debug _input => const ();
     }
 
     bb0: {
-        return;
+        unreachable;
     }
 }
diff --git a/tests/mir-opt/uninhabited_enum.process_void.SimplifyLocals-final.after.mir b/tests/mir-opt/uninhabited_enum.process_void.SimplifyLocals-final.after.mir
@@ -4,7 +4,7 @@ fn process_void(_1: *const Void) -> () {
     debug input => _1;
     let mut _0: ();
     scope 1 {
-        debug _input => _1;
+        debug _input => const ZeroSized: Void;
     }
 
     bb0: {
diff --git a/tests/mir-opt/uninhabited_enum.rs b/tests/mir-opt/uninhabited_enum.rs
@@ -1,18 +1,21 @@
 // skip-filecheck
 #![feature(never_type)]
 
+#[derive(Copy, Clone)]
 pub enum Void {}
 
 // EMIT_MIR uninhabited_enum.process_never.SimplifyLocals-final.after.mir
 #[no_mangle]
 pub fn process_never(input: *const !) {
-    let _input = unsafe { &*input };
+    let _input = unsafe { *input };
 }
 
 // EMIT_MIR uninhabited_enum.process_void.SimplifyLocals-final.after.mir
 #[no_mangle]
 pub fn process_void(input: *const Void) {
-    let _input = unsafe { &*input };
+    let _input = unsafe { *input };
+    // In the future, this should end with `unreachable`, but we currently only do
+    // unreachability analysis for `!`.
 }
 
 fn main() {}
diff --git a/tests/ui/never_type/diverging-place-match.rs b/tests/ui/never_type/diverging-place-match.rs
@@ -44,4 +44,23 @@ fn field_projection() -> ! {
     }
 }
 
+fn covered_arm() -> ! {
+    unsafe {
+    //~^ ERROR mismatched types
+        let x: *const ! = 0 as _;
+        let (_ | 1i32) = *x;
+        //~^ ERROR mismatched types
+    }
+}
+
+// FIXME: This *could* be considered a read of `!`, but we're not that sophisticated..
+fn uncovered_arm() -> ! {
+    unsafe {
+    //~^ ERROR mismatched types
+        let x: *const ! = 0 as _;
+        let (1i32 | _) = *x;
+        //~^ ERROR mismatched types
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/never_type/diverging-place-match.stderr b/tests/ui/never_type/diverging-place-match.stderr
@@ -78,6 +78,56 @@ LL | |     }
    = note:   expected type `!`
            found unit type `()`
 
-error: aborting due to 6 previous errors
+error[E0308]: mismatched types
+  --> $DIR/diverging-place-match.rs:51:18
+   |
+LL |         let (_ | 1i32) = *x;
+   |                  ^^^^    -- this expression has type `!`
+   |                  |
+   |                  expected `!`, found `i32`
+   |
+   = note: expected type `!`
+              found type `i32`
+
+error[E0308]: mismatched types
+  --> $DIR/diverging-place-match.rs:48:5
+   |
+LL | /     unsafe {
+LL | |
+LL | |         let x: *const ! = 0 as _;
+LL | |         let (_ | 1i32) = *x;
+LL | |
+LL | |     }
+   | |_____^ expected `!`, found `()`
+   |
+   = note:   expected type `!`
+           found unit type `()`
+
+error[E0308]: mismatched types
+  --> $DIR/diverging-place-match.rs:61:14
+   |
+LL |         let (1i32 | _) = *x;
+   |              ^^^^        -- this expression has type `!`
+   |              |
+   |              expected `!`, found `i32`
+   |
+   = note: expected type `!`
+              found type `i32`
+
+error[E0308]: mismatched types
+  --> $DIR/diverging-place-match.rs:58:5
+   |
+LL | /     unsafe {
+LL | |
+LL | |         let x: *const ! = 0 as _;
+LL | |         let (1i32 | _) = *x;
+LL | |
+LL | |     }
+   | |_____^ expected `!`, found `()`
+   |
+   = note:   expected type `!`
+           found unit type `()`
+
+error: aborting due to 10 previous errors
 
 For more information about this error, try `rustc --explain E0308`.
diff --git a/tests/ui/raw-ref-op/never-place-isnt-diverging.rs b/tests/ui/raw-ref-op/never-place-isnt-diverging.rs
@@ -10,4 +10,13 @@ fn make_up_a_value<T>() -> T {
     }
 }
 
+
+fn make_up_a_pointer<T>() -> *const T {
+    unsafe {
+        let x: *const ! = 0 as _;
+        &raw const *x
+        //~^ ERROR mismatched types
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/raw-ref-op/never-place-isnt-diverging.stderr b/tests/ui/raw-ref-op/never-place-isnt-diverging.stderr
@@ -15,6 +15,20 @@ LL | |     }
    = note: expected type parameter `T`
                    found unit type `()`
 
-error: aborting due to 1 previous error
+error[E0308]: mismatched types
+  --> $DIR/never-place-isnt-diverging.rs:17:9
+   |
+LL | fn make_up_a_pointer<T>() -> *const T {
+   |                      -       -------- expected `*const T` because of return type
+   |                      |
+   |                      expected this type parameter
+...
+LL |         &raw const *x
+   |         ^^^^^^^^^^^^^ expected `*const T`, found `*const !`
+   |
+   = note: expected raw pointer `*const T`
+              found raw pointer `*const !`
+
+error: aborting due to 2 previous errors
 
 For more information about this error, try `rustc --explain E0308`.
diff --git a/tests/ui/reachable/unwarned-match-on-never.stderr b/tests/ui/reachable/unwarned-match-on-never.stderr
@@ -2,7 +2,7 @@ error: unreachable expression
   --> $DIR/unwarned-match-on-never.rs:10:5
    |
 LL |     match x {}
-   |     ---------- any code following this expression is unreachable
+   |           - any code following this expression is unreachable
 LL |     // But matches in unreachable code are warned.
 LL |     match x {}
    |     ^^^^^^^^^^ unreachable expression

Original file line number	Diff line number	Diff line change
`@@ -4,10 +4,10 @@ fn process_never(_1: *const !) -> () {`
`4`	`4`	`debug input => _1;`
`5`	`5`	`let mut _0: ();`
`6`	`6`	`scope 1 {`
`7`		`- debug _input => _1;`
	`7`	`+ debug _input => const ();`
`8`	`8`	`}`
`9`	`9`
`10`	`10`	`bb0: {`
`11`		`- return;`
	`11`	`+ unreachable;`
`12`	`12`	`}`
`13`	`13`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4,7 +4,7 @@ fn process_void(_1: *const Void) -> () {`
`4`	`4`	`debug input => _1;`
`5`	`5`	`let mut _0: ();`
`6`	`6`	`scope 1 {`
`7`		`- debug _input => _1;`
	`7`	`+ debug _input => const ZeroSized: Void;`
`8`	`8`	`}`
`9`	`9`
`10`	`10`	`bb0: {`
Original file line number	Diff line number	Diff line change
`@@ -10,4 +10,13 @@ fn make_up_a_value<T>() -> T {`
`10`	`10`	`}`
`11`	`11`	`}`
`12`	`12`
	`13`	`+`
	`14`	`+fn make_up_a_pointer<T>() -> *const T {`
	`15`	`+ unsafe {`
	`16`	`+ let x: *const ! = 0 as _;`
	`17`	`+ &raw const *x`
	`18`	`+ //~^ ERROR mismatched types`
	`19`	`+ }`
	`20`	`+}`
	`21`	`+`
`13`	`22`	`fn main() {}`
Original file line number	Diff line number	Diff line change
`@@ -2,7 +2,7 @@ error: unreachable expression`
`2`	`2`	`--> $DIR/unwarned-match-on-never.rs:10:5`
`3`	`3`	`\|`
`4`	`4`	`LL \| match x {}`
`5`		`- \| ---------- any code following this expression is unreachable`
	`5`	`+ \| - any code following this expression is unreachable`
`6`	`6`	`LL \| // But matches in unreachable code are warned.`
`7`	`7`	`LL \| match x {}`
`8`	`8`	`\| ^^^^^^^^^^ unreachable expression`