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 all unsafety used in libcore and libstd #66219

Closed
oli-obk opened this issue Nov 8, 2019 · 18 comments
Closed

Document all unsafety used in libcore and libstd #66219

oli-obk opened this issue Nov 8, 2019 · 18 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Nov 8, 2019

The repository still contains many instances of

// ignore-tidy-undocumented-unsafe

We should eliminate all of them by documenting the unsafe used in those files.

Documenting unsafe blocks works by adding a // SAFETY: comment infront of them explaining why the unsafe block is ok. If there is an explanation about the unsafety elsewhere in the file, you can also leave a // SAFETY: comment explaining that the overarching logic is explained elsewhere (and mention where, too!)

cc @Centril

not sure what labels to add to this issue.

See https://github.com/rust-lang/rust/pull/63793/files for examples and the introduction of the unsafety tidy check

@oli-obk oli-obk added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Nov 8, 2019
@Centril Centril added T-compiler Relevant to the compiler 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. E-help-wanted Call for participation: Help is requested to fix this issue. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Nov 8, 2019
@foeb
Copy link
Contributor

foeb commented Nov 9, 2019

Hi! I'm interested in working on this.

@foeb
Copy link
Contributor

foeb commented Nov 9, 2019

@rustbot claim

@rustbot rustbot self-assigned this Nov 9, 2019
foeb added a commit to foeb/rust that referenced this issue Nov 9, 2019
foeb added a commit to foeb/rust that referenced this issue Nov 9, 2019
foeb added a commit to foeb/rust that referenced this issue Nov 10, 2019
foeb added a commit to foeb/rust that referenced this issue Nov 10, 2019
foeb added a commit to foeb/rust that referenced this issue Nov 10, 2019
foeb added a commit to foeb/rust that referenced this issue Nov 10, 2019
foeb added a commit to foeb/rust that referenced this issue Nov 12, 2019
foeb added a commit to foeb/rust that referenced this issue Nov 18, 2019
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 17, 2020
…l-str, r=Amanieu

Document unsafe blocks in core::{cell, str, sync}

Split from rust-lang#66506 (issue rust-lang#66219). Hopefully doing a chunk at a time is more manageable!

r? @RalfJung
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 17, 2020
…l-str, r=Amanieu

Document unsafe blocks in core::{cell, str, sync}

Split from rust-lang#66506 (issue rust-lang#66219). Hopefully doing a chunk at a time is more manageable!

r? @RalfJung
Centril added a commit to Centril/rust that referenced this issue Mar 11, 2020
…rk-Simulacrum

Document unsafe blocks in core::fmt

r? @RalfJung
CC: @rust-lang/wg-unsafe-code-guidelines
rust-lang#66219

Sorry for the hiatus, but here's a few more files with the unsafe blocks documented! I think working on it smaller chunks like this will be easier for everyone.
Centril added a commit to Centril/rust that referenced this issue Mar 11, 2020
…rk-Simulacrum

Document unsafe blocks in core::fmt

r? @RalfJung
CC: @rust-lang/wg-unsafe-code-guidelines
rust-lang#66219

Sorry for the hiatus, but here's a few more files with the unsafe blocks documented! I think working on it smaller chunks like this will be easier for everyone.
Centril added a commit to Centril/rust that referenced this issue Mar 12, 2020
…rk-Simulacrum

Document unsafe blocks in core::fmt

r? @RalfJung
CC: @rust-lang/wg-unsafe-code-guidelines
rust-lang#66219

Sorry for the hiatus, but here's a few more files with the unsafe blocks documented! I think working on it smaller chunks like this will be easier for everyone.
@LeSeulArtichaut
Copy link
Contributor

@foeb I took the liberty of opening #71063. I hope you weren't working on those 3 files 😅

@hbina
Copy link
Contributor

hbina commented Apr 22, 2020

Hello,

Is anyone working on

