Mir Borrowck: Parity with Ast for E0384 (Cannot assign twice to immutable) #46022

matthewjasper · 2017-11-15T22:55:30Z

Closes MIR-borrowck: ICE during diagnostic search for assignment (unwrap of None) #45199
Don't allow assigning to dropped immutable variables
Show the "first assignment" note on the first assignment that can actually come before the second assignment.
Make "first assignment" notes point to function parameters if needed.
Don't show a "first assignment" note if the first and second assignment have the same span (in a loop). This matches ast borrowck for now, but maybe this we should add "in previous loop iteration" as with some other borrowck errors. (Commit 2)
Use revisions to check mir borrowck for the existing tests for this error. (Commit 3)

~~Still working on a less ad-hoc way to get 'first assignment' notes to show on the correct assignment. Also need to check mutating function arguments.~~ Now using a new dataflow pass.

nikomatsakis · 2017-11-16T14:52:36Z

r? @pnkfelix

arielb1 · 2017-11-16T14:56:31Z

@matthewjasper

I don't think StorageDead kills move-out status, so I think this will break situations such as:

loop {
    let x = String::new();
    drop(x);
}

You probably want to create a separate dataflow for "propagating initializations" (basically the opposite of the MoveOut analysis, that is killed by a StorageDead, but not by moves) and use that instead of using a combination of moveout + maybe-uninitialized.

arielb1 · 2017-11-19T14:10:29Z

@matthewjasper

Nice PR. However, I think we are getting quite a bit of separate move-in implementation sprawl - there are already 2, and now we're getting another one. I think we can merge them - that would imply changing the
code so that MovingOutStatements and drop_flag_effects_for_location use the init_loc_map.

For that, you'll need to have some sort of flag on the init_loc_map so that you won't initialize the return value of calls on the panic path, because that will cause bugs. I think adding a non_panic_path_only flag to Init and checking it where needed would be enough.

arielb1 · 2017-11-19T14:14:58Z

src/librustc_mir/dataflow/move_paths/builder.rs

