Skip to content

Allow writing of incomplete UTF-8 sequences to the Windows console via stdout/stderr - #83342

Merged
bors merged 12 commits into
rust-lang:masterfrom
Count-Count:win-console-incomplete-utf8
Sep 2, 2021
Merged

bors merged 12 commits into
rust-lang:masterfrom
Count-Count:win-console-incomplete-utf8

Conversation

@Count-Count

@Count-Count Count-Count commented Mar 21, 2021

Copy link
Copy Markdown
Contributor

Problem

Writes of just an incomplete UTF-8 byte sequence (e.g. b"\xC3" or b"\xF0\x9F") to stdout/stderr with a Windows console attached error with io::ErrorKind::InvalidData, "Windows stdio in console mode does not support writing non-UTF-8 byte sequences" even though further writes could complete the codepoint. This is currently a rare occurence since the linewritershim implementation flushes complete lines immediately and buffers up to 1024 bytes for incomplete lines. It can still happen as described in #83258.

The problem will become more pronounced once the developer can switch stdout/stderr from line-buffered to block-buffered or immediate when the changes in the "Switchable buffering for Stdout" pull request (#78515) get merged.

Patch description

If there is at least one valid UTF-8 codepoint all valid UTF-8 is passed through to the extracted write_valid_utf8_to_console() fn. The new code only comes into play if write() is being passed a short byte slice comprising an incomplete UTF-8 codepoint. In this case up to three bytes are buffered in the IncompleteUtf8 struct associated with Stdout / Stderr. The bytes are accepted one at a time. As soon as an error can be detected io::ErrorKind::InvalidData, "Windows stdio in console mode does not support writing non-UTF-8 byte sequences" is returned. Once a complete UTF-8 codepoint is received it is passed to the write_valid_utf8_to_console() and the buffer length is set to zero.

Calling flush() will neither error nor write anything if an incomplete codepoint is present in the buffer.

Tests

Currently there are no Windows-specific tests for console writing code at all. Writing (regression) tests for this problem is a bit challenging since unit tests and UI tests don't run in a console and suddenly popping up another console window might be surprising to developers running the testsuite and it might not work at all in CI builds. To just test the new functionality in unit tests the code would need to be refactored. Some guidance on how to proceed would be appreciated.

Public API changes

  • std::str::verifications::utf8_char_width() would be exposed as std::str::utf8_char_width() behind the "str_internals" feature gate.

Related issues

Open questions

  • Add tests?
  • Squash into one commit with better commit message?

@rust-highfive

ghost commented Mar 21, 2021

Copy link
Copy Markdown
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2021
@Count-Count
Count-Count marked this pull request as draft March 21, 2021 10:32
@rust-log-analyzer

This comment has been minimized.

Comment thread library/std/src/sys/windows/stdio.rs Outdated
@jyn514 jyn514 added T-libs-api [DEPRECATED; DO NOT USE] A-Unicode Area: Unicode O-windows Operating system: Windows labels Mar 21, 2021

ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should stdout use STDERR handle here?

@Count-Count

ghost commented Mar 21, 2021

Copy link
Copy Markdown
Contributor Author

Should stdout use STDERR handle here?

Cut&Paste mistake, will fix it right away. Thanks for spotting it!

@Count-Count
Count-Count force-pushed the win-console-incomplete-utf8 branch from cb3fc85 to fb1fa97 Compare March 24, 2021 06:17
@rust-log-analyzer

This comment has been minimized.

@Count-Count
Count-Count marked this pull request as ready for review March 24, 2021 12:32
@Count-Count Count-Count changed the title Allow writing of incomplete UTF-8 sequence to the Windows console via stdout/stderr Allow writing of incomplete UTF-8 sequences to the Windows console via stdout/stderr Mar 30, 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 Apr 23, 2021
@JohnCSimon JohnCSimon removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 9, 2021
@rfcbot rfcbot added the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label Aug 18, 2021
@rfcbot

ghost commented Aug 19, 2021

Copy link
Copy Markdown

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

@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 Aug 19, 2021
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. 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 29, 2021
@rfcbot

ghost commented Aug 29, 2021

Copy link
Copy Markdown

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.

The RFC will be merged soon.

@m-ou-se m-ou-se added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 31, 2021
@m-ou-se

ghost commented Aug 31, 2021

Copy link
Copy Markdown
Member

@bors r+

@bors

ghost commented Aug 31, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit fbfde7e has been approved by m-ou-se

@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 Aug 31, 2021
@bors

ghost commented Sep 2, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit fbfde7e with merge cc9bb15...

@bors

ghost commented Sep 2, 2021

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing cc9bb15 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 2, 2021
@bors
bors merged commit cc9bb15 into rust-lang:master Sep 2, 2021
@rustbot rustbot added this to the 1.56.0 milestone Sep 2, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Unicode Area: Unicode 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. merged-by-bors This PR was explicitly merged by bors. O-windows Operating system: Windows relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api [DEPRECATED; DO NOT USE]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Writing valid UTF-8 to the Windows terminal using stdout().write() can incorrectly return an error