Deny unsafe ops in unsafe fns in libcore #73622

LeSeulArtichaut · 2020-06-22T15:33:50Z

After liballoc, It's time for libcore :D

I planned to do this bit by bit to avoid having a big chunk of diffs, so to make reviews easier, and to make the unsafe blocks narrower and take the time to document them properly.

r? @nikomatsakis cc @RalfJung

RalfJung · 2020-06-22T15:35:19Z

src/libcore/alloc/global.rs

-            ptr::write_bytes(ptr, 0, size);
+            // SAFETY: as allocation succeeded, the region from `ptr`
+            // of size `size` is guaranteed to be valid for writes.
+            unsafe { ptr::write_bytes(ptr, 0, size) };


This is actually a great example where we do still do local unsafety reasoning inside an unsafe fn, instead of just forwarding requirements to the caller.

src/libcore/alloc/mod.rs

nikomatsakis

Holy detailed-unsafety-comments, Batman! This is good work, though to properly review is going to take some time and detailed reading :)

LeSeulArtichaut · 2020-06-23T13:18:41Z

This is good work, though to properly review is going to take some time and detailed reading :)

Which is why I wanted to do it bit by bit 😄 . Please take your time.
Maybe we can cc more people from the compiler/libs teams to read and check the safety comments?

nikomatsakis

I read over as best I could, admittedly somewhat skimming. I have one nit but also some meta-thoughts (that don't mean we shouldn't take the PR). Basically, what I found myself really wanting was a clearer way to say "this is a requirement we pass onto the caller" versus "this is an unsafe requirement we locally prove". I sort of wish we had distinct keywords for that or something like this.

src/libcore/cell.rs

bors · 2020-06-28T21:03:24Z

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

nikomatsakis · 2020-06-29T17:49:49Z

If we're going to land this, I'm inclined to land it and then land successive improvements to the comments as people notice them.

nikomatsakis · 2020-06-29T17:49:55Z

(I think we should land it)

LeSeulArtichaut · 2020-06-29T17:59:59Z

In that case shall I add the last unsafe blocks and slap some ignore-tidy-undocumented?
(I need to rebase it anyway)

LeSeulArtichaut · 2020-06-30T17:11:15Z

@nikomatsakis This is now ready for a final review, and hopefully merging!

And final part!!!

nikomatsakis · 2020-06-30T17:48:16Z

@LeSeulArtichaut CI seems unhappy

LeSeulArtichaut · 2020-06-30T17:58:33Z

Seems like some UI tests are failing. I really don't want to do a full compilation of the compiler for this though, tbh 😅
I'm going to try to bless the tests by hand, to further test my copy-pasting skills!

LeSeulArtichaut · 2020-06-30T19:52:44Z

CI should pass now.

nikomatsakis · 2020-07-01T15:58:02Z

src/libcore/iter/range.rs

