Windows: Don't error on broken non UTF-8 output

[ChrisDenton]

Currently, on Windows, the standard library will error if you try to write invalid UTF-8. Whereas other platforms allow this. The issue arises because the console uses UTF-16 so Rust has to re-encode the output.

This PR fixes it in two ways:

• If the console's code page is set to UTF-8 then we can just write bytes to the handle normally.
• Otherwise we do a lossy conversion to UTF-16.

Fixes #116871

[tbu-]

The reason for the removal of incomplete UTF-8 handling at the end of the string is not clear to me from the commit description. Why was that removed?

[ChrisDenton]

Because it now simply truncates the write to remove incomplete UTF-8 from the end and instead leaves the buffering to buffer types, i.e. LineWriter in this case.

[tbu-]

A LineWriter will flush an incomplete line if its buffer capacity is exceeded. If that happens, the output must support partial UTF-8 writes, or non-ASCII characters might get lost or replaced with the replacement character.

[ChrisDenton]

That can only result in broken UTF-8 if the user writes incomplete UTF-8 to LineWriter themselves.

[tbu-]

I see, digging through the source code, BufWriter makes sure to not split writes that the user issued.

What is the motivation for truncating invalid UTF-8 at the end of the string?

All else being equal, I'd rather expect the previous behavior, that I can construct UTF-8 output byte-by-byte.

[ChrisDenton]

Rather than having a secret stack buffer that can't be inspected or flushed, I'd strongly prefer buffering be done at a higher level. It's also a lot of added complexity for an edge case where the better solution is to set the console code page.

[tbu-]

In any case, if that behavior is wanted, it should probably be documented in the commit message so that it is clear to future readers that this change was on purpose.

Rather than ignoring trailing invalid UTF-8, I think it'd be better to replace it with a replacement character so that it becomes clear that something was removed.

[ChrisDenton]

Rather than ignoring trailing invalid UTF-8, I think it'd be better to replace it with a replacement character so that it becomes clear that something was removed.

That's what happens in this code. No bytes are ever lost. Either the caller is informed that less bytes were written than were provided or, if there is only an incomplete code point, then that is written to the console (which will be converted to replacement characters when lossy translating to UTF-16).

[tbu-]

Ah yes, that makes sense. I didn't realize the caller would be informed by the return value of write.

What is the motivation for special casing trailing invalid UTF-8? It seems to increase the code complexity a little as well, and is not necessary for std's own use cases.

Is it for supporting a potential non-std buffered writer?

[ChrisDenton]

Sure, it could be removed. Stderr is not buffered by us though and there have been proposals for unbuffered stdout.

[tbu-]

Suggested change

	debug_assert!(result != 0, "Unexpected error in MultiByteToWideChar");
	assert!(result != 0, "Unexpected error in MultiByteToWideChar");

I think this should be an assert since this isn't performance critical — we've just done a syscall.

[ChrisDenton]

I'm going to split off the LineWriter and GetConsoleCP changes into their own PRs as they are unrelated to the more controversial changes. Marking this as draft in the meantime.

[bors]

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

[thaliaarchi]

I like the idea of moving the buffering the the outer buffer, especially since we already wrap Stdout/Stderr in a LineWriter. trim_last_char_boundary looks sound. I haven't reviewed the rest as closely, but nothing stands out as wrong. I'm not yet convinced that LineWriter won't endlessly try to write the same incomplete single-char sequence, as I haven't reviewed that type with this invariant in mind.

View changes since this review