Fix dropck issue of SyncOnceCell.

[m-ou-se]

Fixes #76367.

[rust-highfive]

r? @withoutboats

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

[matklad]

Wow, this is super-nice find, thanks @m-ou-se!

Can we add compile-fail tests here (maybe just onto the PhantomData field)?

https://github.com/matklad/once_cell/blob/0b65b1523453a0cfbdeeb756a77e0b85e03ea7bf/src/lib.rs#L980-L994

[matklad]

And I guess the dropcheck test still passes, so that's good:

rust/library/std/src/lazy/tests.rs

Lines 316 to 323 in 578e714

	#[test]
	fn dropck() {
	let cell = SyncOnceCell::new();
	{
	let s = String::new();
	cell.set(&s).unwrap();
	}
	}

.

r? @KodrAus

This looks good awesome to me, but I will be traveling, so won't be able to r+

[matklad]

@m-ou-se out of curiosity, how did you found out about this issue?

[matklad]

Also, I guess cc @RalfJung :) I find it surprising that UnsafeCell<MaybeUninit<T>> and PhantomData<T> differe in their effects on the drop check. Which of UnsafeCell / MaybeUninit / ManuallyDrop is responsible for that?

[m-ou-se]

@m-ou-se out of curiosity, how did you found out about this issue?

Uh, I'm not sure. ^^' I have a lot of tabs open in my browser with interesting PRs I want to take a closer look at at some point, and #75648 was one of those. I don't remember why I had this one open. I guess I wanted to remind myself to take a closer look at how #[may_dangle] works? Came across it today while cleaning up my tabs, and played around with things a bit until it broke.

[m-ou-se]

Also, I guess cc @RalfJung :) I find it surprising that UnsafeCell<MaybeUninit<T>> and PhantomData<T> differe in their effects on the drop check. Which of UnsafeCell / MaybeUninit / ManuallyDrop is responsible for that?

I think it's working as intended. It's perfectly safe to drop a MaybeUninit<T> with a dangling T, since it doesn't drop the T when dropped. Hard to discover problems like this though. Maybe the documentation of #[may_dangle] could use an extra warning about this situation.

[m-ou-se]

Can we add compile-fail tests here (maybe just onto the PhantomData field)?

Sounds like a good idea. What's the best way of doing compile_fail tests in std? As a doctest on some private item, or does std have another preferred way of doing this?

[RalfJung]

Also, I guess cc @RalfJung :) I find it surprising that UnsafeCell<MaybeUninit<T>> and PhantomData<T> differe in their effects on the drop check. Which of UnsafeCell / MaybeUninit / ManuallyDrop is responsible for that?

I guess it's ManuallyDrop that has the effect here. dropck is about the code that runs on drop, and with ManuallyDrop there is no such code, so this makes sense (as @m-ou-se said). If a manual drop anyway drops some T, a PhantomData is required.

[m-ou-se]

I've added a compile_fail test in the doc comment on the _marker: PhantomData.

[nagisa]

@bors r+

[bors]

📌 Commit e56ea68 has been approved by nagisa

[KodrAus]

Nice find! I’ll update our forge docs on #[may_dangle] to include this as something to watch out for.

[bors]

⌛ Testing commit e56ea68 with merge 23e49dd...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: nagisa
Pushing 23e49dd to master...