@@ -189,12 +189,14 @@ macro_rules! step_identical_methods {
    () => {
        #[inline]
        unsafe fn forward_unchecked(start: Self, n: usize) -> Self {
-            start.unchecked_add(n as Self)
+            // SAFETY: the caller has to guarantee that `start + n` doesn't overflow.


Question: the vast majority of comments in these diffs take this form (i.e., "Caller must guarantee X"). Interestingly, usually in functions that have no associated comments, presumably because they are trait impls or in some other context where the comment is elsewhere.

Anyway, the question is -- do we think these comments make sense / add-value? Should we have some shorthand?

I think this comment is a good example where I feel there is added value -- i.e., this doesn't just say "some unsafe thing" but it is rather specific about what we are assuming.

In my opinion, a detailed safety comment is useful:

when the conditions that the caller must guarantee are only a subset of all the conditions required to call do the unsafe operation, because there are other invariants that we prove through other means. I think it is absolutely useful to have a detailed safety comments in these cases.

when only a part of what the caller must guarantee is used, because the other guarantees are used elsewhere in the function. In this case, I think safety comments are useful because they help readers understand where the safety contract comes from, which I think is useful for contributors, or even for any consumer of the standard library who wants to understand where constraints come from.

I'm not an experienced rustacean used to writing unsafe stuff though, and I probably have too few experience to have a good opinion about all of this.

nikomatsakis · 2020-07-01T16:02:46Z

src/libcore/ptr/const_ptr.rs

@@ -699,7 +707,8 @@ impl<T: ?Sized> *const T {
    where
        T: Sized,
    {
-        read_unaligned(self)
+        // SAFETY: the caller must uphold the safety contract for `read_unaligned`.


This is an example of a comment that doesn't add a lot of information, but I don't know that I would change it. =)

I added this kinds of comments mostly in functions where the call is actually delegated to another function, without any additional elements coming from the content of this function. Like:

unsafe fn do_x() { unsafe { actually_do_x() } }

In most of these cases, either the safety conditions are written in the doc comments for do_x because it is a "public interface" function, or there are no comments anywhere because it's an entirely internal function and that makes it really difficult to reason about safety contracts and I was unable to write more detailed comments.

nikomatsakis · 2020-07-01T16:08:13Z

@bors r+

Thought about it, I think we should move, we can always pare back comments later if we decide to do so.

bors · 2020-07-01T16:08:14Z

📌 Commit 6a7a652 has been approved by nikomatsakis

@nikomatsakis

…ikomatsakis Deny unsafe ops in unsafe fns in libcore After `liballoc`, It's time for `libcore` :D I planned to do this bit by bit to avoid having a big chunk of diffs, so to make reviews easier, and to make the unsafe blocks narrower and take the time to document them properly. r? @nikomatsakis cc @RalfJung

@ghost

…arth Rollup of 10 pull requests Successful merges: - rust-lang#73414 (Implement `slice_strip` feature) - rust-lang#73564 (linker: Create GNU_EH_FRAME header by default when producing ELFs) - rust-lang#73622 (Deny unsafe ops in unsafe fns in libcore) - rust-lang#73684 (add spans to injected coverage counters, extract with CoverageData query) - rust-lang#73812 (ast_pretty: Pass some token streams and trees by reference) - rust-lang#73853 (Add newline to rustc MultiSpan docs) - rust-lang#73883 (Compile rustdoc less often.) - rust-lang#73885 (Fix wasm32 being broken due to a NodeJS version bump) - rust-lang#73903 (Changes required for rustc/cargo to build for iOS targets) - rust-lang#73938 (Optimise fast path of checked_ops with `unlikely`) Failed merges: r? @ghost

rust-highfive assigned nikomatsakis Jun 22, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2020

RalfJung reviewed Jun 22, 2020

View reviewed changes

src/libcore/alloc/mod.rs Outdated Show resolved Hide resolved

LeSeulArtichaut mentioned this pull request Jun 22, 2020

#![deny(unsafe_op_in_unsafe_fn)] in libcore and libstd rust-lang/compiler-team#317

Closed

3 tasks

LeSeulArtichaut force-pushed the unsafe-libcore branch 2 times, most recently from 01c0283 to 59e7888 Compare June 22, 2020 16:43

nikomatsakis reviewed Jun 22, 2020

View reviewed changes

LeSeulArtichaut changed the title ~~Deny unsafe ops in unsafe fns~~ Deny unsafe ops in unsafe fns in libcore Jun 23, 2020

nikomatsakis reviewed Jun 24, 2020

View reviewed changes

src/libcore/cell.rs Outdated Show resolved Hide resolved

LeSeulArtichaut force-pushed the unsafe-libcore branch 2 times, most recently from cf17082 to 991d931 Compare June 25, 2020 18:19

LeSeulArtichaut added 5 commits June 30, 2020 16:42

Deny unsafe ops in unsafe fns, part 1

8ee1dec

Deny unsafe ops in unsafe fns, part 2

8a515e9

Deny unsafe ops in unsafe fns, part 3

ac7539c

Deny unsafe ops in unsafe fns, part 4

c68f478

Deny unsafe ops in unsafe fns, part 5

b365233

LeSeulArtichaut force-pushed the unsafe-libcore branch from 991d931 to b365233 Compare June 30, 2020 15:38

LeSeulArtichaut marked this pull request as ready for review June 30, 2020 17:10

LeSeulArtichaut force-pushed the unsafe-libcore branch from 1b7bc93 to 5ba33d1 Compare June 30, 2020 17:22

Deny unsafe ops in unsafe fns, part 6

a1623ff

And final part!!!

LeSeulArtichaut force-pushed the unsafe-libcore branch from 5ba33d1 to a1623ff Compare June 30, 2020 17:29

LeSeulArtichaut force-pushed the unsafe-libcore branch from 713c3ae to 800906d Compare June 30, 2020 18:36

Bless failing tests

6a7a652

LeSeulArtichaut force-pushed the unsafe-libcore branch from 800906d to 6a7a652 Compare June 30, 2020 19:03

LeSeulArtichaut mentioned this pull request Jun 30, 2020

#![deny(unsafe_op_in_unsafe_fn)] in libstd #73904

Closed

22 tasks

nikomatsakis approved these changes Jul 1, 2020

View reviewed changes

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 Jul 1, 2020

Manishearth mentioned this pull request Jul 1, 2020

Rollup of 16 pull requests #73939

Closed

Manishearth mentioned this pull request Jul 2, 2020

Rollup of 10 pull requests #73954

Merged

bors merged commit 500634b into rust-lang:master Jul 2, 2020

LeSeulArtichaut deleted the unsafe-libcore branch July 2, 2020 17:40

LeSeulArtichaut mentioned this pull request Aug 21, 2020

Use intra-doc-links in alloc #75725

Merged

workingjubilee mentioned this pull request Sep 8, 2020

Remove a stray ignore-tidy-undocumented-unsafe #76493

Merged

cuviper added this to the 1.46 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deny unsafe ops in unsafe fns in libcore #73622

Deny unsafe ops in unsafe fns in libcore #73622

LeSeulArtichaut commented Jun 22, 2020

RalfJung Jun 22, 2020

nikomatsakis left a comment

LeSeulArtichaut commented Jun 23, 2020

nikomatsakis left a comment

bors commented Jun 28, 2020

nikomatsakis commented Jun 29, 2020

nikomatsakis commented Jun 29, 2020

LeSeulArtichaut commented Jun 29, 2020 •

edited

Loading

LeSeulArtichaut commented Jun 30, 2020

nikomatsakis commented Jun 30, 2020

LeSeulArtichaut commented Jun 30, 2020

LeSeulArtichaut commented Jun 30, 2020

nikomatsakis Jul 1, 2020

LeSeulArtichaut Jul 1, 2020

nikomatsakis Jul 1, 2020

LeSeulArtichaut Jul 1, 2020

nikomatsakis commented Jul 1, 2020

bors commented Jul 1, 2020

Deny unsafe ops in unsafe fns in libcore #73622

Deny unsafe ops in unsafe fns in libcore #73622

Conversation

LeSeulArtichaut commented Jun 22, 2020

RalfJung Jun 22, 2020

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

LeSeulArtichaut commented Jun 23, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

bors commented Jun 28, 2020

nikomatsakis commented Jun 29, 2020

nikomatsakis commented Jun 29, 2020

LeSeulArtichaut commented Jun 29, 2020 • edited Loading

LeSeulArtichaut commented Jun 30, 2020

nikomatsakis commented Jun 30, 2020

LeSeulArtichaut commented Jun 30, 2020

LeSeulArtichaut commented Jun 30, 2020

nikomatsakis Jul 1, 2020

Choose a reason for hiding this comment

LeSeulArtichaut Jul 1, 2020

Choose a reason for hiding this comment

nikomatsakis Jul 1, 2020

Choose a reason for hiding this comment

LeSeulArtichaut Jul 1, 2020

Choose a reason for hiding this comment

nikomatsakis commented Jul 1, 2020

bors commented Jul 1, 2020

LeSeulArtichaut commented Jun 29, 2020 •

edited

Loading