Skip to content
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

Permit mutable references in all const contexts #78578

Merged
merged 7 commits into from
Jan 25, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 44 additions & 27 deletions compiler/rustc_mir/src/transform/check_consts/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,16 +268,20 @@ impl NonConstOp for CellBorrow {
}

#[derive(Debug)]
/// This op is for `&mut` borrows in the trailing expression of a constant
/// which uses the "enclosing scopes rule" to leak its locals into anonymous
/// static or const items.
pub struct MutBorrow(pub hir::BorrowKind);

impl NonConstOp for MutBorrow {
fn status_in_item(&self, ccx: &ConstCx<'_, '_>) -> Status {
// Forbid everywhere except in const fn with a feature gate
if ccx.const_kind() == hir::ConstContext::ConstFn {
Status::Unstable(sym::const_mut_refs)
} else {
Status::Forbidden
}
fn status_in_item(&self, _ccx: &ConstCx<'_, '_>) -> Status {
Status::Forbidden
}

fn importance(&self) -> DiagnosticImportance {
// If there were primary errors (like non-const function calls), do not emit further
// errors about mutable references.
DiagnosticImportance::Secondary
}

fn build_error(&self, ccx: &ConstCx<'_, 'tcx>, span: Span) -> DiagnosticBuilder<'tcx> {
Expand All @@ -286,25 +290,15 @@ impl NonConstOp for MutBorrow {
hir::BorrowKind::Ref => "",
};

let mut err = if ccx.const_kind() == hir::ConstContext::ConstFn {
feature_err(
&ccx.tcx.sess.parse_sess,
sym::const_mut_refs,
span,
&format!("{}mutable references are not allowed in {}s", raw, ccx.const_kind()),
)
} else {
let mut err = struct_span_err!(
ccx.tcx.sess,
span,
E0764,
"{}mutable references are not allowed in {}s",
raw,
ccx.const_kind(),
);
err.span_label(span, format!("`&{}mut` is only allowed in `const fn`", raw));
err
};
let mut err = struct_span_err!(
ccx.tcx.sess,
span,
E0764,
"{}mutable references are not allowed in the final value of {}s",
raw,
ccx.const_kind(),
);

if ccx.tcx.sess.teach(&err.get_code().unwrap()) {
err.note(
"References in statics and constants may only refer \
Expand All @@ -321,6 +315,29 @@ impl NonConstOp for MutBorrow {
}
}

#[derive(Debug)]
pub struct TransientMutBorrow(pub hir::BorrowKind);

impl NonConstOp for TransientMutBorrow {
fn status_in_item(&self, _: &ConstCx<'_, '_>) -> Status {
Status::Unstable(sym::const_mut_refs)
}

fn build_error(&self, ccx: &ConstCx<'_, 'tcx>, span: Span) -> DiagnosticBuilder<'tcx> {
let raw = match self.0 {
hir::BorrowKind::Raw => "raw ",
hir::BorrowKind::Ref => "",
};

feature_err(
&ccx.tcx.sess.parse_sess,
sym::const_mut_refs,
span,
&format!("{}mutable references are not allowed in {}s", raw, ccx.const_kind()),
)
}
}

#[derive(Debug)]
pub struct MutDeref;
impl NonConstOp for MutDeref {
Expand All @@ -329,7 +346,7 @@ impl NonConstOp for MutDeref {
}

fn importance(&self) -> DiagnosticImportance {
// Usually a side-effect of a `MutBorrow` somewhere.
// Usually a side-effect of a `TransientMutBorrow` somewhere.
DiagnosticImportance::Secondary
}

Expand Down
29 changes: 26 additions & 3 deletions compiler/rustc_mir/src/transform/check_consts/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,29 @@ impl Validator<'mir, 'tcx> {
}
}
}

fn check_mut_borrow(&mut self, local: Local, kind: hir::BorrowKind) {
match self.const_kind() {
// In a const fn all borrows are transient or point to the places given via
// references in the arguments (so we already checked them with
// TransientMutBorrow/MutBorrow as appropriate).
// The borrow checker guarantees that no new non-transient borrows are created.
// NOTE: Once we have heap allocations during CTFE we need to figure out
// how to prevent `const fn` to create long-lived allocations that point
// to mutable memory.
hir::ConstContext::ConstFn => self.check_op(ops::TransientMutBorrow(kind)),
_ => {
// Locals with StorageDead do not live beyond the evaluation and can
// thus safely be borrowed without being able to be leaked to the final
// value of the constant.
if self.local_has_storage_dead(local) {
self.check_op(ops::TransientMutBorrow(kind));
} else {
self.check_op(ops::MutBorrow(kind));
}
}
}
}
}

impl Visitor<'tcx> for Validator<'mir, 'tcx> {
Expand Down Expand Up @@ -562,15 +585,15 @@ impl Visitor<'tcx> for Validator<'mir, 'tcx> {

if !is_allowed {
if let BorrowKind::Mut { .. } = kind {
self.check_op(ops::MutBorrow(hir::BorrowKind::Ref));
self.check_mut_borrow(place.local, hir::BorrowKind::Ref)
} else {
self.check_op(ops::CellBorrow);
}
}
}

