Auto merge of rust-lang#53258 - nikomatsakis:issue-53189-optimize-reassignment-immutable-state, r=pnkfelix

optimize reassignment immutable state

This is the "simple fix" when it comes to checking for reassignment. We just shoot for compatibility with the AST-based checker. Makes no attempt to solve rust-lang#21232.

I opted for this simpler fix because I didn't want to think about complications [like the ones described here](rust-lang#21232 (comment)).

Let's do some profiling measurements.

Fixes rust-lang#53189

r? @pnkfelix

Author: bors

src/librustc_mir/borrow_check/mod.rs

@@ -798,12 +798,6 @@ enum LocalMutationIsAllowed {
     No,
 }
 
-struct AccessErrorsReported {
-    mutability_error: bool,
-    #[allow(dead_code)]
-    conflict_error: bool,
-}
-
 #[derive(Copy, Clone)]
 enum InitializationRequiringAction {
     Update,
@@ -1072,7 +1066,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         kind: (ShallowOrDeep, ReadOrWrite),
         is_local_mutation_allowed: LocalMutationIsAllowed,
         flow_state: &Flows<'cx, 'gcx, 'tcx>,
-    ) -> AccessErrorsReported {
+    ) {
         let (sd, rw) = kind;
 
         if let Activation(_, borrow_index) = rw {
@@ -1082,10 +1076,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                      place: {:?} borrow_index: {:?}",
                     place_span.0, borrow_index
                 );
-                return AccessErrorsReported {
-                    mutability_error: false,
-                    conflict_error: true,
-                };
+                return;
             }
         }
 
@@ -1097,10 +1088,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 "access_place: suppressing error place_span=`{:?}` kind=`{:?}`",
                 place_span, kind
             );
-            return AccessErrorsReported {
-                mutability_error: false,
-                conflict_error: true,
-            };
+            return;
         }
 
         let mutability_error =
@@ -1122,11 +1110,6 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             self.access_place_error_reported
                 .insert((place_span.0.clone(), place_span.1));
         }
-
-        AccessErrorsReported {
-            mutability_error,
-            conflict_error,
-        }
     }
 
     fn check_access_for_conflict(
@@ -1275,23 +1258,30 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             }
         }
 
-        let errors_reported = self.access_place(
+        // 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;
+            }
+        }
+
+        // Otherwise, use the normal access permission rules.
+        self.access_place(
             context,
             place_span,
             (kind, Write(WriteKind::Mutate)),
-            // We want immutable upvars to cause an "assignment to immutable var"
-            // error, not an "reassignment of immutable var" error, because the
-            // latter can't find a good previous assignment span.
-            //
-            // There's probably a better way to do this.
-            LocalMutationIsAllowed::ExceptUpvars,
+            LocalMutationIsAllowed::No,
             flow_state,
         );
