Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stabilise entry_insert #90345

Merged
merged 5 commits into from
Dec 21, 2021
Merged

Stabilise entry_insert #90345

merged 5 commits into from
Dec 21, 2021

Conversation

passcod
Copy link
Contributor

@passcod passcod commented Oct 27, 2021

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
Copy link
Collaborator

r? @kennytm

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 27, 2021
@kennytm
Copy link
Member

kennytm commented Oct 27, 2021

r? @joshtriplett

@kennytm kennytm added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 27, 2021
@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Oct 30, 2021

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

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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 30, 2021
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Dec 10, 2021
@rfcbot
Copy link

rfcbot commented Dec 10, 2021

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Dec 10, 2021
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 20, 2021
@rfcbot
Copy link

rfcbot commented Dec 20, 2021

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.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Dec 20, 2021
@dtolnay
Copy link
Member

dtolnay commented Dec 20, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Dec 20, 2021

📌 Commit a2fd84a has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2021
@bors
Copy link
Contributor

bors commented Dec 20, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 20, 2021
@matthiaskrgr
Copy link
Member

@bors retry docker.io 503

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2021
@rust-log-analyzer
Copy link
Collaborator

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.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2021
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#90345 (Stabilise entry_insert)
 - rust-lang#91412 (Improve suggestions for importing out-of-scope traits reexported as `_`)
 - rust-lang#91770 (Suggest adding a `#[cfg(test)]` to to a test module)
 - rust-lang#91823 (Make `PTR::as_ref` and similar methods `const`.)
 - rust-lang#92127 (Move duplicates removal when generating results instead of when displaying them)
 - rust-lang#92129 (JoinHandle docs: add missing 'the')
 - rust-lang#92131 (Sync rustc_codegen_cranelift)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3009dd7 into rust-lang:master Dec 21, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 21, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 23, 2021
@qm3ster
Copy link
Contributor

qm3ster commented Feb 17, 2022

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 Amanieu added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. and removed I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Feb 17, 2022
@Amanieu
Copy link
Member

Amanieu commented Feb 17, 2022

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

@passcod passcod deleted the entry-insert branch February 17, 2022 12:53
@eaglgenes101
Copy link

eaglgenes101 commented Feb 17, 2022

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
Copy link
Member

Amanieu commented Feb 17, 2022

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
Copy link
Member

cuviper commented Feb 17, 2022

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

@qm3ster
Copy link
Contributor

qm3ster commented Feb 17, 2022

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
Copy link
Member

m-ou-se commented Feb 17, 2022

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
Copy link
Member

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
Copy link
Member

cuviper commented Feb 17, 2022

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

@yaahc
Copy link
Member

yaahc commented Feb 17, 2022

Has anyone started working on the revert?

@5225225
Copy link
Contributor

5225225 commented Feb 17, 2022

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).

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 18, 2022
…=Mark-Simulacrum

Destabilise entry_insert

See: rust-lang#90345

I didn't revert the rename that was done in that PR, I left it as `entry_insert`.

Additionally, before that PR, `VacantEntry::insert_entry` seemingly had no stability attribute on it? I kept the attribute, just made it an unstable one, same as the one on `Entry`.

There didn't seem to be any mention of this in the RELEASES.md, so I don't think there's anything for me to do other than this?
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2022
…ark-Simulacrum

Destabilise entry_insert

See: rust-lang#90345

I didn't revert the rename that was done in that PR, I left it as `entry_insert`.

Additionally, before that PR, `VacantEntry::insert_entry` seemingly had no stability attribute on it? I kept the attribute, just made it an unstable one, same as the one on `Entry`.

There didn't seem to be any mention of this in the RELEASES.md, so I don't think there's anything for me to do other than this?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.