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

Document the unsafe keyword #73943

Merged
merged 1 commit into from
Aug 15, 2020
Merged

Conversation

poliorcetics
Copy link
Contributor

@poliorcetics poliorcetics commented Jul 1, 2020

Partial fix of #34601 (just one more and it will be done 😄).

I tried to be concise and redirect as much as possible on other, longer resources, exposing only the strict necessary.

I also used SAFETY: comments to promote good documentation.

I would like a thorough review to ensure I did not introduce mistakes that would confuse people or worse, lead them to write unsound code.

@rustbot modify labels: T-doc,C-enhancement

Edit: this is now the last PR for the original issue: fixes #34601.

@rustbot rustbot added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Jul 1, 2020
@rust-highfive
Copy link
Collaborator

r? @dtolnay

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2020
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super experienced in unsafe, just a few ideas I had off the top of my head.

src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
@poliorcetics
Copy link
Contributor Author

poliorcetics commented Jul 2, 2020

Modified lots of things ! The biggest change is the new The different meanings of unsafe section, the others changes are mot simple fixes taken from suggestions during review, including the link to Send and Sync.

src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
@dtolnay
Copy link
Member

dtolnay commented Jul 6, 2020

I don't know who on T-doc reviews this kind of thing but let's try this:

r? @rylev

@dtolnay
Copy link
Member

dtolnay commented Jul 6, 2020

@bors delegate=rylev

@bors
Copy link
Contributor

bors commented Jul 6, 2020

✌️ @rylev can now approve this pull request

Copy link
Member

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtolnay I'm not sure if I'm the right person since the dissolution of the docs team. Happy to do reviews though.

src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
@poliorcetics
Copy link
Contributor Author

@rylev I changed the wording, is it better ?

I'm still not particularly satisfied about my solution to the last point you made: here is what I wrote now but it feels heavy and forced to me, I you have anything better I'll take it !

-/// This different meanings have led to [discussion]s about the ability to make
-/// `unsafe` operations inside `unsafe fn`.
+/// As of now (Rust 1.44), this meanings become entwined when writing an
+/// `unsafe fn`: this syntax acts like an `unsafe` block around the code inside
+/// the function **and** as a signal to callers about the conditions they must
+/// met before using it.
+///
+/// Mixing this two meanings as one can be confusing and [discussion]s exist
+/// about the necessity to use `unsafe` blocks inside such functions when making
+/// `unsafe` operations.

@pickfire
Copy link
Contributor

pickfire commented Jul 7, 2020

@poliorcetics What is entwined? I had to read that up, I wonder if if this would be better.

-/// This different meanings have led to [discussion]s about the ability to make
-/// `unsafe` operations inside `unsafe fn`.
+/// As of now (Rust 1.44), `unsafe fn` can act like an `unsafe` block around
+/// the code inside the function **or** a signal to callers that some conditions
+/// must be met before using it.
+///
+/// Mixing these two meanings can be confusing and [discussion]s exist to use
+/// `unsafe` blocks inside such functions when making `unsafe` operations.

about the conditions they must met before using it.

What conditions? unsafe?

@poliorcetics
Copy link
Contributor Author

What conditions? unsafe?

I was thinking about the conditions to safely call an unsafe fn, like passing a correct pointer and the correct associated length for a function like quick_sort(int32_t *tab, size_t size).

src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
src/libstd/keyword_docs.rs Outdated Show resolved Hide resolved
@poliorcetics poliorcetics force-pushed the unsafe-keyword branch 3 times, most recently from 909b558 to 7b61eaf Compare July 20, 2020 18:10
@bors
Copy link
Contributor

bors commented Jul 28, 2020

☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts.

@Muirrum Muirrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2020
@Dylan-DPC-zz
Copy link

r? @steveklabnik

Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I have two small edits, r=me after

library/std/src/keyword_docs.rs Outdated Show resolved Hide resolved
library/std/src/keyword_docs.rs Outdated Show resolved Hide resolved
@poliorcetics
Copy link
Contributor Author

@steveklabnik done, do you want me to squash ?

@Dylan-DPC-zz
Copy link

@poliorcetics better to squash

@poliorcetics
Copy link
Contributor Author

Rebased and squashed :)

@Dylan-DPC-zz
Copy link

@bors r=steveklabnik rollup

@bors
Copy link
Contributor

bors commented Aug 14, 2020

📌 Commit 0e610bb has been approved by steveklabnik

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2020
Rollup of 17 pull requests

Successful merges:

 - rust-lang#73943 (Document the unsafe keyword)
 - rust-lang#74062 (deny(unsafe_op_in_unsafe_fn) in libstd/ffi/c_str.rs)
 - rust-lang#74185 (Remove liballoc unneeded explicit link)
 - rust-lang#74192 (Improve documentation on process::Child.std* fields)
 - rust-lang#74409 (Change Debug impl of SocketAddr and IpAddr to match their Display output)
 - rust-lang#75195 (BTreeMap: purge innocent use of into_kv_mut)
 - rust-lang#75214 (Use intra-doc links in `mem::manually_drop` & `mem::maybe_uninit`)
 - rust-lang#75432 (Switch to intra-doc links in `std::process`)
 - rust-lang#75482 (Clean up E0752 explanation)
 - rust-lang#75501 (Move to intra doc links in std::ffi)
 - rust-lang#75509 (Tweak suggestion for `this` -> `self`)
 - rust-lang#75511 (Do not emit E0228 when it is implied by E0106)
 - rust-lang#75515 (Bump std's libc version to 0.2.74)
 - rust-lang#75517 (Promotion and const interning comments)
 - rust-lang#75519 (BTreeMap: refactor splitpoint and move testing over to unit test)
 - rust-lang#75530 (Switch to intra-doc links in os/raw/*.md)
 - rust-lang#75531 (Migrate unit tests of btree collections to their native breeding ground)

Failed merges:

r? @ghost
@bors bors merged commit f163ec5 into rust-lang:master Aug 15, 2020
@poliorcetics poliorcetics deleted the unsafe-keyword branch August 15, 2020 08:01
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add documentation for all keywords like the primitive types' documentation