From ec0924f9649ea472782df6d21595714cb594f038 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 16 Aug 2020 10:07:33 +0200 Subject: [PATCH 1/4] do not apply DerefMut on union field --- compiler/rustc_typeck/src/check/place_op.rs | 16 +++++++++++++++- src/test/ui/union/union-deref.rs | 13 +++++++++++++ src/test/ui/union/union-deref.stderr | 11 +++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/union/union-deref.rs create mode 100644 src/test/ui/union/union-deref.stderr diff --git a/compiler/rustc_typeck/src/check/place_op.rs b/compiler/rustc_typeck/src/check/place_op.rs index 4bef9aecd2ecb..be2bc491c311b 100644 --- a/compiler/rustc_typeck/src/check/place_op.rs +++ b/compiler/rustc_typeck/src/check/place_op.rs @@ -193,7 +193,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { /// Convert auto-derefs, indices, etc of an expression from `Deref` and `Index` /// into `DerefMut` and `IndexMut` respectively. /// - /// This is a second pass of typechecking derefs/indices. We need this we do not + /// This is a second pass of typechecking derefs/indices. We need this because we do not /// always know whether a place needs to be mutable or not in the first pass. /// This happens whether there is an implicit mutable reborrow, e.g. when the type /// is used as the receiver of a method call. @@ -236,6 +236,20 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if let ty::Ref(region, _, mutbl) = method.sig.output().kind { *deref = OverloadedDeref { region, mutbl }; } + // If this is a union field, also throw an error. + // Union fields should not get mutable auto-deref'd (see RFC 2514). + if let hir::ExprKind::Field(ref outer_expr, _) = expr.kind { + let ty = self.node_ty(outer_expr.hir_id); + if ty.ty_adt_def().map_or(false, |adt| adt.is_union()) { + let mut err = self.tcx.sess.struct_span_err( + expr.span, + "not automatically applying `DerefMut` on union field", + ); + err.help("writing to this field calls the destructor for the old value"); + err.help("add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor"); + err.emit(); + } + } } } source = adjustment.target; diff --git a/src/test/ui/union/union-deref.rs b/src/test/ui/union/union-deref.rs new file mode 100644 index 0000000000000..61bbe73354f04 --- /dev/null +++ b/src/test/ui/union/union-deref.rs @@ -0,0 +1,13 @@ +//! Test the part of RFC 2514 that is about not applying `DerefMut` coercions +//! of union fields. +#![feature(untagged_unions)] + +use std::mem::ManuallyDrop; + +union U { x:(), f: ManuallyDrop<(T,)> } + +fn main() { + let mut u : U> = U { x: () }; + unsafe { (*u.f).0 = Vec::new() }; // explicit deref, this compiles + unsafe { u.f.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on union field +} diff --git a/src/test/ui/union/union-deref.stderr b/src/test/ui/union/union-deref.stderr new file mode 100644 index 0000000000000..66cc90cbd3d01 --- /dev/null +++ b/src/test/ui/union/union-deref.stderr @@ -0,0 +1,11 @@ +error: not automatically applying `DerefMut` on union field + --> $DIR/union-deref.rs:12:14 + | +LL | unsafe { u.f.0 = Vec::new() }; + | ^^^ + | + = help: writing to this field calls the destructor for the old value + = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor + +error: aborting due to previous error + From 44defaea3a2dd2e7e40336d3609df12b83db424a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 16 Aug 2020 10:24:10 +0200 Subject: [PATCH 2/4] also detect DerefMut in nested union fields --- compiler/rustc_typeck/src/check/place_op.rs | 29 ++++++++++++--------- src/test/ui/union/union-deref.rs | 10 +++++-- src/test/ui/union/union-deref.stderr | 13 +++++++-- 3 files changed, 36 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_typeck/src/check/place_op.rs b/compiler/rustc_typeck/src/check/place_op.rs index be2bc491c311b..54b3fdd837e66 100644 --- a/compiler/rustc_typeck/src/check/place_op.rs +++ b/compiler/rustc_typeck/src/check/place_op.rs @@ -211,13 +211,21 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { debug!("convert_place_derefs_to_mutable: exprs={:?}", exprs); // Fix up autoderefs and derefs. + let mut inside_union = false; for (i, &expr) in exprs.iter().rev().enumerate() { debug!("convert_place_derefs_to_mutable: i={} expr={:?}", i, expr); + let mut source = self.node_ty(expr.hir_id); + if matches!(expr.kind, hir::ExprKind::Unary(hir::UnOp::UnDeref, _)) { + // Clear previous flag; after a pointer indirection it does not apply any more. + inside_union = false; + } + if source.ty_adt_def().map_or(false, |adt| adt.is_union()) { + inside_union = true; + } // Fix up the autoderefs. Autorefs can only occur immediately preceding // overloaded place ops, and will be fixed by them in order to get // the correct region. - let mut source = self.node_ty(expr.hir_id); // Do not mutate adjustments in place, but rather take them, // and replace them after mutating them, to avoid having the // typeck results borrowed during (`deref_mut`) method resolution. @@ -238,17 +246,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } // If this is a union field, also throw an error. // Union fields should not get mutable auto-deref'd (see RFC 2514). - if let hir::ExprKind::Field(ref outer_expr, _) = expr.kind { - let ty = self.node_ty(outer_expr.hir_id); - if ty.ty_adt_def().map_or(false, |adt| adt.is_union()) { - let mut err = self.tcx.sess.struct_span_err( - expr.span, - "not automatically applying `DerefMut` on union field", - ); - err.help("writing to this field calls the destructor for the old value"); - err.help("add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor"); - err.emit(); - } + if inside_union { + let mut err = self.tcx.sess.struct_span_err( + expr.span, + "not automatically applying `DerefMut` on union field", + ); + err.help("writing to this field calls the destructor for the old value"); + err.help("add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor"); + err.emit(); } } } diff --git a/src/test/ui/union/union-deref.rs b/src/test/ui/union/union-deref.rs index 61bbe73354f04..4578110cd54eb 100644 --- a/src/test/ui/union/union-deref.rs +++ b/src/test/ui/union/union-deref.rs @@ -4,10 +4,16 @@ use std::mem::ManuallyDrop; -union U { x:(), f: ManuallyDrop<(T,)> } +union U1 { x:(), f: ManuallyDrop<(T,)> } + +union U2 { x:(), f: (ManuallyDrop<(T,)>,) } fn main() { - let mut u : U> = U { x: () }; + let mut u : U1> = U1 { x: () }; unsafe { (*u.f).0 = Vec::new() }; // explicit deref, this compiles unsafe { u.f.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on union field + + let mut u : U2> = U2 { x: () }; + unsafe { (*u.f.0).0 = Vec::new() }; // explicit deref, this compiles + unsafe { u.f.0.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on union field } diff --git a/src/test/ui/union/union-deref.stderr b/src/test/ui/union/union-deref.stderr index 66cc90cbd3d01..93374436d2f66 100644 --- a/src/test/ui/union/union-deref.stderr +++ b/src/test/ui/union/union-deref.stderr @@ -1,5 +1,5 @@ error: not automatically applying `DerefMut` on union field - --> $DIR/union-deref.rs:12:14 + --> $DIR/union-deref.rs:14:14 | LL | unsafe { u.f.0 = Vec::new() }; | ^^^ @@ -7,5 +7,14 @@ LL | unsafe { u.f.0 = Vec::new() }; = help: writing to this field calls the destructor for the old value = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor -error: aborting due to previous error +error: not automatically applying `DerefMut` on union field + --> $DIR/union-deref.rs:18:14 + | +LL | unsafe { u.f.0.0 = Vec::new() }; + | ^^^^^^^ + | + = help: writing to this field calls the destructor for the old value + = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor + +error: aborting due to 2 previous errors From 97974e3cabefe725b6bea24c20e5fb9709e08a02 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 16 Aug 2020 10:32:37 +0200 Subject: [PATCH 3/4] only emit error for ManuallyDrop derefs --- compiler/rustc_typeck/src/check/place_op.rs | 14 +++++++++----- src/test/ui/union/union-deref.rs | 5 +++-- src/test/ui/union/union-deref.stderr | 12 ++++++------ 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_typeck/src/check/place_op.rs b/compiler/rustc_typeck/src/check/place_op.rs index 54b3fdd837e66..c6d5c934a7e3a 100644 --- a/compiler/rustc_typeck/src/check/place_op.rs +++ b/compiler/rustc_typeck/src/check/place_op.rs @@ -244,14 +244,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if let ty::Ref(region, _, mutbl) = method.sig.output().kind { *deref = OverloadedDeref { region, mutbl }; } - // If this is a union field, also throw an error. - // Union fields should not get mutable auto-deref'd (see RFC 2514). - if inside_union { + // If this is a union field, also throw an error for `DerefMut` of `ManuallyDrop` (see RFC 2514). + // This helps avoid accidental drops. + if inside_union + && source.ty_adt_def().map_or(false, |adt| adt.is_manually_drop()) + { let mut err = self.tcx.sess.struct_span_err( expr.span, - "not automatically applying `DerefMut` on union field", + "not automatically applying `DerefMut` on `ManuallyDrop` union field", + ); + err.help( + "writing to this reference calls the destructor for the old value", ); - err.help("writing to this field calls the destructor for the old value"); err.help("add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor"); err.emit(); } diff --git a/src/test/ui/union/union-deref.rs b/src/test/ui/union/union-deref.rs index 4578110cd54eb..d789659a807b5 100644 --- a/src/test/ui/union/union-deref.rs +++ b/src/test/ui/union/union-deref.rs @@ -1,3 +1,4 @@ +// ignore-tidy-linelength //! Test the part of RFC 2514 that is about not applying `DerefMut` coercions //! of union fields. #![feature(untagged_unions)] @@ -11,9 +12,9 @@ union U2 { x:(), f: (ManuallyDrop<(T,)>,) } fn main() { let mut u : U1> = U1 { x: () }; unsafe { (*u.f).0 = Vec::new() }; // explicit deref, this compiles - unsafe { u.f.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on union field + unsafe { u.f.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field let mut u : U2> = U2 { x: () }; unsafe { (*u.f.0).0 = Vec::new() }; // explicit deref, this compiles - unsafe { u.f.0.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on union field + unsafe { u.f.0.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field } diff --git a/src/test/ui/union/union-deref.stderr b/src/test/ui/union/union-deref.stderr index 93374436d2f66..03428489318c0 100644 --- a/src/test/ui/union/union-deref.stderr +++ b/src/test/ui/union/union-deref.stderr @@ -1,19 +1,19 @@ -error: not automatically applying `DerefMut` on union field - --> $DIR/union-deref.rs:14:14 +error: not automatically applying `DerefMut` on `ManuallyDrop` union field + --> $DIR/union-deref.rs:15:14 | LL | unsafe { u.f.0 = Vec::new() }; | ^^^ | - = help: writing to this field calls the destructor for the old value + = help: writing to this reference calls the destructor for the old value = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor -error: not automatically applying `DerefMut` on union field - --> $DIR/union-deref.rs:18:14 +error: not automatically applying `DerefMut` on `ManuallyDrop` union field + --> $DIR/union-deref.rs:19:14 | LL | unsafe { u.f.0.0 = Vec::new() }; | ^^^^^^^ | - = help: writing to this field calls the destructor for the old value + = help: writing to this reference calls the destructor for the old value = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor error: aborting due to 2 previous errors From 66b340f5003910bd8b46f6675cd9a491809aa9fe Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 30 Aug 2020 19:12:36 +0200 Subject: [PATCH 4/4] test more ways of mutably accessing a place --- src/test/ui/union/union-deref.rs | 8 ++++++ src/test/ui/union/union-deref.stderr | 38 +++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/test/ui/union/union-deref.rs b/src/test/ui/union/union-deref.rs index d789659a807b5..df598eea9ef0f 100644 --- a/src/test/ui/union/union-deref.rs +++ b/src/test/ui/union/union-deref.rs @@ -13,8 +13,16 @@ fn main() { let mut u : U1> = U1 { x: () }; unsafe { (*u.f).0 = Vec::new() }; // explicit deref, this compiles unsafe { u.f.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field + unsafe { &mut (*u.f).0 }; // explicit deref, this compiles + unsafe { &mut u.f.0 }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field + unsafe { (*u.f).0.push(0) }; // explicit deref, this compiles + unsafe { u.f.0.push(0) }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field let mut u : U2> = U2 { x: () }; unsafe { (*u.f.0).0 = Vec::new() }; // explicit deref, this compiles unsafe { u.f.0.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field + unsafe { &mut (*u.f.0).0 }; // explicit deref, this compiles + unsafe { &mut u.f.0.0 }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field + unsafe { (*u.f.0).0.push(0) }; // explicit deref, this compiles + unsafe { u.f.0.0.push(0) }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field } diff --git a/src/test/ui/union/union-deref.stderr b/src/test/ui/union/union-deref.stderr index 03428489318c0..fb16649767fb7 100644 --- a/src/test/ui/union/union-deref.stderr +++ b/src/test/ui/union/union-deref.stderr @@ -7,14 +7,50 @@ LL | unsafe { u.f.0 = Vec::new() }; = help: writing to this reference calls the destructor for the old value = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor +error: not automatically applying `DerefMut` on `ManuallyDrop` union field + --> $DIR/union-deref.rs:17:19 + | +LL | unsafe { &mut u.f.0 }; + | ^^^ + | + = help: writing to this reference calls the destructor for the old value + = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor + error: not automatically applying `DerefMut` on `ManuallyDrop` union field --> $DIR/union-deref.rs:19:14 | +LL | unsafe { u.f.0.push(0) }; + | ^^^ + | + = help: writing to this reference calls the destructor for the old value + = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor + +error: not automatically applying `DerefMut` on `ManuallyDrop` union field + --> $DIR/union-deref.rs:23:14 + | LL | unsafe { u.f.0.0 = Vec::new() }; | ^^^^^^^ | = help: writing to this reference calls the destructor for the old value = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor -error: aborting due to 2 previous errors +error: not automatically applying `DerefMut` on `ManuallyDrop` union field + --> $DIR/union-deref.rs:25:19 + | +LL | unsafe { &mut u.f.0.0 }; + | ^^^^^^^ + | + = help: writing to this reference calls the destructor for the old value + = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor + +error: not automatically applying `DerefMut` on `ManuallyDrop` union field + --> $DIR/union-deref.rs:27:14 + | +LL | unsafe { u.f.0.0.push(0) }; + | ^^^^^^^ + | + = help: writing to this reference calls the destructor for the old value + = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor + +error: aborting due to 6 previous errors