Skip to content

Add a HIR pass to check consts for if, loop, etc. #66170

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

Merged
merged 14 commits into from
Nov 13, 2019
Merged
Changes from all 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
35 changes: 35 additions & 0 deletions src/librustc/hir/map/mod.rs
Original file line number Diff line number Diff line change
@@ -79,6 +79,33 @@ impl<'hir> Entry<'hir> {
}
}

fn fn_sig(&self) -> Option<&'hir FnSig> {
match &self.node {
Node::Item(item) => {
match &item.kind {
ItemKind::Fn(sig, _, _) => Some(sig),
_ => None,
}
}

Node::TraitItem(item) => {
match &item.kind {
TraitItemKind::Method(sig, _) => Some(sig),
_ => None
}
}

Node::ImplItem(item) => {
match &item.kind {
ImplItemKind::Method(sig, _) => Some(sig),
_ => None,
}
}

_ => None,
}
}

fn associated_body(self) -> Option<BodyId> {
match self.node {
Node::Item(item) => {
@@ -450,6 +477,14 @@ impl<'hir> Map<'hir> {
}
}

pub fn fn_sig_by_hir_id(&self, hir_id: HirId) -> Option<&'hir FnSig> {
if let Some(entry) = self.find_entry(hir_id) {
entry.fn_sig()
} else {
bug!("no entry for hir_id `{}`", hir_id)
}
}

/// Returns the `HirId` that corresponds to the definition of
/// which this is the body of, i.e., a `fn`, `const` or `static`
/// item (possibly associated), a closure, or a `hir::AnonConst`.
5 changes: 5 additions & 0 deletions src/librustc/query/mod.rs
Original file line number Diff line number Diff line change
@@ -329,6 +329,11 @@ rustc_queries! {
desc { |tcx| "checking for unstable API usage in {}", key.describe_as_module(tcx) }
}

/// Checks the const bodies in the module for illegal operations (e.g. `if` or `loop`).
query check_mod_const_bodies(key: DefId) -> () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Aside: I noticed that the categorization in this module wrt. Other, TypeChecking, and friends is rather messy and there doesn't seem to be a lot of logic to it? -- Maybe another C-cleanup issue...? (cc @Zoxc & @nikomatsakis)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this categorization was added as a very coarse grouping for profiling purposes IIRC, we can certainly clean things up here. cc @wesleywiser

Copy link
Member

Choose a reason for hiding this comment

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

Cleanups wrt categorization are definitely appreciated. I did the initial pass ~1yr ago and I don't think they've changed substantially since that time. Feel free to assign me to any such cleanup PRs.

desc { |tcx| "checking consts in {}", key.describe_as_module(tcx) }
}

/// Checks the loops in the module.
query check_mod_loops(key: DefId) -> () {
desc { |tcx| "checking loops in {}", key.describe_as_module(tcx) }
1 change: 1 addition & 0 deletions src/librustc_interface/passes.rs
Original file line number Diff line number Diff line change
@@ -875,6 +875,7 @@ fn analysis(tcx: TyCtxt<'_>, cnum: CrateNum) -> Result<()> {
tcx.ensure().check_mod_loops(local_def_id);
tcx.ensure().check_mod_attrs(local_def_id);
tcx.ensure().check_mod_unstable_api_usage(local_def_id);
tcx.ensure().check_mod_const_bodies(local_def_id);
});
});
});
9 changes: 8 additions & 1 deletion src/librustc_mir/transform/check_consts/validation.rs
Original file line number Diff line number Diff line change
@@ -461,7 +461,14 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> {
self.super_statement(statement, location);
}
StatementKind::FakeRead(FakeReadCause::ForMatchedPlace, _) => {
self.check_op(ops::IfOrMatch);
// FIXME: make this the `emit_error` impl of `ops::IfOrMatch` once the const
// checker is no longer run in compatability mode.
if !self.tcx.sess.opts.debugging_opts.unleash_the_miri_inside_of_you {
self.tcx.sess.delay_span_bug(
self.span,
"complex control flow is forbidden in a const context",
);
}
}
// FIXME(eddyb) should these really do nothing?
StatementKind::FakeRead(..) |
15 changes: 12 additions & 3 deletions src/librustc_mir/transform/qualify_consts.rs
Original file line number Diff line number Diff line change
@@ -723,8 +723,12 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
bb = target;
}
_ => {
self.not_const(ops::Loop);
validator.check_op(ops::Loop);
if !self.tcx.sess.opts.debugging_opts.unleash_the_miri_inside_of_you {
self.tcx.sess.delay_span_bug(
self.span,
"complex control flow is forbidden in a const context",
);
}
break;
}
}
@@ -1253,7 +1257,12 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
self.super_statement(statement, location);
}
StatementKind::FakeRead(FakeReadCause::ForMatchedPlace, _) => {
self.not_const(ops::IfOrMatch);
if !self.tcx.sess.opts.debugging_opts.unleash_the_miri_inside_of_you {
self.tcx.sess.delay_span_bug(
self.span,
"complex control flow is forbidden in a const context",
);
}
}
// FIXME(eddyb) should these really do nothing?
StatementKind::FakeRead(..) |
160 changes: 160 additions & 0 deletions src/librustc_passes/check_const.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
//! This pass checks HIR bodies that may be evaluated at compile-time (e.g., `const`, `static`,
//! `const fn`) for structured control flow (e.g. `if`, `while`), which is forbidden in a const
//! context.
//!
//! By the time the MIR const-checker runs, these high-level constructs have been lowered to
//! control-flow primitives (e.g., `Goto`, `SwitchInt`), making it tough to properly attribute
//! errors. We still look for those primitives in the MIR const-checker to ensure nothing slips
//! through, but errors for structured control flow in a `const` should be emitted here.

