Avoid deleting temporary files on error #75315
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@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 |
| // This is `None` if the directory should not be deleted; used for | ||
| // -Csave-temps. We don't just pass around a PathBuf to keep type safety | ||
| // (only creating files in temporary locations). | ||
| dir: Option<TempDirReal>, |
There was a problem hiding this comment.
This is a tad convoluted. Why not ManuallyDrop<TempDirReal> alongside a boolean flag?
| .prefix("rustc") | ||
| .tempdir() | ||
| .unwrap_or_else(|err| sess.fatal(&format!("couldn't create a temp dir: {}", err))); | ||
| let path = TempDir::new(tmpdir, sess.opts.cg.save_temps); |
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.
Mark-Simulacrum
force-pushed
the
save-temps
branch
from
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 |
bors
added
S-waiting-on-bors
| 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