From e8d5eb2a2b6cdddd6725501e1b54a33c278d9697 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Fri, 6 Sep 2024 10:19:07 -0400 Subject: [PATCH] Be far more strict about what we consider to be a read of never --- compiler/rustc_hir_typeck/src/coercion.rs | 10 ++- compiler/rustc_hir_typeck/src/expr.rs | 85 ++++++++++--------- compiler/rustc_hir_typeck/src/pat.rs | 7 -- ...ocess_never.SimplifyLocals-final.after.mir | 4 +- ...rocess_void.SimplifyLocals-final.after.mir | 2 +- tests/mir-opt/uninhabited_enum.rs | 7 +- tests/ui/never_type/diverging-place-match.rs | 27 ++++++ .../never_type/diverging-place-match.stderr | 61 ++++++++++++- .../raw-ref-op/never-place-isnt-diverging.rs | 9 ++ .../never-place-isnt-diverging.stderr | 16 +++- .../reachable/unwarned-match-on-never.stderr | 2 +- 11 files changed, 176 insertions(+), 54 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/coercion.rs b/compiler/rustc_hir_typeck/src/coercion.rs index 96bff04dbc7..642db3f36b5 100644 --- a/compiler/rustc_hir_typeck/src/coercion.rs +++ b/compiler/rustc_hir_typeck/src/coercion.rs @@ -1066,7 +1066,12 @@ pub(crate) fn coerce( 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); @@ -1268,6 +1273,9 @@ fn try_find_coercion_lub( // 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 index cef00cf8ce8..2d718295374 100644 --- a/compiler/rustc_hir_typeck/src/expr.rs +++ b/compiler/rustc_hir_typeck/src/expr.rs @@ -65,7 +65,7 @@ pub(crate) fn check_expr_has_type_or_error( // 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 @@ pub(super) fn check_expr_with_expectation_and_args( // 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 @@ pub(super) fn check_expr_with_expectation_and_args( /// 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: . - 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 @@ pub(super) fn expr_constitutes_read(&self, expr: &'tcx hir::Expr<'tcx>) -> bool 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 @@ pub(super) fn expr_constitutes_read(&self, expr: &'tcx hir::Expr<'tcx>) -> bool // 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. @@ -401,36 +401,45 @@ pub(super) fn expr_constitutes_read(&self, expr: &'tcx hir::Expr<'tcx>) -> bool } /// 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 - } + 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_read + + // 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 index c2e25d259c5..49c5a7d8a65 100644 --- a/compiler/rustc_hir_typeck/src/pat.rs +++ b/compiler/rustc_hir_typeck/src/pat.rs @@ -27,7 +27,6 @@ use ty::VariantDef; use super::report_unexpected_variant_res; -use crate::diverges::Diverges; use crate::gather_locals::DeclOrigin; use crate::{FnCtxt, LoweredTy, errors}; @@ -277,12 +276,6 @@ fn check_pat(&self, pat: &'tcx Pat<'tcx>, expected: Ty<'tcx>, pat_info: PatInfo< } }; - // 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 index 64fc81e2989..02e1f4be15e 100644 --- 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 index 51514ba5e5d..64711755f73 100644 --- 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 index 9d845e34fbd..90b5353f291 100644 --- 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 index 185e17c05a7..b9bc29a218c 100644 --- a/tests/ui/never_type/diverging-place-match.rs +++ b/tests/ui/never_type/diverging-place-match.rs @@ -44,4 +44,31 @@ 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 coerce_ref_binding() -> ! { + unsafe { + let x: *const ! = 0 as _; + let ref _x: () = *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 index 14b14f42701..74e1bfa0a6b 100644 --- a/tests/ui/never_type/diverging-place-match.stderr +++ b/tests/ui/never_type/diverging-place-match.stderr @@ -78,6 +78,65 @@ 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[E0308]: mismatched types + --> $DIR/diverging-place-match.rs:69:26 + | +LL | let ref _x: () = *x; + | ^^ expected `()`, found `!` + | + = note: expected unit type `()` + found type `!` + +error: aborting due to 11 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 index 52f4158dbc9..80d441729f7 100644 --- 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 { } } + +fn make_up_a_pointer() -> *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 index 9eba57dde8f..af9e7889821 100644 --- 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() -> *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 index c1ad3511b4e..a296d2a055e 100644 --- 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