New safe associated functions for PinMut #51730

MajorBreakfast · 2018-06-23T11:35:52Z

Add safe get_mut and map
Rename unsafe equivalents to get_mut_unchecked and map_unchecked

The discussion about this starts in this comment on the tracking issue.

rust-highfive · 2018-06-23T11:36:01Z

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

rust-highfive · 2018-06-23T11:41:31Z

The job x86_64-gnu-llvm-3.9 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.

[00:03:05]    Compiling cmake v0.1.30
[00:03:05]    Compiling alloc_jemalloc v0.0.0 (file:///checkout/src/liballoc_jemalloc)
[00:03:09]    Compiling std v0.0.0 (file:///checkout/src/libstd)
[00:03:11]    Compiling rustc_tsan v0.0.0 (file:///checkout/src/librustc_tsan)
[00:03:11] error[E0277]: the trait bound `T: marker::Unpin` is not satisfied in `option::Option<T>`
[00:03:11]     |
[00:03:11]     |
[00:03:11] 278 |             PinMut::get_mut(self).as_mut().map(|x| PinMut::new_unchecked(x))
[00:03:11]     |             ^^^^^^^^^^^^^^^ within `option::Option<T>`, the trait `marker::Unpin` is not implemented for `T`
[00:03:11]     |
[00:03:11]     = help: consider adding a `where T: marker::Unpin` bound
[00:03:11]     = note: required because it appears within the type `option::Option<T>`
[00:03:11] note: required by `<mem::PinMut<'a, T>>::get_mut`
[00:03:11]    --> libcore/mem.rs:1125:5
[00:03:11]     |
[00:03:11] 1125|     pub fn get_mut(this: PinMut<'a, T>) -> &'a mut T {
[00:03:11] 
[00:03:12]    Compiling rustc_msan v0.0.0 (file:///checkout/src/librustc_msan)
[00:03:13] error: aborting due to previous error
[00:03:13] 
[00:03:13] 
[00:03:13] For more information about this error, try `rustc --explain E0277`.
[00:03:13] error: Could not compile `core`.
[00:03:13] 
[00:03:13] Caused by:
[00:03:13]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name core libcore/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=2 -C metadata=e9cdce497aae9e81 -C extra-filename=-e9cdce497aae9e81 --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/release/deps` (exit code: 101)
[00:03:13] warning: build failed, waiting for other jobs to finish...
[00:03:22] error: build failed
[00:03:22] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:03:22] expected success, got: exit code: 101
[00:03:22] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1091:9
[00:03:22] travis_fold:end:stage0-std

[00:03:22] travis_time:end:stage0-std:start=1529754011872707736,finish=1529754036075484498,duration=24202776762


[00:03:22] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:22] Build completed unsuccessfully in 0:00:25
[00:03:22] make: *** [tidy] Error 1
[00:03:22] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:037f73c1
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0746202c:start=1529754036612463694,finish=1529754036620034512,duration=7570818
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:2d1defa0
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:24bf14ee
$ 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)

RalfJung · 2018-06-23T12:25:15Z

I'm pretty sure the map is unsound. The closure gets access to &mut T even though that T should be pinned.

I suggest to only change get_mut for now, and leave map for later (if we can find a useful safe version at all).

EDIT: Oh, there is a T: Unpin as well but it's at the beginning of the impl block. So this can be implemented in safe code. Then I don't see the point, TBH.

RalfJung · 2018-06-23T12:29:13Z

src/libstd/panic.rs

@@ -330,7 +330,7 @@ impl<'a, F: Future> Future for AssertUnwindSafe<F> {
    fn poll(mut self: PinMut<Self>, cx: &mut task::Context) -> Poll<Self::Output> {
        unsafe {
            let pinned_field = PinMut::new_unchecked(
-                &mut PinMut::get_mut(self.reborrow()).0
+                &mut PinMut::get_mut_unchecked(self.reborrow()).0


Couldn't this use PinMut::map_unchecked?

Jep. Good point. I changed it

MajorBreakfast · 2018-06-23T12:49:18Z

I've now removed safe map. I agree that we can always add it later if it turns out it has a use case.

RalfJung · 2018-06-23T13:00:06Z

src/libstd/panic.rs

-            pinned_field.poll(cx)
-        }
+        let pinned_field = unsafe { PinMut::map_unchecked(self, |x| x.0) };
+        pinned_field.poll()


Isn't the cx argument missing here?

^^' yes. Added it

rust-highfive · 2018-06-23T13:05:25Z

The job x86_64-gnu-llvm-3.9 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.

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)

bjorn3 · 2018-06-23T14:02:51Z

@TimNN ^ empty log

rust-highfive · 2018-06-23T16:07:07Z

The job x86_64-gnu-llvm-3.9 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.

[00:03:49]    Compiling panic_unwind v0.0.0 (file:///checkout/src/libpanic_unwind)
[00:03:52] error[E0308]: mismatched types
[00:03:52]    --> libstd/panic.rs:331:69
[00:03:52]     |
[00:03:52] 331 |         let pinned_field = unsafe { PinMut::map_unchecked(self, |x| x.0) };
[00:03:52]     |                                                                     ^^^ expected &mut _, found type parameter
[00:03:52]     = note: expected type `&mut _`
[00:03:52]                found type `F`
[00:03:52] 
[00:03:53] error: aborting due to previous error
[00:03:53] error: aborting due to previous error
[00:03:53] 
[00:03:53] For more information about this error, try `rustc --explain E0308`.
[00:03:53] error: Could not compile `std`.
[00:03:53] 
[00:03:53] Caused by:
[00:03:53]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name std libstd/lib.rs --color always --error-format json --crate-type dylib --crate-type rlib --emit=dep-info,link -C prefer-dynamic -C opt-level=2 --cfg feature="alloc_jemalloc" --cfg feature="backtrace" --cfg feature="jemalloc" --cfg feature="panic-unwind" --cfg feature="panic_unwind" -C metadata=fc6b9a3d7065b2e2 -C extra-filename=-fc6b9a3d7065b2e2 --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/release/deps --extern alloc=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/liballoc-d6cd5f8b78fddf12.rlib --extern rustc_msan=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/librustc_msan-ce1ae9dbeb02af61.rlib --extern libc=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/liblibc-223f966213a60acb.rlib --extern core=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libcore-e9cdce497aae9e81.rlib --extern std_unicode=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libstd_unicode-18039a1361938ca9.rlib --extern rustc_lsan=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/librustc_lsan-a2f6e9f263c82267.rlib --extern panic_unwind=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libpanic_unwind-2bd12fd5ac9768f9.rlib --extern unwind=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libunwind-bd02867d7573c11e.rlib --extern panic_abort=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libpanic_abort-5f4d07ea9b3edda4.rlib --extern compiler_builtins=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libcompiler_builtins-90a13cda2e54742f.rlib --extern alloc_system=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/liballoc_system-386e278237d725f5.rlib --extern rustc_tsan=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/librustc_tsan-ccd411645051f1ef.rlib --extern rustc_asan=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/librustc_asan-a72c0fa0b2d5877e.rlib --extern alloc_jemalloc=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/liballoc_jemalloc-5e6c2ac297f2c2e3.rlib -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/native/libbacktrace/ -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/native/libbacktrace -l static=backtrace -l static=backtrace -l dl -l rt -l pthread -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/build/compiler_builtins-ffd422941bf53e42/out -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/native/jemalloc/lib` (exit code: 101)
[00:03:53] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:03:53] expected success, got: exit code: 101
[00:03:53] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1091:9
[00:03:53] travis_fold:end:stage0-std

[00:03:53] travis_time:end:stage0-std:start=1529769942311724409,finish=1529769982834481818,duration=40522757409


[00:03:53] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:53] Build completed unsuccessfully in 0:00:41
[00:03:53] Makefile:79: recipe for target 'tidy' failed
[00:03:53] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1e25fbfc
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:01ee734c:start=1529769983320928768,finish=1529769983327250258,duration=6321490
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:042dfec0
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1f05ebfa
$ 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)

…ked` and `map_unchecked`

sfackler · 2018-06-23T17:16:28Z

r? @withoutboats

You're closer to the Pin stuff than me 😄

cramertj · 2018-06-23T17:28:05Z

The unsafe map function is pretty valuable, but I don't see much utility in a safe version with unpin bounds since you could write it using get_mut and new, so I personally think we should keep the name map rather than map_unchecked.

MajorBreakfast · 2018-06-24T08:28:49Z

I'm for calling it map_unchecked because IMO consistent naming is nicer than having a slightly shorter name. @withoutboats, @RalfJung what do you think?

RalfJung · 2018-06-24T09:31:22Z

I like map_unchecked because it indicates "be careful". The unsafe also does that, but this may be used in an unsafe block for different reasons (e.g. because it is an unsafe fn) in which case the user could not realize that map is unsafe.

cramertj · 2018-06-25T19:18:24Z

That makes sense. I was worried about the ergonomic cost of map_unchecked vs. map (which is a pretty easy unsafe invariant to get right), but things like the unsafe_pinned macro we've been using in futures mean that map is less common, and I like the benefit of having the unsafety clearly indicated.

@bors r+

bors · 2018-06-25T19:18:25Z

📌 Commit 3bcb85e has been approved by cramertj

withoutboats · 2018-06-25T19:35:10Z

@bors r-

I don't think this is beneficial; we already have DerefMut for PinMut<T> where T: Unpin.

withoutboats · 2018-06-25T19:40:00Z

Sorry, I'm on inflight wifi, loaded the tracking issue and see the lifetime difference now.

@bors r+

bors · 2018-06-25T19:40:02Z

📌 Commit 3bcb85e has been approved by withoutboats

…d, r=withoutboats New safe associated functions for PinMut - Add safe `get_mut` and `map` - Rename unsafe equivalents to `get_mut_unchecked` and `map_unchecked` The discussion about this starts [in this comment](rust-lang#49150 (comment)) on the tracking issue.

@ghost

Rollup of 11 pull requests Successful merges: - #51104 (add `dyn ` to display of dynamic (trait) types) - #51153 (Link panic and compile_error docs) - #51642 (Fix unknown windows build) - #51730 (New safe associated functions for PinMut) - #51731 (Fix ICEs when using continue as an array length inside closures (inside loop conditions)) - #51747 (Add error for using null characters in #[export_name]) - #51769 (Update broken rustc-guide links) - #51786 (Remove unnecessary stat64 pointer casts) - #51788 (Fix typo) - #51789 (Don't ICE when performing `lower_pattern_unadjusted` on a `TyError`) - #51791 (Minify css) Failed merges: r? @ghost

rust-highfive assigned sfackler Jun 23, 2018

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

MajorBreakfast mentioned this pull request Jun 23, 2018

Tracking issue for Pin APIs (RFC 2349) #49150

Closed

3 tasks

MajorBreakfast force-pushed the pin-get-mut-unchecked branch from befe49d to 537514a Compare June 23, 2018 11:50

RalfJung reviewed Jun 23, 2018

View reviewed changes

MajorBreakfast force-pushed the pin-get-mut-unchecked branch from 537514a to 35178a8 Compare June 23, 2018 12:48

MajorBreakfast force-pushed the pin-get-mut-unchecked branch from 35178a8 to 75509b8 Compare June 23, 2018 12:51

RalfJung reviewed Jun 23, 2018

View reviewed changes

MajorBreakfast force-pushed the pin-get-mut-unchecked branch from 75509b8 to e352d6c Compare June 23, 2018 16:01

PinMut: Add safe get_mut and rename unsafe fns to `get_mut_unchec…

3bcb85e

…ked` and `map_unchecked`

MajorBreakfast force-pushed the pin-get-mut-unchecked branch from e352d6c to 3bcb85e Compare June 23, 2018 16:30

rust-highfive assigned withoutboats and unassigned sfackler Jun 23, 2018

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 Jun 25, 2018

cramertj assigned cramertj and unassigned withoutboats Jun 25, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 25, 2018

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 25, 2018

pietroalbini mentioned this pull request Jun 26, 2018

Rollup of 11 pull requests #51805

Merged

bors merged commit 3bcb85e into rust-lang:master Jun 26, 2018

ngg mentioned this pull request Jun 29, 2018

0.2: Fix compilation when "nightly" feature is enabled rust-lang/futures-rs#1056

Merged

New safe associated functions for PinMut #51730

New safe associated functions for PinMut #51730

Uh oh!

Conversation

MajorBreakfast commented Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jun 23, 2018

Uh oh!

rust-highfive commented Jun 23, 2018

Uh oh!

RalfJung commented Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung Jun 23, 2018

Choose a reason for hiding this comment

Uh oh!

MajorBreakfast Jun 23, 2018

Choose a reason for hiding this comment

Uh oh!

MajorBreakfast commented Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung Jun 23, 2018

Choose a reason for hiding this comment

Uh oh!

MajorBreakfast Jun 23, 2018

Choose a reason for hiding this comment

Uh oh!

rust-highfive commented Jun 23, 2018

Uh oh!

bjorn3 commented Jun 23, 2018

Uh oh!

rust-highfive commented Jun 23, 2018

Uh oh!

sfackler commented Jun 23, 2018

Uh oh!

cramertj commented Jun 23, 2018

Uh oh!

MajorBreakfast commented Jun 24, 2018

Uh oh!

RalfJung commented Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cramertj commented Jun 25, 2018

Uh oh!

bors commented Jun 25, 2018

Uh oh!

withoutboats commented Jun 25, 2018

Uh oh!

withoutboats commented Jun 25, 2018

Uh oh!

bors commented Jun 25, 2018

Uh oh!

Uh oh!

MajorBreakfast commented Jun 23, 2018 •

edited

Loading

RalfJung commented Jun 23, 2018 •

edited

Loading

MajorBreakfast commented Jun 23, 2018 •

edited

Loading

RalfJung commented Jun 24, 2018 •

edited

Loading