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

Stabilize `let` bindings and destructuring in constants and const fn #57175

Merged
merged 7 commits into from Jan 12, 2019

Conversation

@oli-obk
Copy link
Contributor

oli-obk commented Dec 28, 2018

r? @Centril

This PR stabilizes the following features in constants and const functions:

  • irrefutable destructuring patterns (e.g. const fn foo((x, y): (u8, u8)) { ... })
  • let bindings (e.g. let x = 1;)
  • mutable let bindings (e.g. let mut x = 1;)
  • assignment (e.g. x = y) and assignment operator (e.g. x += y) expressions, even where the assignment target is a projection (e.g. a struct field or index operation like x[3] = 42)
  • expression statements (e.g. 3;)

This PR does explicitly not stabilize:

  • mutable references (i.e. &mut T)
  • dereferencing mutable references
  • refutable patterns (e.g. Some(x))
  • operations on UnsafeCell types (as that would need raw pointers and mutable references and such, not because it is explicitly forbidden. We can't explicitly forbid it as such values are OK as long as they aren't mutated.)
  • We are not stabilizing let bindings in constants that use && and || short circuiting operations. These are treated as & and | inside const and static items right now. If we stopped treating them as & and | after stabilizing let bindings, we'd break code like let mut x = false; false && { x = true; false };. So to use let bindings in constants you need to change && and || to & and | respectively.
@cramertj

This comment was marked as resolved.

Copy link
Member

cramertj commented Dec 28, 2018

This will be blocked on an FCP for #48821

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 28, 2018

