r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

std::fs::{copy, write} are convenience methods, that do nothing about durability. They'll also happily truncate existing files and then leaving you stranded in the middle of the operation if something goes wrong. They don't do the rename dance, they don't use tempfiles, they don't fsync. Basically, if one is using them then one is signalling not caring much.

So I don't see why these practically least-effort methods are the ones that would warrant extra care.

Such a PR might be more appropriate to atomic write crates... except that they already do fsync, which subsumes this.

• https://github.com/untitaker/rust-atomicwrites/blob/c1b223ad040939501df1a98aeaf19f86f49a7302/src/lib.rs#L200-L203
• https://docs.rs/atomic-write-file/0.3.0/atomic_write_file/index.html#how-it-works).](https://docs.rs/atomic-write-file/0.3.0/atomic_write_file/index.html#how-it-works

We're just returning the error from close(2) that we're calling anyway. I'm just changing the code from ignoring the error to returning it. Why is that such a problem? The OS is telling us something, and you'd prefer to throw the error away over returning it to the user?

Well, I both do not like the precedent, and I also think that if anything really needs to do this then these are the methods where it's the least necessary, which makes the first point worse "look, even these simple, least-defensive std methods do it!"

I'm personally not a huge fan of incrementally doing this in semi-random places in the standard library that operate on files internally and happen to already handle some errors. At minimum, it makes it harder to read code by adding extra operations.

I'm going to mark this waiting on T-libs. I think it makes sense for T-libs and T-libs-api to discuss what we want as policy here for internal implementations and external implementations before we land changes either way.

I'm personally not a huge fan of incrementally doing this in semi-random places in the standard library that operate on files internally and happen to already handle some errors. At minimum, it makes it harder to read code by adding extra operations.

I tried to not make it "semi-random places in the standard library", but "all places in the standard library that drop a file after writing to it", i.e. all the places where the calling code can't close the file itself.

I'm going to mark this waiting on T-libs. I think it makes sense for T-libs and T-libs-api to discuss what we want as policy here for internal implementations and external implementations before we land changes either way.

Why does this need T-libs/T-libs-api decisions on policy?

This looks like a simple quality of implementation thing. It does not regress any use cases. It improves some (see rust-lang/libs-team#705). It doesn't do anything special a normal rust crate couldn't do. It can be reverted if it causes problems. It's not a huge diff, so it's not hard to maintain.