Desugar fn(&mut self) to fn(mut self: &mut self)

[estebank]

In order to improve ergonomics, desugar &mut self to a mutable binding instead of an immutable binding, allowing people to re-borrow self as mutable borrow in the body of the method.

This implements the suggestion in https://github.com/rust-lang/rust/pull/47818/files#r164512390, which allows rustc to start accepting the code in #47774.

r? @nikomatsakis
CC @rust-lang/compiler @rust-lang/lang

Creating the PR in order to coalesce the impact and FCP conversation around the code changes themselves.

[scottmcm]

Silly question: Does this mean I can now accidentally do this?

fn foo(&mut self, other: &mut Self) {
    self = other;
}

[rust-highfive]

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.

Resolving deltas: 100% (615987/615987), completed with 4901 local objects.
---
[00:01:00] configure: rust.quiet-tests     := True
---
[00:49:19] .................................................................................i..................
[00:49:26] ........................i...........................................................................
---
[00:50:09] .......................i...........................................................................i
[00:50:16] ....................................................................................................
[00:50:22] .............ii.....................................................................................
---
[00:51:11] .............................................i......................................................
---
[00:55:22] ..............................i.....................................................................
[00:55:37] ...............................................................i....................................
[00:55:53] .................................................i..................................................
[00:56:14] ....................................................................................................
[00:56:37] ....................................................................................................
[00:56:59] ....................................................................................................
[00:57:25] .......i............................................................................................
[00:57:52] ...i.........................................................................test [run-pass] run-pass/mir_heavy_promoted.rs has been running for over 60 seconds
[00:58:04] .......................
[00:58:38] ....................................................................................................
[00:59:12] .....................................................................ii.............................
[01:00:02] ................................i..........................................test [run-pass] run-pass/saturating-float-casts.rs has been running for over 60 seconds
[01:00:11] ..........i.ii...........
[01:00:58] .............................................................................................iiiiiii
---
[01:03:16] ...................i............................................................ii.iii..............
[01:03:23] ....................................................................................................
[01:03:31] ........i..............................i............................................................
[01:03:39] ....................................................................................................
[01:03:46] ..........i.........................................................................................
[01:03:54] ....................................................................................................
[01:04:04] ....................................................................................................
[01:04:14] ....................................................................................................
[01:04:25] ....................................................................................................
[01:04:38] ...........................................................................F........................
[01:04:47] ...i................................................................................................
[01:04:57] .......i..ii........................................................................................
[01:05:07] ....................................................................................................
[01:05:16] ....................................................................................................
[01:05:26] .........................................................................i..........................
[01:05:36] ...................i................................................................................
---
[01:06:09] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/compile-fail/lint-unused-mut-self.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/lint-unused-mut-self.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/lint-unused-mut-self.stage2-x86_64-unknown-linux-gnu.aux" "-A" "unused"
---
[01:06:09] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/compile-fail" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "compile-fail" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-3.9/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "3.9.1\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:06:09] expected success, got: exit code: 101
[01:06:09]
[01:06:09]
[01:06:09] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:06:09] Build completed unsuccessfully in 0:18:10
[01:06:09] make: *** [check] Error 1
[01:06:09] Makefile:58: recipe for target 'check' failed
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:185bf6a7:start=1523935215501290378,finish=1523935215517011285,duration=15720907
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:00d6cdad
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:00d6cdad:start=1523935215522558883,finish=1523935215529353993,duration=6795110
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0da1ce44
$ dmesg | grep -i kill
[   11.269050] init: failsafe main process (1095) killed by TERM signal

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)

[estebank]

@scottmcm correct

[cramertj]

Our story around "immutably bound mutable references", "mutably bound immutable references", "immutably bound DerefMut types" etc. is already pretty confusing, and IMO this further confounds the issue by allowing people to assign to the self reference without actually mutating self (after which point self no longer refers to the method receiver). If you really need this behavior, you can say let mut this = self; and then set this = other.

Edit: or you can write mut self: &mut Self as @petrochenkov suggested.

[petrochenkov]

If you really need this behavior, you can say let mut this = self; and then set this = other.

Or you can actually write fn foo(mut self: &mut Self) { ... } manually, it works.

[petrochenkov]

Fun fact: we'd need this much more often if not the secret closure borrowing mode UniqueImmBorrow!
I made a little experiment showing the difference back in 2016 - https://github.com/petrochenkov/rust/commits/nouniq.

[Centril]

In any case, if we want to do this change, it should first go through the RFC process IMO :)

[TimNN]

@rust-lang/lang, @rust-lang/compiler: How do you want to move forward with this? Should this go through the RFC process? Or just an FCP here?

[joshtriplett]

I think there was already an RFC that proposed having additional types for self in methods; would it make sense to check that one before filing a new one?

[estebank]

I would like to hear from @nikomatsakis on his thoughts. I've come around to maybe only provide a suggestion for fn foo(mut self: &mut Self) { ... }, so as to avoid the already mentioned drawbacks for this PR.

[nikomatsakis]

Hmm. I agree that permitting self = other seems like an unfortunate side effect. I'm not sure that I think suggesting that people write mut self: &mut Self makes sense, since I suspect that in most cases people probably don't have to write &mut at all -- and/or they should write *mut *self.

[nikomatsakis]

Seems like we are not going to do this, right?

[estebank]

@nikomatsakis If we don't (which I also lean towards), we should provide a suggestion. If we do, what should we suggest then?

[arielb1]

@estebank

We could try to detect if the program only needs to use &mut *self (instead of &mut self) - i.e. whether all the accesses of r start with a deref - and in that case, suggest using &mut *self.

That is a little bit ugly to do in the HIR, but doable. It should be easier in MIR, which is all for the better, given that we are moving to MIR borrowck.

[nikomatsakis]

I like @arielb1's suggestion. Indeed, I suspect that often just self would do..

[estebank]

@nikomatsakis the problem with suggesting passing ownership is that in order to not be misleading we would need to verify the usage of Self in every scope the method is called in order to not suggest it when the callers expect to retain ownership. I'm thinking about calls to foo(&mut self) and bar(&self) in the same scope, if we suggest changing foo to foo(self), then the user will have to fix multiple things. Because of this, even though suggesting self would be appropriate in many cases, &mut *self is probably the better option.

[nikomatsakis]

@estebank yes, sounds reasonable. Shall we close this PR?