From c5dda05e4e47f8435400c27e31f198f88147dd9e Mon Sep 17 00:00:00 2001
From: LeSeulArtichaut <leseulartichaut@gmail.com>
Date: Fri, 23 Jul 2021 15:26:12 +0200
Subject: [PATCH] Implement `AssignToDroppingUnionField` in THIR unsafeck
---
.../rustc_mir_build/src/check_unsafety.rs | 48 ++++++++++++-------
src/test/ui/union/union-unsafe.rs | 4 +-
src/test/ui/union/union-unsafe.thir.stderr | 18 ++++++-
3 files changed, 50 insertions(+), 20 deletions(-)
diff --git a/compiler/rustc_mir_build/src/check_unsafety.rs b/compiler/rustc_mir_build/src/check_unsafety.rs
index 82e19c0552720..21534290d1291 100644
--- a/compiler/rustc_mir_build/src/check_unsafety.rs
+++ b/compiler/rustc_mir_build/src/check_unsafety.rs
@@ -5,7 +5,7 @@ use rustc_errors::struct_span_err;
use rustc_hir as hir;
use rustc_middle::mir::BorrowKind;
use rustc_middle::thir::*;
-use rustc_middle::ty::{self, ParamEnv, TyCtxt};
+use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt};
use rustc_session::lint::builtin::{UNSAFE_OP_IN_UNSAFE_FN, UNUSED_UNSAFE};
use rustc_session::lint::Level;
use rustc_span::def_id::{DefId, LocalDefId};
@@ -27,7 +27,9 @@ struct UnsafetyVisitor<'a, 'tcx> {
/// The `#[target_feature]` attributes of the body. Used for checking
/// calls to functions with `#[target_feature]` (RFC 2396).
body_target_features: &'tcx Vec<Symbol>,
- in_possible_lhs_union_assign: bool,
+ /// When inside the LHS of an assignment to a field, this is the type
+ /// of the LHS and the span of the assignment expression.
+ assignment_info: Option<(Ty<'tcx>, Span)>,
in_union_destructure: bool,
param_env: ParamEnv<'tcx>,
inside_adt: bool,
@@ -287,7 +289,7 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
}
fn visit_expr(&mut self, expr: &Expr<'tcx>) {
- // could we be in a the LHS of an assignment of a union?
+ // could we be in the LHS of an assignment to a field?
match expr.kind {
ExprKind::Field { .. }
| ExprKind::VarRef { .. }
@@ -329,7 +331,12 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
| ExprKind::InlineAsm { .. }
| ExprKind::LlvmInlineAsm { .. }
| ExprKind::LogicalOp { .. }
- | ExprKind::Use { .. } => self.in_possible_lhs_union_assign = false,
+ | ExprKind::Use { .. } => {
+ // We don't need to save the old value and restore it
+ // because all the place expressions can't have more
+ // than one child.
+ self.assignment_info = None;
+ }
};
match expr.kind {
ExprKind::Scope { value, lint_level: LintLevel::Explicit(hir_id), region_scope: _ } => {
@@ -409,11 +416,21 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
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() {
+ let lhs = &self.thir[lhs];
+ if let ty::Adt(adt_def, _) = lhs.ty.kind() {
if adt_def.is_union() {
- if self.in_possible_lhs_union_assign {
- // FIXME: trigger AssignToDroppingUnionField unsafety if needed
+ if let Some((assigned_ty, assignment_span)) = self.assignment_info {
+ // To avoid semver hazard, we only consider `Copy` and `ManuallyDrop` non-dropping.
+ if !(assigned_ty
+ .ty_adt_def()
+ .map_or(false, |adt| adt.is_manually_drop())
+ || assigned_ty
+ .is_copy_modulo_regions(self.tcx.at(expr.span), self.param_env))
+ {
+ self.requires_unsafe(assignment_span, AssignToDroppingUnionField);
+ } else {
+ // write to non-drop union field, safe
+ }
} else {
self.requires_unsafe(expr.span, AccessToUnionField);
}
@@ -421,9 +438,10 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
}
}
ExprKind::Assign { lhs, rhs } | ExprKind::AssignOp { lhs, rhs, .. } => {
+ let lhs = &self.thir[lhs];
// First, check whether we are mutating a layout constrained field
let mut visitor = LayoutConstrainedPlaceVisitor::new(self.thir, self.tcx);
- visit::walk_expr(&mut visitor, &self.thir[lhs]);
+ visit::walk_expr(&mut visitor, lhs);
if visitor.found {
self.requires_unsafe(expr.span, MutationOfLayoutConstrainedField);
}
@@ -431,10 +449,9 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
// Second, check for accesses to union fields
// don't have any special handling for AssignOp since it causes a read *and* write to lhs
if matches!(expr.kind, ExprKind::Assign { .. }) {
- // 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;
+ self.assignment_info = Some((lhs.ty, expr.span));
+ visit::walk_expr(self, lhs);
+ self.assignment_info = None;
visit::walk_expr(self, &self.thir()[rhs]);
return; // we have already visited everything by now
}
@@ -506,12 +523,9 @@ enum UnsafeOpKind {
UseOfMutableStatic,
UseOfExternStatic,
DerefOfRawPointer,
- #[allow(dead_code)] // FIXME
AssignToDroppingUnionField,
AccessToUnionField,
- #[allow(dead_code)] // FIXME
MutationOfLayoutConstrainedField,
- #[allow(dead_code)] // FIXME
BorrowOfLayoutConstrainedField,
CallToFunctionWith,
}
@@ -619,7 +633,7 @@ pub fn check_unsafety<'tcx>(tcx: TyCtxt<'tcx>, def: ty::WithOptConstParam<LocalD
hir_context: hir_id,
body_unsafety,
body_target_features,
- in_possible_lhs_union_assign: false,
+ assignment_info: None,
in_union_destructure: false,
param_env: tcx.param_env(def.did),
inside_adt: false,
diff --git a/src/test/ui/union/union-unsafe.rs b/src/test/ui/union/union-unsafe.rs
index e8414903d548f..3cb3a18cb7544 100644
--- a/src/test/ui/union/union-unsafe.rs
+++ b/src/test/ui/union/union-unsafe.rs
@@ -36,8 +36,8 @@ fn deref_union_field(mut u: URef) {
fn assign_noncopy_union_field(mut u: URefCell) {
// FIXME(thir-unsafeck)
- u.a = (RefCell::new(0), 1); //[mir]~ ERROR assignment to union field that might need dropping
- u.a.0 = RefCell::new(0); //[mir]~ ERROR assignment to union field that might need dropping
+ u.a = (RefCell::new(0), 1); //~ ERROR assignment to union field that might need dropping
+ u.a.0 = RefCell::new(0); //~ ERROR assignment to union field that might need dropping
u.a.1 = 1; // OK
}
diff --git a/src/test/ui/union/union-unsafe.thir.stderr b/src/test/ui/union/union-unsafe.thir.stderr
index 51f19879c8195..e88642b0ff7ad 100644
--- a/src/test/ui/union/union-unsafe.thir.stderr
+++ b/src/test/ui/union/union-unsafe.thir.stderr
@@ -6,6 +6,22 @@ LL | *(u.p) = 13;
|
= note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
+error[E0133]: assignment to union field that might need dropping is unsafe and requires unsafe function or block
+ --> $DIR/union-unsafe.rs:39:5
+ |
+LL | u.a = (RefCell::new(0), 1);
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to union field that might need dropping
+ |
+ = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized
+
+error[E0133]: assignment to union field that might need dropping is unsafe and requires unsafe function or block
+ --> $DIR/union-unsafe.rs:40:5
+ |
+LL | u.a.0 = RefCell::new(0);
+ | ^^^^^^^^^^^^^^^^^^^^^^^ assignment to union field that might need dropping
+ |
+ = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized
+
error[E0133]: access to union field is unsafe and requires unsafe function or block
--> $DIR/union-unsafe.rs:47:6
|
@@ -70,6 +86,6 @@ LL | *u3.a = String::from("new");
|
= note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
-error: aborting due to 9 previous errors
+error: aborting due to 11 previous errors
For more information about this error, try `rustc --explain E0133`.