Allow MIR borrowck to catch unused mutable locals #48605

KiChjang · 2018-02-28T09:10:45Z

nikomatsakis · 2018-02-28T09:50:20Z

This looks about right. There may be some complications around closures:

let mut x = &mut vec![]; 
//  ^^^ not needed
let closure = || {
    x.push(22);
};
closure();

Also, we have to disable the other lint, from old AST borrowck

bors · 2018-03-05T21:52:01Z

☔ The latest upstream changes (presumably #48208) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis

This looks great! My main concern is the tests, left some suggestions below.

nikomatsakis · 2018-03-08T02:56:10Z

src/test/compile-fail/lint-unused-mut-variables.rs

@@ -8,6 +8,8 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

+//compile-flags: -Z borrowck=compare -Z emit-end-regions


I don't think that this will necessarily test the way you want it. How about this instead?

// revisions: lexical nll #![cfg_attr(nll, feature(nll))]

This will run the test twice, once in normal mode and once in nll mode.

Also, can we add another test case for nested closures? I see this one:

let mut a = Vec::new(); callback(|| { a.push(3); });

but I think I want:

let mut a = Vec::new(); callback(|| { callback(|| a.push(3)); // nested call }); });

as well

nikomatsakis · 2018-03-09T23:01:25Z

src/test/compile-fail/lint-unused-mut-variables.rs

@@ -8,6 +8,9 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

+// revisions: lexical nll
+#![cfg_attr(nll, feature(nll))]


We're getting travis errors. The problem is that we have to update the //~ ERROR declarations in this file to account for the revisions.

For each line:

foo //~ ERROR bar

we have to change it to

foo //[lexical]~ ERROR bar //[nll]~^ ERROR bar

KiChjang · 2018-03-12T05:27:41Z

Ok, so I figured out why Travis CI is complaining. Given the following code:

fn main() {
    let mut a = 3;
}

The current implementation sees an assignment to a during initialization, which makes it erroneously believe that the value has been mutated. We need to make it so that it understands that initialization is not equivalent with mutation.

KiChjang · 2018-03-12T15:52:07Z

Ok, so the latest commit still doesn't fix everything yet. It's quite over-eager in its job and flags too much fn args as being unnecessarily mutable. It also doesn't catch the following case:

    fn mut_ref_arg(mut arg : &mut [u8]) -> &mut [u8] {
        &mut arg[..] //[lexical]~^ ERROR: variable does not need to be mutable
                     //[nll]~^ ERROR: variable does not need to be mutable
    }

... which tells me that we need additional logic in consume_rvalue.

bors · 2018-03-12T16:00:27Z

☔ The latest upstream changes (presumably #48770) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2018-03-13T19:12:06Z

@KiChjang

We need to make it so that it understands that initialization is not equivalent with mutation.

We should be able to use the initialization state for this. Presumably we are doing a similar analysis already to report errors in the case that the variable is not mut...

Ah, indeed we do, right here:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 1279 to 1303 in 8c4ff22

    
           fn check_if_reassignment_to_immutable_state( 
        
               &mut self, 
        
               context: Context, 
        
               (place, span): (&Place<'tcx>, Span), 
        
               flow_state: &Flows<'cx, 'gcx, 'tcx>, 
        
           ) { 
        
               debug!("check_if_reassignment_to_immutable_state({:?})", place); 
        
               // determine if this path has a non-mut owner (and thus needs checking). 
        
               if let Ok(()) = self.is_mutable(place, LocalMutationIsAllowed::No) { 
        
                   return; 
        
               } 
        
               debug!( 
        
                   "check_if_reassignment_to_immutable_state({:?}) - is an imm local", 
        
                   place 
        
               ); 
        
               for i in flow_state.ever_inits.elems_incoming() { 
        
                   let init = self.move_data.inits[i]; 
        
                   let init_place = &self.move_data.move_paths[init.path].place; 
        
                   if self.places_conflict(&init_place, place, Deep) { 
        
                       self.report_illegal_reassignment(context, (place, span), init.span); 
        
                       break; 
        
                   } 
        
               } 
        
           }

nikomatsakis · 2018-03-13T19:22:49Z

src/librustc_mir/borrow_check/mod.rs

@@ -1547,6 +1614,23 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                err.emit();
            },
            Reservation(WriteKind::Mutate) | Write(WriteKind::Mutate) => {
+                match place {


I think logic is too simplistic. For example, consider this case:

let mut x = (1, 2); x.0 = 22;

Here, the mutated place will be a field projection, but we still need to record x as a "used mut". We want to integrate more with the is_mutable code, I think -- in particular, maybe it should return the "root reason" that a path was mutable, even on success (right now it does that only on error). Then, if that root reason is a Local, we can add it to the used_mut set? etc

Zoxc · 2018-04-29T07:23:53Z

@bors r=nikomatsakis

bors · 2018-04-29T07:23:54Z

📌 Commit 0a1cb9b has been approved by nikomatsakis

bors · 2018-04-29T07:24:26Z

⌛ Testing commit 0a1cb9b with merge 4073a87ff295c3262735383970366c4f2d12d6ec...

bors · 2018-04-29T09:22:07Z

💔 Test failed - status-travis

rust-highfive · 2018-04-29T09:22:08Z

The job arm-android of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:10:15] Looks like docker image is the same as before, not uploading
[00:10:15] travis_fold:end:build_docker

[00:10:16] + export SHELL=/bin/bash
[00:10:16] + nohup+  nohup emulatorexec @armeabi-v7a-18 -engine /checkout/src/ci/run.sh classic
[00:10:16]  -no-window -partition-size 2047
[00:10:16] travis_time:end:343bda7e:start=1524986800823055427,finish=1524987405636164840,duration=604813109413
[CI_JOB_NAME=arm-android]
[00:10:16] [CI_JOB_NAME=arm-android]
[00:10:16] Starting sccache server...
---
[01:24:15] /checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo/var-captured-in-nested-closure.stage2-arm-linux-androideabi: 1 file pushed. 6.4 MB/s (29380 bytes in 0.004s)
[01:24:17] ok
[01:24:17] test [debuginfo-gdb] debuginfo/var-captured-in-sendable-closure.rs ... [100%] /data/tmp/work/var-captured-in-sendable-closure.stage2-arm-linux-androideabi
[01:24:17] /checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo/var-captured-in-sendable-closure.stage2-arm-linux-androideabi: 1 file pushed. 3.6 MB/s (29428 bytes in 0.008s)
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

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 @TimNN. (Feature Requests)

rust-highfive · 2018-04-29T09:22:37Z

The job arm-android of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:10:15] Looks like docker image is the same as before, not uploading
[00:10:15] travis_fold:end:build_docker

[00:10:16] + export SHELL=/bin/bash
[00:10:16] + nohup+  nohup emulatorexec @armeabi-v7a-18 -engine /checkout/src/ci/run.sh classic
[00:10:16]  -no-window -partition-size 2047
[00:10:16] travis_time:end:343bda7e:start=1524986800823055427,finish=1524987405636164840,duration=604813109413
[CI_JOB_NAME=arm-android]
[00:10:16] [CI_JOB_NAME=arm-android]
[00:10:16] Starting sccache server...
---
[01:24:15] /checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo/var-captured-in-nested-closure.stage2-arm-linux-androideabi: 1 file pushed. 6.4 MB/s (29380 bytes in 0.004s)
[01:24:17] ok
[01:24:17] test [debuginfo-gdb] debuginfo/var-captured-in-sendable-closure.rs ... [100%] /data/tmp/work/var-captured-in-sendable-closure.stage2-arm-linux-androideabi
[01:24:17] /checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo/var-captured-in-sendable-closure.stage2-arm-linux-androideabi: 1 file pushed. 3.6 MB/s (29428 bytes in 0.008s)
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

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 @TimNN. (Feature Requests)

kennytm · 2018-04-29T15:10:43Z

@bors retry

Assuming the 30-minute timeout is spurious.

bors · 2018-04-29T16:25:11Z

⌛ Testing commit 0a1cb9b with merge 79252ff...

@nikomatsakis

Allow MIR borrowck to catch unused mutable locals Fixes #47279. r? @nikomatsakis

bors · 2018-04-29T18:40:52Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 79252ff to master...

rust-highfive assigned nikomatsakis Feb 28, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2018

KiChjang changed the title ~~(WIP) Allow MIR borrowck to catch unused mutable locals~~ Allow MIR borrowck to catch unused mutable locals Mar 3, 2018

KiChjang force-pushed the unused-mut-warning branch 7 times, most recently from 4c57b73 to 0c547e1 Compare March 4, 2018 20:42

bors 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 Mar 5, 2018

KiChjang force-pushed the unused-mut-warning branch 3 times, most recently from 9833513 to 29347ae Compare March 6, 2018 19:11

nikomatsakis requested changes Mar 8, 2018

View reviewed changes

KiChjang force-pushed the unused-mut-warning branch from 29347ae to a275eda Compare March 8, 2018 05:32

nikomatsakis approved these changes Mar 9, 2018

View reviewed changes

nikomatsakis requested changes Mar 9, 2018

View reviewed changes

KiChjang force-pushed the unused-mut-warning branch 2 times, most recently from 8679570 to 49b51c7 Compare March 11, 2018 02:13

KiChjang force-pushed the unused-mut-warning branch from 49b51c7 to b221e0b Compare March 12, 2018 15:48

KiChjang force-pushed the unused-mut-warning branch from b221e0b to 403e49a Compare March 13, 2018 05:57

nikomatsakis reviewed Mar 13, 2018

View reviewed changes

KiChjang added 3 commits April 28, 2018 01:55

Determine unused mutable variables based on the 'root' place

71d221f

Return LocalMutationIsAllowed in is_mutable

180e242

Skip implicit self argument for closures

8c607ea

KiChjang force-pushed the unused-mut-warning branch from e697cb7 to d37b1df Compare April 28, 2018 08:55

nikomatsakis and others added 2 commits April 28, 2018 22:25

Return RootPlace in is_mutable

ded0697

Add the actual used mutable var to the set

0a1cb9b

KiChjang force-pushed the unused-mut-warning branch from d37b1df to 0a1cb9b Compare April 29, 2018 05:26

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 29, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 29, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2018

bors added a commit that referenced this pull request Apr 29, 2018

Auto merge of #48605 - KiChjang:unused-mut-warning, r=nikomatsakis

79252ff

Allow MIR borrowck to catch unused mutable locals Fixes #47279. r? @nikomatsakis

bors merged commit 0a1cb9b into rust-lang:master Apr 29, 2018

bors mentioned this pull request Apr 29, 2018

Remove some unused code #50198

Merged

KiChjang deleted the unused-mut-warning branch April 29, 2018 18:51

pietroalbini mentioned this pull request Apr 30, 2018

Confusing warn-unused-mut warnings since 1.27.0-nightly 2018-04-29 #50343

Closed

pnkfelix mentioned this pull request May 8, 2018

NLL erroneous Warning: variable does not need to be mutable #50461

Closed

alexcrichton mentioned this pull request May 10, 2018

Invalid warning unused_mut #50463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow MIR borrowck to catch unused mutable locals #48605

Allow MIR borrowck to catch unused mutable locals #48605

KiChjang commented Feb 28, 2018 •

edited

Loading

nikomatsakis commented Feb 28, 2018

bors commented Mar 5, 2018

nikomatsakis left a comment

nikomatsakis Mar 8, 2018

nikomatsakis Mar 8, 2018

nikomatsakis Mar 9, 2018

KiChjang commented Mar 12, 2018

KiChjang commented Mar 12, 2018 •

edited

Loading

bors commented Mar 12, 2018

nikomatsakis commented Mar 13, 2018 •

edited

Loading

nikomatsakis Mar 13, 2018

Zoxc commented Apr 29, 2018

bors commented Apr 29, 2018

bors commented Apr 29, 2018

bors commented Apr 29, 2018

rust-highfive commented Apr 29, 2018

rust-highfive commented Apr 29, 2018

kennytm commented Apr 29, 2018

bors commented Apr 29, 2018

bors commented Apr 29, 2018

Allow MIR borrowck to catch unused mutable locals #48605

Allow MIR borrowck to catch unused mutable locals #48605

Conversation

KiChjang commented Feb 28, 2018 • edited Loading

nikomatsakis commented Feb 28, 2018

bors commented Mar 5, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Mar 8, 2018

Choose a reason for hiding this comment

nikomatsakis Mar 8, 2018

Choose a reason for hiding this comment

nikomatsakis Mar 9, 2018

Choose a reason for hiding this comment

KiChjang commented Mar 12, 2018

KiChjang commented Mar 12, 2018 • edited Loading

bors commented Mar 12, 2018

nikomatsakis commented Mar 13, 2018 • edited Loading

nikomatsakis Mar 13, 2018

Choose a reason for hiding this comment

Zoxc commented Apr 29, 2018

bors commented Apr 29, 2018

bors commented Apr 29, 2018

bors commented Apr 29, 2018

rust-highfive commented Apr 29, 2018

rust-highfive commented Apr 29, 2018

kennytm commented Apr 29, 2018

bors commented Apr 29, 2018

bors commented Apr 29, 2018

KiChjang commented Feb 28, 2018 •

edited

Loading

KiChjang commented Mar 12, 2018 •

edited

Loading

nikomatsakis commented Mar 13, 2018 •

edited

Loading