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.

Sign up for GitHub

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

Jump to bottom

NLL diagnostics miss opportunities to suggest "use &mut Type here to make mutable" #51032

Closed
9 tasks
pnkfelix opened this issue May 24, 2018 · 1 comment
Closed
9 tasks

NLL diagnostics miss opportunities to suggest "use &mut Type here to make mutable" #51032

pnkfelix opened this issue May 24, 2018 · 1 comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal

Comments

@pnkfelix
Copy link
Member

pnkfelix commented May 24, 2018 •
edited
Loading

The following tests rightly suggest under AST borrowck to use a &mut reference in the type, but there is no such suggestion under NLL.

Tests:

  • did_you_mean/issue-38147-1.rs
  • did_you_mean/issue-38147-2.rs
  • did_you_mean/issue-38147-3.rs
  • did_you_mean/issue-38147-4.rs
  • span/borrowck-call-is-borrow-issue-12224.rs
  • span/borrowck-call-method-from-mut-aliasable.rs
  • span/borrowck-fn-in-const-b.rs
  • span/borrowck-object-mutability.rs
  • span/mut-arg-hint.rs

(This list of tests is drawn from an informal paper document that I have been using to keep notes for myself as I work on this...)
@pnkfelix pnkfelix added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll NLL-diagnostics Working towards the "diagnostic parity" goal labels May 24, 2018
@estebank estebank added the A-diagnostics Area: Messages for errors, warnings, and lints label May 25, 2018
@pnkfelix pnkfelix mentioned this issue May 31, 2018
Closed
8 tasks
@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 4, 2018

I believe this will be addressed by PR #51275 once that is finalized.

@pnkfelix pnkfelix mentioned this issue Jun 18, 2018
Merged
bors added a commit that referenced this issue Jun 19, 2018
@bors
Auto merge of #51275 - pnkfelix:nll-diagnostics-revise-check-access-p…
f28c7ae 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@pnkfelix @estebank