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

Enable loop and while in constants behind a feature flag #67216

Merged
merged 19 commits into from
Dec 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
2 changes: 1 addition & 1 deletion src/librustc_error_codes/error_codes/E0744.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ 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`.

```compile_fail,E0744
```compile_fail,E0658
const _: i32 = {
let mut x = 0;
loop {
Expand Down
14 changes: 14 additions & 0 deletions src/librustc_feature/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,17 @@ macro_rules! declare_features {
pub fn walk_feature_fields(&self, mut f: impl FnMut(&str, bool)) {
$(f(stringify!($feature), self.$feature);)+
}

/// Is the given feature enabled?
///
/// Panics if the symbol doesn't correspond to a declared feature.
pub fn enabled(&self, feature: Symbol) -> bool {
ecstatic-morse marked this conversation as resolved.
Show resolved Hide resolved
match feature {
$( sym::$feature => self.$feature, )*

_ => panic!("`{}` was not listed in `declare_features`", feature),
}
}
}
};
}
Expand Down Expand Up @@ -526,6 +537,9 @@ declare_features! (
/// Allows using `&mut` in constant functions.
(active, const_mut_refs, "1.41.0", Some(57349), None),

/// Allows the use of `loop` and `while` in constants.
(active, const_loop, "1.41.0", Some(52000), None),

// -------------------------------------------------------------------------
// feature-group-end: actual feature gates
// -------------------------------------------------------------------------
Expand Down
4 changes: 4 additions & 0 deletions src/librustc_mir/transform/check_consts/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@ impl NonConstOp for LiveDrop {
#[derive(Debug)]
pub struct Loop;
impl NonConstOp for Loop {
fn feature_gate(tcx: TyCtxt<'_>) -> Option<bool> {
Some(tcx.features().const_loop)
}

fn emit_error(&self, item: &Item<'_, '_>, span: Span) {
// This should be caught by the HIR const-checker.
item.tcx.sess.delay_span_bug(
Expand Down
6 changes: 5 additions & 1 deletion src/librustc_mir/transform/qualify_min_const_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -390,8 +390,12 @@ fn check_terminator(
cleanup: _,
} => check_operand(tcx, cond, span, def_id, body),

| TerminatorKind::FalseUnwind { .. }
if feature_allowed(tcx, def_id, sym::const_loop)
=> Ok(()),
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 very fragile. FalseUnwind is supposed to be ignored (unless you care about unwind paths).

What this code should be doing is a CFG cyclicity check.

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Jan 17, 2020

Choose a reason for hiding this comment

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

There is one in check_consts that will run even if #![feature(const_fn)] is specified. I hope we can lose that feature gate in favor of fine-grained ones in the future, as well as merge check_consts and qualify_min_const_fn.

Copy link
Member

@eddyb eddyb Jan 17, 2020

Choose a reason for hiding this comment

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

Do both run? That makes me happier, I guess, but then the FalseUnwind check here is not needed at all, is it?

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Jan 17, 2020

Choose a reason for hiding this comment

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

check_consts validation is always run. qualify_min_const_fn is run when #![feature(const_fn)] is not specified. My opinion is that this pass should be removed along with #![feature(const_fn)] and any check that isn't already in check_consts given its own feature gate along with an entry in check_consts/ops.rs. I don't wanna spend political capital to spearhead this, however.

Copy link
Member

Choose a reason for hiding this comment

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

qualify_min_const_fn was added as a hack back when the normal const-checking pass was such a mess that nobody was confident in its ability to rule out all bad code. It was always meant to be temporary.

So, assuming const-check these days is well-structured enough that everyone is confident in its ability to do what it is supposed to do, I don't think you'll need to spend any political capital to get rid of it. Quite the contrary, at least I personally would be delighted to see it go. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer if we could wait with removing qualify_min_const_fn. Not necessarily because the new const checker is messy, but because I don't understand the new code (which is based on data-flow) nearly as well as the old code. And as the lang team point-person for the const things, I would like to understand the thing that checks for stability wrt. const. So I'd appreciate if we don't do it right now and allow me some time to study the new stuff. (Also, is there an urgency to this?)

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no urgency, there are some preliminary things that can be done like eliminating the const_fn feature gate

Copy link
Member

@eddyb eddyb Jan 17, 2020

Choose a reason for hiding this comment

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

You could just make the handling of FalseUnwind consistent, for now.
It's not, and should not be mistaken for, a loop.

As the name says, it's a "false" (or "phantom") unwind edge. It's for situations where analyses which care about unwinding / cleanup blocks need to be conservative and assume unwinding can happen "out of the blue".

Everything else in const-checking ignores unwind edges (e.g. from calls) and associated cleanup blocks, because we completely bypass unwinding at compile-time.

Copy link
Member

Choose a reason for hiding this comment

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

See #62274 for precedent.

Copy link
Member

Choose a reason for hiding this comment

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

Also looks like FalseEdges is also mishandled?


TerminatorKind::FalseUnwind { .. } => {
Err((span, "loops are not allowed in const fn".into()))
},
}
}
}
87 changes: 68 additions & 19 deletions src/librustc_passes/check_const.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ use rustc::hir::map::Map;
use rustc::hir;
use rustc::ty::TyCtxt;
use rustc::ty::query::Providers;
use rustc_feature::Features;
use rustc::session::config::nightly_options;
use syntax::ast::Mutability;
use syntax::feature_gate::feature_err;
use syntax::span_err;
use syntax_pos::{sym, Span};
use syntax_pos::{sym, Span, Symbol};
use rustc_error_codes::*;

use std::fmt;
Expand All @@ -37,18 +37,31 @@ impl NonConstExpr {
}
}

/// Returns `true` if all feature gates required to enable this expression are turned on, or
/// `None` if there is no feature gate corresponding to this expression.
fn is_feature_gate_enabled(self, features: &Features) -> Option<bool> {
fn required_feature_gates(self) -> Option<&'static [Symbol]> {
use hir::MatchSource::*;
match self {
use hir::LoopSource::*;

let gates: &[_] = match self {
| Self::Match(Normal)
| Self::Match(IfDesugar { .. })
| Self::Match(IfLetDesugar { .. })
=> Some(features.const_if_match),
=> &[sym::const_if_match],

_ => None,
}
| Self::Loop(Loop)
=> &[sym::const_loop],

| Self::Loop(While)
| Self::Loop(WhileLet)
| Self::Match(WhileDesugar)
| Self::Match(WhileLetDesugar)
=> &[sym::const_loop, sym::const_if_match],

// A `for` loop's desugaring contains a call to `IntoIterator::into_iter`,
// so they are not yet allowed with `#![feature(const_loop)]`.
_ => return None,
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
};

