consider assignments of union field of ManuallyDrop type safe

[RalfJung]

Assigning to Copy union fields is safe because that assignment will never drop anything. However, with #77547, unions may also have ManuallyDrop fields, and their assignments are currently still unsafe. That seems unnecessary though, as assigning ManuallyDrop does not drop anything either, and is thus safe even for union fields.

I assume this will at least require FCP.

[rust-highfive]

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

[RalfJung]

What slightly concerns me here is that this is the context of the entire place, but we are considering here just a single place projection. What if there is a deref involved?

#![feature(untagged_unions)]

union U {
    p: &'static mut i32,
}

fn test(u: U) {
    *(u.p) = 13;
}

This does error, but the error claims "assignment to non-Copy union field", which is not really what happens (the value of p is not changed). The problem is that this is reading a union field. If I made the test here even smarter to take into account needs_drop, then that code above would be incorrectly accepted!

In fact, union fields that drop are not even permitted any more (with no amount of feature gates).

[RalfJung]

Fixed this by checking for Deref explicitly. The logic is kind of strange now but I think it works -- and I added a test to confirm that the test above how fails with the right error:

error[E0133]: access to union field is unsafe and requires unsafe function or block
  --> $DIR/union-unsafe.rs:31:5
   |
LL |     *(u.p) = 13;
   |     ^^^^^^^^^^^ access to union field
   |
   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior

(instead of "assignment to union field")

[RalfJung]

This leads to the funny situation where, inside core::mem::manually_drop, unions are unsound, because we can do things like

union U {
  f: ManuallyDrop<Vec<i32>>,
}

fn test(u: U) {
  u.f.value = Vec::new();
}

It's a bit awkward to be checking the type of the field here when really we care about the type of the assignment (which I guess is the type of the overall place).

[RalfJung]

Fixed this by checking the actually assigned type, not the type of the projection.

[nikomatsakis]

Nominating for lang-team discussion.

[RalfJung]

This strikes me as rather suboptimal formatting, bur rustfmt insists on it...

[scottmcm]

Ouch. This is poor even in isolation, but with the || chain it's terrible 🙁

You're in the compiler, so I guess you could try matches!(context, PlaceContext::MutatingUse(MutatingUseContext::Store | MutatingUseContext::Drop | MutatingUseContext::AsmOutput)?

[RalfJung]

I reported this against rustfmt: rust-lang/rustfmt#4492

[RalfJung]

Your proposal is formatted slightly better:

let assign_to_field = matches!(
                        context,
                        PlaceContext::MutatingUse(
                            MutatingUseContext::Store
                                | MutatingUseContext::Drop
                                | MutatingUseContext::AsmOutput
                        )
                    );

Still not great, but the three alternatives do not fit on one line so it is not easy to make this nice.

[scottmcm]

nit: You mention on line 25 that the field doesn't actually need dropping, so I wonder if there's a way to rephrase this to avoid a potential "no it doesn't" reaction from someone reading it. The best that comes to mind is "... that is neither Copy nor ManuallyDrop<_>", but that's not great either.

[RalfJung]

I considered "that might need dropping", what do you think about that?

[scottmcm]

I guess if I take a step back the answer might be that this is still unstable so it doesn't really matter.

Reading the tracking issue more, it seems like this would only be stabilized with a "this will never be drop in the future" trait, so if that were to happen then this message could just mention that.

So I'll just call this resolved.

(Hmm, impl !Drop for Foo{} got implemented, didn't it...)

[scottmcm]

This sounds good to me.

I agree that it can be safe (which is itself a weak should), and I don't think it's materially more likely to lead to accidental leaking than other things that have already been made safe. Notably, if you have union(u32, ManuallyDrop<String>), the safe assignment to the u32 is already a safe way to leak that String. And the assignment some_union.f = some_manually_drop; is just as leak-inducing as if it were some_local = some_manually_drop; -- the "lint" about leaking is the ManuallyDrop::new call.

And the fact that it's wrapped in ManuallyDrop means it can never need dropping, so there are no semver worries.

So while we haven't actually talked about this in a meeting yet, let's see what happens with
@rfcbot merge
(I see at least one other lang thumb right now.)

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

This seems fine to me. You can't implicitly turn a T into a ManuallyDrop<T>, and once you have a ManuallyDrop<T> I see no problem with being able to assign that safely like any other non-Drop type.

@rfcbot reviewed

🔔 This is now entering its final comment period, as per the review above. 🔔

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@scottmcm can you take over review of this PR?

@nikomatsakis I did (a version of) the refactoring you asked for. I think the new iter_projections would also be useful in other situations (and I hope Place::ty_from can be made private by properly using PlaceRef::ty everywhere instead), but that is definitely material for a separate PR.

I also disentangled the place safety checks, so it's no longer one big mess checking 3 or 4 safety conditions at once. I found that really hard to follow.

Unfortunately, for some reason I cannot figure out, this change leads to the unsafety error for accessing mutable statics to be raised twice, with slightly different spans, as you can see in the "static-mut-foreign-requires-unsafe" test. Any idea why that might be happening? Cc @oli-obk

Unfortunately, for some reason I cannot figure out, this change leads to the unsafety error for accessing mutable statics to be raised twice, with slightly different spans, as you can see in the "static-mut-foreign-requires-unsafe" test. Any idea why that might be happening?

This is because visit_place is invoked on both the assignment _tmp = const { &STATIC } and on the usage: *_tmp = ...; or ... = *_tmp; Before this PR, the duplication of diagnostics was avoided by only checking for derefs of the StaticRef local.

This is because visit_place is invoked on both the assignment _tmp = const { &STATIC } and on the usage: *_tmp = ...; or ... = *_tmp; Before this PR, the duplication of diagnostics was avoided by only checking for derefs of the StaticRef local.

Hm, I see... that does seem rather fragile TBH. But I guess we have tests. I'll try to work with this, thanks!

Yeah that seems to work, the "unsafe" and "static" tests works now. :) Let's see about the rest of the test suite.

@RalfJung thanks for pursuing that. I'll take a look.

this looks good -- I have one nit

ah, cute, I missed that this was a double-ended iterator at first and wondered how you were going to manage this

I think it would be useful to give an example based on Rust code. For example:

Given the place a.b.c, this would yield:

• (a, b)
• (a.b, c)

I am a bit surprised by this structure -- I guess I expected it to return a, a.b, and a.b.c, rather than a tuple, and to have people match on the "tail" projection (if any). But I guess this is ok too.

I expanded the comment.

I expected it to return a, a.b, and a.b.c, rather than a tuple, and to have people match on the "tail" projection (if any). But I guess this is ok too.

I first thought of something like this, but it doesn't really match what clients need, at least what this particular client needs. The point is to check the projections, so the iterator really should yield as often as there are projections. And given that it also seemed odd to not make the projection itself directly available.

In a follow-up PR I hope to port more clients to this API, I guess then we will see how generally useful it is.

@bors r+

📌 Commit 0bb82c4 has been approved by nikomatsakis

⌛ Testing commit 0bb82c4 with merge 99baddb...

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 99baddb to master...

Possibly relnotes needed?