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

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

Merged
merged 14 commits into from
Nov 13, 2019
Merged
Show file tree
Hide file tree
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
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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`.
Expand Down
5 changes: 5 additions & 0 deletions src/librustc/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
Expand Down
1 change: 1 addition & 0 deletions src/librustc_interface/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});
Expand Down
9 changes: 8 additions & 1 deletion src/librustc_mir/transform/check_consts/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(..) |
Expand Down
15 changes: 12 additions & 3 deletions src/librustc_mir/transform/qualify_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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(..) |
Expand Down
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<'_>) {
Centril marked this conversation as resolved.
Show resolved Hide resolved
*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
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions src/librustc_passes/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand Down
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
Expand Up @@ -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
Expand Down
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
Expand Up @@ -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
Expand All @@ -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 {
Expand Down
Loading