I don't see #53515 mentioned here-- isn't this a blocker to stabilization? (or does #56160 somehow gate the problematic behavior?)

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 28, 2018

I don't see #53515 mentioned here-- isn't this a blocker to stabilization? (or does #56160 somehow gate the problematic behavior?)

yes, we're preventing both let bindings and short circuiting operators from being used within the same constant https://github.com/rust-lang/rust/search?utf8=%E2%9C%93&q=control_flow_destroyed&type=

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 28, 2018

I see, e.g. this test here.

Show resolved Hide resolved src/libsyntax/feature_gate.rs Outdated
Show resolved Hide resolved src/test/run-pass/ctfe/locals-in-const-fn.rs Outdated
Show resolved Hide resolved src/test/run-pass/ctfe/const-block-non-item-statement-3.rs Outdated
Show resolved Hide resolved src/test/run-pass/ctfe/const-block-non-item-statement.rs
Show resolved Hide resolved src/test/run-pass/ctfe/const-block-non-item-statement.rs
Show resolved Hide resolved src/test/ui/error-codes/E0010-teach.stderr
Show resolved Hide resolved src/test/ui/issues/issue-32829-2.rs
Show resolved Hide resolved src/test/ui/unsafe/ranged_ints2_const.rs Outdated
Show resolved Hide resolved src/test/ui/unsafe/ranged_ints3_const.rs Outdated
Show resolved Hide resolved src/test/ui/unsafe/ranged_ints4_const.rs Outdated

@Centril Centril requested a review from RalfJung Dec 29, 2018

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 29, 2018

Assigning over to:
r? @nikomatsakis
for review of the code changes since I primarily looked at tests.

@cramertj I will write up a stabilization report in a bit either as a separate issue or in this PR and I'll fcp merge that one.

@Centril

This comment was marked as resolved.

Copy link
Contributor

Centril commented Dec 29, 2018

Some more notes inspired by the old notes from #56160:

  • The PR also stabilizes const X: () = { 0; () }; which means that expression statements (i.e. StmtKind::Expr from the view of the AST) are also permitted so let's include that in the PR description.

  • The PR, from what I can tell, also stabilizes assignment operator expressions (i.e. ExprKind::AssignOp from the view of the AST), so let's include that in the PR description with a bullet point akin to:

    Assignment operators (+=, -=, ...) for primitive types except for floating points (i.e. f32 and f64).

  • Let's add a note about && and let in constants in the "This PR does explicitly not stabilize:" section.
    In the previous PR we noted at one point that:

    The reason we are only stabilizing this in const fn is that const fn does not allow the && and || short circuiting operations, which are treated as & and | inside const and static items right now. If we stopped treating them as & and | after stabilizing let bindings, we'd break code like let mut x = false; false && { x = true; false };. So we are waiting to stabilize let bindings in constants until && and || are supported fully together with if and match.

    We need to rework that since this is stabilizing let in const items.

  • Let's elaborate on "assignment statements" by adding a note that projections are permitted (x[y] = z; and x.y = z;).

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 30, 2018

Stabilization proposal

I propose that we stabilize #![feature(const_let)].

@rfcbot merge

Tracking issue: #48821
Version target: 1.33 (2019-01-15 => beta, 2019-03-01 => stable).

What is stabilized

In the following places:

We stabilize:

  • expression statements (e.g. 3;)

  • irrefutable destructuring patterns; for example:

    let (x, y) = expr;
    const fn foo([x, y]: [u8; 2]) {}
  • let bindings including mut ones; for example:

    let x = 42;
    let x: u8;
    let mut x: u8;
    let mut x = 24;
  • assignment (e.g. x = y) and assignment operator (e.g. x += y) expressions, even where the assignment target is a projection (e.g. a struct field or index operation like x[3] = 42).

What is not stabilized

  • mutable references (i.e. &mut T) and borrows (i.e. &mut x)
  • dereferencing mutable references
  • refutable patterns (e.g. Some(x))
  • operations on UnsafeCell types (as that would need raw pointers, mutable references, and such, not because it is explicitly forbidden. We cannot explicitly forbid it as such values are OK as long as they aren't mutated.)
  • We are not stabilizing let bindings in const contexts that use && and || short circuiting operations. These are treated as & and | inside const and static items right now. If we stopped treating them as & and | after stabilizing let bindings, we'd break code like let mut x = false; false && { x = true; false };. So to use let bindings in constants you need to change && and || to & and | respectively.

Divergences from RFC

  • See the note above about && and ||. These are control flow operations which should be stabilized in let bindings when we otherwise stabilize && with other control flow in the places aforementioned.
  • The RFC does not explicitly mention assignment and expression statements. However, as these are a natural consequence of having let bindings, it might as well be implied.

Tests

The tests can be primarily seen in the PR itself. Here are some of them:

Motivation

From the RFC:

It makes writing const fns much more like writing regular functions and is not possible right now because the old constant evaluator was a constant folder that could only process expressions. With the miri const evaluator this feature exists but is still disallowed.

As for motivations to do this now despite the lack of stable control flow,
given let bindings and assignments we may still:

  • Make overflowing_add into const fns on stable.

  • Write things such as:

    const fn bar() -> [u32; 500] {
        let mut foo = [0; 500];
        foo[3] = 42;
        foo
    }

History

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 30, 2018

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@alexreg

This comment was marked as off-topic.

Copy link
Contributor

alexreg commented Dec 31, 2018

Nice work. Speaking of control-flow (&& and || in this case), maybe it's time we should start working on that? Would you know how to approach it, @oli-obk?

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Dec 31, 2018

Since we already had ugly let statements by just making another function, I'm glad to see real ones.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 2, 2019

Two clarifications here:

  • Why is that we're banning let in combination with && and || rather than making it work? Is there something challenging about implementing the short-circuiting behavior? Or is this just because we haven't yet stabilized const_if?

We are not stabilizing let bindings in const contexts that use && and || short circuiting operations. These are treated as & and | inside const and static items right now.

  • "[T]reated as & and |" (which I normally associated with std::ops::BitAnd) just means that they do not yet short-circuit, right?
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 2, 2019

  • Why is that we're banning let in combination with && and || rather than making it work? Is there something challenging about implementing the short-circuiting behavior? Or is this just because we haven't yet stabilized const_if?

@oli-obk knows better than me, but the gist of it is that we haven't implemented match or other control flow yet so it's not even stabilization; on nightly you cannot even write:

#![feature(const_fn)]

const fn foo() {
    match 0 { // error[E0019]: constant function contains unimplemented expression type
        1 => {}
        _ => {}
    };
}

And we can think of && as equivalent to:

match lhs { false => false, true => rhs }

Meanwhile, || can be thought of as:

match lhs { false => rhs, true => true }

I think the main hurdle is just doing the work, which we eventually will, but it will take its time.

  • "[T]reated as & and |" (which I normally associated with std::ops::BitAnd) just means that they do not yet short-circuit, right?

Yep, and sadly we already support a buggy, non-short-circuiting, version of && and || in const items but not in const fn; once we do get control flow working in in const eval we can stabilize && and || in const fn as well.

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Jan 5, 2019

@oli-obk knows better than me, but the gist of it is that we haven't implemented match or other control flow yet so it's not even stabilization; on nightly you cannot even write:

This is correct. It requires control-flow support, which does not exist whatsoever yet. I'm hoping @oli-obk can pick up where eddyb left off with this. I can help perhaps.

@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Jan 8, 2019

☔️ The latest upstream changes (presumably #57332) made this pull request unmergeable. Please resolve the merge conflicts.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 8, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@oli-obk oli-obk force-pushed the oli-obk:const_let_stabilization branch from 0822254 to 80262e6 Jan 9, 2019

@nikomatsakis
Copy link
Contributor

nikomatsakis left a comment

Code looks good to me. r=me presuming @oli-obk confirms the two "potential deviations" I identified below where our behavior maybe changes.

&format!("let bindings in {}s are unstable",self.mode));
}
LocalKind::Arg |
LocalKind::Var if self.mode == Mode::Fn => {

This comment has been minimized.

@nikomatsakis

nikomatsakis Jan 9, 2019

Contributor

Question: we used to treat Arg differently, right? Is this a bug fix?

(I'm mainly comparing "before-after" in this PR and this is one of the few places where seem to be doing something different, if you assume that the const_let feature gate is enabled)

This comment has been minimized.

@oli-obk

oli-obk Jan 11, 2019

Contributor

I undid this change. The regular code can handle this just fine. I think this ended up being inserted due to some intermediate change

Show resolved Hide resolved src/librustc_mir/transform/qualify_consts.rs
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 11, 2019

@bors r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 11, 2019

📌 Commit 16a4e47 has been approved by nikomatsakis

Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2019

Rollup merge of rust-lang#57175 - oli-obk:const_let_stabilization, r=…
…nikomatsakis

Stabilize `let` bindings and destructuring in constants and const fn

r? @Centril

This PR stabilizes the following features in constants and `const` functions:

* irrefutable destructuring patterns (e.g. `const fn foo((x, y): (u8, u8)) { ... }`)
* `let` bindings (e.g. `let x = 1;`)
* mutable `let` bindings (e.g. `let mut x = 1;`)
* assignment (e.g. `x = y`) and assignment operator (e.g. `x += y`) expressions, even where the assignment target is a projection (e.g. a struct field or index operation like `x[3] = 42`)
* expression statements (e.g. `3;`)

This PR does explicitly *not* stabilize:

* mutable references (i.e. `&mut T`)
* dereferencing mutable references
* refutable patterns (e.g. `Some(x)`)
* operations on `UnsafeCell` types (as that would need raw pointers and mutable references and such, not because it is explicitly forbidden. We can't explicitly forbid it as such values are OK as long as they aren't mutated.)
* We are not stabilizing `let` bindings in constants that use `&&` and `||` short circuiting operations. These are treated as `&` and `|` inside `const` and `static` items right now. If we stopped treating them as `&` and `|` after stabilizing `let` bindings, we'd break code like `let mut x = false; false && { x = true; false };`. So to use `let` bindings in constants you need to change `&&` and `||` to `&` and `|` respectively.

Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2019

Rollup merge of rust-lang#57175 - oli-obk:const_let_stabilization, r=…
…nikomatsakis

Stabilize `let` bindings and destructuring in constants and const fn

r? @Centril

This PR stabilizes the following features in constants and `const` functions:

* irrefutable destructuring patterns (e.g. `const fn foo((x, y): (u8, u8)) { ... }`)
* `let` bindings (e.g. `let x = 1;`)
* mutable `let` bindings (e.g. `let mut x = 1;`)
* assignment (e.g. `x = y`) and assignment operator (e.g. `x += y`) expressions, even where the assignment target is a projection (e.g. a struct field or index operation like `x[3] = 42`)
* expression statements (e.g. `3;`)

This PR does explicitly *not* stabilize:

* mutable references (i.e. `&mut T`)
* dereferencing mutable references
* refutable patterns (e.g. `Some(x)`)
* operations on `UnsafeCell` types (as that would need raw pointers and mutable references and such, not because it is explicitly forbidden. We can't explicitly forbid it as such values are OK as long as they aren't mutated.)
* We are not stabilizing `let` bindings in constants that use `&&` and `||` short circuiting operations. These are treated as `&` and `|` inside `const` and `static` items right now. If we stopped treating them as `&` and `|` after stabilizing `let` bindings, we'd break code like `let mut x = false; false && { x = true; false };`. So to use `let` bindings in constants you need to change `&&` and `||` to `&` and `|` respectively.

bors added a commit that referenced this pull request Jan 11, 2019

Auto merge of #57513 - Centril:rollup, r=Centril
Rollup of 2 pull requests

Successful merges:

 - #57175 (Stabilize `let` bindings and destructuring in constants and const fn)
 - #57234 (Const-stabilize `const_int_ops` + `const_ip`)

Failed merges:

r? @ghost
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 12, 2019

@bors r=nikomatsakis p=1

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 12, 2019

📌 Commit 6c62322 has been approved by nikomatsakis

Centril added a commit to Centril/rust that referenced this pull request Jan 12, 2019

Rollup merge of rust-lang#57175 - oli-obk:const_let_stabilization, r=…
…nikomatsakis

Stabilize `let` bindings and destructuring in constants and const fn

r? @Centril

This PR stabilizes the following features in constants and `const` functions:

* irrefutable destructuring patterns (e.g. `const fn foo((x, y): (u8, u8)) { ... }`)
* `let` bindings (e.g. `let x = 1;`)
* mutable `let` bindings (e.g. `let mut x = 1;`)
* assignment (e.g. `x = y`) and assignment operator (e.g. `x += y`) expressions, even where the assignment target is a projection (e.g. a struct field or index operation like `x[3] = 42`)
* expression statements (e.g. `3;`)

This PR does explicitly *not* stabilize:

* mutable references (i.e. `&mut T`)
* dereferencing mutable references
* refutable patterns (e.g. `Some(x)`)
* operations on `UnsafeCell` types (as that would need raw pointers and mutable references and such, not because it is explicitly forbidden. We can't explicitly forbid it as such values are OK as long as they aren't mutated.)
* We are not stabilizing `let` bindings in constants that use `&&` and `||` short circuiting operations. These are treated as `&` and `|` inside `const` and `static` items right now. If we stopped treating them as `&` and `|` after stabilizing `let` bindings, we'd break code like `let mut x = false; false && { x = true; false };`. So to use `let` bindings in constants you need to change `&&` and `||` to `&` and `|` respectively.

bors added a commit that referenced this pull request Jan 12, 2019

Auto merge of #57542 - Centril:rollup, r=Centril
Rollup of 26 pull requests

Successful merges:

 - #56425 (Redo the docs for Vec::set_len)
 - #56906 (Issue #56905)
 - #57042 (Don't call `FieldPlacement::count` when count is too large)
 - #57175 (Stabilize `let` bindings and destructuring in constants and const fn)
 - #57192 (Change std::error::Error trait documentation to talk about `source` instead of `cause`)
 - #57296 (Fixed the link to the ? operator)
 - #57368 (Use CMAKE_{C,CXX}_COMPILER_LAUNCHER for ccache)
 - #57400 (Rustdoc: update Source Serif Pro and replace Heuristica italic)
 - #57417 (rustdoc: use text-based doctest parsing if a macro is wrapping main)
 - #57433 (Add link destination for `read-ownership`)
 - #57434 (Remove `CrateNum::Invalid`.)
 - #57441 (Supporting backtrace for x86_64-fortanix-unknown-sgx.)
 - #57450 (actually take a slice in this example)
 - #57459 (Reference tracking issue for inherent associated types in diagnostic)
 - #57463 (docs: Fix some 'second-edition' links)
 - #57466 (Remove outdated comment)
 - #57493 (use structured suggestion when casting a reference)
 - #57498 (make note of one more normalization that Paths do)
 - #57499 (note that FromStr does not work for borrowed types)
 - #57505 (Remove submodule step from README)
 - #57510 (Add a profiles section to the manifest)
 - #57511 (Fix undefined behavior)
 - #57519 (Correct RELEASES.md for 1.32.0)
 - #57522 (don't unwrap unexpected tokens in `format!`)
 - #57530 (Fixing a typographical error.)
 - #57535 (Stabilise irrefutable if-let and while-let patterns)

Failed merges:

r? @ghost

@bors bors merged commit 6c62322 into rust-lang:master Jan 12, 2019

@Centril Centril referenced this pull request Jan 12, 2019

Open

Document `const_let` #506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment