From 4386f97a93ea0b5f17807f894db5cb662893de03 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Thu, 18 May 2017 13:40:15 +0300 Subject: [PATCH 1/2] Make assignments to `Copy` union fields safe --- src/librustc/middle/effect.rs | 18 ++++++++ src/test/compile-fail/union/union-unsafe.rs | 46 +++++++++++++++++---- 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/librustc/middle/effect.rs b/src/librustc/middle/effect.rs index 5360a86560d39..94362e69c8dd9 100644 --- a/src/librustc/middle/effect.rs +++ b/src/librustc/middle/effect.rs @@ -218,6 +218,24 @@ 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 param_env = self.tcx.parameter_environment(adt.did); + if field_ty.moves_by_default(self.tcx, ¶m_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 + } + } + } + } _ => {} } diff --git a/src/test/compile-fail/union/union-unsafe.rs b/src/test/compile-fail/union/union-unsafe.rs index 97e1ec2cba865..a67603675f175 100644 --- a/src/test/compile-fail/union/union-unsafe.rs +++ b/src/test/compile-fail/union/union-unsafe.rs @@ -10,15 +10,47 @@ #![feature(untagged_unions)] -union U { +union U1 { a: u8 } +union U2 { + a: String +} + +union U3 { + a: T +} + +union U4 { + a: T +} + +fn generic_noncopy() { + let mut u3 = U3 { a: T::default() }; + u3.a = T::default(); //~ ERROR assignment to non-`Copy` union field requires unsafe +} + +fn generic_copy() { + let mut u3 = U3 { a: T::default() }; + // FIXME: it should be known here that `T: Copy`, need to use correct "parameter environment" + u3.a = T::default(); //~ ERROR assignment to non-`Copy` union field requires unsafe + 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 } From fa13cd3489c09d2de6ae4b2d0d455c1cf3db82fc Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Thu, 18 May 2017 14:28:40 +0300 Subject: [PATCH 2/2] Use parameter environment associated with field use, not field definition --- src/librustc/middle/effect.rs | 10 ++++++++-- src/test/compile-fail/union/union-unsafe.rs | 3 +-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/librustc/middle/effect.rs b/src/librustc/middle/effect.rs index 94362e69c8dd9..2261f296454ef 100644 --- a/src/librustc/middle/effect.rs +++ b/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, @@ -223,8 +227,9 @@ impl<'a, 'tcx> Visitor<'tcx> for EffectCheckVisitor<'a, 'tcx> { 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 param_env = self.tcx.parameter_environment(adt.did); - if field_ty.moves_by_default(self.tcx, ¶m_env, field.span) { + 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"); } @@ -261,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), }; diff --git a/src/test/compile-fail/union/union-unsafe.rs b/src/test/compile-fail/union/union-unsafe.rs index a67603675f175..2e018e696a415 100644 --- a/src/test/compile-fail/union/union-unsafe.rs +++ b/src/test/compile-fail/union/union-unsafe.rs @@ -33,8 +33,7 @@ fn generic_noncopy() { fn generic_copy() { let mut u3 = U3 { a: T::default() }; - // FIXME: it should be known here that `T: Copy`, need to use correct "parameter environment" - u3.a = T::default(); //~ ERROR assignment to non-`Copy` union field requires unsafe + u3.a = T::default(); // OK let mut u4 = U4 { a: T::default() }; u4.a = T::default(); // OK }