]> git.lizzy.rs Git - rust.git/commitdiff
Clean-up assignment checking in borrowck
authorAriel Ben-Yehuda <ariel.byd@gmail.com>
Wed, 17 Jun 2015 18:54:22 +0000 (21:54 +0300)
committerAriel Ben-Yehuda <arielb1@mail.tau.ac.il>
Tue, 30 Jun 2015 21:12:12 +0000 (00:12 +0300)
src/librustc_borrowck/borrowck/check_loans.rs
src/librustc_borrowck/borrowck/gather_loans/lifetime.rs
src/librustc_borrowck/borrowck/gather_loans/mod.rs
src/librustc_borrowck/borrowck/gather_loans/restrictions.rs
src/librustc_borrowck/borrowck/mod.rs

index 9d4fb4c994d404bd18fa2046567282aa8b7f8dcc..f06dc105d9cc1389e803a82749b76a308b87d905 100644 (file)
@@ -565,13 +565,6 @@ pub fn report_error_if_loan_conflicts_with_restriction(&self,
         true
     }
 
-    fn is_local_variable_or_arg(&self, cmt: mc::cmt<'tcx>) -> bool {
-        match cmt.cat {
-          mc::cat_local(_) => true,
-          _ => false
-        }
-    }
-
     fn consume_common(&self,
                       id: ast::NodeId,
                       span: Span,
@@ -793,198 +786,40 @@ fn check_assignment(&self,
                         mode: euv::MutateMode) {
         debug!("check_assignment(assignee_cmt={:?})", assignee_cmt);
 
-        // Mutable values can be assigned, as long as they obey loans
-        // and aliasing restrictions:
-        if assignee_cmt.mutbl.is_mutable() {
-            if check_for_aliasable_mutable_writes(self, assignment_span, assignee_cmt.clone()) {
-                if mode != euv::Init {
-                    check_for_assignment_to_borrowed_path(
-                        self, assignment_id, assignment_span, assignee_cmt.clone());
-                    mark_variable_as_used_mut(self, assignee_cmt);
-                }
-            }
-
-            return;
-        }
-
-        // Initializations are OK if and only if they aren't partial
-        // reinitialization of a partially-uninitialized structure.
+        // Initializations never cause borrow errors as they only
+        // affect a fresh local.
         if mode == euv::Init {
             return
         }
 
-        // For immutable local variables, assignments are legal
-        // if they cannot already have been assigned
-        if self.is_local_variable_or_arg(assignee_cmt.clone()) {
-            assert!(assignee_cmt.mutbl.is_immutable()); // no "const" locals
-            let lp = opt_loan_path(&assignee_cmt).unwrap();
-            self.move_data.each_assignment_of(assignment_id, &lp, |assign| {
-                self.bccx.report_reassigned_immutable_variable(
-                    assignment_span,
-                    &*lp,
-                    assign);
+        // Check that we don't invalidate any outstanding loans
+        if let Some(loan_path) = opt_loan_path(&assignee_cmt) {
+            let scope = region::CodeExtent::from_node_id(assignment_id);
+            self.each_in_scope_loan_affecting_path(scope, &*loan_path, |loan| {
+                self.report_illegal_mutation(assignment_span, &*loan_path, loan);
                 false
             });
-            return;
-        }
-
-        // Otherwise, just a plain error.
-        match assignee_cmt.note {
-            mc::NoteClosureEnv(upvar_id) => {
-                // If this is an `Fn` closure, it simply can't mutate upvars.
-                // If it's an `FnMut` closure, the original variable was declared immutable.
-                // We need to determine which is the case here.
-                let kind = match assignee_cmt.upvar().unwrap().cat {
-                    mc::cat_upvar(mc::Upvar { kind, .. }) => kind,
-                    _ => unreachable!()
-                };
-                if kind == ty::FnClosureKind {
-                    self.bccx.span_err(
-                        assignment_span,
-                        &format!("cannot assign to {}",
-                                self.bccx.cmt_to_string(&*assignee_cmt)));
-                    self.bccx.span_help(
-                        self.tcx().map.span(upvar_id.closure_expr_id),
-                        "consider changing this closure to take self by mutable reference");
-                } else {
-                    self.bccx.span_err(
-                        assignment_span,
-                        &format!("cannot assign to {} {}",
-                                assignee_cmt.mutbl.to_user_str(),
-                                self.bccx.cmt_to_string(&*assignee_cmt)));
-                }
-            }
-            _ => match opt_loan_path(&assignee_cmt) {
-                Some(lp) => {
-                    self.bccx.span_err(
-                        assignment_span,
-                        &format!("cannot assign to {} {} `{}`",
-                                assignee_cmt.mutbl.to_user_str(),
-                                self.bccx.cmt_to_string(&*assignee_cmt),
-                                self.bccx.loan_path_to_string(&*lp)));
-                }
-                None => {
-                    self.bccx.span_err(
-                        assignment_span,
-                        &format!("cannot assign to {} {}",
-                                assignee_cmt.mutbl.to_user_str(),
-                                self.bccx.cmt_to_string(&*assignee_cmt)));
-                }
-            }
-        }
-        return;
-
-        fn mark_variable_as_used_mut<'a, 'tcx>(this: &CheckLoanCtxt<'a, 'tcx>,
-                                               mut cmt: mc::cmt<'tcx>) {
-            //! If the mutability of the `cmt` being written is inherited
-            //! from a local variable, liveness will
-            //! not have been able to detect that this variable's mutability
-            //! is important, so we must add the variable to the
-            //! `used_mut_nodes` table here.
-
-            loop {
-                debug!("mark_variable_as_used_mut(cmt={:?})", cmt);
-                match cmt.cat.clone() {
-                    mc::cat_upvar(mc::Upvar { id: ty::UpvarId { var_id: id, .. }, .. }) |
-                    mc::cat_local(id) => {
-                        this.tcx().used_mut_nodes.borrow_mut().insert(id);
-                        return;
-                    }
-
-                    mc::cat_rvalue(..) |
-                    mc::cat_static_item |
-                    mc::cat_deref(_, _, mc::UnsafePtr(..)) |
-                    mc::cat_deref(_, _, mc::Implicit(..)) => {
-                        assert_eq!(cmt.mutbl, mc::McDeclared);
-                        return;
-                    }
-
-                    mc::cat_deref(_, _, mc::BorrowedPtr(..)) => {
-                        assert_eq!(cmt.mutbl, mc::McDeclared);
-                        // We need to drill down to upvar if applicable
-                        match cmt.upvar() {
-                            Some(b) => cmt = b,
-                            None => return
-                        }
-                    }
-
-                    mc::cat_deref(b, _, mc::Unique) => {
-                        assert_eq!(cmt.mutbl, mc::McInherited);
-                        cmt = b;
-                    }
-
-                    mc::cat_downcast(b, _) |
-                    mc::cat_interior(b, _) => {
-                        assert_eq!(cmt.mutbl, mc::McInherited);
-                        cmt = b;
-                    }
-                }
-            }
         }
 
-        fn check_for_aliasable_mutable_writes<'a, 'tcx>(this: &CheckLoanCtxt<'a, 'tcx>,
-                                                        span: Span,
-                                                        cmt: mc::cmt<'tcx>) -> bool {
-            //! Safety checks related to writes to aliasable, mutable locations
-
-            let guarantor = cmt.guarantor();
-            debug!("check_for_aliasable_mutable_writes(cmt={:?}, guarantor={:?})",
-                   cmt, guarantor);
-            if let mc::cat_deref(ref b, _, mc::BorrowedPtr(ty::MutBorrow, _)) = guarantor.cat {
-                // Statically prohibit writes to `&mut` when aliasable
-                check_for_aliasability_violation(this, span, b.clone());
+        // Local variables can always be assigned to, expect for reassignments
+        // of immutable variables (or assignments that invalidate loans,
+        // of course).
+        if let mc::cat_local(local_id) = assignee_cmt.cat {
+            if assignee_cmt.mutbl.is_mutable() {
+                self.tcx().used_mut_nodes.borrow_mut().insert(local_id);
             }
 
-            return true; // no errors reported
-        }
-
-        fn check_for_aliasability_violation<'a, 'tcx>(this: &CheckLoanCtxt<'a, 'tcx>,
-                                                      span: Span,
-                                                      cmt: mc::cmt<'tcx>)
-                                                      -> bool {
-            match cmt.freely_aliasable(this.tcx()) {
-                mc::Aliasability::NonAliasable => {
-                    return true;
-                }
-                mc::Aliasability::FreelyAliasable(mc::AliasableStaticMut(..)) => {
-                    return true;
-                }
-                mc::Aliasability::ImmutableUnique(_) => {
-                    this.bccx.report_aliasability_violation(
-                        span,
-                        MutabilityViolation,
-                        mc::AliasableReason::UnaliasableImmutable);
-                    return false;
-                }
-                mc::Aliasability::FreelyAliasable(cause) => {
-                    this.bccx.report_aliasability_violation(
-                        span,
-                        MutabilityViolation,
-                        cause);
-                    return false;
+            let lp = opt_loan_path(&assignee_cmt).unwrap();
+            self.move_data.each_assignment_of(assignment_id, &lp, |assign| {
+                if !assignee_cmt.mutbl.is_mutable() {
+                    self.bccx.report_reassigned_immutable_variable(
+                        assignment_span,
+                        &*lp,
+                        assign);
                 }
-            }
-        }
-
-        fn check_for_assignment_to_borrowed_path<'a, 'tcx>(
-            this: &CheckLoanCtxt<'a, 'tcx>,
-            assignment_id: ast::NodeId,
-            assignment_span: Span,
-            assignee_cmt: mc::cmt<'tcx>)
-        {
-            //! Check for assignments that violate the terms of an
-            //! outstanding loan.
-
-            let loan_path = match opt_loan_path(&assignee_cmt) {
-                Some(lp) => lp,
-                None => { return; /* no loan path, can't be any loans */ }
-            };
-
-            let scope = region::CodeExtent::from_node_id(assignment_id);
-            this.each_in_scope_loan_affecting_path(scope, &*loan_path, |loan| {
-                this.report_illegal_mutation(assignment_span, &*loan_path, loan);
                 false
             });
+            return
         }
     }
 
index 427d78e89b3e2dbe11a787298ac418e45c1f7d5d..919bc45f00ddfe955e4ae188574ef83af8cb3a54 100644 (file)
@@ -137,7 +137,7 @@ fn scope(&self, cmt: &mc::cmt) -> ty::Region {
     fn report_error(&self, code: bckerr_code) {
         self.bccx.report(BckError { cmt: self.cmt_original.clone(),
                                     span: self.span,
-                                    cause: self.cause,
+                                    cause: BorrowViolation(self.cause),
                                     code: code });
     }
 }
index 44a4a0d250402ec8c2493b15f374eef1d8275c09..39b9f07604353e864337938e1dd5b6c988d40f9a 100644 (file)
@@ -151,22 +151,10 @@ fn mutate(&mut self,
               assignee_cmt: mc::cmt<'tcx>,
               mode: euv::MutateMode)
     {
-        let opt_lp = opt_loan_path(&assignee_cmt);
-        debug!("mutate(assignment_id={}, assignee_cmt={:?}) opt_lp={:?}",
-               assignment_id, assignee_cmt, opt_lp);
-
-        match opt_lp {
-            Some(lp) => {
-                gather_moves::gather_assignment(self.bccx, &self.move_data,
-                                                assignment_id, assignment_span,
-                                                lp, assignee_cmt.id, mode);
-            }
-            None => {
-                // This can occur with e.g. `*foo() = 5`.  In such
-                // cases, there is no need to check for conflicts
-                // with moves etc, just ignore.
-            }
-        }
+        self.guarantee_assignment_valid(assignment_id,
+                                        assignment_span,
+                                        assignee_cmt,
+                                        mode);
     }
 
     fn decl_without_init(&mut self, id: ast::NodeId, span: Span) {
@@ -177,7 +165,7 @@ fn decl_without_init(&mut self, id: ast::NodeId, span: Span) {
 /// Implements the A-* rules in README.md.
 fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
                                 borrow_span: Span,
-                                loan_cause: euv::LoanCause,
+                                loan_cause: AliasableViolationKind,
                                 cmt: mc::cmt<'tcx>,
                                 req_kind: ty::BorrowKind)
                                 -> Result<(),()> {
@@ -203,7 +191,7 @@ fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
         (mc::Aliasability::ImmutableUnique(_), ty::MutBorrow) => {
             bccx.report_aliasability_violation(
                         borrow_span,
-                        BorrowViolation(loan_cause),
+                        loan_cause,
                         mc::AliasableReason::UnaliasableImmutable);
             Err(())
         }
@@ -211,7 +199,7 @@ fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
         (mc::Aliasability::FreelyAliasable(alias_cause), ty::MutBorrow) => {
             bccx.report_aliasability_violation(
                         borrow_span,
-                        BorrowViolation(loan_cause),
+                        loan_cause,
                         alias_cause);
             Err(())
         }
@@ -221,9 +209,94 @@ fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
     }
 }
 
+/// Implements the M-* rules in README.md.
+fn check_mutability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
+                              borrow_span: Span,
+                              cause: AliasableViolationKind,
+                              cmt: mc::cmt<'tcx>,
+                              req_kind: ty::BorrowKind)
+                              -> Result<(),()> {
+    debug!("check_mutability(cause={:?} cmt={:?} req_kind={:?}",
+           cause, cmt, req_kind);
+    match req_kind {
+        ty::UniqueImmBorrow | ty::ImmBorrow => {
+            match cmt.mutbl {
+                // I am intentionally leaving this here to help
+                // refactoring if, in the future, we should add new
+                // kinds of mutability.
+                mc::McImmutable | mc::McDeclared | mc::McInherited => {
+                    // both imm and mut data can be lent as imm;
+                    // for mutable data, this is a freeze
+                    Ok(())
+                }
+            }
+        }
+
+        ty::MutBorrow => {
+            // Only mutable data can be lent as mutable.
+            if !cmt.mutbl.is_mutable() {
+                Err(bccx.report(BckError { span: borrow_span,
+                                           cause: cause,
+                                           cmt: cmt,
+                                           code: err_mutbl }))
+            } else {
+                Ok(())
+            }
+        }
+    }
+}
+
 impl<'a, 'tcx> GatherLoanCtxt<'a, 'tcx> {
     pub fn tcx(&self) -> &'a ty::ctxt<'tcx> { self.bccx.tcx }
 
+    /// Guarantees that `cmt` is assignable, or reports an error.
+    fn guarantee_assignment_valid(&mut self,
+                                  assignment_id: ast::NodeId,
+                                  assignment_span: Span,
+                                  cmt: mc::cmt<'tcx>,
+                                  mode: euv::MutateMode) {
+
+        let opt_lp = opt_loan_path(&cmt);
+        debug!("guarantee_assignment_valid(assignment_id={}, cmt={:?}) opt_lp={:?}",
+               assignment_id, cmt, opt_lp);
+
+        if let mc::cat_local(..) = cmt.cat {
+            // Only re-assignments to locals require it to be
+            // mutable - this is checked in check_loans.
+        } else {
+            // Check that we don't allow assignments to non-mutable data.
+            if check_mutability(self.bccx, assignment_span, MutabilityViolation,
+                                cmt.clone(), ty::MutBorrow).is_err() {
+                return; // reported an error, no sense in reporting more.
+            }
+        }
+
+        // Check that we don't allow assignments to aliasable data
+        if check_aliasability(self.bccx, assignment_span, MutabilityViolation,
+                              cmt.clone(), ty::MutBorrow).is_err() {
+            return; // reported an error, no sense in reporting more.
+        }
+
+        match opt_lp {
+            Some(lp) => {
+                if let mc::cat_local(..) = cmt.cat {
+                    // Only re-assignments to locals require it to be
+                    // mutable - this is checked in check_loans.
+                } else {
+                    self.mark_loan_path_as_mutated(&lp);
+                }
+                gather_moves::gather_assignment(self.bccx, &self.move_data,
+                                                assignment_id, assignment_span,
+                                                lp, cmt.id, mode);
+            }
+            None => {
+                // This can occur with e.g. `*foo() = 5`.  In such
+                // cases, there is no need to check for conflicts
+                // with moves etc, just ignore.
+            }
+        }
+    }
+
     /// Guarantees that `addr_of(cmt)` will be valid for the duration of `static_scope_r`, or
     /// reports an error.  This may entail taking out loans, which will be added to the
     /// `req_loan_map`.
@@ -256,13 +329,13 @@ fn guarantee_valid(&mut self,
         }
 
         // Check that we don't allow mutable borrows of non-mutable data.
-        if check_mutability(self.bccx, borrow_span, cause,
+        if check_mutability(self.bccx, borrow_span, BorrowViolation(cause),
                             cmt.clone(), req_kind).is_err() {
             return; // reported an error, no sense in reporting more.
         }
 
         // Check that we don't allow mutable borrows of aliasable data.
-        if check_aliasability(self.bccx, borrow_span, cause,
+        if check_aliasability(self.bccx, borrow_span, BorrowViolation(cause),
                               cmt.clone(), req_kind).is_err() {
             return; // reported an error, no sense in reporting more.
         }
@@ -368,43 +441,6 @@ fn guarantee_valid(&mut self,
             //        restrictions: restrictions
             //    }
         // }
-
-        fn check_mutability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
-                                      borrow_span: Span,
-                                      cause: euv::LoanCause,
-                                      cmt: mc::cmt<'tcx>,
-                                      req_kind: ty::BorrowKind)
-                                      -> Result<(),()> {
-            //! Implements the M-* rules in README.md.
-            debug!("check_mutability(cause={:?} cmt={:?} req_kind={:?}",
-                   cause, cmt, req_kind);
-            match req_kind {
-                ty::UniqueImmBorrow | ty::ImmBorrow => {
-                    match cmt.mutbl {
-                        // I am intentionally leaving this here to help
-                        // refactoring if, in the future, we should add new
-                        // kinds of mutability.
-                        mc::McImmutable | mc::McDeclared | mc::McInherited => {
-                            // both imm and mut data can be lent as imm;
-                            // for mutable data, this is a freeze
-                            Ok(())
-                        }
-                    }
-                }
-
-                ty::MutBorrow => {
-                    // Only mutable data can be lent as mutable.
-                    if !cmt.mutbl.is_mutable() {
-                        Err(bccx.report(BckError { span: borrow_span,
-                                                   cause: cause,
-                                                   cmt: cmt,
-                                                   code: err_mutbl }))
-                    } else {
-                        Ok(())
-                    }
-                }
-            }
-        }
     }
 
     pub fn mark_loan_path_as_mutated(&self, loan_path: &LoanPath) {
@@ -495,7 +531,8 @@ fn visit_expr(&mut self, ex: &Expr) {
             let base_cmt = mc.cat_expr(&**base).unwrap();
             let borrow_kind = ty::BorrowKind::from_mutbl(mutbl);
             // Check that we don't allow borrows of unsafe static items.
-            if check_aliasability(self.bccx, ex.span, euv::AddrOf,
+            if check_aliasability(self.bccx, ex.span,
+                                  BorrowViolation(euv::AddrOf),
                                   base_cmt, borrow_kind).is_err() {
                 return; // reported an error, no sense in reporting more.
             }
index 345f5378f69e106c0fd3142f66d31f94229f3de1..4c186dd840610a071816952be41a50c671314d4f 100644 (file)
@@ -124,7 +124,7 @@ fn restrict(&self,
                             self.bccx.report(
                                 BckError {
                                     span: self.span,
-                                    cause: self.cause,
+                                    cause: BorrowViolation(self.cause),
                                     cmt: cmt_base,
                                     code: err_borrowed_pointer_too_short(
                                         self.loan_region, lt)});
index 4f726044a1bac866f385396a1608beec40076175..65e3e443e7de2e0222d89212370a628d10998a61 100644 (file)
@@ -546,12 +546,12 @@ pub enum bckerr_code {
 #[derive(PartialEq)]
 pub struct BckError<'tcx> {
     span: Span,
-    cause: euv::LoanCause,
+    cause: AliasableViolationKind,
     cmt: mc::cmt<'tcx>,
     code: bckerr_code
 }
 
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, Debug, PartialEq)]
 pub enum AliasableViolationKind {
     MutabilityViolation,
     BorrowViolation(euv::LoanCause)
@@ -576,8 +576,10 @@ pub fn is_subregion_of(&self, r_sub: ty::Region, r_sup: ty::Region)
     pub fn report(&self, err: BckError<'tcx>) {
         // Catch and handle some particular cases.
         match (&err.code, &err.cause) {
-            (&err_out_of_scope(ty::ReScope(_), ty::ReStatic), &euv::ClosureCapture(span)) |
-            (&err_out_of_scope(ty::ReScope(_), ty::ReFree(..)), &euv::ClosureCapture(span)) => {
+            (&err_out_of_scope(ty::ReScope(_), ty::ReStatic),
+             &BorrowViolation(euv::ClosureCapture(span))) |
+            (&err_out_of_scope(ty::ReScope(_), ty::ReFree(..)),
+             &BorrowViolation(euv::ClosureCapture(span))) => {
                 return self.report_out_of_scope_escaping_closure_capture(&err, span);
             }
             _ => { }
@@ -796,10 +798,6 @@ pub fn span_end_note(&self, s: Span, m: &str) {
         self.tcx.sess.span_end_note(s, m);
     }
 
-    pub fn span_help(&self, s: Span, m: &str) {
-        self.tcx.sess.span_help(s, m);
-    }
-
     pub fn fileline_help(&self, s: Span, m: &str) {
         self.tcx.sess.fileline_help(s, m);
     }
@@ -827,19 +825,22 @@ pub fn bckerr_to_string(&self, err: &BckError<'tcx>) -> String {
                 };
 
                 match err.cause {
-                    euv::ClosureCapture(_) => {
+                    MutabilityViolation => {
+                        format!("cannot assign to {}", descr)
+                    }
+                    BorrowViolation(euv::ClosureCapture(_)) => {
                         format!("closure cannot assign to {}", descr)
                     }
-                    euv::OverloadedOperator |
-                    euv::AddrOf |
-                    euv::RefBinding |
-                    euv::AutoRef |
-                    euv::AutoUnsafe |
-                    euv::ForLoop |
-                    euv::MatchDiscriminant => {
+                    BorrowViolation(euv::OverloadedOperator) |
+                    BorrowViolation(euv::AddrOf) |
+                    BorrowViolation(euv::RefBinding) |
+                    BorrowViolation(euv::AutoRef) |
+                    BorrowViolation(euv::AutoUnsafe) |
+                    BorrowViolation(euv::ForLoop) |
+                    BorrowViolation(euv::MatchDiscriminant) => {
                         format!("cannot borrow {} as mutable", descr)
                     }
-                    euv::ClosureInvocation => {
+                    BorrowViolation(euv::ClosureInvocation) => {
                         self.tcx.sess.span_bug(err.span,
                             "err_mutbl with a closure invocation");
                     }