NLL: Suggest ref mut and &mut self
#52242
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
src/librustc_mir/borrow_check/mod.rs
Outdated
|
|
||
| Some(ClearCrossCrate::Clear) => bug!("saw cleared local state"), | ||
|
|
||
| None => bug!(), |
There was a problem hiding this comment.
Not really sure what the most idiomatic way to do this is. I could add .as_ref().unwrap() on line 1844, but that felt kinda ugly.
| ) -> (Span, String) { | ||
| // This implementation attempts to emulate AST-borrowck prioritization | ||
| // by trying (3.), then (2.) and finally falling back on (1.). | ||
| let locations = mir.find_assignments(local); | ||
| if locations.len() > 0 { | ||
| let assignment_rhs_span = mir.source_info(locations[0]).span; | ||
| let snippet = tcx.sess.codemap().span_to_snippet(assignment_rhs_span); | ||
| if let Ok(src) = snippet { |
There was a problem hiding this comment.
It's not clear to me how span_to_snippet could fail, but I left the fallthrough alone just in case.
There was a problem hiding this comment.
(instead of unwrapping the result)
|
cc @pnkfelix if you're interested |
|
@csmoe Does this look right? I rebased your branch onto mine and updated the tests. |
|
Or did you want me to factor out the |
|
@ashtneoi yep, I wanna introduce your |
|
@csmoe Ok, I think I did it. Also fixed that "use a mutable reference instead: |
|
The job 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 |
Specifically, `&self` -> `&mut self` and explicit `ref` -> `ref mut`. Implicit `ref` isn't handled yet and causes an ICE.
Also teach rustc_borrowck not to show useless help messages like "use a mutable reference instead: `x`".
ashtneoi
force-pushed
the
suggest-ref-mut
branch
from
ebf281e
to
1ed8619
Compare
|
Weird, I had tried something similar to your changes here, but I kept hitting a problem where it was making erroneous suggestions for mutations of references in arm guards. In particular for the test issue-27282-reborrow-ref-mut-in-guard.rs, my version of this code would try to suggest changing the But there is (by design) actually no way for the user to change So I had been spending time putting in some infrastructure to be able to filter this case. But it seems like your PR here does not run into that problem. I have not managed to figure out how you have managed to avoid it. heh.
|
|
So, the approach I was planning to use wasn't going to have the In particular, if you look at my versions of tests like rfc-2005-default-binding-mode/enum.nll.stderr and rfc-2005-default-binding-mode/explicit-mut.nll.stderr, I have managed to provide suggestions for bindings like Having said that, there is something buggy in my own approach (which is why I have not put up a PR for it yet) and your approach is clearly fixing the bug in question and helping us bring NLL closer to parity with AST-borrowck in diagnostic quality. So I am going to r+ your PR which will let me put my own branch on the back-burner. |
|
@bors r+ |
|
📌 Commit 1ed8619 has been approved by |
bors
added
S-waiting-on-bors
NLL: Suggest `ref mut` and `&mut self` Fixes rust-lang#51244. Supersedes rust-lang#51249, I think. Under the old lexical lifetimes, the compiler provided helpful suggestions about adding `mut` when you tried to mutate a variable bound as `&self` or (explicit) `ref`. NLL doesn't have those suggestions yet. This pull request adds them. I didn't bother making the help text exactly the same as without NLL, but I can if that's important. (Originally this was supposed to be part of rust-lang#51612, but I got bogged down trying to fit everything in one PR.)
Rollup of 16 pull requests Successful merges: - #51962 (Provide llvm-strip in llvm-tools component) - #52003 (Implement `Option::replace` in the core library) - #52156 (Update std::ascii::ASCIIExt deprecation notes) - #52242 (NLL: Suggest `ref mut` and `&mut self`) - #52244 (Don't display default generic parameters in diagnostics that compare types) - #52290 (Deny bare trait objects in src/librustc_save_analysis) - #52293 (Deny bare trait objects in librustc_typeck) - #52299 (Deny bare trait objects in src/libserialize) - #52300 (Deny bare trait objects in librustc_target and libtest) - #52302 (Deny bare trait objects in the rest of rust) - #52310 (Backport 1.27.1 release notes to master) - #52314 (Fix ICE when using a pointer cast as array size) - #52315 (Resolve FIXME(#27942)) - #52316 (task: remove wrong comments about non-existent LocalWake trait) - #52322 (Update llvm-rebuild-trigger in light of LLVM 7 upgrade) - #52332 (dead-code lint: say "constructed", "called" for structs, functions) Failed merges: r? @ghost
NLL: Suggest `ref mut` and `&mut self` Fixes #51244. Supersedes #51249, I think. Under the old lexical lifetimes, the compiler provided helpful suggestions about adding `mut` when you tried to mutate a variable bound as `&self` or (explicit) `ref`. NLL doesn't have those suggestions yet. This pull request adds them. I didn't bother making the help text exactly the same as without NLL, but I can if that's important. (Originally this was supposed to be part of #51612, but I got bogged down trying to fit everything in one PR.)
|
☀️ Test successful - status-appveyor, status-travis |
|
@pnkfelix As I understand it, the check for ...by just returning Some instead of None from |
|
Hmm I had thought, beyond the problem you describe, that there was also an issue involving But its possible my memory is faulty. Update: Yes my memory was just faulty. The cases I was thinking needed to be addressed are actually handled here. We certainly could consider trying to also do something with implicit ref bindings, but I don't think we need to worry about that case right now. |
Fixes #51244. Supersedes #51249, I think.
Under the old lexical lifetimes, the compiler provided helpful suggestions about adding
mutwhen you tried to mutate a variable bound as&selfor (explicit)ref. NLL doesn't have those suggestions yet. This pull request adds them.I didn't bother making the help text exactly the same as without NLL, but I can if that's important.
(Originally this was supposed to be part of #51612, but I got bogged down trying to fit everything in one PR.)