Skip to content

Commit

Permalink
Auto merge of rust-lang#74595 - lcnr:ConstEvaluatable-fut-compat, r=o…
Browse files Browse the repository at this point in the history
…li-obk

make `ConstEvaluatable` more strict

relevant zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/.60ConstEvaluatable.60.20generic.20functions/near/204125452

Let's see how much this impacts. Depending on how this goes this should probably be a future compat warning.

Short explanation: we currently forbid anonymous constants which depend on generic types, e.g. `[0; std::mem::size_of::<T>]` currently errors.

We previously checked this by evaluating the constant and returned an error if that failed. This however allows things like
```rust
const fn foo<T>() -> usize {
    if std::mem::size_of::<*mut T>() < 8 { // size of *mut T does not depend on T
        std::mem::size_of::<T>()
    } else {
        8
    }
}

fn test<T>() {
    let _ = [0; foo::<T>()];
}
```
which is a backwards compatibility hazard. This also has worrying interactions with mir optimizations (rust-lang#74491 (comment)) and intrinsics (rust-lang#74538).

r? `@oli-obk` `@eddyb`
  • Loading branch information
bors committed Sep 9, 2020
2 parents d92155b + 74e0719 commit e2be5f5
Show file tree
Hide file tree
Showing 14 changed files with 184 additions and 18 deletions.
31 changes: 27 additions & 4 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,23 @@ pub struct Body<'tcx> {
/// FIXME(oli-obk): rewrite the promoted during promotion to eliminate the cell components.
pub ignore_interior_mut_in_const_validation: bool,

/// Does this body use generic parameters. This is used for the `ConstEvaluatable` check.
///
/// Note that this does not actually mean that this body is not computable right now.
/// The repeat count in the following example is polymorphic, but can still be evaluated
/// without knowing anything about the type parameter `T`.
///
/// ```rust
/// fn test<T>() {
/// let _ = [0; std::mem::size_of::<*mut T>()];
/// }
/// ```
///
/// **WARNING**: Do not change this flags after the MIR was originally created, even if an optimization
/// removed the last mention of all generic params. We do not want to rely on optimizations and
/// potentially allow things like `[u8; std::mem::size_of::<T>() * 0]` due to this.
pub is_polymorphic: bool,

predecessor_cache: PredecessorCache,
}

Expand All @@ -208,7 +225,7 @@ impl<'tcx> Body<'tcx> {
local_decls.len()
);

Body {
let mut body = Body {
phase: MirPhase::Build,
basic_blocks,
source_scopes,
Expand All @@ -224,8 +241,11 @@ impl<'tcx> Body<'tcx> {
span,
required_consts: Vec::new(),
ignore_interior_mut_in_const_validation: false,
is_polymorphic: false,
predecessor_cache: PredecessorCache::new(),
}
};
body.is_polymorphic = body.has_param_types_or_consts();
body
}

