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

Tracking Issue for BufWriter::into_parts #80690

Closed
2 of 3 tasks
ijackson opened this issue Jan 4, 2021 · 13 comments · Fixed by #88299
Closed
2 of 3 tasks

Tracking Issue for BufWriter::into_parts #80690

ijackson opened this issue Jan 4, 2021 · 13 comments · Fixed by #88299
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ijackson
Copy link
Contributor

ijackson commented Jan 4, 2021

Feature gate: #![feature(bufwriter_into_parts)]

This is a tracking issue for BufWriter::into_parts and its associated error type etc.

This allows a BufWriter to be disassembled, and the inner writer to be recovered - without attempting to write out any buffered data (instead it is returnedt to the caller), and therefore (unlike into_inner) succeeding even if the underlying writer is returning errors.

Public API

// std::io

/// Disassembles this `BufWriter<W>`, returning the underlying writer, and any buffered but
/// unwritten data.
impl<W> BufWriter<W> {
    pub fn into_parts(mut self) -> (W, Result<Vec<u8>, WriterPanicked>);
}

/// Error newtype, wraps the buffered data, a `Vec<u8>`
pub struct WriterPanicked {...}

impl WriterPanicked {
    pub fn into_inner(self) -> Vec<u8>;
}

impl Error for WriterPanicked {...}
impl Display for WriterPanicked {...}
impl Debug for WriterPanicked {...}

Steps / History

Unresolved Questions

@ijackson ijackson added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 4, 2021
@m-ou-se m-ou-se added the Libs-Tracked Libs issues that are tracked on the team's project board. label Jan 6, 2021
@dtolnay
Copy link
Member

dtolnay commented May 23, 2021

@rust-lang/libs
@rfcbot fcp merge

Check out @ijackson's recap of this API in #84770 (comment).

@rfcbot
Copy link

rfcbot commented May 23, 2021

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

Concerns:

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 May 23, 2021
@BurntSushi
Copy link
Member

Should BufWriter::into_raw_parts be called into_parts instead? In all previous uses of "raw" in similar names, I believe a raw pointer is involved somewhere. But that isn't the case here.

Also, it looks like the WriterPanicked error might not actually be exported? At least, rustdoc doesn't think so: https://doc.rust-lang.org/std/io/struct.BufWriter.html#method.into_raw_parts

@ijackson
Copy link
Contributor Author

Should BufWriter::into_raw_parts be called into_parts instead? In all previous uses of "raw" in similar names, I believe a raw pointer is involved somewhere. But that isn't the case here.

I don't have an opinion about this. I just chose that name to distinguish it more from into_inner. I would be quite happy to change it to into_parts.

Also, it looks like the WriterPanicked error might not actually be exported? At least, rustdoc doesn't think so: https://doc.rust-lang.org/std/io/struct.BufWriter.html#method.into_raw_parts

I will check this. Thanks!

@yaahc
Copy link
Member

yaahc commented Jun 3, 2021

Should BufWriter::into_raw_parts be called into_parts instead? In all previous uses of "raw" in similar names, I believe a raw pointer is involved somewhere. But that isn't the case here.

I'm with @BurntSushi on this one, the raw in this fn name feels odd.

@ijackson
Copy link
Contributor Author

ijackson commented Jun 4, 2021

I'm with @BurntSushi on this one, the raw in this fn name feels odd.

Indeed. I did some looking around and I think your feeling is justified:ad88a43

Hence #85901 where I change this. That ought to go first. I'm not sure what the implications are for the stabilisation - does that mean it should wait some more?

Thanks.

@rfcbot rfcbot added 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 2, 2021
@rfcbot
Copy link

rfcbot commented Jul 2, 2021

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

@joshtriplett
Copy link
Member

@rfcbot concern name

Registering a concern so that this FCP doesn't complete until we finalize the name.

I agree that into_parts makes sense here; this is a safe method returning safe objects, unlike Vec::into_raw_parts which is unsafe and returns a raw pointer.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 2, 2021
@joshtriplett
Copy link
Member

@ijackson If the only thing that changes is the name, we don't need to delay stabilization (other than to wait for the PR changing the name). If something more substantive changes, we can consider if that impacts the evaluation we've done so far or not.

@m-ou-se
Copy link
Member

m-ou-se commented Jul 5, 2021

The 'raw' here refers to the fact that unlike .into_inner(), this function does not attempt to flush the data but just directly gives you the 'raw' parts of the buffer, without trying to process them.

I can see how it can be confusing with our other uses of the word raw, but just calling it .into_parts() might not make clear what the difference with .into_inner() is. (Though on the other hand, one could argue that .into_inner has to consolidate the parts into a single 'inner' again before returning it, wheras .into_parts just returns the parts (the buffer and the underlying writer) directly.)

@ijackson
Copy link
Contributor Author

ijackson commented Jul 6, 2021

into_inner gives you just an inner writer; into_parts gives you all the parts. I don't have a particularly strong feeling about this but I think into_parts is somewhat better. So I'll go and deal with the review comment on the fixups MR #85901.

@m-ou-se
Copy link
Member

m-ou-se commented Jul 28, 2021

Looks like WriterPanicked isn't publicly available: https://doc.rust-lang.org/nightly/std/io/struct.BufWriter.html#method.into_raw_parts Did something go wrong there with the visibility?

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 28, 2021
…iplett

Bufwriter disassembly tweaks

In rust-lang#80690 `@BurntSushi` observed that `WriterPanicked` was erroneously not exported, and suggested renaming `into_raw_parts` to `into_parts`. (More info in my commit messages.)

r? `@BurntSushi`
@ijackson
Copy link
Contributor Author

@m-ou-se: Yes, something did go wrong - I don't think I understood how all the use/visibility/stability plumbing worked. I was trying to fix that in #85901 but there is still something wrong with that. I will fix it tomorrow when I have a clear head. My apologies.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jul 29, 2021
…iplett

Bufwriter disassembly tweaks

In rust-lang#80690 `@BurntSushi` observed that `WriterPanicked` was erroneously not exported, and suggested renaming `into_raw_parts` to `into_parts`. (More info in my commit messages.)

r? `@BurntSushi`
fee1-dead added a commit to fee1-dead-contrib/rust that referenced this issue Jul 29, 2021
…iplett

Bufwriter disassembly tweaks

In rust-lang#80690 ``@BurntSushi`` observed that `WriterPanicked` was erroneously not exported, and suggested renaming `into_raw_parts` to `into_parts`. (More info in my commit messages.)

r? ``@BurntSushi``
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 29, 2021
…lett

Bufwriter disassembly tweaks

In rust-lang#80690 `@BurntSushi` observed that `WriterPanicked` was erroneously not exported, and suggested renaming `into_raw_parts` to `into_parts`. (More info in my commit messages.)

r? `@BurntSushi`
@ijackson ijackson changed the title Tracking Issue for BufWriter::into_raw_parts Tracking Issue for BufWriter::into_parts Aug 24, 2021
@bors bors closed this as completed in 3eee91b Aug 25, 2021
@rfcbot rfcbot removed 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 Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. 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.

7 participants