MIR-borrowck: add permisson checks to fn access_lvalue
#45436
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
kennytm
added
the
S-waiting-on-author
|
Triage ping @zilbuz! How's it going? Have any questions/need any help? |
|
☔ The latest upstream changes (presumably #45538) made this pull request unmergeable. Please resolve the merge conflicts. |
zilbuz
force-pushed
the
issue-44837
branch
2 times, most recently
from
7424113 to
4652b33
Compare
src/librustc_mir/borrow_check.rs
Outdated
| fn check_access_permissions(&self, (lvalue, span): (&Lvalue<'tcx>, Span), kind: ReadOrWrite) { | ||
| match kind { | ||
| Write(WriteKind::MutableBorrow(BorrowKind::Unique)) | ||
| | Write(WriteKind::MutableBorrow(BorrowKind::Mut)) => { |
There was a problem hiding this comment.
Nit: the | goes at the end of the line before under typical style.
| ty::TyRef(_, tnm) => { | ||
| match tnm.mutbl { | ||
| // lvalue represent an aliased location | ||
| hir::MutImmutable => Err(lvalue), |
There was a problem hiding this comment.
Actually, it appears we do allow the use of &mut there. This example compiles:
fn foo(x: *const &mut u32) {
unsafe {
**x += 1;
}
}
fn main() { }
There was a problem hiding this comment.
So I was wrong in my writeup, we should just treat *const the same as *mut.
There was a problem hiding this comment.
Er, I meant this for the raw pointer cases below, sorry =) comment on wrong line
src/librustc_mir/borrow_check.rs
Outdated
| /// Check the permissions for the given lvalue and read or write kind | ||
| fn check_access_permissions(&self, (lvalue, span): (&Lvalue<'tcx>, Span), kind: ReadOrWrite) { | ||
| match kind { | ||
| Write(WriteKind::MutableBorrow(BorrowKind::Unique)) |
There was a problem hiding this comment.
Hmm I forgot about unique borrows when writing up those rules. They have a different set of conditions than mutable borrows. I think that this actually lines up perfectly with what I called can_host_mutable_ref, which we could perhaps just call is_unique. I forget why I changed it from that name. We should just call it is_unique, and then check that Write(WriteKind::MutableBorrow(BorrowKind::Unique)) requires is_unique.
src/librustc_mir/borrow_check.rs
Outdated
| ty::TyRawPtr(tnm) => { | ||
| match tnm.mutbl { | ||
| // `*const` is assumed to be aliasable | ||
| hir::MutImmutable => Err(lvalue), |
There was a problem hiding this comment.
That is, this should be Ok(()).
src/librustc_mir/borrow_check.rs
Outdated
| } | ||
| }, | ||
| // Other projections are unique if the base is unique | ||
| _ => self.can_host_mutable_ref(&proj.base) |
There was a problem hiding this comment.
I'd prefer to avoid the _ here and instead just write out all the cases explicitly. _ makes it much harder to audit. My usual rule is something like this:
- If a new case were added to
ProjectionKind, what is the chance that this match would be affected?
I'd say "pretty high", therefore we should avoid _.
There was a problem hiding this comment.
Should I use bug!("Deref of unexpected type") here? Like in is_mutable()?
src/librustc_mir/borrow_check.rs
Outdated
| } | ||
| }, | ||
| // Other projections are unique if the base is unique | ||
| _ => self.can_host_mutable_ref(&proj.base) |
src/librustc_mir/borrow_check.rs
Outdated
| | ProjectionElem::ConstantIndex { .. } | ||
| | ProjectionElem::Subslice { .. } => Err(lvalue), | ||
| // In general we can move from owned content if owner is mobile | ||
| _ => self.is_mobile(&proj.base) |
src/librustc_mir/borrow_check.rs
Outdated
| // dynamic drop elaboration | ||
| ProjectionElem::Index(..) | ||
| | ProjectionElem::ConstantIndex { .. } | ||
| | ProjectionElem::Subslice { .. } => Err(lvalue), |
src/librustc_mir/borrow_check.rs
Outdated
| }, | ||
| // All other projections are owned by their base path, so mutable if | ||
| // base path is mutable | ||
| _ => self.is_mutable(&proj.base) |
There was a problem hiding this comment.
Avoid _ here (see other comments for rationale).
There was a problem hiding this comment.
Actually, in this particular case, I think we could keep the _, but write bug!("deref of unexpected type: {:?}", base_ty) -- deref should (I believe) only be for &T, *const T, or Box<T>.
src/librustc_mir/borrow_check.rs
Outdated
| }, | ||
| // All other projections are owned by their base path, so mutable if | ||
| // base path is mutable | ||
| _ => self.is_mutable(&proj.base) |
There was a problem hiding this comment.
Avoid _ here (see other comments for rationale).
src/librustc_mir/borrow_check.rs
Outdated
| let local = &self.mir.local_decls[local]; | ||
| if local.mutability == Mutability::Not { | ||
| Err(lvalue) | ||
| } |
There was a problem hiding this comment.
Style nit: we usually write } else {
src/librustc_mir/borrow_check.rs
Outdated
| match *lvalue { | ||
| Lvalue::Local(local) => { | ||
| let local = &self.mir.local_decls[local]; | ||
| if local.mutability == Mutability::Not { |
There was a problem hiding this comment.
I would prefer an (exhaustive) match here, which makes it obvious that we've covered every case. I would say that we should keep if for booleans.
zilbuz
changed the title
fn access_lvaluefn access_lvalue
|
@bors r+ |
|
📌 Commit cbad2e5 has been approved by |
MIR-borrowck: add permisson checks to `fn access_lvalue` WIP : Some FIXME left and some broken tests. Fix #44837
|
☀️ Test successful - status-appveyor, status-travis |
Fix #44837