use rustc::hir::def_id::DefId;
use rustc::hir::intravisit::{Visitor, NestedVisitorMap};
use rustc::hir::map::Map;
use rustc::hir;
use rustc::session::Session;
use rustc::ty::TyCtxt;
use rustc::ty::query::Providers;
use syntax::ast::Mutability;
use syntax::span_err;
use syntax_pos::Span;

use std::fmt;

#[derive(Copy, Clone)]
enum ConstKind {
Static,
StaticMut,
ConstFn,
Const,
AnonConst,
}

impl ConstKind {
fn for_body(body: &hir::Body, hir_map: &Map<'_>) -> Option<Self> {
let is_const_fn = |id| hir_map.fn_sig_by_hir_id(id).unwrap().header.is_const();

let owner = hir_map.body_owner(body.id());
let const_kind = match hir_map.body_owner_kind(owner) {
hir::BodyOwnerKind::Const => Self::Const,
hir::BodyOwnerKind::Static(Mutability::Mutable) => Self::StaticMut,
hir::BodyOwnerKind::Static(Mutability::Immutable) => Self::Static,

hir::BodyOwnerKind::Fn if is_const_fn(owner) => Self::ConstFn,
hir::BodyOwnerKind::Fn | hir::BodyOwnerKind::Closure => return None,
};

Some(const_kind)
}
}

impl fmt::Display for ConstKind {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let s = match self {
Self::Static => "static",
Self::StaticMut => "static mut",
Self::Const | Self::AnonConst => "const",
Self::ConstFn => "const fn",
};

write!(f, "{}", s)
}
}

fn check_mod_const_bodies(tcx: TyCtxt<'_>, module_def_id: DefId) {
let mut vis = CheckConstVisitor::new(tcx);
tcx.hir().visit_item_likes_in_module(module_def_id, &mut vis.as_deep_visitor());
}

pub(crate) fn provide(providers: &mut Providers<'_>) {
*providers = Providers {
check_mod_const_bodies,
..*providers
};
}

#[derive(Copy, Clone)]
struct CheckConstVisitor<'tcx> {
sess: &'tcx Session,
hir_map: &'tcx Map<'tcx>,
const_kind: Option<ConstKind>,
}

impl<'tcx> CheckConstVisitor<'tcx> {
fn new(tcx: TyCtxt<'tcx>) -> Self {
CheckConstVisitor {
sess: &tcx.sess,
hir_map: tcx.hir(),
const_kind: None,
}
}

/// Emits an error when an unsupported expression is found in a const context.
fn const_check_violated(&self, bad_op: &str, span: Span) {
if self.sess.opts.debugging_opts.unleash_the_miri_inside_of_you {
self.sess.span_warn(span, "skipping const checks");
return;
}

let const_kind = self.const_kind
.expect("`const_check_violated` may only be called inside a const context");

span_err!(self.sess, span, E0744, "`{}` is not allowed in a `{}`", bad_op, const_kind);
}

/// Saves the parent `const_kind` before calling `f` and restores it afterwards.
fn recurse_into(&mut self, kind: Option<ConstKind>, f: impl FnOnce(&mut Self)) {
let parent_kind = self.const_kind;
self.const_kind = kind;
f(self);
self.const_kind = parent_kind;
}
}

impl<'tcx> Visitor<'tcx> for CheckConstVisitor<'tcx> {
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
NestedVisitorMap::OnlyBodies(&self.hir_map)
}

fn visit_anon_const(&mut self, anon: &'tcx hir::AnonConst) {
let kind = Some(ConstKind::AnonConst);
self.recurse_into(kind, |this| hir::intravisit::walk_anon_const(this, anon));
}