Rvalue::AddressOf(Mutability::Mut, _) => {
self.check_op(ops::MutBorrow(hir::BorrowKind::Raw))
Rvalue::AddressOf(Mutability::Mut, ref place) => {
self.check_mut_borrow(place.local, hir::BorrowKind::Raw)
}

Rvalue::Ref(_, BorrowKind::Shared | BorrowKind::Shallow, ref place)
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/check-static-immutable-mut-slices.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Checks that immutable static items can't have mutable slices

static TEST: &'static mut [isize] = &mut [];
//~^ ERROR mutable references are not allowed in statics
//~^ ERROR mutable references are not allowed

pub fn main() { }
4 changes: 2 additions & 2 deletions src/test/ui/check-static-immutable-mut-slices.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error[E0764]: mutable references are not allowed in statics
error[E0764]: mutable references are not allowed in the final value of statics
--> $DIR/check-static-immutable-mut-slices.rs:3:37
|
LL | static TEST: &'static mut [isize] = &mut [];
| ^^^^^^^ `&mut` is only allowed in `const fn`
| ^^^^^^^

error: aborting due to previous error

Expand Down
24 changes: 16 additions & 8 deletions src/test/ui/consts/const-address-of-mut.stderr
Original file line number Diff line number Diff line change
@@ -1,20 +1,29 @@
error[E0764]: raw mutable references are not allowed in constants
error[E0658]: raw mutable references are not allowed in constants
--> $DIR/const-address-of-mut.rs:3:32
|
LL | const A: () = { let mut x = 2; &raw mut x; };
| ^^^^^^^^^^ `&raw mut` is only allowed in `const fn`
| ^^^^^^^^^^
|
= note: see issue #57349 <https://github.com/rust-lang/rust/issues/57349> for more information
= help: add `#![feature(const_mut_refs)]` to the crate attributes to enable

error[E0764]: raw mutable references are not allowed in statics
error[E0658]: raw mutable references are not allowed in statics
--> $DIR/const-address-of-mut.rs:5:33
|
LL | static B: () = { let mut x = 2; &raw mut x; };
| ^^^^^^^^^^ `&raw mut` is only allowed in `const fn`
| ^^^^^^^^^^
|
= note: see issue #57349 <https://github.com/rust-lang/rust/issues/57349> for more information
= help: add `#![feature(const_mut_refs)]` to the crate attributes to enable

error[E0764]: raw mutable references are not allowed in statics
error[E0658]: raw mutable references are not allowed in statics
--> $DIR/const-address-of-mut.rs:7:37
|
LL | static mut C: () = { let mut x = 2; &raw mut x; };
| ^^^^^^^^^^ `&raw mut` is only allowed in `const fn`
| ^^^^^^^^^^
|
= note: see issue #57349 <https://github.com/rust-lang/rust/issues/57349> for more information
= help: add `#![feature(const_mut_refs)]` to the crate attributes to enable

error[E0658]: raw mutable references are not allowed in constant functions
--> $DIR/const-address-of-mut.rs:11:13
Expand All @@ -27,5 +36,4 @@ LL | let y = &raw mut x;

error: aborting due to 4 previous errors

Some errors have detailed explanations: E0658, E0764.
For more information about an error, try `rustc --explain E0658`.
For more information about this error, try `rustc --explain E0658`.
9 changes: 6 additions & 3 deletions src/test/ui/consts/const-eval/issue-65394.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
error[E0764]: mutable references are not allowed in constants
error[E0658]: mutable references are not allowed in constants
--> $DIR/issue-65394.rs:8:13
|
LL | let r = &mut x;
| ^^^^^^ `&mut` is only allowed in `const fn`
| ^^^^^^
|
= note: see issue #57349 <https://github.com/rust-lang/rust/issues/57349> for more information
= help: add `#![feature(const_mut_refs)]` to the crate attributes to enable

error[E0493]: destructors cannot be evaluated at compile-time
--> $DIR/issue-65394.rs:7:9
Expand All @@ -15,5 +18,5 @@ LL | };

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0493, E0764.
Some errors have detailed explanations: E0493, E0658.
For more information about an error, try `rustc --explain E0493`.
10 changes: 6 additions & 4 deletions src/test/ui/consts/const-multi-ref.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
error[E0764]: mutable references are not allowed in constants
error[E0658]: mutable references are not allowed in constants
--> $DIR/const-multi-ref.rs:6:13
|
LL | let p = &mut a;
| ^^^^^^ `&mut` is only allowed in `const fn`
| ^^^^^^
|
= note: see issue #57349 <https://github.com/rust-lang/rust/issues/57349> for more information
= help: add `#![feature(const_mut_refs)]` to the crate attributes to enable

