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

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

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

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

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

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.

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?

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.

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

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.

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

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.

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.

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

r? libs-api

@rustbot label S-waiting-on-t-libs-api -S-waiting-on-author

Error: Unknown labels: S-waiting-on-libs-api

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

We discussed this in today's libs-api meeting, and felt like a bit more context would be useful - @WaffleLapkin what particular footguns are possible here?

As a whole, equivalency with ManuallyDrop alone probably isn't a reason to add this unless there is some other need for it

Note the additional motivation:

having PartialEq allows structures containing a MaybeDangling field to derive StructuralPartialEq.

ManuallyDrop works around this with a manual StructuralPartialEq, but ecosystem libraries can't do that.

That's fair, but probably a question for when we get closer to stabilisation of MaybeDangling. Mainly depends on what exactly the mentioned safety concerns are

So the concern that I had is in relation to ManuallyDrop and its (in my opinion) footgun-y design.

Specifically it's easy to get into a situation where you have a value which blows up if you touch it slightly:

let mut x = ManualyDrop::new(...);
unsafe { ManuallyDrop::drop(&mut x) };

// somewhere else
x.method() // UB

I think @danielhenrymantilla put it well on the community server, paraphrasing:

There are two uses of ManuallyDrop which have conflicting needs.

1. Controlling drop order of fields (but the value is always present until the whole structure is dropped)
2. Making data structures which need control over drop

The current design of ManuallyDrop is nice for the 1-st case, because it allows you not to think about ManuallyDrop too much, and the Drop impl is small enough that it's easy to avoid accidental access.

For the 2-nd case the current design is much worse, since "does ManuallyDrop contain a valid value to access" is a non-trivial question (depending on the data structure in question) and avoiding accidental access throughout the code is much harder.

I wish we either had 2 versions of ManuallyDrop, to handle both use-cases, or only had a version without implicit access to the inner value.

The same kind of situation can happen with MaybeDangling, where you make the inner value dangle and then accidentally access it.

All that being said we can't avoid accidental access to the value in MaybeDangling. For one, it has a drop glue which would access the inner value, but we also can't remove the Clone/Copy impls (ManuallyDrop needs to be Copy).

So in the end, I'm not sure if it's worth it trying to restrict MaybeDangling.

We discussed this during today's libs-api meeting. Opinions were mixed, and we were unsure about how people will be using MaybeDangling itself (rather than as part of ManuallyDrop).
Since actual use will inform how footgunny having these traits would be, it seems better to first stabilize the type itself and see how it gets used, if there's actual demand to have them and so on.

So we're deferring this, we can reconsider it later.

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

We didn't discuss this, but personally I think this might be something that could be changed to be consistent with the wait-and-see we're proposing here.