-
-        if !errors_reported.mutability_error {
-            // check for reassignments to immutable local variables
-            self.check_if_reassignment_to_immutable_state(context, place_span, flow_state);
-        }
     }
 
     fn consume_rvalue(
@@ -1590,27 +1580,20 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
     fn check_if_reassignment_to_immutable_state(
         &mut self,
         context: Context,
-        (place, span): (&Place<'tcx>, Span),
+        local: Local,
+        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).
-        let err_place = match self.is_mutable(place, LocalMutationIsAllowed::No) {
-            Ok(..) => return,
-            Err(place) => place,
-        };
-        debug!(
-            "check_if_reassignment_to_immutable_state({:?}) - is an imm local",
-            place
-        );
-
-        for i in flow_state.ever_inits.iter_incoming() {
-            let init = self.move_data.inits[i];
-            let init_place = &self.move_data.move_paths[init.path].place;
-            if places_conflict::places_conflict(self.tcx, self.mir, &init_place, place, Deep) {
-                self.report_illegal_reassignment(context, (place, span), init.span, err_place);
-                break;
-            }
+        debug!("check_if_reassignment_to_immutable_state({:?})", local);
+
+        // Check if any of the initializiations of `local` have happened yet:
+        let mpi = self.move_data.rev_lookup.find_local(local);
+        let init_indices = &self.move_data.init_path_map[mpi];
+        let first_init_index = init_indices.iter().find(|ii| flow_state.ever_inits.contains(ii));
+        if let Some(&init_index) = first_init_index {
+            // And, if so, report an error.
+            let init = &self.move_data.inits[init_index];
+            self.report_illegal_reassignment(context, place_span, init.span, place_span.0);
         }
     }
 

src/test/ui/borrowck/assign_mutable_fields.nll.stderr

@@ -0,0 +1,9 @@
+error[E0381]: use of possibly uninitialized variable: `x`
+  --> $DIR/assign_mutable_fields.rs:29:10
+   |
+LL |     drop(x); //~ ERROR
+   |          ^ use of possibly uninitialized `x`
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0381`.

src/test/ui/borrowck/assign_mutable_fields.rs

@@ -0,0 +1,32 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Currently, we permit you to assign to individual fields of a mut
+// var, but we do not permit you to use the complete var afterwards.
+// We hope to fix this at some point.
+//
+// FIXME(#21232)
+
+fn assign_both_fields_and_use() {
+    let mut x: (u32, u32);
+    x.0 = 1;
+    x.1 = 22;
+    drop(x.0); //~ ERROR
+    drop(x.1); //~ ERROR
+}
+
+fn assign_both_fields_the_use_var() {
+    let mut x: (u32, u32);
+    x.0 = 1;
+    x.1 = 22;
+    drop(x); //~ ERROR
+}
+
+fn main() { }

src/test/ui/borrowck/assign_mutable_fields.stderr

@@ -0,0 +1,21 @@
+error[E0381]: use of possibly uninitialized variable: `x.0`
+  --> $DIR/assign_mutable_fields.rs:21:10
+   |
+LL |     drop(x.0); //~ ERROR
+   |          ^^^ use of possibly uninitialized `x.0`
+
+error[E0381]: use of possibly uninitialized variable: `x.1`
+  --> $DIR/assign_mutable_fields.rs:22:10
+   |
+LL |     drop(x.1); //~ ERROR
+   |          ^^^ use of possibly uninitialized `x.1`
+
+error[E0381]: use of possibly uninitialized variable: `x`
+  --> $DIR/assign_mutable_fields.rs:29:10
+   |
+LL |     drop(x); //~ ERROR
+   |          ^ use of possibly uninitialized `x`
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0381`.

src/test/ui/borrowck/reassignment_immutable_fields.nll.stderr

@@ -0,0 +1,44 @@
+error[E0594]: cannot assign to `x.0`, as `x` is not declared as mutable
+  --> $DIR/reassignment_immutable_fields.rs:17:5
+   |
+LL |     let x: (u32, u32);
+   |         - help: consider changing this to be mutable: `mut x`
+LL |     x.0 = 1; //~ ERROR
+   |     ^^^^^^^ cannot assign
+
+error[E0594]: cannot assign to `x.1`, as `x` is not declared as mutable
+  --> $DIR/reassignment_immutable_fields.rs:18:5
+   |
+LL |     let x: (u32, u32);
+   |         - help: consider changing this to be mutable: `mut x`
+LL |     x.0 = 1; //~ ERROR
+LL |     x.1 = 22; //~ ERROR
+   |     ^^^^^^^^ cannot assign
+
+error[E0594]: cannot assign to `x.0`, as `x` is not declared as mutable
+  --> $DIR/reassignment_immutable_fields.rs:25:5
+   |
+LL |     let x: (u32, u32);
+   |         - help: consider changing this to be mutable: `mut x`
+LL |     x.0 = 1; //~ ERROR
+   |     ^^^^^^^ cannot assign
+
+error[E0594]: cannot assign to `x.1`, as `x` is not declared as mutable
+  --> $DIR/reassignment_immutable_fields.rs:26:5
+   |
+LL |     let x: (u32, u32);
+   |         - help: consider changing this to be mutable: `mut x`
+LL |     x.0 = 1; //~ ERROR
+LL |     x.1 = 22; //~ ERROR
+   |     ^^^^^^^^ cannot assign
+
+error[E0381]: use of possibly uninitialized variable: `x`
+  --> $DIR/reassignment_immutable_fields.rs:27:10
+   |
+LL |     drop(x); //~ ERROR
+   |          ^ use of possibly uninitialized `x`
+
+error: aborting due to 5 previous errors
+
+Some errors occurred: E0381, E0594.
+For more information about an error, try `rustc --explain E0381`.

src/test/ui/borrowck/reassignment_immutable_fields.rs

@@ -0,0 +1,30 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// This test is currently disallowed, but we hope someday to support it.
+//
+// FIXME(#21232)
+
+fn assign_both_fields_and_use() {
+    let x: (u32, u32);
+    x.0 = 1; //~ ERROR
+    x.1 = 22; //~ ERROR
+    drop(x.0); //~ ERROR
+    drop(x.1); //~ ERROR
+}
+
+fn assign_both_fields_the_use_var() {
+    let x: (u32, u32);
+    x.0 = 1; //~ ERROR
+    x.1 = 22; //~ ERROR
+    drop(x); //~ ERROR
+}
+
+fn main() { }

src/test/ui/borrowck/reassignment_immutable_fields.stderr

@@ -0,0 +1,56 @@
+error[E0594]: cannot assign to field `x.0` of immutable binding
+  --> $DIR/reassignment_immutable_fields.rs:17:5
+   |
+LL |     let x: (u32, u32);
+   |         - consider changing this to `mut x`
+LL |     x.0 = 1; //~ ERROR
+   |     ^^^^^^^ cannot mutably borrow field of immutable binding
+
+error[E0594]: cannot assign to field `x.1` of immutable binding
+  --> $DIR/reassignment_immutable_fields.rs:18:5
+   |
+LL |     let x: (u32, u32);
+   |         - consider changing this to `mut x`
+LL |     x.0 = 1; //~ ERROR
+LL |     x.1 = 22; //~ ERROR
+   |     ^^^^^^^^ cannot mutably borrow field of immutable binding
+
+error[E0381]: use of possibly uninitialized variable: `x.0`
+  --> $DIR/reassignment_immutable_fields.rs:19:10
+   |
+LL |     drop(x.0); //~ ERROR
+   |          ^^^ use of possibly uninitialized `x.0`
+
+error[E0381]: use of possibly uninitialized variable: `x.1`
+  --> $DIR/reassignment_immutable_fields.rs:20:10
+   |
+LL |     drop(x.1); //~ ERROR
+   |          ^^^ use of possibly uninitialized `x.1`
+
+error[E0594]: cannot assign to field `x.0` of immutable binding
+  --> $DIR/reassignment_immutable_fields.rs:25:5
+   |
+LL |     let x: (u32, u32);
+   |         - consider changing this to `mut x`
+LL |     x.0 = 1; //~ ERROR
+   |     ^^^^^^^ cannot mutably borrow field of immutable binding
+
+error[E0594]: cannot assign to field `x.1` of immutable binding
+  --> $DIR/reassignment_immutable_fields.rs:26:5
+   |
+LL |     let x: (u32, u32);
+   |         - consider changing this to `mut x`
+LL |     x.0 = 1; //~ ERROR
+LL |     x.1 = 22; //~ ERROR
+   |     ^^^^^^^^ cannot mutably borrow field of immutable binding
+
+error[E0381]: use of possibly uninitialized variable: `x`
+  --> $DIR/reassignment_immutable_fields.rs:27:10
+   |
+LL |     drop(x); //~ ERROR
+   |          ^ use of possibly uninitialized `x`
+
+error: aborting due to 7 previous errors
+
+Some errors occurred: E0381, E0594.
+For more information about an error, try `rustc --explain E0381`.

src/test/ui/borrowck/reassignment_immutable_fields_overlapping.nll.stderr

@@ -0,0 +1,20 @@
+error[E0594]: cannot assign to `x.a`, as `x` is not declared as mutable
+  --> $DIR/reassignment_immutable_fields_overlapping.rs:22:5
+   |
+LL |     let x: Foo;
+   |         - help: consider changing this to be mutable: `mut x`
+LL |     x.a = 1;  //~ ERROR
+   |     ^^^^^^^ cannot assign
+
+error[E0594]: cannot assign to `x.b`, as `x` is not declared as mutable
+  --> $DIR/reassignment_immutable_fields_overlapping.rs:23:5
+   |
+LL |     let x: Foo;
+   |         - help: consider changing this to be mutable: `mut x`
+LL |     x.a = 1;  //~ ERROR
+LL |     x.b = 22; //~ ERROR
+   |     ^^^^^^^^ cannot assign
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0594`.

src/test/ui/borrowck/reassignment_immutable_fields_overlapping.rs

@@ -0,0 +1,26 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// This should never be allowed -- `foo.a` and `foo.b` are
+// overlapping, so since `x` is not `mut` we should not permit
+// reassignment.
+
+union Foo {
+    a: u32,
+    b: u32,
+}
+
+unsafe fn overlapping_fields() {
+    let x: Foo;
+    x.a = 1;  //~ ERROR
+    x.b = 22; //~ ERROR
+}
+
+fn main() { }