Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NLL fails to suggest "try removing &mut here" #51191

Closed
2 of 3 tasks
pnkfelix opened this issue May 29, 2018 · 6 comments
Closed
2 of 3 tasks

NLL fails to suggest "try removing &mut here" #51191

pnkfelix opened this issue May 29, 2018 · 6 comments
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal

Comments

@pnkfelix
Copy link
Member

pnkfelix commented May 29, 2018

For the following tests, AST-borrowck suggests removing &mut, while NLL fails to make such a suggestion:

@estebank estebank added the NLL-diagnostics Working towards the "diagnostic parity" goal label May 29, 2018
@pnkfelix pnkfelix added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll labels May 29, 2018
@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 4, 2018

We might be able to incorporate a fix for this into #51275 ; I haven't quite dug into how we detect this case yet. But its worth at least looking into as part of that PR.

@pnkfelix
Copy link
Member Author

(at this point I have indeed incorporated a fix for this into #51275 )

@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 15, 2018

(except that my fix, in the case of did_you_mean/issue-31424.rs, is suggesting to the user that they write mut self rather than suggesting to the user that they remove the &mut borrow in the method call receiver. Not sure whether that is worth addressing. Probably shouldn't close this issue without at least looking into that.

@pnkfelix pnkfelix self-assigned this Jun 19, 2018
bors added a commit that referenced this issue Jun 19, 2018
…ermissions, r=nikomatsakis

NLL diagnostics: revise `fn check_access_permissions`

NLL: revise `fn check_access_permissions` so that its (still branchy) shares more code paths between the different cases, and also provide more diagnostics in more cases (though the added diagnostics still do not always meet the quality bar established by AST-borrowck)

----

Transcribing "checklist" suggested by Niko, except I am rendering it as a table to make it clear that I do not regard every item in the list to be a "must have" for landing this PR.

goal | does this PR do it?
-----|------------------------------
no suggestions for `ref mut` |  yes
suggestions for direct local assignment (`{ let x = 3; x = 4; }`) | yes (see commits at end)
suggestions for direct field assignment (`{ let x = (3, 4); x.0 = 5; }` | yes (see commits at end)
suggestions for upvars (`let x = 3; let c = \|\| { &mut x; }`) | yes

Note that I added support for a couple of rows via changes that are not strictly part of `fn check_access_permissions`. If desired I can remove those commits from this PR and leave them for a later PR.

Fix #51031
Fix #51032
(bug #51191 needs a little more investigation before closing.)
Fix #51578
@nikomatsakis
Copy link
Contributor

To clarify the current situation. For issue-31424.rs, the AST-based checker gives this error:

error[E0596]: cannot borrow immutable argument `self` as mutable
  --> src/main.rs:19:15
   |
19 |         (&mut self).bar(); //~ ERROR cannot borrow
   |               ^^^^
   |               |
   |               cannot reborrow mutably
   |               try removing `&mut` here

and the NLL one gives this one:

error[E0596]: cannot borrow immutable item `self` as mutable
  --> src/main.rs:19:9
   |
19 |         (&mut self).bar(); //~ ERROR cannot borrow
   |         ^^^^^^^^^^^ cannot borrow as mutable

Note that the hint is missing from the NLL one.

@nikomatsakis
Copy link
Contributor

This doesn't seem to me to rise to the cut of a Edition Preview blocker, or even a RC blocker.

@nikomatsakis nikomatsakis added this to the Rust 2018 Release milestone Jun 29, 2018
@nikomatsakis
Copy link
Contributor

I'll tag with the Release milestone for now, though I think it could plausibly be cut.

@pnkfelix pnkfelix removed their assignment Aug 3, 2018
@davidtwco davidtwco self-assigned this Sep 30, 2018
bors added a commit that referenced this issue Oct 3, 2018
NLL fails to suggest "try removing `&mut` here"

Fixes #51191.

This PR adds ``try removing `&mut` here`` suggestions to functions where a mutable borrow is being taken of a `&mut self` or a `self: &mut Self`. This PR also enables the suggestion for adding a `mut` pattern to by-value implicit self arguments without `mut` patterns already.

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal
Projects
None yet
Development

No branches or pull requests

4 participants