feat(rome_console): redact Unicode control characters #2384
Conversation
crates/rome_console/src/fmt.rs
Outdated
| io::Write::write_all(writer, content.as_bytes()) | ||
| SanitizeAdapter(writer) | ||
| .write_str(content) | ||
| .map_err(|_| io::Error::new(io::ErrorKind::Other, "formatter error")) |
There was a problem hiding this comment.
Nit: Would it be possible to use some more descriptive error messages. Maybe also something that helps us find the cause of the error?
There was a problem hiding this comment.
This is the same error message that would be emitted by the Rust standard library when converting an fmt::Error to io::Error (https://github.com/rust-lang/rust/blob/master/library/std/src/io/mod.rs#L1662), I copied it to keep the errors coherent in case there's another place where we use io::Write::write_fmt (although here the ErrorKind is Other because Uncategorized is private to the standard library)
There was a problem hiding this comment.
Either way, is there a way to put a more descriptive message and how to fix the error? If so, then we should put such information regardless: https://rome.tools/#technical
First and second bullet points
There was a problem hiding this comment.
Reading back the code in std it also includes a bit of logic to forward the original I/O error if it exists, I missed that when I originally searched for the error message but now I'm also going to include that.
This means the only case a "formatter error" could be returned is when an implementation of fmt::Display returned an error that didn't originate from the underlying writer, unfortunately since all displayable types are type-erased it's not really possible to extract more information from the error.
I can probably reword the message to make the exact error that happened more clear, then as the console printing is designed to panic in case of error down the line this error message is going to be printed to the user as part of the panic hook telling the user Rome encountered a bug and asking them to file a bug report, so hopefully all of that should make the error more approachable if it's ever encountered in a production build
There was a problem hiding this comment.
Yeah, that makes sense. At the end it's our bug, so asking the user to open in issue seems to be a good messagging.
Deploying with
|
| Latest commit: |
3faa07e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1c8c4bd6.tools-8rn.pages.dev |
Summary
This change intercepts the writing logic of
rome_consoleto replace on the fly any Unicode character with a width of zero (with the exception of code points with theWhite_Spaceproperty) with theREPLACEMENT CHARACTERcode point. This should cover most code points in theCcandCfcategories, and ensure they are explicitly visible in the terminal output (for instance when printing code spans)