error[E0658]: cannot borrow here, since the borrowed element may contain interior mutability
--> $DIR/const-multi-ref.rs:16:13
Expand All @@ -15,5 +18,4 @@ LL | let p = &a;

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0658, E0764.
For more information about an error, try `rustc --explain E0658`.
For more information about this error, try `rustc --explain E0658`.
3 changes: 1 addition & 2 deletions src/test/ui/consts/const-mut-refs/const_mut_address_of.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// check-pass
#![feature(const_mut_refs)]
#![feature(const_fn)]
#![feature(raw_ref_op)]
Expand All @@ -22,9 +23,7 @@ const fn baz(foo: &mut Foo)-> *mut usize {

const _: () = {
foo().bar();
//~^ ERROR mutable references are not allowed in constants
baz(&mut foo());
//~^ ERROR mutable references are not allowed in constants
};

fn main() {}
15 changes: 0 additions & 15 deletions src/test/ui/consts/const-mut-refs/const_mut_address_of.stderr

This file was deleted.

4 changes: 1 addition & 3 deletions src/test/ui/consts/const-mut-refs/const_mut_refs.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// check-pass
#![feature(const_mut_refs)]

struct Foo {
Expand Down Expand Up @@ -29,9 +30,6 @@ const fn bazz(foo: &mut Foo) -> usize {

fn main() {
let _: [(); foo().bar()] = [(); 1];
//~^ ERROR mutable references are not allowed in constants
let _: [(); baz(&mut foo())] = [(); 2];
//~^ ERROR mutable references are not allowed in constants
let _: [(); bazz(&mut foo())] = [(); 3];
//~^ ERROR mutable references are not allowed in constants
}
21 changes: 0 additions & 21 deletions src/test/ui/consts/const-mut-refs/const_mut_refs.stderr

This file was deleted.

68 changes: 68 additions & 0 deletions src/test/ui/consts/const-mut-refs/mut_ref_in_final.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
#![feature(const_mut_refs)]
#![feature(const_fn)]
#![feature(const_transmute)]
#![feature(raw_ref_op)]
#![feature(const_raw_ptr_deref)]

const NULL: *mut i32 = std::ptr::null_mut();
const A: *const i32 = &4;

// It could be made sound to allow it to compile,
// but we do not want to allow this to compile,
// as that would be an enormous footgun in oli-obk's opinion.
const B: *mut i32 = &mut 4; //~ ERROR mutable references are not allowed

// Ok, no actual mutable allocation exists
const B2: Option<&mut i32> = None;

// Not ok, can't prove that no mutable allocation ends up in final value
const B3: Option<&mut i32> = Some(&mut 42); //~ ERROR temporary value dropped while borrowed

const fn helper() -> Option<&'static mut i32> { unsafe {
// Undefined behaviour, who doesn't love tests like this.
// This code never gets executed, because the static checks fail before that.
Some(&mut *(42 as *mut i32))
} }
// Check that we do not look into function bodies.
// We treat all functions as not returning a mutable reference, because there is no way to
// do that without causing the borrow checker to complain (see the B5/helper2 test below).
const B4: Option<&mut i32> = helper();

const fn helper2(x: &mut i32) -> Option<&mut i32> { Some(x) }
const B5: Option<&mut i32> = helper2(&mut 42); //~ ERROR temporary value dropped while borrowed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not too happy with B4... but also not sure what else to do here. We could start running checks on the return type of functions, but I'm not sure how to do that properly, especially once generics are involved.

Copy link
Member

Choose a reason for hiding this comment

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

because the static checks fail before that

Why is there no //~ ERROR here then?
What if you do Some(&mut *(&mut 42 as *mut i32)) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the static checks on other constants fail. This code will work and then error in validation if we were getting this far, but since everything else erros first, and B4 is not used, it isn't evaluated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if you do Some(&mut *(&mut 42 as *mut i32)) instead?

that would just give us a dangling pointer, but still not get to validation. I could put this test in a separate file, then we would get to validation.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, the static checks in other consts fail.

Looks like this test needs its own file then.

that would just give us a dangling pointer, but still not get to validation. I could put this test in a separate file, then we would get to validation.

Sure, I meant to test that in addition to B4, not instead of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


// Ok, because no references to mutable data exist here, since the `{}` moves
// its value and then takes a reference to that.
const C: *const i32 = &{
let mut x = 42;
x += 3;
x
};

oli-obk marked this conversation as resolved.
Show resolved Hide resolved
use std::cell::UnsafeCell;
struct NotAMutex<T>(UnsafeCell<T>);

unsafe impl<T> Sync for NotAMutex<T> {}

const FOO: NotAMutex<&mut i32> = NotAMutex(UnsafeCell::new(&mut 42));
//~^ ERROR temporary value dropped while borrowed

static FOO2: NotAMutex<&mut i32> = NotAMutex(UnsafeCell::new(&mut 42));
//~^ ERROR temporary value dropped while borrowed

static mut FOO3: NotAMutex<&mut i32> = NotAMutex(UnsafeCell::new(&mut 42));
//~^ ERROR temporary value dropped while borrowed

// `BAR` works, because `&42` promotes immediately instead of relying on
// the enclosing scope rule.
const BAR: NotAMutex<&i32> = NotAMutex(UnsafeCell::new(&42));

fn main() {
println!("{}", unsafe { *A });
unsafe { *B = 4 } // Bad news

unsafe {
**FOO.0.get() = 99;
assert_eq!(**FOO.0.get(), 99);
}
}
Loading