fn visit_body(&mut self, body: &'tcx hir::Body) {
let kind = ConstKind::for_body(body, self.hir_map);
self.recurse_into(kind, |this| hir::intravisit::walk_body(this, body));
}

fn visit_expr(&mut self, e: &'tcx hir::Expr) {
match &e.kind {
// Skip the following checks if we are not currently in a const context.
_ if self.const_kind.is_none() => {}

hir::ExprKind::Loop(_, _, source) => {
self.const_check_violated(source.name(), e.span);
}

hir::ExprKind::Match(_, _, source) => {
use hir::MatchSource::*;

let op = match source {
Normal => Some("match"),
IfDesugar { .. } | IfLetDesugar { .. } => Some("if"),
TryDesugar => Some("?"),
AwaitDesugar => Some(".await"),

// These are handled by `ExprKind::Loop` above.
WhileDesugar | WhileLetDesugar | ForLoopDesugar => None,
};

if let Some(op) = op {
self.const_check_violated(op, e.span);
}
}

_ => {},
}

hir::intravisit::walk_expr(self, e);
}
}
22 changes: 22 additions & 0 deletions src/librustc_passes/error_codes.rs
Original file line number Diff line number Diff line change
@@ -626,6 +626,28 @@ async fn foo() {}
Switch to the Rust 2018 edition to use `async fn`.
"##,

E0744: r##"
Control-flow expressions are not allowed inside a const context.

At the moment, `if` and `match`, as well as the looping constructs `for`,
`while`, and `loop`, are forbidden inside a `const`, `static`, or `const fn`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Upon re-reading this it occurred to me that we could link to the tracking issues... should we perhaps do that?
cc @estebank @oli-obk

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'll do this in a follow-up PR since the bulk of this one is ready.


```compile_fail,E0744
const _: i32 = {
let mut x = 0;
loop {
x += 1;
if x == 4 {
break;
}
}

x
};
```

"##,

;
E0226, // only a single explicit lifetime bound is permitted
E0472, // asm! is unsupported on this target
2 changes: 2 additions & 0 deletions src/librustc_passes/lib.rs
Original file line number Diff line number Diff line change
@@ -23,6 +23,7 @@ use rustc::ty::query::Providers;
pub mod error_codes;

pub mod ast_validation;
mod check_const;
pub mod hir_stats;
pub mod layout_test;
pub mod loops;
@@ -32,6 +33,7 @@ mod liveness;
mod intrinsicck;

pub fn provide(providers: &mut Providers<'_>) {
check_const::provide(providers);
entry::provide(providers);
loops::provide(providers);
liveness::provide(providers);
3 changes: 1 addition & 2 deletions src/test/compile-fail/consts/const-fn-error.rs
Original file line number Diff line number Diff line change
@@ -7,9 +7,8 @@ const fn f(x: usize) -> usize {
for i in 0..x {
//~^ ERROR E0015
//~| ERROR E0017
//~| ERROR E0019
//~| ERROR E0019
//~| ERROR E0080
//~| ERROR E0744
sum += i;
}
sum
8 changes: 4 additions & 4 deletions src/test/compile-fail/issue-52443.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
fn main() {
[(); & { loop { continue } } ]; //~ ERROR mismatched types
//~^ ERROR `loop` is not allowed in a `const`
[(); loop { break }]; //~ ERROR mismatched types
//~^ ERROR `loop` is not allowed in a `const`
[(); {while true {break}; 0}];
//~^ ERROR constant contains unimplemented expression type
//~| ERROR constant contains unimplemented expression type
//~^ ERROR `while` is not allowed in a `const`
//~| WARN denote infinite loops with
[(); { for _ in 0usize.. {}; 0}];
//~^ ERROR calls in constants are limited to constant functions
//~| ERROR `for` is not allowed in a `const`
//~| ERROR references in constants may only refer to immutable values
//~| ERROR constant contains unimplemented expression type
//~| ERROR constant contains unimplemented expression type
//~| ERROR evaluation of constant value failed
}
5 changes: 1 addition & 4 deletions src/test/ui/borrowck/issue-64453.rs
Original file line number Diff line number Diff line change
@@ -2,9 +2,7 @@ struct Project;
struct Value;

static settings_dir: String = format!("");
//~^ ERROR [E0019]
//~| ERROR [E0015]
//~| ERROR [E0015]
//~^ ERROR `match` is not allowed in a `static`

fn from_string(_: String) -> Value {
Value
@@ -13,7 +11,6 @@ fn set_editor(_: Value) {}

fn main() {
let settings_data = from_string(settings_dir);
//~^ ERROR cannot move out of static item `settings_dir` [E0507]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's unfortunate that this error no longer occurs. I believe it's simply because the compiler stops working before the move checker runs?

let args: i32 = 0;

match args {
Loading