Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not apply DerefMut on union field #75584

Merged
merged 4 commits into from
Sep 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions compiler/rustc_typeck/src/check/place_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -236,6 +244,21 @@ 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 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 `ManuallyDrop` union field",
);
err.help(
"writing to this reference 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;
Expand Down
28 changes: 28 additions & 0 deletions src/test/ui/union/union-deref.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// ignore-tidy-linelength
//! Test the part of RFC 2514 that is about not applying `DerefMut` coercions
//! of union fields.
#![feature(untagged_unions)]

use std::mem::ManuallyDrop;

union U1<T> { x:(), f: ManuallyDrop<(T,)> }

union U2<T> { x:(), f: (ManuallyDrop<(T,)>,) }

fn main() {
let mut u : U1<Vec<i32>> = 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<Vec<i32>> = 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
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
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
}
56 changes: 56 additions & 0 deletions src/test/ui/union/union-deref.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
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 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() };
| ^^^^^^^
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The span for this is slightly wrong, I'd expect it to only cover u.f.0. I don't know how this is happening, since the span is right above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess this has to do with the special handling of multiple tuple field accesses in a row. Perhaps 0.0 is a float token.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know there was such special handling.^^ So this would be a problem elsewhere, in the code that sets up the span for these places?

|
= 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: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