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

Guarantees of content preservation on try_reserve failure? #99606

Closed
lo48576 opened this issue Jul 22, 2022 · 7 comments · Fixed by #100331
Closed

Guarantees of content preservation on try_reserve failure? #99606

lo48576 opened this issue Jul 22, 2022 · 7 comments · Fixed by #100331
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@lo48576
Copy link
Contributor

lo48576 commented Jul 22, 2022

Location

Any try_reserve methods of container types (12 in Rust 1,62.1, including duplicated ones in alloc), and maybe corresponding try_reserve_exact methods.

  • std::collections::BinaryHeap::try_reserve[_exact]
  • std::collections::VecDeque::try_reserve[_exact]
  • std::collections::HashMap::try_reserve
  • std::collections::HashSet::try_reserve
  • std::ffi::OsString::try_reserve[_exact]
  • std::path::PathBuf::try_reserve[_exact]
  • std::string::String::try_reserve[_exact]
  • std::vec::Vec::try_reserve[_exact]
  • ... and corresponding methods in alloc.

Summary

Documentations for try_reserve describes "If the capacity overflows, or the allocator reports a failure, then an error is returned.".
However, it does not give any guarantees about whether the content is preserved or not on that error.
This guarantee (or absence of the guarantee) should be explicitly described.

@lo48576 lo48576 added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Jul 22, 2022
@scottmcm scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Jul 22, 2022
@scottmcm
Copy link
Member

At least for Vec (and Vec-based things like String) it's obvious to guarantee that, so we might as well. (Thank you, Rust, for insisting on move-is-memcpy so we don't have the "what if the move constructor panics" problems like C++.)

I think it'll need to be added to hashbrown before we can consider promising it for HashMap/HashSet, through -- there's no guarantee in https://rust-lang.github.io/hashbrown/hashbrown/hash_map/struct.HashMap.html#method.try_reserve.

@joshtriplett
Copy link
Member

Modulo needing to add it in hashbrown, shall we see if we have consensus to say that try_reserve failures preserve the current contents?

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jul 27, 2022

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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jul 27, 2022
@rfcbot
Copy link

rfcbot commented Jul 27, 2022

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

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Aug 6, 2022
@rfcbot
Copy link

rfcbot commented Aug 6, 2022

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 to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 6, 2022
@m-ou-se m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Aug 9, 2022
@joshtriplett
Copy link
Member

Please feel free to post a PR for this.

@lo48576
Copy link
Contributor Author

lo48576 commented Aug 9, 2022

Created a PR #100331 for this.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Aug 22, 2022
…ilure, r=thomcc

Guarantee `try_reserve` preserves the contents on error

Update doc comments to make the guarantee explicit. However, some
implementations does not have the statement though.

* `HashMap`, `HashSet`: require guarantees on hashbrown side.
* `PathBuf`: simply redirecting to `OsString`.

Fixes rust-lang#99606.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Aug 22, 2022
…ilure, r=thomcc

Guarantee `try_reserve` preserves the contents on error

Update doc comments to make the guarantee explicit. However, some
implementations does not have the statement though.

* `HashMap`, `HashSet`: require guarantees on hashbrown side.
* `PathBuf`: simply redirecting to `OsString`.

Fixes rust-lang#99606.
@bors bors closed this as completed in 2bb7e1e Aug 22, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants