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.

Sign up for GitHub

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

Const qualification comments #61492

Merged
merged 5 commits into from
Jun 11, 2019
Merged

Const qualification comments #61492

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ea21a8a
some more comments for const_qualif
RalfJung Jun 3, 2019
ed02b15
replace some mode comparisons by more readable function call, rename …
RalfJung Jun 3, 2019
f2b49d9
avoid 'const-context' terminology
RalfJung Jun 6, 2019
b823cc1
const-correctness might be confusing for C++ people
RalfJung Jun 6, 2019
0edf46f
comments
RalfJung Jun 6, 2019
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
124 changes: 81 additions & 43 deletions src/librustc_mir/transform/qualify_consts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,25 @@ use super::promote_consts::{self, Candidate, TempState};
/// What kind of item we are in.
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
enum Mode {
Const,
/// A `static` item.
Static,
/// A `static mut` item.
StaticMut,
/// A `const fn` item.
ConstFn,
Fn
/// A `const` item or an anonymous constant (e.g. in array lengths).
Const,
/// Other type of `fn`.
NonConstFn,
}

impl Mode {
/// Determine whether we have to do full const-checking because syntactically, we
/// are required to be "const".
#[inline]
fn requires_const_checking(self) -> bool {
self != Mode::NonConstFn
}
}

impl fmt::Display for Mode {
Expand All @@ -48,7 +62,7 @@ impl fmt::Display for Mode {
Mode::Const => write!(f, "constant"),
Mode::Static | Mode::StaticMut => write!(f, "static"),
Mode::ConstFn => write!(f, "constant function"),
Mode::Fn => write!(f, "function")
Mode::NonConstFn => write!(f, "function")
}
}
}
Expand Down Expand Up @@ -135,6 +149,12 @@ enum ValueSource<'a, 'tcx> {
},
}

/// A "qualif"(-ication) is a way to look for something "bad" in the MIR that would disqualify some
/// code for promotion or prevent it from evaluating at compile time. So `return true` means
/// "I found something bad, no reason to go on searching". `false` is only returned if we
/// definitely cannot find anything bad anywhere.
///
/// The default implementations proceed structurally.
trait Qualif {
const IDX: usize;

Expand Down Expand Up @@ -285,7 +305,11 @@ trait Qualif {
}
}

// Constant containing interior mutability (UnsafeCell).
/// Constant containing interior mutability (`UnsafeCell<T>`).
/// This must be ruled out to make sure that evaluating the constant at compile-time
/// and at *any point* during the run-time would produce the same result. In particular,
/// promotion of temporaries must not change program behavior; if the promoted could be
/// written to, that would be a problem.
struct HasMutInterior;

impl Qualif for HasMutInterior {
Expand Down Expand Up @@ -314,10 +338,10 @@ impl Qualif for HasMutInterior {
_ => return true,
}
} else if let ty::Array(_, len) = ty.sty {
// FIXME(eddyb) the `cx.mode == Mode::Fn` condition
// FIXME(eddyb) the `cx.mode == Mode::NonConstFn` condition
// seems unnecessary, given that this is merely a ZST.
match len.assert_usize(cx.tcx) {
Some(0) if cx.mode == Mode::Fn => {},
Some(0) if cx.mode == Mode::NonConstFn => {},
_ => return true,
}
} else {
Expand All @@ -343,7 +367,10 @@ impl Qualif for HasMutInterior {
}
}

// Constant containing an ADT that implements Drop.
/// Constant containing an ADT that implements `Drop`.
/// This must be ruled out (a) because we cannot run `Drop` during compile-time
/// as that might not be a `const fn`, and (b) because implicit promotion would
/// remove side-effects that occur as part of dropping that value.
struct NeedsDrop;

impl Qualif for NeedsDrop {
Expand All @@ -366,8 +393,12 @@ impl Qualif for NeedsDrop {
}
}

// Not promotable at all - non-`const fn` calls, asm!,
// pointer comparisons, ptr-to-int casts, etc.
/// Not promotable at all - non-`const fn` calls, `asm!`,
/// pointer comparisons, ptr-to-int casts, etc.
/// Inside a const context all constness rules apply, so promotion simply has to follow the regular
/// constant rules (modulo interior mutability or `Drop` rules which are handled `HasMutInterior`
/// and `NeedsDrop` respectively). Basically this duplicates the checks that the const-checking
/// visitor enforces by emitting errors when working in const context.
struct IsNotPromotable;