unsafe {
// If the first two elements are out-of-order...
if len >= 2 && is_less(v.get_unchecked(1), v.get_unchecked(0)) {
// Read the first element into a stack-allocated variable. If a following comparison
// operation panics, `hole` will get dropped and automatically write the element back
// into the slice.
let mut tmp = mem::ManuallyDrop::new(ptr::read(v.get_unchecked(0)));
let mut hole = CopyOnDrop { src: &mut *tmp, dest: v.get_unchecked_mut(1) };
ptr::copy_nonoverlapping(v.get_unchecked(1), v.get_unchecked_mut(0), 1);
for i in 2..len {
if !is_less(v.get_unchecked(i), &*tmp) {
break;
}
// Move `i`-th element one place to the left, thus shifting the hole to the right.
ptr::copy_nonoverlapping(v.get_unchecked(i), v.get_unchecked_mut(i - 1), 1);
hole.dest = v.get_unchecked_mut(i);
}
// `hole` gets dropped and thus copies `tmp` into the remaining hole in `v`.
}

This can be shown to be safe because len > 2 => len - 2 > 0, right?

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 1, 2020
…r=Mark-Simulacrum

Document unsafety for `*const T` and `*mut T`

Helps with rust-lang#66219
r? @Mark-Simulacrum
Manishearth added a commit to Manishearth/rust that referenced this issue Jun 19, 2020
… r=joshtriplett

Document unsafety in slice/sort.rs

Let me know if these documentations are accurate c:

I don't think I am capable enough to document the safety of `partition_blocks`, however.

Related issue rust-lang#66219
@LeSeulArtichaut LeSeulArtichaut added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 21, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 12, 2020
…e-slice, r=LukasKalbertodt

Document unsafety in library/core/src/slice/mod.rs

Restart where rust-lang#73555 left off, helping with rust-lang#66219.
@RalfJung
Copy link
Member

RalfJung commented Sep 4, 2020

I did maybe_uninit.rs as part of #76217, not realizing that this was still an ongoing effort.^^ I checked the box above.

yokodake added a commit to yokodake/rust that referenced this issue Oct 5, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 25, 2020
Document unsafety in core::slice::memchr

Contributes to rust-lang#66219

Note sure if that's good enough, especially for the `align_to` call.
The docs only mention transmuting and I don't think that everything related to reference lifetimes and state validity mentioned in the [nomicon](https://doc.rust-lang.org/nomicon/transmutes.html) are relevant here.
@LeSeulArtichaut LeSeulArtichaut removed their assignment Jun 2, 2021
@poliorcetics
Copy link
Contributor

With #87127, only some unsafe blocks in src/libcore/slice/sort.rs are left. I looked them over and I don't think I understand the algorithm well enough to make a SAFETY comment for them though

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 15, 2021
…cottmcm

Add safety comments in private core::slice::rotate::ptr_rotate function

Helps with rust-lang#66219.

`@rustbot` label C-cleanup T-compiler T-libs
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 15, 2021
…cottmcm

Add safety comments in private core::slice::rotate::ptr_rotate function

Helps with rust-lang#66219.

``@rustbot`` label C-cleanup T-compiler T-libs
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 15, 2021
…cottmcm

Add safety comments in private core::slice::rotate::ptr_rotate function

Helps with rust-lang#66219.

```@rustbot``` label C-cleanup T-compiler T-libs
LeSeulArtichaut added a commit to LeSeulArtichaut/rust that referenced this issue Aug 25, 2021
…Mark-Simulacrum

Add SAFETY comments to core::slice::sort::partition_in_blocks

A few more SAFETY comments for rust-lang#66219. There are still a few more in this module.

`@rustbot` label T-libs T-compiler C-cleanup
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 29, 2021
Remove ignore-tidy-undocumented-unsafe from core::slice::sort

Write down the missing safety arguments to be able to remove `ignore-tidy-undocumented-unsafe` from `core::slice::sort`.

Helps with rust-lang#66219

`@rustbot` label C-cleanup T-libs
ehuss added a commit to ehuss/rust that referenced this issue Sep 30, 2021
Remove ignore-tidy-undocumented-unsafe from core::slice::sort

Write down the missing safety arguments to be able to remove `ignore-tidy-undocumented-unsafe` from `core::slice::sort`.

Helps with rust-lang#66219

``@rustbot`` label C-cleanup T-libs
@mdsn
Copy link
Contributor

mdsn commented Sep 30, 2021

With #88412 merged I believe libstd is now free of // ignore-tidy-undocumented-unsafe which means this issue might be done.

@dtolnay dtolnay closed this as completed Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests