From 73d49f8c697da34ee1372b51540bfb0f1ae0c712 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 5 Sep 2024 07:49:38 -0400 Subject: [PATCH] Fix up tests --- compiler/rustc_hir_typeck/src/coercion.rs | 17 ++++- .../src/fn_ctxt/suggestions.rs | 6 +- .../miri/tests/pass/underscore_pattern.rs | 51 ++++++++++++- ...t_read.main.SimplifyLocals-final.after.mir | 49 ++++++++++++ tests/mir-opt/uninhabited_not_read.rs | 27 +++++++ tests/ui/never_type/diverging-place-match.rs | 36 ++++++++- .../never_type/diverging-place-match.stderr | 75 +++++++++++++++++-- 7 files changed, 245 insertions(+), 16 deletions(-) create mode 100644 tests/mir-opt/uninhabited_not_read.main.SimplifyLocals-final.after.mir create mode 100644 tests/mir-opt/uninhabited_not_read.rs diff --git a/compiler/rustc_hir_typeck/src/coercion.rs b/compiler/rustc_hir_typeck/src/coercion.rs index 6a1018883c9..1705421c2f4 100644 --- a/compiler/rustc_hir_typeck/src/coercion.rs +++ b/compiler/rustc_hir_typeck/src/coercion.rs @@ -82,6 +82,10 @@ struct Coerce<'a, 'tcx> { /// See #47489 and #48598 /// See docs on the "AllowTwoPhase" type for a more detailed discussion allow_two_phase: AllowTwoPhase, + /// Whether we allow `NeverToAny` coercions. This is unsound if we're + /// coercing a place expression without it counting as a read in the MIR. + /// This is a side-effect of HIR not really having a great distinction + /// between places and values. coerce_never: bool, } @@ -1083,7 +1087,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { debug!("coercion::can_with_predicates({:?} -> {:?})", source, target); let cause = self.cause(DUMMY_SP, ObligationCauseCode::ExprAssignable); - // We don't ever need two-phase here since we throw out the result of the coercion + // We don't ever need two-phase here since we throw out the result of the coercion. + // We also just always set `coerce_never` to true, since this is a heuristic. let coerce = Coerce::new(self, cause, AllowTwoPhase::No, true); self.probe(|_| { let Ok(ok) = coerce.coerce(source, target) else { @@ -1096,11 +1101,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } /// Given a type and a target type, this function will calculate and return - /// how many dereference steps needed to achieve `expr_ty <: target`. If + /// how many dereference steps needed to coerce `expr_ty` to `target`. If /// it's not possible, return `None`. - pub(crate) fn deref_steps(&self, expr_ty: Ty<'tcx>, target: Ty<'tcx>) -> Option { + pub(crate) fn deref_steps_for_suggestion( + &self, + expr_ty: Ty<'tcx>, + target: Ty<'tcx>, + ) -> Option { let cause = self.cause(DUMMY_SP, ObligationCauseCode::ExprAssignable); - // We don't ever need two-phase here since we throw out the result of the coercion + // We don't ever need two-phase here since we throw out the result of the coercion. let coerce = Coerce::new(self, cause, AllowTwoPhase::No, true); coerce .autoderef(DUMMY_SP, expr_ty) diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs index 435b7d0f39a..1df4d32f3cb 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs @@ -2608,7 +2608,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } if let hir::ExprKind::Unary(hir::UnOp::Deref, inner) = expr.kind - && let Some(1) = self.deref_steps(expected, checked_ty) + && let Some(1) = self.deref_steps_for_suggestion(expected, checked_ty) { // We have `*&T`, check if what was expected was `&T`. // If so, we may want to suggest removing a `*`. @@ -2738,7 +2738,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } } (_, &ty::RawPtr(ty_b, mutbl_b), &ty::Ref(_, ty_a, mutbl_a)) => { - if let Some(steps) = self.deref_steps(ty_a, ty_b) + if let Some(steps) = self.deref_steps_for_suggestion(ty_a, ty_b) // Only suggest valid if dereferencing needed. && steps > 0 // The pointer type implements `Copy` trait so the suggestion is always valid. @@ -2782,7 +2782,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } } _ if sp == expr.span => { - if let Some(mut steps) = self.deref_steps(checked_ty, expected) { + if let Some(mut steps) = self.deref_steps_for_suggestion(checked_ty, expected) { let mut expr = expr.peel_blocks(); let mut prefix_span = expr.span.shrink_to_lo(); let mut remove = String::new(); diff --git a/src/tools/miri/tests/pass/underscore_pattern.rs b/src/tools/miri/tests/pass/underscore_pattern.rs index f0afe558954..f59bb9f5c82 100644 --- a/src/tools/miri/tests/pass/underscore_pattern.rs +++ b/src/tools/miri/tests/pass/underscore_pattern.rs @@ -1,5 +1,7 @@ // Various tests ensuring that underscore patterns really just construct the place, but don't check its contents. #![feature(strict_provenance)] +#![feature(never_type)] + use std::ptr; fn main() { @@ -9,6 +11,7 @@ fn main() { invalid_let(); dangling_let_type_annotation(); invalid_let_type_annotation(); + never(); } fn dangling_match() { @@ -34,6 +37,13 @@ fn invalid_match() { _ => {} } } + + unsafe { + let x: Uninit = Uninit { uninit: () }; + match x.value { + _ => {} + } + } } fn dangling_let() { @@ -41,6 +51,11 @@ fn dangling_let() { let ptr = ptr::without_provenance::(0x40); let _ = *ptr; } + + unsafe { + let ptr = ptr::without_provenance::(0x40); + let _ = *ptr; + } } fn invalid_let() { @@ -49,6 +64,12 @@ fn invalid_let() { let ptr = ptr::addr_of!(val).cast::(); let _ = *ptr; } + + unsafe { + let val = 3u8; + let ptr = ptr::addr_of!(val).cast::(); + let _ = *ptr; + } } // Adding a type annotation used to change how MIR is generated, make sure we cover both cases. @@ -57,6 +78,11 @@ fn dangling_let_type_annotation() { let ptr = ptr::without_provenance::(0x40); let _: bool = *ptr; } + + unsafe { + let ptr = ptr::without_provenance::(0x40); + let _: ! = *ptr; + } } fn invalid_let_type_annotation() { @@ -65,7 +91,28 @@ fn invalid_let_type_annotation() { let ptr = ptr::addr_of!(val).cast::(); let _: bool = *ptr; } + + unsafe { + let val = 3u8; + let ptr = ptr::addr_of!(val).cast::(); + let _: ! = *ptr; + } } -// FIXME: we should also test `!`, not just `bool` -- but that s currently buggy: -// https://github.com/rust-lang/rust/issues/117288 +// Regression test from . +fn never() { + unsafe { + let x = 3u8; + let x: *const ! = &x as *const u8 as *const _; + let _: ! = *x; + } + + // Without a type annotation, make sure we don't implicitly coerce `!` to `()` + // when we do the noop `*x` (as that would require a `!` *value*, creating + // which is UB). + unsafe { + let x = 3u8; + let x: *const ! = &x as *const u8 as *const _; + let _ = *x; + } +} diff --git a/tests/mir-opt/uninhabited_not_read.main.SimplifyLocals-final.after.mir b/tests/mir-opt/uninhabited_not_read.main.SimplifyLocals-final.after.mir new file mode 100644 index 00000000000..6bf4be652be --- /dev/null +++ b/tests/mir-opt/uninhabited_not_read.main.SimplifyLocals-final.after.mir @@ -0,0 +1,49 @@ +// MIR for `main` after SimplifyLocals-final + +fn main() -> () { + let mut _0: (); + let _1: u8; + let mut _2: *const !; + let mut _3: *const u8; + let _4: u8; + let mut _5: *const !; + let mut _6: *const u8; + scope 1 { + debug x => _1; + scope 2 { + debug x => _2; + scope 3 { + } + } + } + scope 4 { + debug x => _4; + scope 5 { + debug x => _5; + scope 6 { + } + } + } + + bb0: { + StorageLive(_1); + _1 = const 3_u8; + StorageLive(_2); + StorageLive(_3); + _3 = &raw const _1; + _2 = move _3 as *const ! (PtrToPtr); + StorageDead(_3); + StorageDead(_2); + StorageDead(_1); + StorageLive(_4); + _4 = const 3_u8; + StorageLive(_5); + StorageLive(_6); + _6 = &raw const _4; + _5 = move _6 as *const ! (PtrToPtr); + StorageDead(_6); + StorageDead(_5); + StorageDead(_4); + return; + } +} diff --git a/tests/mir-opt/uninhabited_not_read.rs b/tests/mir-opt/uninhabited_not_read.rs new file mode 100644 index 00000000000..39ffdbdbb33 --- /dev/null +++ b/tests/mir-opt/uninhabited_not_read.rs @@ -0,0 +1,27 @@ +// skip-filecheck + +//@ edition: 2021 +// In ed 2021 and below, we don't fallback `!` to `()`. +// This would introduce a `! -> ()` coercion which would +// be UB if we didn't disallow this explicitly. + +#![feature(never_type)] + +// EMIT_MIR uninhabited_not_read.main.SimplifyLocals-final.after.mir +fn main() { + // With a type annotation + unsafe { + let x = 3u8; + let x: *const ! = &x as *const u8 as *const _; + let _: ! = *x; + } + + // Without a type annotation, make sure we don't implicitly coerce `!` to `()` + // when we do the noop `*x`. + unsafe { + let x = 3u8; + let x: *const ! = &x as *const u8 as *const _; + let _ = *x; + } +} + diff --git a/tests/ui/never_type/diverging-place-match.rs b/tests/ui/never_type/diverging-place-match.rs index 7bc00773c0b..185e17c05a7 100644 --- a/tests/ui/never_type/diverging-place-match.rs +++ b/tests/ui/never_type/diverging-place-match.rs @@ -1,6 +1,6 @@ #![feature(never_type)] -fn make_up_a_value() -> T { +fn not_a_read() -> ! { unsafe { //~^ ERROR mismatched types let x: *const ! = 0 as _; @@ -10,4 +10,38 @@ fn make_up_a_value() -> T { } } +fn not_a_read_implicit() -> ! { + unsafe { + //~^ ERROR mismatched types + let x: *const ! = 0 as _; + let _ = *x; + } +} + +fn not_a_read_guide_coercion() -> ! { + unsafe { + //~^ ERROR mismatched types + let x: *const ! = 0 as _; + let _: () = *x; + //~^ ERROR mismatched types + } +} + +fn empty_match() -> ! { + unsafe { + //~^ ERROR mismatched types + let x: *const ! = 0 as _; + match *x { _ => {} }; + } +} + +fn field_projection() -> ! { + unsafe { + //~^ ERROR mismatched types + let x: *const (!, ()) = 0 as _; + let _ = (*x).0; + // ^ I think this is still UB, but because of the inbounds projection. + } +} + fn main() {} diff --git a/tests/ui/never_type/diverging-place-match.stderr b/tests/ui/never_type/diverging-place-match.stderr index e86c634d591..14b14f42701 100644 --- a/tests/ui/never_type/diverging-place-match.stderr +++ b/tests/ui/never_type/diverging-place-match.stderr @@ -1,8 +1,6 @@ error[E0308]: mismatched types --> $DIR/diverging-place-match.rs:4:5 | -LL | fn make_up_a_value() -> T { - | - expected this type parameter LL | / unsafe { LL | | LL | | let x: *const ! = 0 as _; @@ -10,11 +8,76 @@ LL | | let _: ! = *x; LL | | // Since `*x` "diverges" in HIR, but doesn't count as a read in MIR, this LL | | // is unsound since we act as if it diverges but it doesn't. LL | | } - | |_____^ expected type parameter `T`, found `()` + | |_____^ expected `!`, found `()` | - = note: expected type parameter `T` - found unit type `()` + = note: expected type `!` + found unit type `()` -error: aborting due to 1 previous error +error[E0308]: mismatched types + --> $DIR/diverging-place-match.rs:14:5 + | +LL | / unsafe { +LL | | +LL | | let x: *const ! = 0 as _; +LL | | let _ = *x; +LL | | } + | |_____^ expected `!`, found `()` + | + = note: expected type `!` + found unit type `()` + +error[E0308]: mismatched types + --> $DIR/diverging-place-match.rs:25:21 + | +LL | let _: () = *x; + | -- ^^ expected `()`, found `!` + | | + | expected due to this + | + = note: expected unit type `()` + found type `!` + +error[E0308]: mismatched types + --> $DIR/diverging-place-match.rs:22:5 + | +LL | / unsafe { +LL | | +LL | | let x: *const ! = 0 as _; +LL | | let _: () = *x; +LL | | +LL | | } + | |_____^ expected `!`, found `()` + | + = note: expected type `!` + found unit type `()` + +error[E0308]: mismatched types + --> $DIR/diverging-place-match.rs:31:5 + | +LL | / unsafe { +LL | | +LL | | let x: *const ! = 0 as _; +LL | | match *x { _ => {} }; +LL | | } + | |_____^ expected `!`, found `()` + | + = note: expected type `!` + found unit type `()` + +error[E0308]: mismatched types + --> $DIR/diverging-place-match.rs:39:5 + | +LL | / unsafe { +LL | | +LL | | let x: *const (!, ()) = 0 as _; +LL | | let _ = (*x).0; +LL | | // ^ I think this is still UB, but because of the inbounds projection. +LL | | } + | |_____^ expected `!`, found `()` + | + = note: expected type `!` + found unit type `()` + +error: aborting due to 6 previous errors For more information about this error, try `rustc --explain E0308`.