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

re-do documentation for Drop #57449

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@steveklabnik
Copy link
Member

steveklabnik commented Jan 8, 2019

Fixes #36073

Drop's docs were not great, and as mentioned in #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.

I'm not feeling super awesome about the Copy vs Drop section. Anyone have any advice?

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 8, 2019

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

Show resolved Hide resolved src/libcore/ops/drop.rs Outdated
Show resolved Hide resolved src/libcore/ops/drop.rs Outdated
/// Dropping HasDrop!
/// ```
///
/// Similarly, a slice will drop each element in order.

This comment has been minimized.

@Xanewok

Xanewok Jan 8, 2019

Member

Maybe it'd make sense to introduce a subheading ## Drop order here? I'd imagine people will be often specifically looking for that (the last version that something similar). What do you think?

This comment has been minimized.

@steveklabnik

steveklabnik Jan 8, 2019

Member

I was torn. It seems a bit small, but maybe it is worth it. I'm open to suggestions!

Show resolved Hide resolved src/libcore/ops/drop.rs Outdated
Show resolved Hide resolved src/libcore/ops/drop.rs Outdated
///
/// ```
///
/// When a value goes out of scope, it will call `Drop::drop` on that value. For example,

This comment has been minimized.

@shepmaster

shepmaster Jan 8, 2019

Member

it will call

What is "it"?

Show resolved Hide resolved src/libcore/ops/drop.rs
Show resolved Hide resolved src/libcore/ops/drop.rs Outdated
/// call to `Drop::drop`, you'd get a compiler error.
///
/// If you'd like to call `drop` yourself, there is something you can do: call [`std::mem::drop`].
/// This function will drop its argument. For more, see its documentation.

This comment has been minimized.

@shepmaster

shepmaster Jan 8, 2019

Member

For more, see its documentation.

Is this required when the function itself is linked?

This comment has been minimized.

@steveklabnik

steveklabnik Jan 8, 2019

Member

I'm trying to allude to there being more to it, if that makes any sense.

///
/// Which of our two `HasDrop` drops first, though? It's the same order that
/// they're declared: first `one`, then `two`. If you'd like to try this
/// yourself, you can modify `HasDrop` above to contain some data, like an

This comment has been minimized.

@shepmaster

shepmaster Jan 8, 2019

Member

you can modify HasDrop above to contain some data

Should we proactively do that to prove the point?

This comment has been minimized.

@steveklabnik

steveklabnik Jan 8, 2019

Member

I was thinking about it but 1. it's long 2. it's not a huge change, and people sometimes like examples they can try. i'm open to including it, it'd just make this section even longer than it already is. maybe that's okay!

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 8, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:00749598:start=1546979279108596061,finish=1546979280061836116,duration=953240055
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---

[00:03:48] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:2: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:8: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:10: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:18: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:20: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:22: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:25: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:31: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:36: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:39: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:40: unexplained "```ignore" doctest; try one:
[00:03:48] 
[00:03:48] * make the test actually pass, by adding necessary imports and declarations, or
[00:03:48] * use "```text", if the code is not Rust code, or
[00:03:48] * use "```compile_fail,Ennnn", if the code is expected to fail at compile time, or
[00:03:48] * use "```should_panic", if the code is expected to fail at run time, or
[00:03:48] * use "```no_run", if the code should type-check but not necessary linkable/runnable, or
[00:03:48] * explain it like "```ignore (cannot-test-this-because-xxxx)", if the annotation cannot be avoided.
[00:03:48] 
[00:03:48] 
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:43: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:47: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:49: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:51: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:55: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:58: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:61: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:63: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:65: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:69: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:72: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:78: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:83: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:89: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:94: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:96: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:102: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:104: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:110: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:112: trailing whitespace
[00:03:48] tidy error: /checkout/src/libcore/ops/drop.rs:116: trailing whitespace
[00:03:49] some tidy checks failed
[00:03:49] 
[00:03:49] 
[00:03:49] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:49] 
[00:03:49] 
[00:03:49] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:49] Build completed unsuccessfully in 0:00:47
[00:03:49] Build completed unsuccessfully in 0:00:47
[00:03:49] Makefile:69: recipe for target 'tidy' failed
[00:03:49] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:00abbfd0
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Tue Jan  8 20:31:58 UTC 2019
---
travis_time:end:07f6ba6d:start=1546979519445687158,finish=1546979519449816042,duration=4128884
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0044d720
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:187cddd8
travis_time:start:187cddd8
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0617cae0
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@steveklabnik steveklabnik force-pushed the steveklabnik:gh36073 branch from cb54c99 to b4be0ba Jan 8, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 8, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0f13aeba:start=1546982709807677472,finish=1546982711384878035,duration=1577200563
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

