Auto merge of #42083 - petrochenkov:safeassign, r=nikomatsakis

Make assignments to `Copy` union fields safe

This is an accompanying PR to PR #42068 stabilizing FFI unions.

This was first proposed in #32836 (comment), see subsequent comments as well.
Assignments to `Copy` union fields do not read any data from the union and are [equivalent](#32836 (comment)) to whole union assignments, which are safe, so they should be safe as well. This removes a significant number of "false positive" unsafe blocks, in code dealing with FFI unions in particular.

It desirable to make this change now, together with stabilization of FFI unions, because now it affecfts only unstable code, but later it will cause warnings/errors caused by `unused_unsafe` lint in stable code.

cc #32836
r? @nikomatsakis

Author: bors

src/librustc/middle/effect.rs

@@ -52,6 +52,7 @@ fn type_is_unsafe_function(ty: Ty) -> bool {
 struct EffectCheckVisitor<'a, 'tcx: 'a> {
     tcx: TyCtxt<'a, 'tcx, 'tcx>,
     tables: &'a ty::TypeckTables<'tcx>,
+    body_id: hir::BodyId,
 
     /// Whether we're in an unsafe context.
     unsafe_context: UnsafeContext,
@@ -99,10 +100,13 @@ impl<'a, 'tcx> Visitor<'tcx> for EffectCheckVisitor<'a, 'tcx> {
 
     fn visit_nested_body(&mut self, body: hir::BodyId) {
         let old_tables = self.tables;
+        let old_body_id = self.body_id;
         self.tables = self.tcx.body_tables(body);
+        self.body_id = body;
         let body = self.tcx.hir.body(body);
         self.visit_body(body);
         self.tables = old_tables;
+        self.body_id = old_body_id;
     }
 
     fn visit_fn(&mut self, fn_kind: FnKind<'tcx>, fn_decl: &'tcx hir::FnDecl,
@@ -218,6 +222,25 @@ impl<'a, 'tcx> Visitor<'tcx> for EffectCheckVisitor<'a, 'tcx> {
                     }
                 }
             }
+            hir::ExprAssign(ref lhs, ref rhs) => {
+                if let hir::ExprField(ref base_expr, field) = lhs.node {
+                    if let ty::TyAdt(adt, ..) = self.tables.expr_ty_adjusted(base_expr).sty {
+                        if adt.is_union() {
+                            let field_ty = self.tables.expr_ty_adjusted(lhs);
+                            let owner_def_id = self.tcx.hir.body_owner_def_id(self.body_id);
+                            let param_env = self.tcx.param_env(owner_def_id);
+                            if field_ty.moves_by_default(self.tcx, param_env, field.span) {
+                                self.require_unsafe(field.span,
+                                                    "assignment to non-`Copy` union field");
+                            }
+                            // Do not walk the field expr again.
+                            intravisit::walk_expr(self, base_expr);
+                            intravisit::walk_expr(self, rhs);
+                            return
+                        }
+                    }
+                }
+            }
             _ => {}
         }
 
@@ -243,6 +266,7 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
     let mut visitor = EffectCheckVisitor {
         tcx: tcx,
         tables: &ty::TypeckTables::empty(),
+        body_id: hir::BodyId { node_id: ast::CRATE_NODE_ID },
         unsafe_context: UnsafeContext::new(SafeContext),
     };
 

src/test/compile-fail/union/union-unsafe.rs

@@ -10,15 +10,46 @@
 
 #![feature(untagged_unions)]
 
-union U {
+union U1 {
     a: u8
 }
 
+union U2 {
+    a: String
+}
+
+union U3<T> {
+    a: T
+}
+
+union U4<T: Copy> {
+    a: T
+}
+
+fn generic_noncopy<T: Default>() {
+    let mut u3 = U3 { a: T::default() };
+    u3.a = T::default(); //~ ERROR assignment to non-`Copy` union field requires unsafe
+}
+
+fn generic_copy<T: Copy + Default>() {
+    let mut u3 = U3 { a: T::default() };
+    u3.a = T::default(); // OK
+    let mut u4 = U4 { a: T::default() };
+    u4.a = T::default(); // OK
+}
+
 fn main() {
-    let mut u = U { a: 10 }; // OK
-    let a = u.a; //~ ERROR access to union field requires unsafe function or block
-    u.a = 11; //~ ERROR access to union field requires unsafe function or block
-    let U { a } = u; //~ ERROR matching on union field requires unsafe function or block
-    if let U { a: 12 } = u {} //~ ERROR matching on union field requires unsafe function or block
-    // let U { .. } = u; // OK
+    let mut u1 = U1 { a: 10 }; // OK
+    let a = u1.a; //~ ERROR access to union field requires unsafe
+    u1.a = 11; // OK
+    let U1 { a } = u1; //~ ERROR matching on union field requires unsafe
+    if let U1 { a: 12 } = u1 {} //~ ERROR matching on union field requires unsafe
+    // let U1 { .. } = u1; // OK
+
+    let mut u2 = U2 { a: String::from("old") }; // OK
+    u2.a = String::from("new"); //~ ERROR assignment to non-`Copy` union field requires unsafe
+    let mut u3 = U3 { a: 0 }; // OK
+    u3.a = 1; // OK
+    let mut u3 = U3 { a: String::from("old") }; // OK
+    u3.a = String::from("new"); //~ ERROR assignment to non-`Copy` union field requires unsafe
 }