-
Notifications
You must be signed in to change notification settings - Fork 14k
Fix variable does not need to be mutable warning #54621
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
Conversation
src/test/ui/nll/issue-54499.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems totally wrong :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh, wrong copy & paste
src/librustc_mir/borrow_check/mod.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just remove this field altogether now?
9ac4832 to
257efad
Compare
This comment has been minimized.
This comment has been minimized.
src/librustc_mir/borrow_check/mod.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it appears I was a bit aggressive in suggesting that this code be cut entirely. Rather, it should be modified. The goal here is to say: "if we see a = 22 and a was previously initialized, then the mut is needed". That is correct.
The problem here is that the actual assignment was to a.0 and — when a is not initialized — we only permit that if a is declared mutable.
I think that to fix this code we probably need to modify add_used_mut so that it knows both the root_place (as now) and also the place that the user was originally writing to; in that case, we could distinguish between a = 5 (which may not need mut, if a was not previously initialized) and a.0 (which always does).
Annoying.
|
Left more detailed notes on Zulip |
43a346d to
1c74f4a
Compare
src/librustc_mir/borrow_check/mod.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we want a comment here, but seems good.
1c74f4a to
a410705
Compare
This comment has been minimized.
This comment has been minimized.
a410705 to
503da10
Compare
This comment has been minimized.
This comment has been minimized.
503da10 to
425a1de
Compare
This comment has been minimized.
This comment has been minimized.
622327a to
c05bb2b
Compare
nikomatsakis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Left some nits
src/librustc_mir/borrow_check/mod.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment:
FIXME(#21232). For now, error if assigning to a projection from an uninitialized local variable -- so e.g. doing a.b = 22 when a is not yet initialized is simply an error. In the future, we could sometimes support this.
src/librustc_mir/borrow_check/mod.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment:
Pre-emptively insert local into the used_mut set to avoid any warnings related to whether the mut declaration is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to check whether local is actually declared as mut, something like
if let Mutability::Mut = self.mir.local_decls[local].mutability {
self.used_mut.insert(local);
}
This comment has been minimized.
This comment has been minimized.
c05bb2b to
4603111
Compare
nikomatsakis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit
|
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 |
4603111 to
b7eb35d
Compare
|
@nikomatsakis we don't want these kind of errors 56011eb#diff-27087a37a3701eb55da69c6420fb0fa8R1 Need to figure out how to get rid of the error when there's already a move error. |
| error[E0718]: cannot assign to `src.next` when `src` is not initialized | ||
| --> $DIR/borrowck-issue-48962.rs:26:5 | ||
| | | ||
| LL | src.next = None; //~ ERROR use of moved value: `src` [E0382] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I've already chatted with @spastorino about this; we can avoid this (IMO redundant/unnecessary) error if we revise the strategy slightly. PR #54941 has some hidden suggestions )
|
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 |
|
I'm closing this given @pnkfelix is taking care of this now. |
Fixes #54499
r? @nikomatsakis