Some(gates)
}
}

Expand Down Expand Up @@ -120,11 +133,15 @@ impl<'tcx> CheckConstVisitor<'tcx> {

/// Emits an error when an unsupported expression is found in a const context.
fn const_check_violated(&self, expr: NonConstExpr, span: Span) {
match expr.is_feature_gate_enabled(self.tcx.features()) {
let features = self.tcx.features();
let required_gates = expr.required_feature_gates();
match required_gates {
// Don't emit an error if the user has enabled the requisite feature gates.
Some(true) => return,
Some(gates) if gates.iter().all(|&g| features.enabled(g)) => return,

// Users of `-Zunleash-the-miri-inside-of-you` must use feature gates when possible.
// `-Zunleash-the-miri-inside-of-you` only works for expressions that don't have a
// corresponding feature gate. This encourages nightly users to use feature gates when
// possible.
None if self.tcx.sess.opts.debugging_opts.unleash_the_miri_inside_of_you => {
self.tcx.sess.span_warn(span, "skipping const checks");
return;
Expand All @@ -135,15 +152,47 @@ impl<'tcx> CheckConstVisitor<'tcx> {

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

let msg = format!("`{}` is not allowed in a `{}`", expr.name(), const_kind);
match expr {
| NonConstExpr::Match(hir::MatchSource::Normal)
| NonConstExpr::Match(hir::MatchSource::IfDesugar { .. })
| NonConstExpr::Match(hir::MatchSource::IfLetDesugar { .. })
=> feature_err(&self.tcx.sess.parse_sess, sym::const_if_match, span, &msg).emit(),

_ => span_err!(self.tcx.sess, span, E0744, "{}", msg),
let required_gates = required_gates.unwrap_or(&[]);
let missing_gates: Vec<_> = required_gates
.iter()
.copied()
.filter(|&g| !features.enabled(g))
.collect();

match missing_gates.as_slice() {
&[] => span_err!(self.tcx.sess, span, E0744, "{}", msg),

// If the user enabled `#![feature(const_loop)]` but not `#![feature(const_if_match)]`,
// explain why their `while` loop is being rejected.
&[gate @ sym::const_if_match] if required_gates.contains(&sym::const_loop) => {
feature_err(&self.tcx.sess.parse_sess, gate, span, &msg)
.note("`#![feature(const_loop)]` alone is not sufficient, \
since this loop expression contains an implicit conditional")
.emit();
}

&[missing_primary, ref missing_secondary @ ..] => {
let mut err = feature_err(&self.tcx.sess.parse_sess, missing_primary, span, &msg);

// If multiple feature gates would be required to enable this expression, include
// them as help messages. Don't emit a separate error for each missing feature gate.
//
// FIXME(ecstaticmorse): Maybe this could be incorporated into `feature_err`? This
Centril marked this conversation as resolved.
Show resolved Hide resolved
// is a pretty narrow case, however.
if nightly_options::is_nightly_build() {
for gate in missing_secondary {
let note = format!(
"add `#![feature({})]` to the crate attributes to enable",
gate,
);
err.help(&note);
}
}

err.emit();
}
}
}

Expand Down
1 change: 1 addition & 0 deletions src/librustc_passes/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#![feature(in_band_lifetimes)]
#![feature(nll)]
#![feature(slice_patterns)]

#![recursion_limit="256"]

Expand Down
1 change: 1 addition & 0 deletions src/libsyntax_pos/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ symbols! {
const_indexing,
const_in_array_repeat_expressions,
const_let,
const_loop,
const_mut_refs,
const_panic,
const_raw_ptr_deref,
Expand Down
7 changes: 5 additions & 2 deletions src/test/ui/closures/issue-52437.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,14 @@ error: invalid label name `'static`
LL | [(); &(&'static: loop { |x| {}; }) as *const _ as usize]
| ^^^^^^^

error[E0744]: `loop` is not allowed in a `const`
error[E0658]: `loop` is not allowed in a `const`
--> $DIR/issue-52437.rs:2:13
|
LL | [(); &(&'static: loop { |x| {}; }) as *const _ as usize]
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: for more information, see https://github.com/rust-lang/rust/issues/52000
= help: add `#![feature(const_loop)]` to the crate attributes to enable

error[E0282]: type annotations needed
--> $DIR/issue-52437.rs:2:30
Expand All @@ -18,5 +21,5 @@ LL | [(); &(&'static: loop { |x| {}; }) as *const _ as usize]

error: aborting due to 3 previous errors

Some errors have detailed explanations: E0282, E0744.
Some errors have detailed explanations: E0282, E0658.
For more information about an error, try `rustc --explain E0282`.
8 changes: 6 additions & 2 deletions src/test/ui/consts/const-eval/infinite_loop.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error[E0744]: `while` is not allowed in a `const`
error[E0658]: `while` is not allowed in a `const`
--> $DIR/infinite_loop.rs:7:9
|
LL | / while n != 0 {
Expand All @@ -8,6 +8,10 @@ LL | |
LL | |
LL | | }
| |_________^
|
= note: for more information, see https://github.com/rust-lang/rust/issues/52000
= help: add `#![feature(const_loop)]` to the crate attributes to enable
= help: add `#![feature(const_if_match)]` to the crate attributes to enable

error[E0658]: `if` is not allowed in a `const`
--> $DIR/infinite_loop.rs:9:17
Expand Down Expand Up @@ -39,5 +43,5 @@ LL | n = if n % 2 == 0 { n/2 } else { 3*n + 1 };

error: aborting due to 3 previous errors

Some errors have detailed explanations: E0080, E0658, E0744.
Some errors have detailed explanations: E0080, E0658.
For more information about an error, try `rustc --explain E0080`.
7 changes: 5 additions & 2 deletions src/test/ui/consts/const-eval/issue-52442.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
error[E0744]: `loop` is not allowed in a `const`
error[E0658]: `loop` is not allowed in a `const`
--> $DIR/issue-52442.rs:2:14
|
LL | [(); { &loop { break } as *const _ as usize } ];
| ^^^^^^^^^^^^^^
|
= note: for more information, see https://github.com/rust-lang/rust/issues/52000
= help: add `#![feature(const_loop)]` to the crate attributes to enable

error[E0658]: casting pointers to integers in constants is unstable
--> $DIR/issue-52442.rs:2:13
Expand All @@ -21,5 +24,5 @@ LL | [(); { &loop { break } as *const _ as usize } ];

error: aborting due to 3 previous errors

Some errors have detailed explanations: E0080, E0658, E0744.
Some errors have detailed explanations: E0080, E0658.
For more information about an error, try `rustc --explain E0080`.
8 changes: 6 additions & 2 deletions src/test/ui/consts/const-eval/issue-52475.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error[E0744]: `while` is not allowed in a `const`
error[E0658]: `while` is not allowed in a `const`
--> $DIR/issue-52475.rs:6:9
|
LL | / while n < 5 {
Expand All @@ -7,6 +7,10 @@ LL | | n = (n + 1) % 5;
LL | | x = &0; // Materialize a new AllocId
LL | | }
| |_________^
|
= note: for more information, see https://github.com/rust-lang/rust/issues/52000
= help: add `#![feature(const_loop)]` to the crate attributes to enable
= help: add `#![feature(const_if_match)]` to the crate attributes to enable

warning: Constant evaluating a complex constant, this might take some time
--> $DIR/issue-52475.rs:2:18
Expand All @@ -29,5 +33,5 @@ LL | n = (n + 1) % 5;

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0080, E0744.
Some errors have detailed explanations: E0080, E0658.
For more information about an error, try `rustc --explain E0080`.
12 changes: 9 additions & 3 deletions src/test/ui/consts/const-eval/issue-62272.stderr
Original file line number Diff line number Diff line change
@@ -1,15 +1,21 @@
error[E0744]: `loop` is not allowed in a `const`
error[E0658]: `loop` is not allowed in a `const`
--> $DIR/issue-62272.rs:7:17
|
LL | const FOO: () = loop { break; };
| ^^^^^^^^^^^^^^^
|
= note: for more information, see https://github.com/rust-lang/rust/issues/52000
= help: add `#![feature(const_loop)]` to the crate attributes to enable

error[E0744]: `loop` is not allowed in a `const`
error[E0658]: `loop` is not allowed in a `const`
--> $DIR/issue-62272.rs:10:20
|
LL | [FOO; { let x; loop { x = 5; break; } x }];
| ^^^^^^^^^^^^^^^^^^^^^^
|
= note: for more information, see https://github.com/rust-lang/rust/issues/52000
= help: add `#![feature(const_loop)]` to the crate attributes to enable

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0744`.
For more information about this error, try `rustc --explain E0658`.
8 changes: 6 additions & 2 deletions src/test/ui/consts/const-labeled-break.stderr
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
error[E0744]: `while` is not allowed in a `const`
error[E0658]: `while` is not allowed in a `const`
--> $DIR/const-labeled-break.rs:10:19
|
LL | const CRASH: () = 'a: while break 'a {};
| ^^^^^^^^^^^^^^^^^^^^^
|
= note: for more information, see https://github.com/rust-lang/rust/issues/52000
= help: add `#![feature(const_loop)]` to the crate attributes to enable
= help: add `#![feature(const_if_match)]` to the crate attributes to enable

error: aborting due to previous error

For more information about this error, try `rustc --explain E0744`.
For more information about this error, try `rustc --explain E0658`.
Loading