Avoid deleting temporary files on error#75315
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
@bors try This'll generate linux artifacts which hopefully @infinity0 can test with |
|
⌛ Trying commit 74427e8870e6d11c9d726dae8f4aeb336a58e32a with merge 7d10458b44a5240ee001ce5e62ba6bc1460022fb... |
|
☀️ Try build successful - checks-actions, checks-azure |
There was a problem hiding this comment.
This is a tad convoluted. Why not ManuallyDrop<TempDirReal> alongside a boolean flag?
There was a problem hiding this comment.
Can we use a different name for this? MaybeTempDir or something? The boolean argument to new is surprising if you're used to tempfile::TempDir.
Previously if the compiler error'd, fatally, then temporary directories which should be preserved by -Csave-temps would be deleted due to fatal compiler errors being implemented as panics.
74427e8 to
2627eed
Compare
|
Yes, I also checked the other places where we use save_temps for potential problems with RAII. I would've liked a way to ensure that we don't add more, but I think that'll be hard. Fixed both comments; I also filed an issue upstream asking if we can get this implemented in the tempfile crate itself. @bors r=ecstatic-morse |
|
📌 Commit 2627eed has been approved by |
| fn drop(&mut self) { | ||
| // Safety: We are in the destructor, and no further access will | ||
| // occur. | ||
| let dir = unsafe { ManuallyDrop::take(&mut self.dir) }; |
There was a problem hiding this comment.
No need for unsafe. Just drop(self.dir.into_inner()) if keep is false.
There was a problem hiding this comment.
Oh NVM, we can't move out of it in the destructor. I would probably have done this with ManuallyDrop::drop, but that's also unsafe.
There was a problem hiding this comment.
I considered that, but then we need to have two unsafe blocks I think - one for the into_path case, with take, the other with drop. Seems more complicated.
There was a problem hiding this comment.
...otherwise we leak the PathBuf inside TempDir. Whoops. I'm all caught up now.
Rollup of 10 pull requests Successful merges: - rust-lang#75098 (Clippy pointer cast lint experiment) - rust-lang#75249 (Only add a border for the rust logo) - rust-lang#75315 (Avoid deleting temporary files on error) - rust-lang#75316 (Don't try to use wasm intrinsics on vectors) - rust-lang#75337 (instance: only polymorphize upvar substs) - rust-lang#75339 (evaluate required_consts when pushing stack frame in Miri engine) - rust-lang#75363 (Use existing `infcx` when emitting trait impl diagnostic) - rust-lang#75366 (Add help button) - rust-lang#75369 (Move to intra-doc links in /library/core/src/borrow.rs) - rust-lang#75379 (Use intra-doc links in /library/core/src/cmp.rs) Failed merges: r? @ghost
Previously if the compiler error'd, fatally, then temporary directories which
should be preserved by -Csave-temps would be deleted due to fatal compiler
errors being implemented as panics.
cc @infinity0
(Hopefully) fixes #75275, but I haven't tested