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

Invalid "variable does not need to be mutable" warning in 2018 edition with partially initialized structs #54499

Closed
KamilaBorowska opened this issue Sep 23, 2018 · 5 comments
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal

Comments

@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Sep 23, 2018

fn main() {
    let mut s: (i32, i32);
    s.0 = 3;
    s.1 = 4;
    println!("{} {}", s.0, s.1);
}

Causes the following warning:

warning: variable does not need to be mutable
 --> src/main.rs:2:9
  |
2 |     let mut s: (i32, i32);
  |         ----^
  |         |
  |         help: remove this `mut`
  |
  = note: #[warn(unused_mut)] on by default

Removing mut causes an error:

error[E0594]: cannot assign to `s.0`, as `s` is not declared as mutable
 --> src/main.rs:3:5
  |
2 |     let s: (i32, i32);
  |         - help: consider changing this to be mutable: `mut s`
3 |     s.0 = 3;
  |     ^^^^^^^ cannot assign

error[E0594]: cannot assign to `s.1`, as `s` is not declared as mutable
 --> src/main.rs:4:5
  |
2 |     let s: (i32, i32);
  |         - help: consider changing this to be mutable: `mut s`
3 |     s.0 = 3;
4 |     s.1 = 4;
  |     ^^^^^^^ cannot assign

error: aborting due to 2 previous errors

Tested on nightly on the playground. This code is stupid, but I don't think it should report "unused mut" warnings.

@matthewjasper matthewjasper added A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal labels Sep 23, 2018
@eddyb
Copy link
Member

eddyb commented Sep 23, 2018

I think the error (that partial initialization requires mut) is wrong, or at least surprising.
cc @nikomatsakis @pnkfelix

@nikomatsakis
Copy link
Contributor

So, we decided to echo existing borrowck behavior here, which does require a mut for.. no particularly good reason (this is #21232, a long-standing issue). I did this as a performance optimization in #53258 with the intention that we would expand the handling of immutable variables at some later point.

The simplest thing would be to update the "mut checker" to understand this limitation, I suppose. The more complex thing would be to try and fix #21232 — this feels like something we might consider, as NLL is reaching a decent point.

@nikomatsakis nikomatsakis added this to the Edition 2018 RC 2 milestone Sep 25, 2018
@spastorino spastorino self-assigned this Sep 25, 2018
@nikomatsakis
Copy link
Contributor

In terms of how to suppress the false warning, here are a few tips:

The BorrowCheckContext has a field used_muts indicating the variables and things where a mut declaration was *required::

/// This field keeps track of all the local variables that are declared mut and are mutated.
/// Used for the warning issued by an unused mutable local variable.
used_mut: FxHashSet<Local>,

So we probably need to add something to that set when we see a.b = c. That is a MIR assignment, so it winds up invoking mutate_place:

self.mutate_place(
ContextKind::AssignLhs.new(location),
(lhs, span),
Shallow(None),
JustWrite,
flow_state,
);

Here we special case out assignments to immutable local variables (not this case):

// Special case: you can assign a immutable local variable
// (e.g., `x = ...`) so long as it has never been initialized
// before (at this point in the flow).
if let &Place::Local(local) = place_span.0 {
if let Mutability::Not = self.mir.local_decls[local].mutability {
// check for reassignments to immutable local variables
self.check_if_reassignment_to_immutable_state(
context,
local,
place_span,
flow_state,
);
return;
}
}

and then fall through to access_place:

// Otherwise, use the normal access permission rules.
self.access_place(
context,
place_span,
(kind, Write(WriteKind::Mutate)),
LocalMutationIsAllowed::No,
flow_state,
);

Something in there must be failing to update used_muts...

@nikomatsakis
Copy link
Contributor

@xfix out of curiosity, how did you notice this bug? Are you relying on the current behavior of being able to assign to (but not read from) a field of an uninitialized variable?

@KamilaBorowska
Copy link
Contributor Author

KamilaBorowska commented Oct 5, 2018

@nikomatsakis I just like abusing compiler edge cases hoping to find bugs, this isn't from an actual program.

@spastorino spastorino assigned pnkfelix and unassigned spastorino Oct 11, 2018
pnkfelix added a commit to pnkfelix/rust that referenced this issue Oct 16, 2018
…#54986.

Treat attempt to partially intialize local `l` as uses of a `mut` in `let mut l;`, to fix rust-lang#54499.
bors added a commit that referenced this issue Oct 17, 2018
…nikomatsakis

reject partial init and reinit of uninitialized data

Reject partial initialization of uninitialized structured types (i.e. structs and tuples) and also reject partial *reinitialization* of such types.

Fix #54986

Fix #54499

cc #21232
davidtwco added a commit to davidtwco/rust that referenced this issue Nov 3, 2018
This commit makes two changes:

First, it updates the dataflow builder to add an init for the place
containing a union if there is an assignment into the field of
that union.

Second, it stops a "use of uninitialized" error occuring when there is an
assignment into the field of an uninitialized union that was previously
initialized. Making this assignment would re-initialize the union, as
tested in `src/test/ui/borrowck/borrowck-union-move-assign.nll.stderr`.
The check for previous initialization ensures that we do not start
supporting partial initialization yet (cc rust-lang#21232, rust-lang#54499, rust-lang#54986).
bors added a commit that referenced this issue Nov 11, 2018
NLL Diagnostic Review 3: Unions not reinitialized after assignment into field

Fixes #55651, #55652.

This PR makes two changes:

First, it updates the dataflow builder to add an init for the place
containing a union if there is an assignment into the field of
that union.

Second, it stops a "use of uninitialized" error occuring when there is an
assignment into the field of an uninitialized union that was previously
initialized. Making this assignment would re-initialize the union, as
tested in `src/test/ui/borrowck/borrowck-union-move-assign.nll.stderr`.
The check for previous initialization ensures that we do not start
supporting partial initialization yet (cc #21232, #54499, #54986).

This PR also fixes #55652 which was marked as requiring investigation
as the changes in this PR add an error that was previously missing
(and mentioned in the review comments) and confirms that the error
that was present is correct and a result of earlier partial
initialization changes in NLL.

r? @pnkfelix (due to earlier work with partial initialization)
cc @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

6 participants