NLL: fix E0594 "change to mutable ref" suggestion

[ashtneoi]

Fix #51515.
Fix #51879.

Questions:

• Is this the right place to fix this? It feels brittle, being so close to the frontend. It's probably fine.
• Have I missed any other cases that trigger this behavior?
• Is it okay to use HELP and SUGGESTION in the UI test? Yes.
• Do I need more tests for this? No.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[ashtneoi]

I broke some tests, didn't I. Gimme a sec.

[rust-highfive]

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:51:24] ...i..............................................................................i.................
[00:51:30] ..................................................................................................FF
[00:51:35] ....................................................................................................
[00:51:41] ....................................................................................................
,"spans":[{"file_name":"/checkout/src/test/ui/rfc-2005-default-binding-mode/explicit-mut.rs","byte_start":828,"byte_end":835,"line_start":25,"line_end":25,"column_start":13,"column_end":20,"is_primary":true,"text":[{"text":"            *n += 1; //~ ERROR cannot assign to immutable","highlight_start":13,"highlight_end":20}],"label":"cannot mutate","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0594]: cannot assign to immutable item `*n`\n  --> /checkout/src/test/ui/rfc-2005-default-binding-mode/explicit-mut.rs:25:13\n   |\nLL |             *n += 1; //~ ERROR cannot assign to immutable\n   |             ^^^^^^^ cannot mutate\n\n"}
[00:51:47] {"message":"cannot assign to immutable item `*n`","code":{"code":"E0594","explanation":null},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/rfc-2005-default-binding-mode/explicit-mut.rs","byte_start":997,"byte_end":1004,"line_start":33,"line_end":33,"column_start":13,"column_end":20,"is_primary":true,"text":[{"text":"            *n += 1; //~ ERROR cannot assign to immutable","highlight_start":13,"highlight_end":20}],"label":"cannot mutate","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0594]: cannot assign to immutable item `*n`\n  --> /checkout/src/test/ui/rfc-2005-default-binding-mode/explicit-mut.rs:33:13\n   |\nLL |             *n += 1; //~ ERROR cannot assign to immutable\n   |             ^^^^^^^ cannot mutate\n\n"}
[00:51:47] {"message":"aborting due to 3 previous errors","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to 3 previous errors\n\n"}
[00:51:47] {"message":"For more information about this error, try `rustc --explain E0594`.","code":null,"level":"","spans":[],"children":[],"rendered":"For more information about this error, try `rustc --explain E0594`.\n"}
[00:51:47] ------------------------------------------
[00:51:47] 
[00:51:47] thread '[ui (nll)] ui/rfc-2005-default-binding-mode/explicit-mut.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3139:9
[00:51:47] 
---
[00:51:47] 
[00:51:47] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:498:22
[00:51:47] 
[00:51:47] 
[00:51:47] 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/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--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" "--compare-mode" "nll"
[00:51:47] 
[00:51:47] 
[00:51:47] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:51:47] Build completed unsuccessfully in 0:03:37
[00:51:47] Build completed unsuccessfully in 0:03:37
[00:51:47] make: *** [check] Error 1
[00:51:47] Makefile:58: recipe for target 'check' failed

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)

[ashtneoi]

Okay, I think this is ready for review. Sorry about that.

[nikomatsakis]

r? @nikomatsakis

[nikomatsakis]

r? @pnkfelix -- this may conflict with work that @pnkfelix has been doing, going to have them review

[pnkfelix]

Hmm I think I may rebase this atop #51275 myself, preserving authorship, just to get it integrated into that rewrite of check_access_permissions. The core idea, namely checking whether the extracted snippet starts with & or ref (including that crucial space character at the end), is a good one.

[ashtneoi]

Sounds good! Do you need anything from my end?

[ashtneoi]

Side note: I just realized I should be matching any whitespace character after ref, not just U+0020.

[ashtneoi]

https://github.com/ashtneoi/rust/tree/51515-missing-first-char-spaces

Here's my idea for properly matching the whitespace after ref. No idea whether it's useful. Let me know if you want me to do something with it.

[bors]

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

[ashtneoi]

Should I rebase onto master or merge from it?

[eddyb]

@ashtneoi Always rebase, for Rust PRs.

[ashtneoi]

Sorry, still working on the rebase---having some trouble finding where the "ref mut" part should go. I'll let y'all know if I can't figure it out.

[nikomatsakis]

@ashtneoi sorry for the radio silence here! @pnkfelix had an unexpected issue and hasn't been able to review PRs etc and I didn't realize this was here. I can try to help you rebase maybe in a bit, but if you want, please do drop in on Zulip where the NLL working group hangs out, maybe we can help?

[nikomatsakis]

in any case, i've assigned this to myself so that I will notice it on my daily PR sweeps =)

[ashtneoi]

@nikomatsakis Hey, thanks for the update. I was busy with other things today, but I'll throw some questions up on Zulip tomorrow if things still aren't making sense.

[nikomatsakis]

@ashtneoi ok, I read the code. It seems pretty reasonable. Regarding your questions:

Is this the right place to fix this? It feels brittle, being so close to the frontend.

Seems about right. The check to look at the source span... is probably good. It's hard to "construct" good suggestions without going back to the source bytes, ultimately, although it often feels a bit hokey.

Have I missed any other cases that trigger this behavior?

One thought I had was to check &self etc. Something like this:

struct Foo { x: u32 }

impl Foo {
  fn update(&self) {
    self.x += 1;
  }
}

Does this give a bad suggestion?

Is it okay to use HELP and SUGGESTION in the UI test?

Yes

Do I need more tests for this?

No

[nikomatsakis]

@ashtneoi can you verify that this also fixes #51879 ?

[ashtneoi]

Been kinda busy the past few days, but tomorrow (Wednesday) night I should have some time to look at #51879 and finish rebasing. #51275 changed some aspects of how we do "add mut" suggestions and this in particular was giving me trouble last time I looked at it. Once I figure out the code path for ref bindings the rest should be straightforward. Sorry for all the delays.

[ashtneoi]

Status report: I think I have most of a solution and I just need to glue everything together. I'll stop trying to fit everything into a single commit and just push what I have tomorrow (Monday).

[ashtneoi]

As suggested by @.pnkfelix on Zulip, I'm narrowing the scope to just issue #51515 (&mut oo).

[ashtneoi]

This also fixes #51879 (which means it's a dup of #51515).

[ashtneoi]

I'm not a huge fan of case (1.) here, but I can live with it, and more importantly I think this PR is done. Hopefully I learned my lesson about trying to do too much at once.

[ashtneoi]

Actually &self seem to trigger a different code path so never mind about that part of the test.

[pnkfelix]

@bors r+ rollup

[bors]

📌 Commit dc8ae26 has been approved by pnkfelix

[bors]

⌛ Testing commit dc8ae26 with merge e5f6498...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing e5f6498 to master...