re-do documentation for Drop
Fixes #36073

Drop's docs were not great, and as mentioned in #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.

@steveklabnik steveklabnik force-pushed the steveklabnik:gh36073 branch from b4be0ba to 66e98f4 Jan 8, 2019

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Jan 9, 2019

@steveklabnik I'm not opposed to reviewing this, but is there someone more documentation-minded that would be better?

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jan 9, 2019

Yeah, does someone from @rust-lang/docs want to take this on?

Show resolved Hide resolved src/libcore/ops/drop.rs Outdated
@crlf0710

This comment has been minimized.

Copy link
Contributor

crlf0710 commented Jan 9, 2019

Mmmm, i think the text here is not really consistent with the Reference. It has been this way for a long time, but i'll really appreciate it if we can fix it.

The reference text is something like this:

The destructor of a type consists of

1. Calling its std::ops::Drop::drop method, if it has one.
2. Recursively running the destructor of all of its fields.
   * The fields of a struct, tuple or enum variant are dropped in declaration order. *
   * The elements of an array or owned slice are dropped from the first element to the last. *
   * The captured values of a closure are dropped in an unspecified order.
   * Trait objects run the destructor of the underlying type.
   * Other types don't result in any further drops.

So you see, Drop is not actually the destructor itself, but some "prologue" of it. (I'm not native English speaker, so my wording may be inaccurate,) I really wish this can be clarified here.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jan 9, 2019

@crlf0710 i was trying to bring it in line :)

The destructor consists of calling Drop on things. I'm not sure that the distinction between the code calling drop and the drop code itself is important enough to make a big deal out of this. I'm open to suggestions though.

@crlf0710

This comment has been minimized.

Copy link
Contributor

crlf0710 commented Jan 10, 2019

@steveklabnik I think the distinction is important. Every type has its own destructor. It's just that sometimes the destructor is, in C++ terminology, trivial. The types implementing Drop is a subset of the the types having a nontrivial destructor, which in turn is a subset of all types.

///
/// impl Drop for Inner {
/// Our custom implementation of `Drop` will be called at the end of `main`.

This comment has been minimized.

@crlf0710

crlf0710 Jan 10, 2019

Contributor

A little misleading here, it seems as if there's a default implementation of Drop, which is not true.

///
/// [`std::mem::drop`]: ../../std/mem/fn.drop.html
///
/// ## `Drop` is recursive

This comment has been minimized.

@crlf0710

crlf0710 Jan 10, 2019

Contributor

Maybe it's the destructor that is recursive...

///
/// You cannot implement both [`Copy`] and `Drop` on the same type. Types that
/// are `Copy` don't manage resources, and can be freely copied. As such, they
/// cannot have destructors.

This comment has been minimized.

@crlf0710

crlf0710 Jan 10, 2019

Contributor

Rust terminology of "nontrivial destructors"

/// order that they're declared: first `one`, then `two`. If you'd like to try
/// this yourself, you can modify `HasDrop` above to contain some data, like an
/// integer, and then use it in the `println!` inside of `Drop`. This behavior is
/// guaranteed by the language. In other situations, the rules may be different,

This comment has been minimized.

@liigo

liigo Jan 11, 2019

Contributor

Basicly there're two drop orders in Rust: by declare order (for structs/arrays), by reverse order (for stack variables). Only talk one of them, but omit another, will mislead people (to think that variables are droped by declare order too).

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jan 11, 2019

@rust-lang/lang, what do you think about the above language?

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 11, 2019

I think this is a definite improvement, but I agree with the above comments that I think it's important to highlight the difference between running drop glue on a value (which happens to values of every type) and running the Drop::drop method, which only happens on types implementing the Drop trait, just before running drop glue on each of their fields. Values which don't implement the Drop trait are still dropped, but Drop::drop is not called on them before the drop glue for fields is run.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jan 11, 2019

Thanks @cramertj . The reference doesn't really draw a distinction between "drop glue" and "Drop"; do you think it should there as well?

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 11, 2019

@steveklabnik yeah, I do-- I think that's an important distinction to make. I wish we had better language for it, but "drop glue" is what I've heard it called most consistently. Drop::drop running is pretty different from drop glue running on a value, so I think we need separate terminology for those.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jan 11, 2019

Cool, thanks! I'll pursue that as well, separate from this ticket.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jan 11, 2019

I've given this a few tries, and am still not quite happy with it. Given that this PR is going to change significantly thanks to feedback, I'm going to give it a close and try again later. Thanks for the reviews, everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment