Add From impls for wrapper types

[Jules-Bertholet]

• From<T: Copy> for ManuallyDrop<T>
• From<T: UnwindSafe> for AssertUnwindSafe<T>
• From<T> for LazyCell<T, F>
• From<T> for LazyLock<T, F>
• From<T> for ThinBox<T> (unstable)
• From<T> for UniqueRc<T> (unstable)
• From<T> for UniqueArc<T> (unstable)

@rustbot label T-libs-api needs-fcp

Also needs a crater run, as the insta-stable impls are technically breaking changes.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Kivooeo]

@rustbot label +needs-crater

[rustbot]

Error: Label needs-crater can only be set by Rust team members

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

[tgross35]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 804fff1 (804fff1ae95da9d03bd44eee27f258020a575dab, parent: fe55364329579d361b1ab565728bc033a7dba07e)

[tgross35]

@craterbot check

[craterbot]

👌 Experiment pr-146013 created and queued.
🤖 Automatically detected try build 804fff1
🔍 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-146013 is now running

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

[craterbot]

🎉 Experiment pr-146013 is completed!
📊 1312 regressed and 1466 fixed (691117 total)
📊 2007 spurious results on the retry-regessed-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-146013/retry-regressed-list.txt ↩

[Jules-Bertholet]

I haven't been able to review all the regressions yet. But from the few I spot-checked: most are spurious (filesystem errors), but a few are not. All the real regressions I saw were due to the ManuallyDrop impl. And, upon reflection, that one as well as the AssertUnwindSafe impl seem dubious, so I removed the former and restricted the latter.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 69cf481 (69cf48176da7795067be67cef76e41a8aac9b134, parent: 23718020b12d7e5a54f82a19910e8356ee719667)

[Jules-Bertholet]

Alright… now we can retry crater

[tgross35]

@craterbot check

[craterbot]

👌 Experiment pr-146013-1 created and queued.
🤖 Automatically detected try build 69cf481
🔍 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

[tgross35]

We probably only need to regressions from the first run, but I'm not sure if it's the same retry-regressed list as above. @Mark-Simulacrum if you get the chance could you update the job?

[Urgau]

@craterbot name=pr-146013-1 mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-146013/retry-regressed-list.txt

[craterbot]

📝 Configuration of the pr-146013-1 experiment changed.

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

[craterbot]

🚧 Experiment pr-146013-1 is now running

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

[craterbot]

🎉 Experiment pr-146013-1 is completed!
📊 0 regressed and 0 fixed (2679 total)
📊 169 spurious results on the retry-regessed-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-146013-1/retry-regressed-list.txt ↩

[Jules-Bertholet]

No regressions 🎉

[the8472]

There's has been no issue discussing this, no ACP. Can you add some motivation why these are needed, why those trait bounds are on there and so on?

[Jules-Bertholet]

These are for consistency with the existing From<T> impls for Box<T>, Rc<T>, Arc<T>, Option<T>, Poll<T>, Cell<T>, UnsafeCell<T>, SyncUnsafeCell<T>, OnceCell<T>, RefCell<T>, UnsafePinned<T>, Exclusive<T>, Mutex<T>, OnceLock<T>, ReentrantLock<T>, and RwLock<T>. Also, they are useful for convenience: value.into() is shorter than WrapperType::new(value).

The Copy bound on the ManuallyDrop impl is to ensure users can’t accidentally leak data with an into(), and to ensure the conversion is “value-preserving” as From’s docs require (Copy implies a trivial destructor). The UnwindSafe bound on the AssertUnwindSafe impl has an analogous motivation.

[tgross35]

The Copy bound on the ManuallyDrop impl is to ensure users can’t accidentally leak data with an into(), and to ensure the conversion is “value-preserving” as From’s docs require (Copy implies a trivial destructor). The UnwindSafe bound on the AssertUnwindSafe impl has an analogous motivation.

This makes things a bit unconvincing for the ManuallyDrop impl imo. How often is ManuallyDrop going to come into play for types that are known to be Copy? Subtly constructing a ManuallyDrop via .into() doesn't seem like it would be the best coding practice anyway, Copy or not.

In any case, the bounds here would be good to cover in a doc comment to remove the guesswork.

[Jules-Bertholet]

How often is ManuallyDrop going to come into play for types that are known to be Copy?

Probably not very. But it could come up when constructing a type like Foo below, with a T that happens to be Copy:

union Foo<T> {
    // `ManuallyDrop` needed because it’s a union and `T` has no `Copy` bound
    field: ManuallyDrop<T>,

    // ...
}

Subtly constructing a ManuallyDrop via .into() doesn't seem like it would be the best coding practice anyway, Copy or not.

I don’t see it as any worse than all the other wrapper type From impls I listed?