impl Qualif for IsNotPromotable {
Expand Down Expand Up @@ -398,9 +429,10 @@ impl Qualif for IsNotPromotable {
ProjectionElem::Index(_) => {}

ProjectionElem::Field(..) => {
if cx.mode == Mode::Fn {
if cx.mode == Mode::NonConstFn {
let base_ty = proj.base.ty(cx.body, cx.tcx).ty;
if let Some(def) = base_ty.ty_adt_def() {
// No promotion of union field accesses.
if def.is_union() {
return true;
}
Expand All @@ -414,7 +446,7 @@ impl Qualif for IsNotPromotable {

fn in_rvalue(cx: &ConstCx<'_, 'tcx>, rvalue: &Rvalue<'tcx>) -> bool {
match *rvalue {
Rvalue::Cast(CastKind::Misc, ref operand, cast_ty) if cx.mode == Mode::Fn => {
Rvalue::Cast(CastKind::Misc, ref operand, cast_ty) if cx.mode == Mode::NonConstFn => {
let operand_ty = operand.ty(cx.body, cx.tcx);
let cast_in = CastTy::from_ty(operand_ty).expect("bad input type for cast");
let cast_out = CastTy::from_ty(cast_ty).expect("bad output type for cast");
Expand All @@ -428,7 +460,7 @@ impl Qualif for IsNotPromotable {
}
}

Rvalue::BinaryOp(op, ref lhs, _) if cx.mode == Mode::Fn => {
Rvalue::BinaryOp(op, ref lhs, _) if cx.mode == Mode::NonConstFn => {
if let ty::RawPtr(_) | ty::FnPtr(..) = lhs.ty(cx.body, cx.tcx).sty {
assert!(op == BinOp::Eq || op == BinOp::Ne ||
op == BinOp::Le || op == BinOp::Lt ||
Expand Down Expand Up @@ -511,12 +543,9 @@ impl Qualif for IsNotPromotable {

/// Refers to temporaries which cannot be promoted *implicitly*.
/// Explicit promotion happens e.g. for constant arguments declared via `rustc_args_required_const`.
/// Inside a const context all constness rules
/// apply, so implicit promotion simply has to follow the regular constant rules (modulo interior
/// mutability or `Drop` rules which are handled `HasMutInterior` and `NeedsDrop` respectively).
/// Implicit promotion inside regular functions does not happen if `const fn` calls are involved,
/// as the call may be perfectly alright at runtime, but fail at compile time e.g. due to addresses
/// being compared inside the function.
/// Implicit promotion has almost the same rules, except that disallows `const fn` except for
/// those marked `#[rustc_promotable]`. This is to avoid changing a legitimate run-time operation
/// into a failing compile-time operation e.g. due to addresses being compared inside the function.
struct IsNotImplicitlyPromotable;

impl Qualif for IsNotImplicitlyPromotable {
Expand All @@ -528,7 +557,7 @@ impl Qualif for IsNotImplicitlyPromotable {
args: &[Operand<'tcx>],
_return_ty: Ty<'tcx>,
) -> bool {
if cx.mode == Mode::Fn {
if cx.mode == Mode::NonConstFn {
if let ty::FnDef(def_id, _) = callee.ty(cx.body, cx.tcx).sty {
// Never promote runtime `const fn` calls of
// functions without `#[rustc_promotable]`.
Expand Down Expand Up @@ -589,6 +618,11 @@ impl ConstCx<'_, 'tcx> {
}
}

/// Checks MIR for being admissible as a compile-time constant, using `ConstCx`
/// for value qualifications, and accumulates writes of
/// rvalue/call results to locals, in `local_qualif`.
/// It also records candidates for promotion in `promotion_candidates`,
/// both in functions and const/static items.
struct Checker<'a, 'tcx> {
cx: ConstCx<'a, 'tcx>,

Expand Down Expand Up @@ -672,7 +706,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
// slightly pointless (even with feature-gating).
fn not_const(&mut self) {
unleash_miri!(self);
if self.mode != Mode::Fn {
if self.mode.requires_const_checking() {
let mut err = struct_span_err!(
self.tcx.sess,
self.span,
Expand Down Expand Up @@ -707,7 +741,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
qualifs[HasMutInterior] = false;
qualifs[IsNotPromotable] = true;

if self.mode != Mode::Fn {
if self.mode.requires_const_checking() {
if let BorrowKind::Mut { .. } = kind {
let mut err = struct_span_err!(self.tcx.sess, self.span, E0017,
"references in {}s may only refer \
Expand Down Expand Up @@ -737,7 +771,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {

// We might have a candidate for promotion.
let candidate = Candidate::Ref(location);
// We can only promote interior borrows of promotable temps.
// Start by traversing to the "base", with non-deref projections removed.
let mut place = place;
while let Place::Projection(ref proj) = *place {
if proj.elem == ProjectionElem::Deref {
Expand All @@ -746,6 +780,10 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
place = &proj.base;
}
debug!("qualify_consts: promotion candidate: place={:?}", place);
// We can only promote interior borrows of promotable temps (non-temps
// don't get promoted anyway).
// (If we bailed out of the loop due to a `Deref` above, we will definitely
// not enter the conditional here.)
if let Place::Base(PlaceBase::Local(local)) = *place {
if self.body.local_kind(local) == LocalKind::Temp {
debug!("qualify_consts: promotion candidate: local={:?}", local);
Expand All @@ -756,6 +794,10 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
// `HasMutInterior`, from a type that does, e.g.:
// `let _: &'static _ = &(Cell::new(1), 2).1;`
let mut local_qualifs = self.qualifs_in_local(local);
// Any qualifications, except HasMutInterior (see above), disqualify
// from promotion.
// This is, in particular, the "implicit promotion" version of
// the check making sure that we don't run drop glue during const-eval.
Copy link
Member

Choose a reason for hiding this comment

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

This seems oddly specific - this is the original sole check, so comparing it to something newer seems off.
You could move the comment one line above and make it more about "any qualifications, except HasMutInterior (see above), disqualify from promotion".

Copy link
Member Author

@RalfJung RalfJung Jun 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think which check came first historically has any bearing here. Just because it grew that way doesn't mean that's the best way to explain it.

The other check is IMO clearly "more principled" by looking at the place where the side-effect actually occurs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have adjusted the wording a bit.

local_qualifs[HasMutInterior] = false;
if !local_qualifs.0.iter().any(|&qualif| qualif) {
debug!("qualify_consts: promotion candidate: {:?}", candidate);
Expand Down Expand Up @@ -803,7 +845,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
debug!("store to {:?} {:?}", kind, index);

// Only handle promotable temps in non-const functions.
if self.mode == Mode::Fn {
if self.mode == Mode::NonConstFn {
if kind != LocalKind::Temp ||
!self.temp_promotion_state[index].is_promotable() {
return;
Expand Down Expand Up @@ -920,11 +962,6 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
}
}

/// Checks MIR for const-correctness, using `ConstCx`
/// for value qualifications, and accumulates writes of
/// rvalue/call results to locals, in `local_qualif`.
/// For functions (constant or not), it also records
/// candidates for promotion in `promotion_candidates`.
impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
fn visit_place_base(
&mut self,
Expand All @@ -943,7 +980,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
.get_attrs(*def_id)
.iter()
.any(|attr| attr.check_name(sym::thread_local)) {
if self.mode != Mode::Fn {
if self.mode.requires_const_checking() {
span_err!(self.tcx.sess, self.span, E0625,
"thread-local statics cannot be \
accessed at compile-time");
Expand All @@ -967,7 +1004,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
}
unleash_miri!(self);

if self.mode != Mode::Fn {
if self.mode.requires_const_checking() {
let mut err = struct_span_err!(self.tcx.sess, self.span, E0013,
"{}s cannot refer to statics, use \
a constant instead", self.mode);
Expand Down Expand Up @@ -1005,7 +1042,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
}
let base_ty = proj.base.ty(self.body, self.tcx).ty;
match self.mode {
Mode::Fn => {},
Mode::NonConstFn => {},
_ => {
if let ty::RawPtr(_) = base_ty.sty {
if !self.tcx.features().const_raw_ptr_deref {
Expand Down Expand Up @@ -1041,7 +1078,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
}
},

| Mode::Fn
| Mode::NonConstFn
| Mode::Static
| Mode::StaticMut
| Mode::Const
Expand Down Expand Up @@ -1131,7 +1168,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
let cast_out = CastTy::from_ty(cast_ty).expect("bad output type for cast");
match (cast_in, cast_out) {
(CastTy::Ptr(_), CastTy::Int(_)) |
(CastTy::FnPtr, CastTy::Int(_)) if self.mode != Mode::Fn => {
(CastTy::FnPtr, CastTy::Int(_)) if self.mode != Mode::NonConstFn => {
unleash_miri!(self);
if !self.tcx.features().const_raw_ptr_to_usize_cast {
// in const fn and constants require the feature gate
Expand All @@ -1158,7 +1195,9 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
op == BinOp::Offset);

unleash_miri!(self);
if self.mode != Mode::Fn && !self.tcx.features().const_compare_raw_pointers {
if self.mode.requires_const_checking() &&
!self.tcx.features().const_compare_raw_pointers
{
// require the feature gate inside constants and const fn
// FIXME: make it unsafe to use these operations
emit_feature_err(
Expand All @@ -1174,7 +1213,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {

Rvalue::NullaryOp(NullOp::Box, _) => {
unleash_miri!(self);
if self.mode != Mode::Fn {
if self.mode.requires_const_checking() {
let mut err = struct_span_err!(self.tcx.sess, self.span, E0010,
"allocations are not allowed in {}s", self.mode);
err.span_label(self.span, format!("allocation not allowed in {}s", self.mode));
Expand Down Expand Up @@ -1219,8 +1258,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
// special intrinsic that can be called diretly without an intrinsic
// feature gate needs a language feature gate
"transmute" => {
// never promote transmute calls
if self.mode != Mode::Fn {
if self.mode.requires_const_checking() {
// const eval transmute calls only with the feature gate
if !self.tcx.features().const_transmute {
emit_feature_err(
Expand All @@ -1243,7 +1281,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
}
_ => {
// In normal functions no calls are feature-gated.
if self.mode != Mode::Fn {
if self.mode.requires_const_checking() {
let unleash_miri = self
.tcx
.sess
Expand Down Expand Up @@ -1302,7 +1340,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
}
}
ty::FnPtr(_) => {
if self.mode != Mode::Fn {
if self.mode.requires_const_checking() {
let mut err = self.tcx.sess.struct_span_err(
self.span,
&format!("function pointers are not allowed in const fn"));
Expand Down Expand Up @@ -1361,7 +1399,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
self.super_terminator_kind(kind, location);

// Deny *any* live drops anywhere other than functions.
if self.mode != Mode::Fn {
if self.mode.requires_const_checking() {
unleash_miri!(self);
// HACK(eddyb): emulate a bit of dataflow analysis,
// conservatively, that drop elaboration will do.
Expand Down Expand Up @@ -1472,12 +1510,12 @@ impl MirPass for QualifyAndPromoteConstants {
let id = tcx.hir().as_local_hir_id(def_id).unwrap();
let mut const_promoted_temps = None;
let mode = match tcx.hir().body_owner_kind_by_hir_id(id) {
hir::BodyOwnerKind::Closure => Mode::Fn,
hir::BodyOwnerKind::Closure => Mode::NonConstFn,
hir::BodyOwnerKind::Fn => {
if tcx.is_const_fn(def_id) {
Mode::ConstFn
} else {
Mode::Fn
Mode::NonConstFn
}
}
hir::BodyOwnerKind::Const => {
Expand All @@ -1489,7 +1527,7 @@ impl MirPass for QualifyAndPromoteConstants {
};

debug!("run_pass: mode={:?}", mode);
if mode == Mode::Fn || mode == Mode::ConstFn {
if mode == Mode::NonConstFn || mode == Mode::ConstFn {
// This is ugly because Checker holds onto mir,
// which can't be mutated until its scope ends.
let (temps, candidates) = {
Expand Down