Try to clarify destructor not being run scenario. #1107
Conversation
|
rust-lang/nomicon#135 has some background. |
|
@ehuss Niko mentioned a lang-team FCP on the PR is the usual procedure. Should we kick one off? On the other hand, this change is pretty uncontroversial. Things would get more complex if we tried to specify when things are dropped after a panic. I don't think we have the language to define that precisely at the moment.
But we would need to define the call stack (if that's even the correct abstraction) and what "normal execution" is (no |
src/destructors.md
Outdated
| variable or field from being dropped automatically. | ||
|
|
||
| > Note: Preventing a destructor from being run via `forget` or other means is safe in Rust | ||
| > even if it has a type that isn't `'static`. This means that publicly exposed APIs cannot | ||
| > rely on destructor being run for soundness. |
There was a problem hiding this comment.
It seems like it might be good to expand a little on this: for one thing, publicly exposed APIs seems like a somewhat "interesting" term. Certainly even in a crate-local context I would expect to see unsafe fn on the constructor or similar if the destructor must run for soundness reasons.
Maybe we can adjust to something like?
Note that while code can safely prevent a destructor from running, they are still guaranteed to automatically run in places defined by this document (such as the end of scopes). Authors may rely on destructors running at these locations for soundness, but must not otherwise assume destructors are run.
There was a problem hiding this comment.
Ping @nbdd0121, was wondering if you'd be willing to change the note to something like the suggestion given here?
There was a problem hiding this comment.
Sorry for not being responsive, I am quite busy recently.
I don't think it's good to explicit saying that "you can expect destructor at the end of scope to run for soundness", as I think it could create additional confusion, as these are really just part of control flow that users expect.
The purpose of this change is to keep its original intention while removing the possibility to interpret it as a permit for compilers to omit destructor calls.
There was a problem hiding this comment.
No worries, I was just checking in.
How about something like this?
Note: Preventing a destructor from being run via
std::mem::forgetor other
means is safe even if it has a type that isn't'static.
Besides the places where destructors are guaranteed to run as defined by
this document, types may not safely rely on a destructor being run for
soundness.
I'm trying to blend the different ideas here:
- It should always be safe to call
forget. - There are places where destructors are guaranteed to run, and you may rely on that.
- Types may not rely on their destructor being run for soundness.
I agree with Mark about the "publicly exposed APIs", so I'm trying to avoid that phrasing.
There was a problem hiding this comment.
Sounds very good. I have applied your suggestion.
Update books ## reference 8 commits in 9d289c05fce7254b99c6a0d354d84abb7fd7a032..0a2fe6651fbccc6416c5110fdf5b93fb3cb29247 2022-02-23 08:58:20 -0800 to 2022-03-15 09:32:25 -0700 - Documentation PR for cfg_panic (rust-lang/reference#1157) - Document aarch64 `target_feature` options (rust-lang/reference#1102) - Try to clarify destructor not being run scenario. (rust-lang/reference#1107) - Add undocumented Punctuation token Tilde `~` (rust-lang/reference#1149) - update UB list for safe target_feature (rust-lang/reference#1050) - Update const_eval.md for feature stabilization (rust-lang/reference#1166) - Remove `.intel_syntax`/`.att_syntax` support entirely. - Fix `.intel_syntax` directive ## book 3 commits in 3f255ed40b8c82a0434088568fbed270dc31bf00..036e88a4f135365de85358febe5324976a56030a 2022-02-27 21:26:12 -0500 to 2022-03-04 21:53:33 -0500 - Fix some links and small wordings - Snapshot of chapter 19 for nostarch - Clarify fully-qualified syntax explanation ## rust-by-example 2 commits in 2a928483a20bb306a7399c0468234db90d89afb5..d504324f1e7dc7edb918ac39baae69f1f1513b8e 2022-02-28 11:36:59 -0300 to 2022-03-07 09:26:32 -0300 - Fixed extra indentation at line 43 in Phantom Testcase example. (rust-lang/rust-by-example#1515) - Typo fixed in description of inline ASM cpuid function (rust-lang/rust-by-example#1514) ## rustc-dev-guide 3 commits in 32f2a5b..0e4b961 2022-03-01 10:45:24 -0600 to 2022-03-14 08:40:37 -0700 - update winget install instructions to ensure proper packages are installed (-e for --exact, and full package names to ensure arbitrary packages from the msstore source aren't installed) - Add missing rustdoc tests explanations - Fix incorrectly escaped backtick
This is being discussed in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Guarantees.20around.20Drop.20.28.2390752.29.