-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle const-checks for &mut
outside of HasMutInterior
#66654
Changes from 11 commits
b0b7dc3
6596165
66e9f88
5a3ddcd
5a10430
5117407
dd32d91
d2bdaa8
5b1e10b
a70ac50
846be82
ccb4eed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,13 +23,6 @@ use super::qualifs::{self, HasMutInterior, NeedsDrop}; | |
use super::resolver::FlowSensitiveAnalysis; | ||
use super::{ConstKind, Item, Qualif, is_lang_panic_fn}; | ||
|
||
#[derive(Copy, Clone, Debug, PartialEq, Eq)] | ||
pub enum CheckOpResult { | ||
Forbidden, | ||
Unleashed, | ||
Allowed, | ||
} | ||
|
||
pub type IndirectlyMutableResults<'mir, 'tcx> = | ||
old_dataflow::DataflowResultsCursor<'mir, 'tcx, IndirectlyMutableLocals<'mir, 'tcx>>; | ||
|
||
|
@@ -149,17 +142,6 @@ pub struct Validator<'a, 'mir, 'tcx> { | |
|
||
/// The span of the current statement. | ||
span: Span, | ||
|
||
/// True if the local was assigned the result of an illegal borrow (`ops::MutBorrow`). | ||
/// | ||
/// This is used to hide errors from {re,}borrowing the newly-assigned local, instead pointing | ||
/// the user to the place where the illegal borrow occurred. This set is only populated once an | ||
/// error has been emitted, so it will never cause an erroneous `mir::Body` to pass validation. | ||
/// | ||
/// FIXME(ecstaticmorse): assert at the end of checking that if `tcx.has_errors() == false`, | ||
/// this set is empty. Note that if we start removing locals from | ||
/// `derived_from_illegal_borrow`, just checking at the end won't be enough. | ||
derived_from_illegal_borrow: BitSet<Local>, | ||
} | ||
|
||
impl Deref for Validator<'_, 'mir, 'tcx> { | ||
|
@@ -213,7 +195,6 @@ impl Validator<'a, 'mir, 'tcx> { | |
span: item.body.span, | ||
item, | ||
qualifs, | ||
derived_from_illegal_borrow: BitSet::new_empty(item.body.local_decls.len()), | ||
} | ||
} | ||
|
||
|
@@ -258,15 +239,15 @@ impl Validator<'a, 'mir, 'tcx> { | |
} | ||
|
||
/// Emits an error at the given `span` if an expression cannot be evaluated in the current | ||
/// context. Returns `Forbidden` if an error was emitted. | ||
pub fn check_op_spanned<O>(&mut self, op: O, span: Span) -> CheckOpResult | ||
/// context. | ||
pub fn check_op_spanned<O>(&mut self, op: O, span: Span) | ||
where | ||
O: NonConstOp | ||
{ | ||
trace!("check_op: op={:?}", op); | ||
|
||
if op.is_allowed_in_item(self) { | ||
return CheckOpResult::Allowed; | ||
return; | ||
} | ||
|
||
// If an operation is supported in miri (and is not already controlled by a feature gate) it | ||
|
@@ -276,20 +257,19 @@ impl Validator<'a, 'mir, 'tcx> { | |
|
||
if is_unleashable && self.tcx.sess.opts.debugging_opts.unleash_the_miri_inside_of_you { | ||
self.tcx.sess.span_warn(span, "skipping const checks"); | ||
return CheckOpResult::Unleashed; | ||
return; | ||
} | ||
|
||
op.emit_error(self, span); | ||
CheckOpResult::Forbidden | ||
} | ||
|
||
/// Emits an error if an expression cannot be evaluated in the current context. | ||
pub fn check_op(&mut self, op: impl NonConstOp) -> CheckOpResult { | ||
pub fn check_op(&mut self, op: impl NonConstOp) { | ||
let span = self.span; | ||
self.check_op_spanned(op, span) | ||
} | ||
|
||
fn check_static(&mut self, def_id: DefId, span: Span) -> CheckOpResult { | ||
fn check_static(&mut self, def_id: DefId, span: Span) { | ||
let is_thread_local = self.tcx.has_attr(def_id, sym::thread_local); | ||
if is_thread_local { | ||
self.check_op_spanned(ops::ThreadLocalAccess, span) | ||
|
@@ -322,20 +302,9 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { | |
fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { | ||
trace!("visit_rvalue: rvalue={:?} location={:?}", rvalue, location); | ||
|
||
// Check nested operands and places. | ||
// Special-case reborrows to be more like a copy of a reference. | ||
if let Rvalue::Ref(_, kind, ref place) = *rvalue { | ||
// Special-case reborrows to be more like a copy of a reference. | ||
let mut reborrow_place = None; | ||
if let &[ref proj_base @ .., elem] = place.projection.as_ref() { | ||
if elem == ProjectionElem::Deref { | ||
let base_ty = Place::ty_from(&place.base, proj_base, self.body, self.tcx).ty; | ||
if let ty::Ref(..) = base_ty.kind { | ||
reborrow_place = Some(proj_base); | ||
} | ||
} | ||
} | ||
|
||
if let Some(proj) = reborrow_place { | ||
if let Some(reborrowed_proj) = place_as_reborrow(self.tcx, self.body, place) { | ||
let ctx = match kind { | ||
BorrowKind::Shared => PlaceContext::NonMutatingUse( | ||
NonMutatingUseContext::SharedBorrow, | ||
|
@@ -351,14 +320,13 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { | |
), | ||
}; | ||
self.visit_place_base(&place.base, ctx, location); | ||
self.visit_projection(&place.base, proj, ctx, location); | ||
} else { | ||
self.super_rvalue(rvalue, location); | ||
self.visit_projection(&place.base, reborrowed_proj, ctx, location); | ||
return; | ||
} | ||
} else { | ||
self.super_rvalue(rvalue, location); | ||
} | ||
|
||
self.super_rvalue(rvalue, location); | ||
|
||
match *rvalue { | ||
Rvalue::Use(_) | | ||
Rvalue::Repeat(..) | | ||
|
@@ -369,9 +337,59 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { | |
Rvalue::Cast(CastKind::Pointer(_), ..) | | ||
Rvalue::Discriminant(..) | | ||
Rvalue::Len(_) | | ||
Rvalue::Ref(..) | | ||
Rvalue::Aggregate(..) => {} | ||
|
||
| Rvalue::Ref(_, kind @ BorrowKind::Mut { .. }, ref place) | ||
| Rvalue::Ref(_, kind @ BorrowKind::Unique, ref place) | ||
=> { | ||
let ty = place.ty(self.body, self.tcx).ty; | ||
let is_allowed = match ty.kind { | ||
// Inside a `static mut`, `&mut [...]` is allowed. | ||
ty::Array(..) | ty::Slice(_) if self.const_kind() == ConstKind::StaticMut | ||
=> true, | ||
|
||
// FIXME(ecstaticmorse): We could allow `&mut []` inside a const context given | ||
// that this is merely a ZST and it is already eligible for promotion. | ||
// This may require an RFC? | ||
/* | ||
ty::Array(_, len) if len.try_eval_usize(cx.tcx, cx.param_env) == Some(0) | ||
=> true, | ||
*/ | ||
|
||
_ => false, | ||
}; | ||
|
||
if !is_allowed { | ||
self.check_op(ops::MutBorrow(kind)); | ||
} | ||
} | ||
|
||
// Taking a shared borrow of a `static` is always legal, even if that `static` has | ||
// interior mutability. | ||
| Rvalue::Ref(_, BorrowKind::Shared, ref place) | ||
| Rvalue::Ref(_, BorrowKind::Shallow, ref place) | ||
if matches!(place.base, PlaceBase::Static(_)) | ||
=> {} | ||
|
||
| Rvalue::Ref(_, kind @ BorrowKind::Shared, ref place) | ||
| Rvalue::Ref(_, kind @ BorrowKind::Shallow, ref place) | ||
=> { | ||
// FIXME: Change the `in_*` methods to take a `FnMut` so we don't have to manually | ||
// seek the cursors beforehand. | ||
self.qualifs.has_mut_interior.cursor.seek_before(location); | ||
self.qualifs.indirectly_mutable.seek(location); | ||
|
||
let borrowed_place_has_mut_interior = HasMutInterior::in_place( | ||
&self.item, | ||
&|local| self.qualifs.has_mut_interior_eager_seek(local), | ||
place.as_ref(), | ||
); | ||
|
||
if borrowed_place_has_mut_interior { | ||
self.check_op(ops::MutBorrow(kind)); | ||
} | ||
} | ||
|
||
Rvalue::Cast(CastKind::Misc, ref operand, cast_ty) => { | ||
let operand_ty = operand.ty(self.body, self.tcx); | ||
let cast_in = CastTy::from_ty(operand_ty).expect("bad input type for cast"); | ||
|
@@ -436,58 +454,6 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { | |
} | ||
} | ||
|
||
fn visit_assign(&mut self, dest: &Place<'tcx>, rvalue: &Rvalue<'tcx>, location: Location) { | ||
trace!("visit_assign: dest={:?} rvalue={:?} location={:?}", dest, rvalue, location); | ||
|
||
// Error on mutable borrows or shared borrows of values with interior mutability. | ||
// | ||
// This replicates the logic at the start of `assign` in the old const checker. Note that | ||
// it depends on `HasMutInterior` being set for mutable borrows as well as values with | ||
// interior mutability. | ||
if let Rvalue::Ref(_, kind, ref borrowed_place) = *rvalue { | ||
// FIXME: Change the `in_*` methods to take a `FnMut` so we don't have to manually seek | ||
// the cursors beforehand. | ||
self.qualifs.has_mut_interior.cursor.seek_before(location); | ||
self.qualifs.indirectly_mutable.seek(location); | ||
|
||
let rvalue_has_mut_interior = HasMutInterior::in_rvalue( | ||
&self.item, | ||
&|local| self.qualifs.has_mut_interior_eager_seek(local), | ||
rvalue, | ||
); | ||
|
||
if rvalue_has_mut_interior { | ||
let is_derived_from_illegal_borrow = match borrowed_place.as_local() { | ||
// If an unprojected local was borrowed and its value was the result of an | ||
// illegal borrow, suppress this error and mark the result of this borrow as | ||
// illegal as well. | ||
Some(borrowed_local) | ||
if self.derived_from_illegal_borrow.contains(borrowed_local) => | ||
{ | ||
true | ||
} | ||
|
||
// Otherwise proceed normally: check the legality of a mutable borrow in this | ||
// context. | ||
_ => self.check_op(ops::MutBorrow(kind)) == CheckOpResult::Forbidden, | ||
}; | ||
|
||
// When the target of the assignment is a local with no projections, mark it as | ||
// derived from an illegal borrow if necessary. | ||
// | ||
// FIXME: should we also clear `derived_from_illegal_borrow` when a local is | ||
// assigned a new value? | ||
if is_derived_from_illegal_borrow { | ||
if let Some(dest) = dest.as_local() { | ||
self.derived_from_illegal_borrow.insert(dest); | ||
} | ||
} | ||
} | ||
} | ||
|
||
self.super_assign(dest, rvalue, location); | ||
} | ||
|
||
fn visit_projection_elem( | ||
&mut self, | ||
place_base: &PlaceBase<'tcx>, | ||
|
@@ -725,3 +691,37 @@ fn check_return_ty_is_sync(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, hir_id: HirId) | |
} | ||
}); | ||
} | ||
|
||
fn place_as_reborrow( | ||
tcx: TyCtxt<'tcx>, | ||
body: &Body<'tcx>, | ||
place: &'a Place<'tcx>, | ||
) -> Option<&'a [PlaceElem<'tcx>]> { | ||
place | ||
.projection | ||
.split_last() | ||
.and_then(|(outermost, inner)| { | ||
if outermost != &ProjectionElem::Deref { | ||
return None; | ||
} | ||
|
||
// A borrow of a `static` also looks like `&(*_1)` in the MIR, but `_1` is a `const` | ||
// that points to the allocation for the static. Don't treat these as reborrows. | ||
if let PlaceBase::Local(local) = place.base { | ||
let decl = &body.local_decls[local]; | ||
if let LocalInfo::StaticRef { .. } = decl.local_info { | ||
return None; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @matthewjasper isn't this supposed to use a helper method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, there's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @matthewjasper Resolved in ccb4eed. |
||
|
||
// Ensure the type being derefed is a reference and not a raw pointer. | ||
// | ||
// This is sufficient to prevent an access to a `static mut` from being marked as a | ||
// reborrow, even if the check above were to disappear. | ||
let inner_ty = Place::ty_from(&place.base, inner, body, tcx).ty; | ||
match inner_ty.kind { | ||
ty::Ref(..) => Some(inner), | ||
_ => None, | ||
} | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,24 @@ | ||
const _X: i32 = { | ||
// Ensure that we point the user to the erroneous borrow but not to any subsequent borrows of that | ||
// initial one. | ||
|
||
const _: i32 = { | ||
let mut a = 5; | ||
let p = &mut a; //~ ERROR references in constants may only refer to immutable values | ||
let p = &mut a; //~ ERROR references in constants may only refer to immutable values | ||
|
||
let reborrow = {p}; //~ ERROR references in constants may only refer to immutable values | ||
let reborrow = {p}; | ||
let pp = &reborrow; | ||
let ppp = &pp; | ||
***ppp | ||
}; | ||
|
||
const _: std::cell::Cell<i32> = { | ||
let mut a = std::cell::Cell::new(5); | ||
let p = &a; //~ ERROR cannot borrow a constant which may contain interior mutability | ||
|
||
let reborrow = {p}; | ||
let pp = &reborrow; | ||
let ppp = &pp; | ||
a | ||
}; | ||
|
||
fn main() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,16 @@ | ||
error[E0017]: references in constants may only refer to immutable values | ||
--> $DIR/const-multi-ref.rs:3:13 | ||
--> $DIR/const-multi-ref.rs:6:13 | ||
| | ||
LL | let p = &mut a; | ||
| ^^^^^^ constants require immutable values | ||
|
||
error[E0017]: references in constants may only refer to immutable values | ||
--> $DIR/const-multi-ref.rs:5:21 | ||
error[E0492]: cannot borrow a constant which may contain interior mutability, create a static instead | ||
--> $DIR/const-multi-ref.rs:16:13 | ||
| | ||
LL | let reborrow = {p}; | ||
| ^ constants require immutable values | ||
LL | let p = &a; | ||
| ^^ | ||
|
||
error: aborting due to 2 previous errors | ||
|
||
For more information about this error, try `rustc --explain E0017`. | ||
Some errors have detailed explanations: E0017, E0492. | ||
For more information about an error, try `rustc --explain E0017`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this make sense anymore, with the
static
changes?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this shouldn't be reachable, since the only
PlaceBase::Static
s are promoted. This maybe should bebug!()
for now.