Auto merge of rust-lang#85263 - Smittyvb:thir-unsafeck-union-field, r=oli-obk

Check for union field accesses in THIR unsafeck

see also rust-lang#85259, rust-lang#83129, rust-lang/project-thir-unsafeck#7

r? `@LeSeulArtichaut`

Author: bors

compiler/rustc_mir_build/src/check_unsafety.rs

@@ -26,6 +26,8 @@ struct UnsafetyVisitor<'a, 'tcx> {
     /// calls to functions with `#[target_feature]` (RFC 2396).
     body_target_features: &'tcx Vec<Symbol>,
     is_const: bool,
+    in_possible_lhs_union_assign: bool,
+    in_union_destructure: bool,
 }
 
 impl<'tcx> UnsafetyVisitor<'_, 'tcx> {
@@ -158,14 +160,115 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
         }
     }
 
+    fn visit_pat(&mut self, pat: &Pat<'tcx>) {
+        use PatKind::*;
+
+        if self.in_union_destructure {
+            match *pat.kind {
+                // binding to a variable allows getting stuff out of variable
+                Binding { .. }
+                // match is conditional on having this value
+                | Constant { .. }
+                | Variant { .. }
+                | Leaf { .. }
+                | Deref { .. }
+                | Range { .. }
+                | Slice { .. }
+                | Array { .. } => {
+                    self.requires_unsafe(pat.span, AccessToUnionField);
+                    return; // don't walk pattern
+                }
+                // wildcard doesn't take anything
+                Wild |
+                // these just wrap other patterns
+                Or { .. } |
+                AscribeUserType { .. } => {}
+            }
+        };
+
+        if let ty::Adt(adt_def, _) = pat.ty.kind() {
+            // check for extracting values from union via destructuring
+            if adt_def.is_union() {
+                match *pat.kind {
+                    // assigning the whole union is okay
+                    // let x = Union { ... };
+                    // let y = x; // safe
+                    Binding { .. } |
+                    // binding to wildcard is okay since that never reads anything and stops double errors
+                    // with implict wildcard branches from `if let`s
+                    Wild |
+                    // doesn't have any effect on semantics
+                    AscribeUserType { .. } |
+                    // creating a union literal
+                    Constant { .. } => {},
+                    Leaf { .. } | Or { .. } => {
+                        // pattern matching with a union and not doing something like v = Union { bar: 5 }
+                        self.in_union_destructure = true;
+                        visit::walk_pat(self, pat);
+                        self.in_union_destructure = false;
+                        return; // don't walk pattern
+                    }
+                    Variant { .. } | Deref { .. } | Range { .. } | Slice { .. } | Array { .. } =>
+                        unreachable!("impossible union destructuring type"),
+                }
+            }
+        }
+
+        visit::walk_pat(self, pat);
+    }
+
     fn visit_expr(&mut self, expr: &Expr<'tcx>) {
+        // could we be in a the LHS of an assignment of a union?
+        match expr.kind {
+            ExprKind::Field { .. }
+            | ExprKind::VarRef { .. }
+            | ExprKind::UpvarRef { .. }
+            | ExprKind::Scope { .. }
+            | ExprKind::Cast { .. } => {}
+
+            ExprKind::AddressOf { .. }
+            | ExprKind::Adt { .. }
+            | ExprKind::Array { .. }
+            | ExprKind::Binary { .. }
+            | ExprKind::Block { .. }
+            | ExprKind::Borrow { .. }
+            | ExprKind::Literal { .. }
+            | ExprKind::ConstBlock { .. }
+            | ExprKind::Deref { .. }
+            | ExprKind::Index { .. }
+            | ExprKind::NeverToAny { .. }
+            | ExprKind::PlaceTypeAscription { .. }
+            | ExprKind::ValueTypeAscription { .. }
+            | ExprKind::Pointer { .. }
+            | ExprKind::Repeat { .. }
+            | ExprKind::StaticRef { .. }
+            | ExprKind::ThreadLocalRef { .. }
+            | ExprKind::Tuple { .. }
+            | ExprKind::Unary { .. }
+            | ExprKind::Call { .. }
+            | ExprKind::Assign { .. }
+            | ExprKind::AssignOp { .. }
+            | ExprKind::Break { .. }
+            | ExprKind::Closure { .. }
+            | ExprKind::Continue { .. }
+            | ExprKind::Return { .. }
+            | ExprKind::Yield { .. }
+            | ExprKind::Loop { .. }
+            | ExprKind::Match { .. }
+            | ExprKind::Box { .. }
+            | ExprKind::If { .. }
+            | ExprKind::InlineAsm { .. }
+            | ExprKind::LlvmInlineAsm { .. }
+            | ExprKind::LogicalOp { .. }
+            | ExprKind::Use { .. } => self.in_possible_lhs_union_assign = false,
+        };
         match expr.kind {
             ExprKind::Scope { value, lint_level: LintLevel::Explicit(hir_id), region_scope: _ } => {
                 let prev_id = self.hir_context;
                 self.hir_context = hir_id;
                 self.visit_expr(&self.thir[value]);
                 self.hir_context = prev_id;
-                return;
+                return; // don't visit the whole expression
             }
             ExprKind::Call { fun, ty: _, args: _, from_hir_call: _, fn_span: _ } => {
                 if self.thir[fun].ty.fn_sig(self.tcx).unsafety() == hir::Unsafety::Unsafe {
@@ -246,9 +349,29 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
                 // Unsafe blocks can be used in closures, make sure to take it into account
                 self.safety_context = closure_visitor.safety_context;
             }
+            ExprKind::Field { lhs, .. } => {
+                // assigning to union field is okay for AccessToUnionField
+                if let ty::Adt(adt_def, _) = &self.thir[lhs].ty.kind() {
+                    if adt_def.is_union() {
+                        if self.in_possible_lhs_union_assign {
+                            // FIXME: trigger AssignToDroppingUnionField unsafety if needed
+                        } else {
+                            self.requires_unsafe(expr.span, AccessToUnionField);
+                        }
+                    }
+                }
+            }
+            // don't have any special handling for AssignOp since it causes a read *and* write to lhs
+            ExprKind::Assign { lhs, rhs } => {
+                // assigning to a union is safe, check here so it doesn't get treated as a read later
+                self.in_possible_lhs_union_assign = true;
+                visit::walk_expr(self, &self.thir()[lhs]);
+                self.in_possible_lhs_union_assign = false;
+                visit::walk_expr(self, &self.thir()[rhs]);
+                return; // don't visit the whole expression
+            }
             _ => {}
         }
-
         visit::walk_expr(self, expr);
     }
 }
