NLL diagnostics: revise fn check_access_permissions
#51275
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
pnkfelix
added
S-waiting-on-author
nikomatsakis
reviewed
Jun 1, 2018
src/librustc_mir/borrow_check/mod.rs
Outdated
| } | ||
| } | ||
|
|
||
| if note_the_place_err && place != the_place_err { |
There was a problem hiding this comment.
I personally would remove this message. If we can identify such a place, we should suggest how to change it to mutable. Otherwise, I suspect it will confuse users as much as anything.
There was a problem hiding this comment.
My plan is indeed to remove this message once we’ve added all the diagnostic cases of interest
src/librustc_mir/borrow_check/mod.rs
Outdated
| // ... but it doesn't make sense to suggest it on | ||
| // variables that are already `ref x` or `ref mut x` | ||
| // (such variables are simply not mutable). | ||
| if let Some(ty::BindingMode::BindByValue(_)) = |
There was a problem hiding this comment.
In such cases, often the user should have written &mut *foo instead of &mut foo — maybe we can think how to suggest that. (I've seen this before...)
src/librustc_mir/borrow_check/mod.rs
Outdated
| // (such variables are simply not mutable). | ||
| if let Some(ty::BindingMode::BindByValue(_)) = | ||
| self.mir.local_decls[*local].is_user_variable { | ||
| true |
There was a problem hiding this comment.
What about upvars? We should test this example:
let x = (3, );
let c = || x.0 += 1;
and see that we issue a suggestion here to make it let mut x
There was a problem hiding this comment.
Yeah, if #51031 does not currently include any examples of upvars, then we should make sure that those cases are being tested... I know I haven't done anything special to account for them ... yet!
3 tasks
|
This check-list is from #51260 — some of these cases I think you handle, but not all...
|
|
I want to preserve this comment from @nikomatsakis somewhere that won't become outdated by updated to the commit series:
|
This was referenced Jun 4, 2018
|
Current state is closer but still WIP. In particular:
|
|
(Oh and clearly I will need to rebase too. Which is just as well because I also want to refactor the commit series before actually submitting them for formal review.) |
|
So I have some prototype commits that attempt to thread information about explicitly provided types, when present, down into the This does seem to be workable in terms of improving the diagnostics for NLL. But something is wrong with my approach because I think something I did to the compiler data structure has caused the dep-graph tests in I'm going to push the commits in case @nikomatsakis has a chance to peek at them. But I also assume that he won't really have any time before I get a chance to start fresh on this tomorrow. |
|
Ah i just realized: I bet the problem in the commits I pushed is that I added NodeId’s to the state of the HIR and MIR, and those won’t be stable in the face of other changes to the crate, which messes up DepInfo (right?) Luckily I don’t need the NodeId; I just had it there in case I wanted to try to use it for something. (Still, I am curious about what I should be inserting here to capture this state. Or maybe I should just deliberately omit the NodeId from the implementations of things like hash_stable? |
|
Okay I've figured out how to use |
|
I think I'm going to make one last push to see if I can thread down the information about the initializer expression span in something like Once that experiment is done (whether it works or not), I'll switch to focusing on cleaning up this PR and getting it ready for review. |
pnkfelix
force-pushed
the
nll-diagnostics-revise-check-access-permissions
branch
2 times, most recently
from
45ec3c0
to
557f946
Compare
|
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 |
pnkfelix
force-pushed
the
nll-diagnostics-revise-check-access-permissions
branch
from
2a1ceb8
to
ce9988a
Compare
|
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 |
pnkfelix
force-pushed
the
nll-diagnostics-revise-check-access-permissions
branch
from
ce9988a
to
cb65ef1
Compare
|
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 |
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
|
Legit. Needs to update a UI test result. |
kennytm
added
S-waiting-on-author
…ypes, down into `mir::LocalDecls`. As a drive-by: the ref_for_guards created by `fn declare_binding` should not have been tagged as user_variables in the first place. These secret internal locals are *pointers* to user variables, but themselves are not such (IMO. For now at least.)
…g more fields in future.
Namely, we thread down the `HirId` of the explicit type of the argument. In the case of the special `self` variable with an implicit type, we also thread down a description of its structure (`self`/`mut self`/`&self`/`&mut self`).
… control flow. As a drive-by, removed some dead-code.
Tried to unify various common code paths and also vaguely approximate the AST-borrowck diagnostics. The change in (subjective) quality of diagnostics is not a universal improvement. But I think this is a better code base to work from for future fixes.
(Follow-on commits updating the test suite show the resulting changes to diagnostic output.)
…tests (since I made this mistake at first but the tests didn't catch it): we should not suggest adding `mut` to a reassigned `ref` or `ref mut` binding. (The Rust language, since at least 1.0, does not have `mut ref mut` or `ref mut mut` etc.)
pnkfelix
force-pushed
the
nll-diagnostics-revise-check-access-permissions
branch
from
032eee1
to
4684649
Compare
|
@bors r=nikomatsakis |
|
📌 Commit 4684649 has been approved by |
bors
added
S-waiting-on-bors
4 tasks
bors
added a commit
that referenced
this pull request
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
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NLL: revise
fn check_access_permissionsso 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.
ref mut{ let x = 3; x = 4; }){ let x = (3, 4); x.0 = 5; }let x = 3; let c = || { &mut x; })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