Open estebank opened 7 months ago
This is where we overwrite file content when using the FilesEmitter
, which is the default emitter. https://github.com/rust-lang/rustfmt/blob/bf967319e258acb9b1648a952bba52665eceaf52/src/emitter/files.rs#L30
different emitters are used depending on what output you'd like rustfmt to generate. For example, Passing --backup
when invoking rustfmt will use the FilesWithBackupEmitter
, while passing --check
will use the DiffEmitter
.
Is there a different approach we should be taking other than calling fs::write
? Also, I'm pretty sure that the io::Error
would bubble up and produce an explicit error since our ErrorKind
maps io::Error
to ErrorKind::IoError
.
Specifically, I'd expect the error to be emitted in format_and_emit_report
.
@estebank do you know if calling std::fs::File::set_len would return an error if we can't extend the file size to the new length? Maybe that's OS specific? I'm thinking one approach we could take is to open the file for writing, try to extend the length to the new length, and then write the new content. I'm thinking this would resolve this issue.
@ytmimi that's a reasonable approach, although a safer action would be to create a new file with the new contents, verify that there were no errors when writing to that file (including a check for the appropriate expected length) and then in a single operation replacing the existing file with the new one.
Thank you for the feedback on my initial approach. Your suggestion also makes sense to me. Do you have any interested in working on a fix for this issue?
I just want to point out that we've got a single anecdotal report that doesn't have any details that I'd consider to be salient, such as platform, when this occurred/with what version of rustfmt, etc.
I think it's worth taking a pause to consider whether this is currently reproducible and whether it's a common, practical scenario before we invest into any significant overhaul of the codebase and/or behavior
@ytmimi sadly, I do not have the spare cycles at the moment to look at this, but it seems like a good ticket for first-time contributors.
@calebcartwright I don't disagree with the concern, but given the fact that it has happened at least once, I'm tempted to believe the issue is reproducible. There's no need to drop everything and deal with it now, but now that there's a ticket you have somewhere to point people at if this happens again.
From https://news.ycombinator.com/item?id=39121489
rustfmt should produce an explicit error when encountering a full disk and avoid attempting to replace an existing file with one that has no content. This later case might be a good unconditional sanity check against other potential bugs.