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

Stabilize Bufwriter::into_raw_parts #84770

Closed
wants to merge 2 commits into from

Conversation

ijackson
Copy link
Contributor

@ijackson ijackson commented May 1, 2021

Update 2021-08-24

The FCP for this has already completed in #80690. This was just blocked on #85901 (which changed the name), which is now merged.

Update 2021-07-24

The methods on IntoInnerError were stabilised in #87175 (thanks, @inquisitivecrystal).

Stabilisation of BufWriter::into[_raw]_parts is blocked on #85901 which fixes some missing exports and changes the name. That MR is awaiting review.

Background and discussion

These two sets of methods both relate to disassembling a BufWriter. It is most convenient to consider them together:

Without at least one of these, if the underlying writer is failing, it is not possible to get all of the innards out in a useable form for error recovery, no matter how much you know about the underlying writer and what went wrong.

Strictly speaking, BufWriter::into_raw_parts is sufficient. That avoids calling BufWriter::into_inner and therefore an IntoInnerError cannot arise. It is possibly also simpler in some cases where the caller wants to take care about errors. But into_raw_parts is clumsy in simpler situations (where what's wanted is a call that does the flush before disassembly and where errors will be considered to invalidate the underlying writer).

BufWriter::into_raw_parts was merged on 2021-01-19, and the IntoInnerError disassembly methods on 2020-12-05.

Alternatives

  • Stabilise only into_raw_parts. This would be sufficient, although it does leave into_inner as something of a decoy - if into_inner is used, but later a need to do error recovery arises, the maintenance programmers will have to replace into_inner with into_raw_parts, so as to avoid ending up with an intractable IntoInnerError, and reorganise the deconstruction code. Some of the pain here could be mitigated through documentation, which could explain that that this is what is needed. But it still seems rather perverse: it would be deliberately avoiding providing into_* deconstruction methods on a type which could easily have them.
  • Stabilise into_raw_parts and deprecate into_inner, IntoInnerError, and never stabilise the new disassembly methods on IntoInnerError. This seems unfriendly as it makes this complicated in simple situations.
  • Stabilise neither of these facilities. This means that when error recovery is needed, the programmer would need to copy BufWriter's source into their project to add it. This (the current situation on stable) is quite poor.

Open questions

None.

Resolved questions

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
@rust-highfive
Copy link
Collaborator

r? @dtolnay

(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 May 1, 2021
@ijackson
Copy link
Contributor Author

ijackson commented May 1, 2021

@rustbot modify labels +T-libs +needs-fcp

@rustbot
Copy link
Collaborator

rustbot commented May 1, 2021

Error: Label needs-fcp can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

@ijackson
Copy link
Contributor Author

ijackson commented May 1, 2021

@rustbot modify labels +T-libs

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 1, 2021
@jonas-schievink jonas-schievink added the relnotes Marks issues that should be documented in the release notes of the next release. label May 1, 2021
@JohnTitor JohnTitor added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label May 3, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2021
@dtolnay dtolnay added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2021
@JohnTitor
Copy link
Member

Triage note: FCPs are open on these issues: #80690 (comment), #79704 (comment)

@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 25, 2021
@m-ou-se m-ou-se added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 7, 2021
@bors
Copy link
Contributor

bors commented Jul 23, 2021

☔ The latest upstream changes (presumably #87413) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 23, 2021
@ijackson ijackson changed the title Stabilize Bufwriter::into_raw_parts and IntoInnerError::into_raw_parts, ::into_error Stabilize Bufwriter::into_raw_parts <strike>and IntoInnerError::into_raw_parts, ::into_error</strike> Jul 26, 2021
@ijackson ijackson changed the title Stabilize Bufwriter::into_raw_parts <strike>and IntoInnerError::into_raw_parts, ::into_error</strike> Stabilize Bufwriter::into_raw_parts Jul 26, 2021
@ijackson
Copy link
Contributor Author

This is (still) blocked on #85901. The merge conflict is due to #87175 landing (yay), and I will fix it after #85901.

@JohnTitor JohnTitor added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 26, 2021
@ijackson ijackson closed this Aug 24, 2021
@ijackson ijackson deleted the bufwriter branch August 24, 2021 17:05
@ijackson ijackson restored the bufwriter branch August 24, 2021 17:10
@ijackson
Copy link
Contributor Author

Uh. I did not mean to close this. I meant to update it. Here we are, rebased onto master.

LeSeulArtichaut added a commit to LeSeulArtichaut/rust that referenced this pull request Aug 25, 2021
Stabilise BufWriter::into_parts

The FCP for this has already completed, in rust-lang#80690.

This was just blocked on rust-lang#85901 (which changed the name), which is now merged.  The original stabilisation MR was rust-lang#84770 but that has a lot of noise in it, and I also accidentally deleted the branch while trying to tidy up.  So here is a new MR.  Sorry for the noise.

Closes rust-lang#80690
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. 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.

9 participants