-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Fix variable does not need to be mutable warning #54621
Conversation
src/test/ui/nll/issue-54499.rs
Outdated
// requirement that `T: 'static`. This arose because we actually had | ||
// to propagate both that `T: 'a` but also `T: 'b` where `'b` is the | ||
// higher-ranked lifetime that appears in the type of the closure | ||
// parameter `x` -- since `'b` cannot be expressed in the caller's |
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
) { | ||
match root_place { | ||
RootPlace { | ||
place: Place::Local(local), | ||
is_local_mutation_allowed, | ||
is_local_mutation_allowed: _, |
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
if flow_state.ever_inits.contains(index) { | ||
self.used_mut.insert(*local); | ||
break; | ||
} |
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
self.used_mut.insert(*local); | ||
break; | ||
match accessed_place { | ||
// FIXME(#21232) |
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
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
|
||
} | ||
|
||
if let &Place::Projection(_) = place { |
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
if let Some(local) = place.base_local() { | ||
let mpi = self.move_data.rev_lookup.find_local(local); | ||
if flow_state.uninits.contains(mpi) { | ||
self.used_mut.insert(local); |
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
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