@@ -337,6 +369,7 @@ impl<'b, 'a, 'gcx, 'tcx> Gatherer<'b, 'a, 'gcx, 'tcx> {
                }
                if let Some((ref destination, _bb)) = *destination {
                    self.create_move_path(destination);


This only initializes the destination on the non-panic-path, so you'll probably want some label on the Init you create to indicate that.

arielb1 · 2017-11-19T14:37:45Z

src/librustc_mir/dataflow/impls/mod.rs

+                              self.mir,
+                              move_data,
+                              move_data.rev_lookup.find(dest_lval),
+                              |mpi| for ii in &init_path_map[mpi] {


This looks wrong - you don't want to "christmas tree" every initialization for this lvalue, you want just the initialization for the call.

e.g. in a code like

let x; if true { x = 2; } else { x = get(); x = 3; }

You want to highlight just the get, not the other assignment to x.

If you want to do this correctly, you could just do

let call_loc = Location { // why doesn't MIR have a terminator_loc method? block: call_bb, // feel free to add one statement_index: self.mir[call_bb].statements.len() }; for init_index in &init_loc_map[call_loc] { zero_to_one(sets.gen_set.words_mut(), *init_index); }

arielb1 · 2017-11-19T14:38:25Z

r=me with the initialization sprawl bought under control.

shepmaster · 2017-11-24T20:49:39Z

@matthewjasper the title of this PR still says "[WIP]", but it looks like your review is close to finished. Please make sure to remove that if you feel this PR should be merged.

arielb1 · 2017-11-25T22:11:10Z

@matthewjasper

How are you doing with this PR? Could you address the nits so we can move forward?

matthewjasper · 2017-11-26T00:43:48Z

Comments addressed.

arielb1 · 2017-11-26T14:41:35Z

src/librustc_mir/borrow_check.rs

-                self.report_illegal_reassignment(
-                    context, (lvalue, span), assignment_stmt.source_info.span);
+            for ii in &move_data.init_path_map[mpi] {
+                if flow_state.ever_inits.curr_state.contains(ii)


style: opening brace for ifs is in the same line as the if:

if flow_state.ever_inits.curr_state.contains(ii) {

arielb1 · 2017-11-26T14:43:21Z

src/librustc_mir/dataflow/drop_flag_effects.rs

        }
    }
-}
+}


style: no newline at end of file

arielb1 · 2017-11-26T15:04:14Z

src/librustc_mir/dataflow/impls/mod.rs

+        let bits_per_block = self.bits_per_block();
+        for init_index in &init_loc_map[location] {
+            assert!(init_index.index() < bits_per_block);
+            zero_to_one(sets.gen_set.words_mut(), *init_index);


I would prefer to use a in_out.add rather than a zero_to_one here - I'm not sure of a good reason for this to always be for an uninitialized variable (it could be that right now we only assign to temporaries, but that might not remain so in the future).

Also, even if it doesn't matter for borrow-check (because you can't have assignments in the panic path) you should not mark call destinations as uninitialized if the call unwinds (i.e. if the init loc map entry is a NonPanicPathOnly).

You mean as initialized?

arielb1 · 2017-11-26T15:10:29Z

src/librustc_mir/dataflow/move_paths/builder.rs

+            let path = self.data.rev_lookup.locals[arg];
+            let span = self.mir.local_decls[arg].source_info.span;
+
+            let init = self.data.inits.push(Init {


~~You want to have arguments assigned before the first statement, to avoid edge cases such as~~ my wrong.

arielb1 · 2017-11-26T15:14:52Z

This is cool.

r=me modulo nits

matthewjasper · 2017-11-26T23:13:53Z

Nits addressed

* Used for new dataflow to track if a variable has every been initialized * Used for other dataflows that need to be updated for initializations

Matches current ast-borrowck behavior.

arielb1 · 2017-11-27T12:26:56Z

@bors r+

bors · 2017-11-27T12:26:56Z

📌 Commit d64a318 has been approved by arielb1

…rror, r=arielb1 Mir Borrowck: Parity with Ast for E0384 (Cannot assign twice to immutable) - Closes rust-lang#45199 - Don't allow assigning to dropped immutable variables - Show the "first assignment" note on the first assignment that can actually come before the second assignment. - Make "first assignment" notes point to function parameters if needed. - Don't show a "first assignment" note if the first and second assignment have the same span (in a loop). This matches ast borrowck for now, but maybe this we should add "in previous loop iteration" as with some other borrowck errors. (Commit 2) - Use revisions to check mir borrowck for the existing tests for this error. (Commit 3) ~~Still working on a less ad-hoc way to get 'first assignment' notes to show on the correct assignment. Also need to check mutating function arguments.~~ Now using a new dataflow pass.

bors · 2017-11-27T17:13:28Z

⌛ Testing commit d64a318 with merge 560a5da...

…elb1 Mir Borrowck: Parity with Ast for E0384 (Cannot assign twice to immutable) - Closes #45199 - Don't allow assigning to dropped immutable variables - Show the "first assignment" note on the first assignment that can actually come before the second assignment. - Make "first assignment" notes point to function parameters if needed. - Don't show a "first assignment" note if the first and second assignment have the same span (in a loop). This matches ast borrowck for now, but maybe this we should add "in previous loop iteration" as with some other borrowck errors. (Commit 2) - Use revisions to check mir borrowck for the existing tests for this error. (Commit 3) ~~Still working on a less ad-hoc way to get 'first assignment' notes to show on the correct assignment. Also need to check mutating function arguments.~~ Now using a new dataflow pass.

bors · 2017-11-27T19:49:52Z

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

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 16, 2017

rust-highfive assigned pnkfelix Nov 16, 2017

matthewjasper force-pushed the cannot-assign-twice-error branch from 838def0 to 8347043 Compare November 18, 2017 21:51

arielb1 mentioned this pull request Nov 19, 2017

MIR: asm statements don't participate in moveck #45695

Closed

arielb1 reviewed Nov 19, 2017

View reviewed changes

matthewjasper force-pushed the cannot-assign-twice-error branch from 8347043 to d895346 Compare November 26, 2017 00:35

matthewjasper changed the title ~~[WIP] Mir Borrowck: Parity with Ast for E0384 (Cannot assign twice to immutable)~~ Mir Borrowck: Parity with Ast for E0384 (Cannot assign twice to immutable) Nov 26, 2017

arielb1 reviewed Nov 26, 2017

View reviewed changes

src/librustc_mir/dataflow/drop_flag_effects.rs Outdated

}

}

}

}

Copy link

Contributor

arielb1 Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: no newline at end of file

arielb1 reviewed Nov 26, 2017

View reviewed changes

