Fixed mutable vars being marked used when they weren't #43582

ivanbakel · 2017-08-01T02:12:09Z

NB : bootstrapping is slow on my machine, even with `keep-stage` - fixes for occurances in the current codebase are in the pipeline done. This PR is being put up for review of the fix of the issue.

Fixes #43526, Fixes #30280, Fixes #25049

Issue

Whenever the compiler detected a mutable deref being used mutably, it marked an associated value as being used mutably as well. In the case of derefencing local variables which were mutable references, this incorrectly marked the reference itself being used mutably, instead of its contents - with the consequence of making the following code emit no warnings

fn do_thing<T>(mut arg : &mut T) {
    ... // don't touch arg - just deref it to access the T
}

Fix

Make dereferences not be counted as a mutable use, but only when they're on borrows on local variables.

Why not on things other than local variables?

Whenever you capture a variable in a closure, it gets turned into a hidden reference - when you use it in the closure, it gets dereferenced. If the closure uses the variable mutably, that is actually a mutable use of the thing being dereffed to, so it has to be counted.
If you deref a mutable Box to access the contents mutably, you are using the Box mutably - so it has to be counted.

rust-highfive · 2017-08-01T02:12:21Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

kennytm · 2017-08-01T07:17:52Z

You caught bugs in libcore! 😄

Also, you need to add three compile-fail test cases.

