Stabilise entry_insert

[passcod]

This stabilises HashMap:Entry::insert_entry etc. Tracking issue #65225. It will need an FCP.

This was implemented in #64656 two years ago.

This PR includes the rename and change discussed in #65225 (comment), happy to split if needed.

[rust-highfive]

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

[kennytm]

r? @joshtriplett

[joshtriplett]

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[dtolnay]

@bors r+

[bors]

📌 Commit a2fd84a has been approved by dtolnay

[bors]

💔 Test failed - checks-actions

[matthiaskrgr]

@bors retry docker.io 503

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Attempting with retry: docker build --rm -t rust-ci -f /gha/_work/rust/rust/src/ci/docker/host-aarch64/aarch64-gnu/Dockerfile /gha/_work/rust/rust/src/ci/docker
Sending build context to Docker daemon  506.4kB

Step 1/6 : FROM ubuntu:20.04
Head https://registry-1.docker.io/v2/library/ubuntu/manifests/20.04: received unexpected HTTP status: 503 Service Unavailable
Sending build context to Docker daemon  506.4kB

Step 1/6 : FROM ubuntu:20.04
Step 1/6 : FROM ubuntu:20.04
Head https://registry-1.docker.io/v2/library/ubuntu/manifests/20.04: received unexpected HTTP status: 503 Service Unavailable
Sending build context to Docker daemon  506.4kB

Step 1/6 : FROM ubuntu:20.04
Step 1/6 : FROM ubuntu:20.04
Head https://registry-1.docker.io/v2/library/ubuntu/manifests/20.04: received unexpected HTTP status: 503 Service Unavailable
Sending build context to Docker daemon  506.4kB

Step 1/6 : FROM ubuntu:20.04
Step 1/6 : FROM ubuntu:20.04
Head https://registry-1.docker.io/v2/library/ubuntu/manifests/20.04: received unexpected HTTP status: 503 Service Unavailable
Sending build context to Docker daemon  506.4kB

Step 1/6 : FROM ubuntu:20.04
Step 1/6 : FROM ubuntu:20.04
Head https://registry-1.docker.io/v2/library/ubuntu/manifests/20.04: received unexpected HTTP status: 503 Service Unavailable
##[error]Process completed with exit code 1.
Post job cleanup.

[qm3ster]

I object, because this locks OccupiedEntry.key to be an Option<K>.
This is unnecessary, as until this feature all of the methods unwrapping it are assumed to be infallible, and will now start panicking, which is technically somewhat a breaking change.
This is also incompatible with #44286.
OccupiedEntry.key should become just K, and instead of putting an OccupiedEntry into a dysfunctional state, a new type should be introduced.
The new type, which is only a view into the map and doesn't hold an owned key, can then be used as an implementation detail of OccupiedEntry.
That type might also happen to wrap RawOccupiedEntryMut 🤔 (I am probably grossly misunderstanding this part and OccupiedEntry is smaller/faster than RawOccupiedEntryMut somehow).

This way, anything that requires a unified type meaning "mutable entry into the map for a particular key" as opposed to a plain &mut V can still have it, both from pre-occupied entries and from insert_entry.

[Amanieu]

Unfortunately I think that it's too late to revert the stabilization at this point: the stable release is coming out next week.

[eaglgenes101]

I'll second not stabilizing this method, as the documentation makes no mention either on insert_entryor the key-getting methods that the key getting methods might panic if insert_entry is used. This is an important detail that users should know before using insert_entry, and the possibility of panic is usually prominently indicated in other std APIs.

I do have other objections, but this is the most immediate and pertinent one.

As it turns out, this panic can only occur with the other unstable feature map_entry_replace. My bad. I still object on the grounds that I would much prefer the API that this API clashes horribly with.

[Amanieu]

cc @rust-lang/libs-api There seem to be several last-minute objections to stabilizing this feature (which is due to be released in stable next week). Thoughts?

[cuviper]

It's not too late to revert yet, but that decision should be made ASAP.

[qm3ster]

Since unlike hashbrown, std doesn't have Entry::insert(v) -> OccupiedEntry, this can all be resolved very happily in a backward-compatible way.
At no point am I saying we should have #44286 instead of this.
We can ergonomically have all of the semantics of both.

However, not reverting this now will permanently import this accidental complexity from hashbrown. We will never get rid of key: Option. We will never have statically-distinguished keyful and keyless entries. (And we will never have infallible #44286)

The current situation is a consequence of not enough communication between the features, not a fundamental/hard problem.

[m-ou-se]

These look like valid concerns to me. Reverting stabilization right now is a bit late, but not too late. If possible, I'd prefer delaying/reverting stabilizaiton for now, so we have time to discuss these concerns.

[Mark-Simulacrum]

If we would like to revert stabilization, please file a PR targeting master and beta nominate+accept it; it can get picked up in the last round we typically do.

[cuviper]

Just as a reminder, we'll need to remove it from the release notes and blog post as well.

[yaahc]

Has anyone started working on the revert?

[5225225]

i'll do it. currently AFK but I can get a PR up in 30ish minutes.

that would just be a revert of this PR, plus removing it from any release notes that may be stored in this repo? (not sure where they are).