matthewjasper force-pushed the cannot-assign-twice-error branch 2 times, most recently from 356316b to 88776fd Compare November 26, 2017 23:02

matthewjasper added 3 commits November 27, 2017 08:06

Add initialization info to MoveData

d409dbf

* Used for new dataflow to track if a variable has every been initialized * Used for other dataflows that need to be updated for initializations

Don't show first assignment in loops

6853b34

Matches current ast-borrowck behavior.

Update tests involving E0384 for mir-borrowck

d64a318

matthewjasper force-pushed the cannot-assign-twice-error branch from 88776fd to d64a318 Compare November 27, 2017 08:08

arielb1 mentioned this pull request Nov 27, 2017

Rollup of 2 pull requests #46304

Closed

bors merged commit d64a318 into rust-lang:master Nov 27, 2017

bors mentioned this pull request Nov 27, 2017

MIR borrowck: finalize check_access_permissions() #46041

Merged

davidtwco mentioned this pull request Nov 27, 2017

MIR-borrowck: immutable unique closure upvars can be mutated #46236

Merged

matthewjasper deleted the cannot-assign-twice-error branch May 11, 2018 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mir Borrowck: Parity with Ast for E0384 (Cannot assign twice to immutable) #46022

Mir Borrowck: Parity with Ast for E0384 (Cannot assign twice to immutable) #46022

matthewjasper commented Nov 15, 2017 •

edited

Loading

nikomatsakis commented Nov 16, 2017

arielb1 commented Nov 16, 2017

arielb1 commented Nov 19, 2017

arielb1 Nov 19, 2017

arielb1 Nov 26, 2017

arielb1 Nov 19, 2017

arielb1 commented Nov 19, 2017

shepmaster commented Nov 24, 2017

arielb1 commented Nov 25, 2017

matthewjasper commented Nov 26, 2017

arielb1 Nov 26, 2017

arielb1 Nov 26, 2017

arielb1 Nov 26, 2017 •

edited

Loading

arielb1 Nov 26, 2017 •

edited

Loading

matthewjasper Nov 26, 2017

arielb1 Nov 26, 2017

arielb1 Nov 26, 2017 •

edited

Loading

arielb1 commented Nov 26, 2017 •

edited

Loading

matthewjasper commented Nov 26, 2017

arielb1 commented Nov 27, 2017

bors commented Nov 27, 2017

bors commented Nov 27, 2017

bors commented Nov 27, 2017

Mir Borrowck: Parity with Ast for E0384 (Cannot assign twice to immutable) #46022

Mir Borrowck: Parity with Ast for E0384 (Cannot assign twice to immutable) #46022

Conversation

matthewjasper commented Nov 15, 2017 • edited Loading

nikomatsakis commented Nov 16, 2017

arielb1 commented Nov 16, 2017

arielb1 commented Nov 19, 2017

arielb1 Nov 19, 2017

Choose a reason for hiding this comment

arielb1 Nov 26, 2017

Choose a reason for hiding this comment

arielb1 Nov 19, 2017

Choose a reason for hiding this comment

arielb1 commented Nov 19, 2017

shepmaster commented Nov 24, 2017

arielb1 commented Nov 25, 2017

matthewjasper commented Nov 26, 2017

arielb1 Nov 26, 2017

Choose a reason for hiding this comment

arielb1 Nov 26, 2017

Choose a reason for hiding this comment

arielb1 Nov 26, 2017 • edited Loading

Choose a reason for hiding this comment

arielb1 Nov 26, 2017 • edited Loading

Choose a reason for hiding this comment

matthewjasper Nov 26, 2017

Choose a reason for hiding this comment

arielb1 Nov 26, 2017

Choose a reason for hiding this comment

arielb1 Nov 26, 2017 • edited Loading

Choose a reason for hiding this comment

arielb1 commented Nov 26, 2017 • edited Loading

matthewjasper commented Nov 26, 2017

arielb1 commented Nov 27, 2017

bors commented Nov 27, 2017

bors commented Nov 27, 2017

bors commented Nov 27, 2017

matthewjasper commented Nov 15, 2017 •

edited

Loading

arielb1 Nov 26, 2017 •

edited

Loading

arielb1 Nov 26, 2017 •

edited

Loading

arielb1 Nov 26, 2017 •

edited

Loading

arielb1 commented Nov 26, 2017 •

edited

Loading