Derive Eq, Ord, Hash for MaybeDangling

[Jules-Bertholet]

Fixes #154890

Alternative to #154891

Tracking issue: #118166

[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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @scottmcm, libs
• @scottmcm, libs expanded to 8 candidates
• Random selection from Mark-Simulacrum, scottmcm

[WaffleLapkin]

MaybeDangling deliberately doesn't implement Eq/Ord/Hash, as to not make it easy to accidentally use a dangling reference.

[Jules-Bertholet]

Then why does it implement Debug and Clone? Those implicitly take a reference too

[WaffleLapkin]

Clone: because it needs to implement Copy. Debug: I don't know, might have been a mistake.

[theemathas]

Could we do impl<T: Copy> Clone for MaybeDangling<T> ? MaybeUninit does that.

[Jules-Bertholet]

Clone: because it needs to implement Copy.

Then there should be a manual impl<T: Copy> Clone for MaybeDangling<T>; it should not be derived.

[Jules-Bertholet]

Anyway, did T-lang or T-libs-api ever discuss or agree to the "not make it easy to accidentally use a dangling reference" rationale?

[Jules-Bertholet]

The RFC just says:

MaybeDangling<P> propagates auto traits, drops the P when it is dropped, and has (at least) derive(Copy, Clone, Debug).

So it seems this was left as an open question.

(Edit) See also the lang FCP comment:

rust-lang/rfcs#3336 (comment)

There's still some uncertainty about exactly what the form this should take for stabilization, but that that didn't need to block the lang FCP, since we want to have this available to solve some problems in the library implementation and can figure out API details in nightly.

Given that it sounds like this type can allow Deref soundly, this might be fine as-is, as code using it might often be minimally-impacted. But we can also explore attributes or something later if needed.

[WaffleLapkin]

I've added an unresolved question to the tracking issue.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Mark-Simulacrum]

Is the PR description outdated? Both the fixes issue and alternative PRs are closed/merged, but presumably you still want this?

r=me with that fixed up

[WaffleLapkin]

I think this is blocked on T-libs-api (or maaaybe T-lang) decision about which traits should be implemented for MaybeDangling. I personally think that it should implement as little as possible, to prevent footguns similarly to the ones ManuallyDrop has. But given said ManuallyDrop prior art I guess it is up for debate.

[Mark-Simulacrum]

I don't have a strong opinion either way - it feels a bit dubious to skip the implementations when we have safe method accessors. If we had deref it would be even weirder IMO, but we don't have that at least.

I think in general we don't have a good way of surfacing "safe but be careful" in the language today, and I'm not sure what that would look like either :)

It seems relatively unlikely we'll get significant usage reports either way - too easy to workaround not having the impls, and hard to check if people are actually making use of the pause of not having them to reason through if what they're doing is a good idea.

[Jules-Bertholet]

FWIW, I don't have a strong opinion either. My assumption was that, in the absence of strong motivation to the contrary, the precedent of ManuallyDrop should apply. But if libs-API says they want to do it differently, I won't object