Do not automatically DerefMut ManuallyDrop through union references

[eggyal]

View all comments

Extends #75584 to situations where the union itself is accessed through a reference.

Fixes #141621
r? compiler

[eggyal]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[eggyal]

Hm. Well, that build failure isn't wrong... the smallvec crate is indeed accessing the contents of a ManuallyDrop field of a union through an implied DerefMut... but clearly this change would cause a lot of breakage. I suppose this should be a (suppressible) lint, rather than a hard error?

[eggyal]

That said, reading the motivating Potential pitfalls around DerefMut section of RFC 2514 leads to me to believe the intention is that this should be a hard error, albeit now a breaking change to stable?

Not sure who needs to be brought in on resolving this? @RalfJung (as author of the RFC and current implementation), can you point me the right way or ping the relevant team?

[RalfJung]

I lost all context for this.^^

Can you summarize what the problem is?

[RalfJung]

I'm not sure I follow -- the assignment is unsafe either way precisely because the destructor gets called.

Can you explain what smallvec does and why it triggers your version of the lint?

[RalfJung]

I would say the general goal is to avoid accidentally calling drop glue without the user intending to do so. I don't think being inside an unsafe block is relevant for that. All these examples require unsafe, both the cases that already work as intended and the ones that do not.

I will nominate the issue for t-lang discussion to get their take on this.

[RalfJung]

It would be good to get an idea of the overall fallout of this via crater. Not sure what the best way to do that is when a rustc dependency fails to build.

• We could skip the union check if the crate is called "smallvec" -- a complete hack obviously but for a crater run this seems fine.
• You could try forking smallvec, fixing the problem there, and adding a [patch] section to use your fork when building rustc.
• Any other ideas?

[RalfJung]

Another idea might be to extend the BikeshedGuaranteedNoDrop trait to also be implemented for MaybeUninit, and to observe that trait when checking whether we reject a field projection. That trait is magic so you'd have to adjust its builtin impls in the compiler directly.

[eggyal]

Thanks for the feedback, @RalfJung. I've added a comment and further tests as suggested.

I don't think being inside an unsafe block is relevant for that. All these examples require unsafe, both the cases that already work as intended and the ones that do not.

Agh, of course you're right. I got myself in a twist from the starting point of it being safe to assign directly to a union variant (because they are never Drop), and clearly from there I just caused myself and you an awful lot of unnecessary confusion. My apologies.

Any other ideas?

Perhaps there is (or could be) a flag or environment variable that we'd use when building rustc; then disable this check if its presence is detected at runtime? Also quite hacky, but maybe acceptable to get a crater run going.

I'm going to explore the BikeshedGuaranteedNoDrop idea.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[eggyal]

Okay! I think that's now doing what we want: auto DerefMut through a union field is only permitted to BikeshedGuaranteedNoDrop targets (irrespective of whether the union itself is accessed through a reference); explicit * is still allowed.

Shall we see what crater impact there is?

[saethlin]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 54cf65b (54cf65b8d066c082e536455049b95943cf30378f, parent: f8463896a9b36a04899c013bd8825a7fd29dd7a4)

[saethlin]

@craterbot check

[craterbot]

👌 Experiment pr-151920 created and queued.
🤖 Automatically detected try build 54cf65b
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-151920 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-151920 is completed!
📊 515 regressed and 4 fixed (815512 total)
📊 2231 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-151920/retry-regressed-list.txt ↩

[eggyal]

The currently proposed error is surely too pessimistic. For example, take the failures in the divan crate—which are something like this:

union EitherVec<A, B> {
    vec_of_a: ManuallyDrop<Vec<A>>,
    vec_of_b: ManuallyDrop<Vec<B>>,
}

impl<A, B> EitherVec<A, B> {
    /// `Self` is either always a `Vec<A>` or always a `Vec<B>`.  This indicates which.
    const VARIANT_A: bool = is_variant_a();

    pub fn bikeshed_mutate(&mut self) {
        unsafe {
            if Self::VARIANT_A {
               self.vec_of_a.bikeshed_mutate(); //~ ERROR not automatically applying `DerefMut`
            } else {
               self.vec_of_b.bikeshed_mutate(); //~ ERROR not automatically applying `DerefMut`
            }
        }
    }
}

Even if bikeshed_mutate were to cause the drop glue to be invoked...

impl<T> Vec<T> {
    fn bikeshed_mutate(&mut self) {
        *self = vec![];
    }
}

...why is that a problem? What exactly is the footgun that we're trying to prevent here? 🤔

I would say the general goal is to avoid accidentally calling drop glue without the user intending to do so.

I guess this needs refining. When is the calling of drop glue "accidental" / when does the user "intend to do so"? I think the footgun most like arises when writing an assignment expression where resolving the assignee place entails a deref coercion through a union field to a target that is not BikeshedGuaranteedNoDrop. Does that sound right?

EDIT: What about Vec::bikeshed_mutate(unsafe { &mut self.vec_of_a })? At that call site, the union field is being deref-coerced through the ManuallyDrop. I suppose that's a footgun too, in a way that the method-call version was not (as much)?

[petrochenkov]

r? @RalfJung

[rustbot]

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

[RalfJung]

I think the footgun most like arises when writing an assignment expression where resolving the assignee place entails a deref coercion through a union field to a target that is not BikeshedGuaranteedNoDrop. Does that sound right?

Yes... but of course there could be such an assignment inside bikeshed_mutate, as in your example.

Thanks for doing the crater run! That should give us enough data to nominate the issue for t-lang discussion.

[RalfJung]

Previously this checked source, now it checks adjustment.target... I don't know anything about this code so I can't say what the consequences of that are.

[eggyal]

Previously it was an error if source was a ManuallyDrop, irrespective of the type it contains; now it's an error if the expression dereferences to a target that doesn't implement BikeshedGuaranteedNoDrop - so ManuallyDrop<MaybeUninit<T>> is fine, because the target of dereferencing the ManuallyDrop is guaranteed not to have drop glue; but ManuallyDrop<Vec<T>> errors because Vec<T> obviously doesn't have that guarantee.

[RalfJung]

I have also found the code that I had in mind when I said we might be relying on the union drop check things somewhere:

rust/compiler/rustc_mir_build/src/check_unsafety.rs

Lines 643 to 650 in 9cc2924

	if assigned_ty.needs_drop(self.tcx, self.typing_env) {
	// This would be unsafe, but should be outright impossible since we
	// reject such unions.
	assert!(
	self.tcx.dcx().has_errors().is_some(),
	"union fields that need dropping should be impossible: {assigned_ty}"
	);
	}

So, this is a different check from what we are discussing here, and also it is guarded by an assertion. So I think we are indeed free to relax this check in any way we see fit.

[rust-bors]

☔ The latest upstream changes (presumably #153217) made this pull request unmergeable. Please resolve the merge conflicts.