@@ -296,7 +419,6 @@ enum UnsafeOpKind {
     DerefOfRawPointer,
     #[allow(dead_code)] // FIXME
     AssignToDroppingUnionField,
-    #[allow(dead_code)] // FIXME
     AccessToUnionField,
     #[allow(dead_code)] // FIXME
     MutationOfLayoutConstrainedField,
@@ -417,6 +539,8 @@ pub fn check_unsafety<'tcx>(tcx: TyCtxt<'tcx>, def: ty::WithOptConstParam<LocalD
         body_unsafety,
         body_target_features,
         is_const,
+        in_possible_lhs_union_assign: false,
+        in_union_destructure: false,
     };
     visitor.visit_expr(&thir[expr]);
 }

compiler/rustc_mir_build/src/thir/visit.rs

@@ -153,8 +153,8 @@ pub fn walk_expr<'a, 'tcx: 'a, V: Visitor<'a, 'tcx>>(visitor: &mut V, expr: &Exp
 }
 
 pub fn walk_stmt<'a, 'tcx: 'a, V: Visitor<'a, 'tcx>>(visitor: &mut V, stmt: &Stmt<'tcx>) {
-    match stmt.kind {
-        StmtKind::Expr { expr, scope: _ } => visitor.visit_expr(&visitor.thir()[expr]),
+    match &stmt.kind {
+        StmtKind::Expr { expr, scope: _ } => visitor.visit_expr(&visitor.thir()[*expr]),
         StmtKind::Let {
             initializer,
             remainder_scope: _,
@@ -163,7 +163,7 @@ pub fn walk_stmt<'a, 'tcx: 'a, V: Visitor<'a, 'tcx>>(visitor: &mut V, stmt: &Stm
             lint_level: _,
         } => {
             if let Some(init) = initializer {
-                visitor.visit_expr(&visitor.thir()[init]);
+                visitor.visit_expr(&visitor.thir()[*init]);
             }
             visitor.visit_pat(pattern);
         }

src/test/ui/issues/issue-47412.mir.stderr

@@ -7,7 +7,7 @@ LL |     match u.void {}
    = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
 
 error[E0133]: dereference of raw pointer is unsafe and requires unsafe function or block
-  --> $DIR/issue-47412.rs:21:11
+  --> $DIR/issue-47412.rs:20:11
    |
 LL |     match *ptr {}
    |           ^^^^ dereference of raw pointer

src/test/ui/issues/issue-47412.rs

@@ -12,8 +12,7 @@ fn union_field() {
     union Union { unit: (), void: Void }
     let u = Union { unit: () };
     match u.void {}
-    //[mir]~^ ERROR access to union field is unsafe
-    // FIXME(thir-unsafeck): AccessToUnionField unimplemented
+    //~^ ERROR access to union field is unsafe
 }
 
 fn raw_ptr_deref() {

src/test/ui/issues/issue-47412.thir.stderr

@@ -1,11 +1,19 @@
+error[E0133]: access to union field is unsafe and requires unsafe function or block
+  --> $DIR/issue-47412.rs:14:11
+   |
+LL |     match u.void {}
+   |           ^^^^^^ access to union field
+   |
+   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
+
 error[E0133]: dereference of raw pointer is unsafe and requires unsafe function or block
-  --> $DIR/issue-47412.rs:21:11
+  --> $DIR/issue-47412.rs:20:11
    |
 LL |     match *ptr {}
    |           ^^^^ dereference of raw pointer
    |
    = note: raw pointers may be null, dangling or unaligned; they can violate aliasing rules and cause data races: all of these are undefined behavior
 
-error: aborting due to previous error
+error: aborting due to 2 previous errors
 
 For more information about this error, try `rustc --explain E0133`.

src/test/ui/union/union-align.rs

@@ -1,4 +1,7 @@
 // run-pass
+// revisions: mirunsafeck thirunsafeck
+// [thirunsafeck]compile-flags: -Z thir-unsafeck
+
 #![allow(dead_code)]
 
 use std::mem::{size_of, size_of_val, align_of, align_of_val};

src/test/ui/union/union-backcomp.rs

@@ -1,4 +1,7 @@
 // run-pass
+// revisions: mirunsafeck thirunsafeck
+// [thirunsafeck]compile-flags: -Z thir-unsafeck
+
 #![allow(path_statements)]
 #![allow(dead_code)]
 

src/test/ui/union/union-basic.rs

@@ -1,4 +1,7 @@
 // run-pass
+// revisions: mirunsafeck thirunsafeck
+// [thirunsafeck]compile-flags: -Z thir-unsafeck
+
 #![allow(unused_imports)]
 
 // aux-build:union.rs

src/test/ui/union/union-borrow-move-parent-sibling.stderr renamed to src/test/ui/union/union-borrow-move-parent-sibling.mirunsafeck.stderr

@@ -1,5 +1,5 @@
 error[E0502]: cannot borrow `u` (via `u.y`) as immutable because it is also borrowed as mutable (via `u.x.0`)
-  --> $DIR/union-borrow-move-parent-sibling.rs:53:13
+  --> $DIR/union-borrow-move-parent-sibling.rs:56:13
    |
 LL |     let a = &mut u.x.0;
    |             ---------- mutable borrow occurs here (via `u.x.0`)
@@ -11,7 +11,7 @@ LL |     use_borrow(a);
    = note: `u.y` is a field of the union `U`, so it overlaps the field `u.x.0`
 
 error[E0382]: use of moved value: `u`
-  --> $DIR/union-borrow-move-parent-sibling.rs:60:13
+  --> $DIR/union-borrow-move-parent-sibling.rs:63:13
    |
 LL |     let u = U { x: ((MockVec::new(), MockVec::new()), MockVec::new()) };
    |         - move occurs because `u` has type `U`, which does not implement the `Copy` trait
@@ -21,7 +21,7 @@ LL |     let b = u.y;
    |             ^^^ value used here after move
 
 error[E0502]: cannot borrow `u` (via `u.y`) as immutable because it is also borrowed as mutable (via `u.x.0.0`)
-  --> $DIR/union-borrow-move-parent-sibling.rs:66:13
+  --> $DIR/union-borrow-move-parent-sibling.rs:69:13
    |
 LL |     let a = &mut (u.x.0).0;
    |             -------------- mutable borrow occurs here (via `u.x.0.0`)
@@ -33,7 +33,7 @@ LL |     use_borrow(a);
    = note: `u.y` is a field of the union `U`, so it overlaps the field `u.x.0.0`
 
 error[E0382]: use of moved value: `u`
-  --> $DIR/union-borrow-move-parent-sibling.rs:73:13
+  --> $DIR/union-borrow-move-parent-sibling.rs:76:13
    |
 LL |     let u = U { x: ((MockVec::new(), MockVec::new()), MockVec::new()) };
    |         - move occurs because `u` has type `U`, which does not implement the `Copy` trait
@@ -43,7 +43,7 @@ LL |     let b = u.y;
    |             ^^^ value used here after move
 
 error[E0502]: cannot borrow `u` (via `u.x`) as immutable because it is also borrowed as mutable (via `u.y`)
-  --> $DIR/union-borrow-move-parent-sibling.rs:79:13
+  --> $DIR/union-borrow-move-parent-sibling.rs:82:13
    |
 LL |     let a = &mut *u.y;
    |                   --- mutable borrow occurs here (via `u.y`)

src/test/ui/union/union-borrow-move-parent-sibling.rs

@@ -1,3 +1,6 @@
+// revisions: mirunsafeck thirunsafeck
+// [thirunsafeck]compile-flags: -Z thir-unsafeck
+
 #![feature(untagged_unions)]
 #![allow(unused)]