[00:17:02] error: variable does not need to be mutable
[00:17:02]    --> /checkout/src/libcore/ops/function.rs:190:41
[00:17:02]     |
[00:17:02] 190 |         extern "rust-call" fn call_once(mut self, args: A) -> F::Output {
[00:17:02]     |                                         ^^^^^^^^
[00:17:02]     |
[00:17:02] note: lint level defined here
[00:17:02]    --> /checkout/src/libcore/lib.rs:68:9
[00:17:02]     |
[00:17:02] 68  | #![deny(warnings)]
[00:17:02]     |         ^^^^^^^^
[00:17:02]     = note: #[deny(unused_mut)] implied by #[deny(warnings)]
[00:17:02] 
[00:17:03] error: variable does not need to be mutable
[00:17:03]    --> /checkout/src/libcore/option.rs:875:18
[00:17:03]     |
[00:17:03] 875 |     fn into_iter(mut self) -> IterMut<'a, T> {
[00:17:03]     |                  ^^^^^^^^
[00:17:03] 
[00:17:03] error: variable does not need to be mutable
[00:17:03]    --> /checkout/src/libcore/result.rs:912:18
[00:17:03]     |
[00:17:03] 912 |     fn into_iter(mut self) -> IterMut<'a, T> {
[00:17:03]     |                  ^^^^^^^^
[00:17:03] 
[00:17:03] error: aborting due to 3 previous errors
[00:17:03] 
[00:17:03] error: Could not compile `core`.

arielb1 · 2017-08-01T09:54:40Z

This would still be broken when you have a deref of a field:

fn main() {
    let mut x = 0;
    let mut y = (&mut x,);
    *y.0 = 1;
}

You can detect upvar derefs (aka derefs of captured variables in closures) by looking for a NoteUpvarRef (and NoteClosureEnv) on the Deref - the note created in https://github.com/rust-lang/rust/blob/master/src/librustc/middle/mem_categorization.rs#L759.

That means you'll have to use the cmt instead of the loan path, but you have a cmt available.

ivanbakel · 2017-08-01T12:39:42Z

What three test cases are needed? The three issues are all instances of the same problem, or should I test for use of mutable references inside structs/tuples as well?

@arielb1 I didn't notice that. The mut-checker can track upvars with LoanPathKind, the trouble being that the nesting is slightly difficult to manage - there's a need to recurse into the LoanPath to see if it terminates in an upvar. I'll work on the fix for that now.

kennytm · 2017-08-01T13:27:49Z

@ivanbakel The test case for each issue reported. Perhaps modify src/test/compile-fail/lint-unused-mut-variables.rs to include the new tests.

#![allow(unused_assignments)]
#![allow(unused_variables)]
#![allow(dead_code)]
#![deny(unused_mut)]

fn main() {
   ...
   // issue 43526
   let mut x43526 = &mut 5; //~ ERROR: variable does not need to be mutable
   *x43526 = 4;

   // include 30280 here as well
}

// issue 25049, reduced
fn foo(mut buf: &mut [u8]) { //~ ERROR: variable does not need to be mutable
    &mut buf[..];
}

...

The example of arielb1's comment should be added as well if you fixed it.

ivanbakel · 2017-08-01T14:25:08Z

The code has been updated to catch derefs of borrows inside other types as well, which resolves the issue arielb1 raised - this has raised some new violations in core, so I'm waiting for those to be found and fixed as well.

Is there something special about the order of operations for 25049 that makes it different from the other bugs? I'm not that familiar with slice logic.

kennytm · 2017-08-01T14:39:38Z

@ivanbakel Nothing special. The test cases are there to prove that you do have fixed all 3 issues, and to catch future regression.

ivanbakel · 2017-08-01T21:09:29Z

Alright, I've added tests for the 3 issue cases and the tuple case as well.

Boxes are the exception - since a Box is as mutable as its contents.

The mutability system now checks where derefs go through borrows in the loan chain, and can correctly detect mutable borrows inside structs and tuples.

kennytm · 2017-08-02T07:11:45Z

Now libcore and libstd test cases need to be updated 😂

[00:50:15] error: variable does not need to be mutable
[00:50:15]    --> /checkout/src/libcore/../libcore/tests/slice.rs:108:9
[00:50:15]     |
[00:50:15] 108 |     let mut v: &mut [i32] = &mut [0, 1, 2, 3, 4, 5];
[00:50:15]     |         ^^^^^
[00:50:15]     |
[00:50:15] 
[00:50:15] error: variable does not need to be mutable
[00:50:15]    --> /checkout/src/libcore/../libcore/tests/slice.rs:112:9
[00:50:15]     |
[00:50:15] 112 |     let mut v2: &mut [i32] = &mut [0, 1, 2, 3, 4];
[00:50:15]     |         ^^^^^^
[00:50:15] 
[00:50:15] error: variable does not need to be mutable
[00:50:15]    --> /checkout/src/libcore/../libcore/tests/slice.rs:116:9
[00:50:15]     |
[00:50:15] 116 |     let mut v3: &mut [i32] = &mut [];
[00:50:15]     |         ^^^^^^
[00:50:15] 
[00:50:15] error: variable does not need to be mutable
[00:50:15]    --> /checkout/src/libcore/../libcore/tests/slice.rs:123:9
[00:50:15]     |
[00:50:15] 123 |     let mut v: &mut [i32] = &mut [0, 1, 2, 3, 4, 5];
[00:50:15]     |         ^^^^^
[00:50:15] 
[00:50:15] error: variable does not need to be mutable
[00:50:15]    --> /checkout/src/libcore/../libcore/tests/slice.rs:128:9
[00:50:15]     |
[00:50:15] 128 |     let mut v2: &mut [i32] = &mut [0, 1, 2, 3, 4];
[00:50:15]     |         ^^^^^^
[00:50:15] 
[00:50:15] error: variable does not need to be mutable
[00:50:15]    --> /checkout/src/libcore/../libcore/tests/slice.rs:197:9
[00:50:15]     |
[00:50:15] 197 |     let mut v: &mut [i32] = &mut [0, 1, 2, 3, 4, 5];
[00:50:15]     |         ^^^^^
[00:50:15] 
[00:50:20] error: variable does not need to be mutable
[00:50:20]    --> /checkout/src/libstd/error.rs:517:13
[00:50:20]     |
[00:50:20] 517 |         let mut a = &mut a as &mut (Error + 'static);
[00:50:20]     |             ^^^^^
[00:50:20]     |
[00:50:20] 
[00:50:20] error: variable does not need to be mutable
[00:50:20]    --> /checkout/src/libstd/io/cursor.rs:459:13
[00:50:20]     |
[00:50:20] 459 |         let mut reader = &mut &in_buf[..];
[00:50:20]     |             ^^^^^^^^^^
[00:50:20]

ivanbakel · 2017-08-02T14:18:45Z

I finally understand the meaning of the words "technical debt".

kennytm · 2017-08-03T09:56:57Z

@arielb1 @pnkfelix Travis is green now.

arielb1 · 2017-08-06T12:52:39Z

src/librustc_borrowck/borrowck/gather_loans/mod.rs

+                    None
+                }
+                LpUpvar(ty::UpvarId{ var_id: local_id, closure_expr_id: _ }) => {
+                    self.tcx().used_mut_nodes.borrow_mut().insert(local_id);


No through_borrow here?

fn main() { let mut z = 0; let mut x = &mut z; let mut klos = || { *x = 1; }; klos(); }

I think it's not needed because closures don't borrow mutably (they borrow uniquely) unless they have to. But please add a test.

Maybe I'm don't understand your example: This already gives a warning for x being mut, even with the new code. through_borrow is always true reaching the upvar case, since it is always wrapped in a LpExtend(.., .., LpDeref(..)).

Sure. I understand why it works. Just add it as a test to make sure it does work, because it could be an edge case (e.g. when we move to MIR borrowck, we'll like to keep everything working).

arielb1 · 2017-08-06T12:54:32Z

r=me with closure test (or is such a test already present?)

arielb1 · 2017-08-06T12:54:36Z

Nice PR!

ivanbakel · 2017-08-06T17:29:08Z

Alright, I've added a test for the closure case.

arielb1 · 2017-08-07T08:02:19Z

@bors r+

bors · 2017-08-07T08:02:20Z

📌 Commit d1fffd8 has been approved by arielb1

bors · 2017-08-07T12:35:21Z

⌛ Testing commit d1fffd8 with merge e399d7ee6953f3960158ea009f2b93e9a86292b0...

ivanbakel · 2017-08-07T17:53:59Z

😢
Is there a way to force this to build on GNU/Linux? Are there any platform-specific tests to build and run as well?

arielb1 · 2017-08-08T10:23:29Z

@ivanbakel

I'll reboot to my Windows tonight and push a fix to your repo.

arielb1 · 2017-08-09T09:31:38Z

@bors r+

bors · 2017-08-09T09:31:39Z

📌 Commit 3b0ff0d has been approved by arielb1

bors · 2017-08-09T10:09:15Z

⌛ Testing commit 3b0ff0d with merge 4a8360c3a24d96daf26234ac15cc15b53584d975...

bors · 2017-08-09T11:10:21Z

💔 Test failed - status-travis

kennytm · 2017-08-09T11:56:37Z

Legit. It's cargo this time.

[00:53:34] error: variable does not need to be mutable
[00:53:34]     --> src/tools/cargo/src/cargo/core/resolver/mod.rs:1112:17
[00:53:34]      |
[00:53:34] 1112 |             let mut set = self.resolve_features.entry(pkgid.clone())
[00:53:34]      |                 ^^^^^^^
[00:53:34]      |
[00:53:34] 
[00:53:34] error: variable does not need to be mutable
[00:53:34]    --> src/tools/cargo/src/cargo/util/config.rs:484:13
[00:53:34]     |
[00:53:34] 484 |         let mut cfg = match *cfg {
[00:53:34]     |             ^^^^^^^
[00:53:34] 
[00:53:34] error: variable does not need to be mutable
[00:53:34]   --> src/tools/cargo/src/cargo/util/read2.rs:15:18
[00:53:34]    |
[00:53:34] 15 |                  mut data: &mut FnMut(bool, &mut Vec<u8>, bool)) -> io::Result<()> {
[00:53:34]    |                  ^^^^^^^^
[00:53:34] 
[00:53:34] error: aborting due to 3 previous errors
[00:53:34] 
[00:53:34] error: Could not compile `cargo`.

@ivanbakel You need to

submit a PR to rust-lang/cargo
after the cargo PR is merged, update the cargo submodule in this PR.

Meanwhile you may want to check if RLS and other dependencies (if they are compiled with #[deny(warnings)]) need to be updated as well.

ivanbakel · 2017-08-09T12:21:56Z

I'll get on making the Cargo PR. Is there an easy way to build all submodules to check for these problems?

kennytm · 2017-08-09T12:33:22Z

@ivanbakel I'd just do it manually 😂 Out of all submodules,

 6f1a03dae6bcea44976918186f2d554186b3499c src/doc/book (remotes/origin/ch11-edits-95-g6f1a03da)
 f570bcb681771d691aa4fdb8dfcfad1939844bf5 src/doc/nomicon (remotes/origin/HEAD)
 1abfbaa70313fdf527cf799ffd9b9a096a62105c src/doc/reference (remotes/origin/HEAD)
 11bfb0dcf85f7aa92abd30524bb1e42e18d108c6 src/jemalloc (3.6.0-539-g11bfb0d)
 5e49856003f33aa5781a0edca148be21025e18e7 src/libcompiler_builtins (remotes/origin/HEAD)
 2a5b50b7f7f539a0fd201331d6c1e0534aa332f5 src/liblibc (0.2.29-28-g2a5b50b7f)
 d9e7d2696e41983b6b5a0b4fac604b4e548a84d3 src/llvm (remotes/origin/rust-llvm-release-4-0-1)
 da282f1bb7277b4d30fa1599ee29ad8eb4dd2a92 src/rt/hoedown (2.0.0-226-gda282f1)
 305bc25d5e105e84ffe261655b46cf74570f6e5b src/tools/cargo (0.14.0-1065-g305bc25d)
 5d4bbd9052fe2af849a7d017b85df98ad002c20f src/tools/rls (alpha-388-g5d4bbd9)
 b4ff403041f17957f735ad750c3241a3a428b9b7 src/tools/rust-installer (remotes/origin/HEAD)

book, nomicon and reference are doc-tests and they don't test by denying all warnings AFAIK
jemalloc, hoedown and llvm are C libraries, so irrelevant
libcompiler_builtins and liblibc are used during compilation, and compilation succeeded, so they should be fine

That leaves cargo, rls and rust-installer, where we already know cargo needs to be updated. Since there are only 3 submodules, doing RUSTC=/path/to/your/rustc cargo test three times seems easy enough.

Fixed some variables being unnecessarily mutable ### Changes Some variables are marked `mut` when they don't need to be. This PR changes those variables to no longer be `mut`. ### Context PR on rust-lang/rust#43582 tl:dr; There's a bug with the mutability checker that sometimes marks mutable ref variables as being used when they're not.

ivanbakel · 2017-08-09T18:11:16Z

After some work:

cargo changes are merged!
rls builds, but some tests fail due to output differences. Not sure this is me.
rust_installer fails to build, since error_chain was built with 1.21.0. Not sure how to resolve this - do I need to update the repo and rebuild my local compiler for it to work?

kennytm · 2017-08-09T22:33:39Z

Maybe someone could just r+ and let bors figure it out (after the cargo submodule is updated, that is).

arielb1 · 2017-08-10T08:46:10Z

@bors r+

bors · 2017-08-10T08:46:11Z

📌 Commit 8f78d45 has been approved by arielb1

bors · 2017-08-10T08:53:28Z

⌛ Testing commit 8f78d45 with merge d21ec9b...

Fixed mutable vars being marked used when they weren't #### NB : bootstrapping is slow on my machine, even with `keep-stage` - fixes for occurances in the current codebase are <s>in the pipeline</s> done. This PR is being put up for review of the fix of the issue. Fixes #43526, Fixes #30280, Fixes #25049 ### Issue Whenever the compiler detected a mutable deref being used mutably, it marked an associated value as being used mutably as well. In the case of derefencing local variables which were mutable references, this incorrectly marked the reference itself being used mutably, instead of its contents - with the consequence of making the following code emit no warnings ``` fn do_thing<T>(mut arg : &mut T) { ... // don't touch arg - just deref it to access the T } ``` ### Fix Make dereferences not be counted as a mutable use, but only when they're on borrows on local variables. #### Why not on things other than local variables? * Whenever you capture a variable in a closure, it gets turned into a hidden reference - when you use it in the closure, it gets dereferenced. If the closure uses the variable mutably, that is actually a mutable use of the thing being dereffed to, so it has to be counted. * If you deref a mutable `Box` to access the contents mutably, you are using the `Box` mutably - so it has to be counted.

bors · 2017-08-10T11:20:09Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing d21ec9b to master...

`iter_mut()` and `or_insert()` yields mutable references, the variables they bind to does not need to be mutable. Previously, because of rust-lang/rust#30280 this was not considered a warning. The fix is in rust-lang/rust#43582 This means on recently nightlies this warning is emitted.

spamwax · 2018-03-04T22:51:25Z

Compiling a bin project under 1.22.1 issues unused_mut warning while the same project compiled with 1.24 stable does not!

In other words, code gets compiled under 1.24 with or without the unused mut

Trying to get a minimal snippet to reproduce this but the offending line is like this:

{
  let (x, mut y) = setup().unwrap_or_else(process::exit(1));
  foo(x, &y);
  bar(x, &y);
}

Was just wondering if this PR is related to above issue?

rust-highfive assigned pnkfelix Aug 1, 2017

ivanbakel added 7 commits August 1, 2017 23:01

Fixed refs being marked used mutably on mut deref

35b9bd0

Boxes are the exception - since a Box is as mutable as its contents.

Fixed all unnecessary muts in language core

400075d

Fixed interior mutable references causing a mark

8e4c9e4

The mutability system now checks where derefs go through borrows in the loan chain, and can correctly detect mutable borrows inside structs and tuples.

Fixed extra cases found in better checking.

c623375

Fixed formatting.

d817d58

Added tests for bugs fixed.

bb578b6

Fixed another problem.

d3236a8

ivanbakel force-pushed the unused_mut_ref branch from 916e914 to d3236a8 Compare August 2, 2017 00:11

Fixed errors in libstd.

9c854db

aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2017

arielb1 reviewed Aug 6, 2017

View reviewed changes

Added closure test case.

d1fffd8

arielb1 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2017

arielb1 assigned arielb1 and unassigned pnkfelix Aug 9, 2017

Fix errors on Windows

3b0ff0d

ivanbakel mentioned this pull request Aug 9, 2017

Fixed some variables being unnecessarily mutable rust-lang/cargo#4384

Merged

Updated cargo submodule to fix compile error

8f78d45

bors merged commit 8f78d45 into rust-lang:master Aug 10, 2017

This was referenced Aug 10, 2017

Use hir::ItemLocalId as keys in TypeckTables. #43740

Merged

Generator support #43076

Merged

Fixed mutable vars being marked used when they weren't #43582

Fixed mutable vars being marked used when they weren't #43582

Conversation

ivanbakel commented Aug 1, 2017 • edited Loading

NB : bootstrapping is slow on my machine, even with keep-stage - fixes for occurances in the current codebase are in the pipeline done. This PR is being put up for review of the fix of the issue.

Issue

Fix

Why not on things other than local variables?

rust-highfive commented Aug 1, 2017

kennytm commented Aug 1, 2017 • edited Loading

arielb1 commented Aug 1, 2017 • edited Loading

ivanbakel commented Aug 1, 2017

kennytm commented Aug 1, 2017 • edited Loading

ivanbakel commented Aug 1, 2017

kennytm commented Aug 1, 2017

ivanbakel commented Aug 1, 2017

kennytm commented Aug 2, 2017

ivanbakel commented Aug 2, 2017

kennytm commented Aug 3, 2017

arielb1 Aug 6, 2017

Choose a reason for hiding this comment

arielb1 Aug 6, 2017

Choose a reason for hiding this comment

ivanbakel Aug 6, 2017

Choose a reason for hiding this comment

arielb1 Aug 6, 2017

Choose a reason for hiding this comment

arielb1 commented Aug 6, 2017

arielb1 commented Aug 6, 2017

ivanbakel commented Aug 6, 2017

arielb1 commented Aug 7, 2017

bors commented Aug 7, 2017

bors commented Aug 7, 2017

ivanbakel commented Aug 7, 2017

arielb1 commented Aug 8, 2017

arielb1 commented Aug 9, 2017

bors commented Aug 9, 2017

bors commented Aug 9, 2017

bors commented Aug 9, 2017

kennytm commented Aug 9, 2017

ivanbakel commented Aug 9, 2017

kennytm commented Aug 9, 2017 • edited Loading

ivanbakel commented Aug 9, 2017

kennytm commented Aug 9, 2017

arielb1 commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 10, 2017

spamwax commented Mar 4, 2018 • edited Loading

ivanbakel commented Aug 1, 2017 •

edited

Loading

NB : bootstrapping is slow on my machine, even with `keep-stage` - fixes for occurances in the current codebase are in the pipeline done. This PR is being put up for review of the fix of the issue.

kennytm commented Aug 1, 2017 •

edited

Loading

arielb1 commented Aug 1, 2017 •

edited

Loading

kennytm commented Aug 1, 2017 •

edited

Loading

kennytm commented Aug 9, 2017 •

edited

Loading

spamwax commented Mar 4, 2018 •

edited

Loading