/// Returns a partially initialized MIR body containing only a list of basic blocks.
Expand All @@ -234,7 +254,7 @@ impl<'tcx> Body<'tcx> {
/// is only useful for testing but cannot be `#[cfg(test)]` because it is used in a different
/// crate.
pub fn new_cfg_only(basic_blocks: IndexVec<BasicBlock, BasicBlockData<'tcx>>) -> Self {
Body {
let mut body = Body {
phase: MirPhase::Build,
basic_blocks,
source_scopes: IndexVec::new(),
Expand All @@ -250,8 +270,11 @@ impl<'tcx> Body<'tcx> {
generator_kind: None,
var_debug_info: Vec::new(),
ignore_interior_mut_in_const_validation: false,
is_polymorphic: false,
predecessor_cache: PredecessorCache::new(),
}
};
body.is_polymorphic = body.has_param_types_or_consts();
body
}

#[inline]
Expand Down
11 changes: 11 additions & 0 deletions compiler/rustc_session/src/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,16 @@ declare_lint! {
};
}

declare_lint! {
pub CONST_EVALUATABLE_UNCHECKED,
Warn,
"detects a generic constant is used in a type without a emitting a warning",
@future_incompatible = FutureIncompatibleInfo {
reference: "issue #76200 <https://github.com/rust-lang/rust/issues/76200>",
edition: None,
};
}

declare_lint_pass! {
/// Does nothing as a lint pass, but registers some `Lint`s
/// that are used by other parts of the compiler.
Expand Down Expand Up @@ -612,6 +622,7 @@ declare_lint_pass! {
UNSAFE_OP_IN_UNSAFE_FN,
INCOMPLETE_INCLUDE,
CENUM_IMPL_DROP_CAST,
CONST_EVALUATABLE_UNCHECKED,
]
}

Expand Down
61 changes: 61 additions & 0 deletions compiler/rustc_trait_selection/src/traits/const_evaluatable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
use rustc_hir::def::DefKind;
use rustc_infer::infer::InferCtxt;
use rustc_middle::mir::interpret::ErrorHandled;
use rustc_middle::ty::subst::SubstsRef;
use rustc_middle::ty::{self, TypeFoldable};
use rustc_session::lint;
use rustc_span::def_id::DefId;
use rustc_span::Span;

pub fn is_const_evaluatable<'cx, 'tcx>(
infcx: &InferCtxt<'cx, 'tcx>,
def: ty::WithOptConstParam<DefId>,
substs: SubstsRef<'tcx>,
param_env: ty::ParamEnv<'tcx>,
span: Span,
) -> Result<(), ErrorHandled> {
let future_compat_lint = || {
if let Some(local_def_id) = def.did.as_local() {
infcx.tcx.struct_span_lint_hir(
lint::builtin::CONST_EVALUATABLE_UNCHECKED,
infcx.tcx.hir().local_def_id_to_hir_id(local_def_id),
span,
|err| {
err.build("cannot use constants which depend on generic parameters in types")
.emit();
},
);
}
};

// FIXME: We should only try to evaluate a given constant here if it is fully concrete
// as we don't want to allow things like `[u8; std::mem::size_of::<*mut T>()]`.
//
// We previously did not check this, so we only emit a future compat warning if
// const evaluation succeeds and the given constant is still polymorphic for now
// and hopefully soon change this to an error.
//
// See #74595 for more details about this.
let concrete = infcx.const_eval_resolve(param_env, def, substs, None, Some(span));

let def_kind = infcx.tcx.def_kind(def.did);
match def_kind {
DefKind::AnonConst => {
let mir_body = if let Some(def) = def.as_const_arg() {
infcx.tcx.optimized_mir_of_const_arg(def)
} else {
infcx.tcx.optimized_mir(def.did)
};
if mir_body.is_polymorphic && concrete.is_ok() {
future_compat_lint();
}
}
_ => {
if substs.has_param_types_or_consts() && concrete.is_ok() {
future_compat_lint();
}
}
}

concrete.map(drop)
}
13 changes: 7 additions & 6 deletions compiler/rustc_trait_selection/src/traits/fulfill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use rustc_middle::ty::ToPredicate;
use rustc_middle::ty::{self, Binder, Const, Ty, TypeFoldable};
use std::marker::PhantomData;

use super::const_evaluatable;
use super::project;
use super::select::SelectionContext;
use super::wf;
Expand Down Expand Up @@ -458,15 +459,15 @@ impl<'a, 'b, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'b, 'tcx> {
}

ty::PredicateAtom::ConstEvaluatable(def_id, substs) => {
match self.selcx.infcx().const_eval_resolve(
obligation.param_env,
match const_evaluatable::is_const_evaluatable(
self.selcx.infcx(),
def_id,
substs,
None,
Some(obligation.cause.span),
obligation.param_env,
obligation.cause.span,
) {
Ok(_) => ProcessResult::Changed(vec![]),
Err(err) => ProcessResult::Error(CodeSelectionError(ConstEvalFailure(err))),
Ok(()) => ProcessResult::Changed(vec![]),
Err(e) => ProcessResult::Error(CodeSelectionError(ConstEvalFailure(e))),
}
}

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_trait_selection/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ pub mod auto_trait;
mod chalk_fulfill;
pub mod codegen;
mod coherence;
mod const_evaluatable;
mod engine;
pub mod error_reporting;
mod fulfill;
Expand Down
11 changes: 6 additions & 5 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use self::EvaluationResult::*;
use self::SelectionCandidate::*;

use super::coherence::{self, Conflict};
use super::const_evaluatable;
use super::project;
use super::project::normalize_with_depth_to;
use super::util;
Expand Down Expand Up @@ -542,14 +543,14 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}

ty::PredicateAtom::ConstEvaluatable(def_id, substs) => {
match self.tcx().const_eval_resolve(
obligation.param_env,
match const_evaluatable::is_const_evaluatable(
self.infcx,
def_id,
substs,
None,
None,
obligation.param_env,
obligation.cause.span,
) {
Ok(_) => Ok(EvaluatedToOk),
Ok(()) => Ok(EvaluatedToOk),
Err(ErrorHandled::TooGeneric) => Ok(EvaluatedToAmbig),
Err(_) => Ok(EvaluatedToErr),
}
Expand Down
11 changes: 11 additions & 0 deletions src/test/ui/const_evaluatable/associated-const.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// check-pass
struct Foo<T>(T);
impl<T> Foo<T> {
const VALUE: usize = std::mem::size_of::<T>();
}

fn test<T>() {
let _ = [0; Foo::<u8>::VALUE];
}

fn main() {}
19 changes: 19 additions & 0 deletions src/test/ui/const_evaluatable/function-call.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// check-pass

const fn foo<T>() -> usize {
// We might instead branch on `std::mem::size_of::<*mut T>() < 8` here,
// which would cause this function to fail on 32 bit systems.
if false {
std::mem::size_of::<T>()
} else {
8
}
}

fn test<T>() {
let _ = [0; foo::<T>()];
//~^ WARN cannot use constants which depend on generic parameters in types
//~| WARN this was previously accepted by the compiler but is being phased out
}

fn main() {}
12 changes: 12 additions & 0 deletions src/test/ui/const_evaluatable/function-call.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
warning: cannot use constants which depend on generic parameters in types
--> $DIR/function-call.rs:14:17
|
LL | let _ = [0; foo::<T>()];
| ^^^^^^^^^^
|
= note: `#[warn(const_evaluatable_unchecked)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #76200 <https://github.com/rust-lang/rust/issues/76200>

warning: 1 warning emitted

Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
// run-pass

#![feature(arbitrary_enum_discriminant, core_intrinsics)]

extern crate core;
Expand All @@ -9,6 +8,8 @@ use core::intrinsics::discriminant_value;
enum MyWeirdOption<T> {
None = 0,
Some(T) = core::mem::size_of::<*mut T>(),
//~^ WARN cannot use constants which depend on generic parameters in types
//~| WARN this was previously accepted by the compiler but is being phased out
}

fn main() {
Expand Down
12 changes: 12 additions & 0 deletions src/test/ui/enum-discriminant/issue-70453-polymorphic-ctfe.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
warning: cannot use constants which depend on generic parameters in types
--> $DIR/issue-70453-polymorphic-ctfe.rs:10:15
|
LL | Some(T) = core::mem::size_of::<*mut T>(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(const_evaluatable_unchecked)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #76200 <https://github.com/rust-lang/rust/issues/76200>

warning: 1 warning emitted

3 changes: 1 addition & 2 deletions src/test/ui/impl-trait/issue-56445.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@

use std::marker::PhantomData;

pub struct S<'a>
{
pub struct S<'a> {
pub m1: PhantomData<&'a u8>,
pub m2: [u8; S::size()],
}
Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/lazy_normalization_consts/issue-73980.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,7 @@ impl<T: ?Sized> L<T> {
}

impl<T> X<T, [u8; L::<T>::S]> {}
//~^ WARN cannot use constants which depend on generic parameters
//~| WARN this was previously accepted by the compiler but is being phased out

fn main() {}
12 changes: 12 additions & 0 deletions src/test/ui/lazy_normalization_consts/issue-73980.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
warning: cannot use constants which depend on generic parameters in types
--> $DIR/issue-73980.rs:12:9
|
LL | impl<T> X<T, [u8; L::<T>::S]> {}
| ^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(const_evaluatable_unchecked)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #76200 <https://github.com/rust-lang/rust/issues/76200>

warning: 1 warning emitted

0 comments on commit e2be5f5

Please sign in to comment.