Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std::ops::Drop documentation focuses on scope, ignores other causes of drops #36073

Closed
hanna-kruppe opened this issue Aug 28, 2016 · 5 comments · Fixed by #71598
Closed

std::ops::Drop documentation focuses on scope, ignores other causes of drops #36073

hanna-kruppe opened this issue Aug 28, 2016 · 5 comments · Fixed by #71598
Labels
A-destructors Area: Destructors (`Drop`, …) A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Medium priority T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@hanna-kruppe
Copy link
Contributor

There was some confusion in #rust over when values are dropped. It was pointed out that the std::ops::Drop documentation only says drop is called if "a value goes out of scope", which is inaccurate:

  • Scopes apply to variables, not values
  • Values can be dropped for other reasons than scopes ending. For example, if a variable is re-assigned, the old value is dropped. There are also temporaries.

I don't know how to best phrase it, but focusing only on scopes is evidently a good way to confuse people.

@nagisa
Copy link
Member

nagisa commented Aug 28, 2016

I feel like scopes are still the best way to model the intuition about drops, but it is important to understand that scopes used for drop generation aren’t exactly the lexical scopes which you have in the source file.

Most notably drop scopes may overlap in various ways in which the lexical scopes cannot. Similarly, each let binding introduces a scope, which is distinct from any lexical scope you have.

(Re-)assignments are best considered as a sugar for drop(::std::mem::replace(&mut x, y) which does not exactly invalidate the scope-based intuition either.

@hanna-kruppe
Copy link
Contributor Author

hanna-kruppe commented Aug 28, 2016

What exactly is a "scope", then? An arbitrary execution path in the CFG? I can't see them being anything less, since all the various operations that cause early drops may be conditional or in loops.

And I find it very strange to "desugar" a primitive language construct such as assignment to a combination of various API calls. I understand it's meant for "intuitive" understanding, but even so it sounds strange and possibly counter-productive for people confused about drop placement.

@Aatch Aatch changed the title std::ops::Drop focuses on scope, ignores other causes of drops std::ops::Drop documentation focuses on scope, ignores other causes of drops Aug 28, 2016
@Aatch Aatch added the A-docs label Aug 28, 2016
@Aatch
Copy link
Contributor

Aatch commented Aug 28, 2016

I adjusted the title to make it more clear this is talking about documentation, rather than the operation of destructors in general.

@steveklabnik steveklabnik added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools P-medium Medium priority and removed A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Mar 10, 2017
@steveklabnik
Copy link
Member

doc team triage: p-medium

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@steveklabnik
Copy link
Member

Triage: no changes, though with NLL, referring to it as a scope will become even more confusing, making this a higher priority.

steveklabnik added a commit to steveklabnik/rust that referenced this issue Jan 8, 2019
Fixes rust-lang#36073

Drop's docs were not great, and as mentioned in rust-lang#36073, focused too much on scope.
I've re-done them a bit, using the same examples, and making it clear that scope is not
the only thing that causes destructors to run.
@jonas-schievink jonas-schievink added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. A-destructors Area: Destructors (`Drop`, …) labels Mar 6, 2020
@bors bors closed this as completed in bd704f7 May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-destructors Area: Destructors (`Drop`, …) A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Medium priority T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants