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

Can't implement a const Option::unwrap because of "destructors cannot be evaluated at compile-time" error. #66753

Closed
rodrimati1992 opened this issue Nov 25, 2019 · 9 comments · Fixed by #71824
Assignees
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-destructors Area: Destructors (`Drop`, …) B-unstable Blocker: Implemented in the nightly compiler and unstable. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@rodrimati1992
Copy link
Contributor

It's not possible to implement Option::unwrap as a const fn like:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=8ad0c1cdd371ca0488fab9d872bd6ffe

#![feature(const_fn)]
#![feature(const_if_match)]
#![feature(const_panic)]

const fn unwrap<T>(opt:Option<T>)->T{
    match opt {
        Some(x)=>x,
        None=>panic!("Trying to unwrap a None"),
    }
}

Because the function triggers this error:

error[E0493]: destructors cannot be evaluated at compile-time
 --> src/lib.rs:5:20
  |
5 | const fn unwrap<T>(opt:Option<T>)->T{
  |                    ^^^ constant functions cannot evaluate destructors

error: aborting due to previous error
@ecstatic-morse ecstatic-morse self-assigned this Nov 25, 2019
@ecstatic-morse ecstatic-morse added F-const_if_match B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Nov 25, 2019
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Nov 25, 2019

This was discussed here in the initial PR for #![feature(const_if_match)]. Basically, we don't clear qualifs from a local (opt in this case) if we move out of the only field of that local. I still think we want to special-case unit structs/enum variants because it will cover commonly used types like Option and Result. Alternatively, we could maybe suppress Drop terminators entirely when a local is moved out of completely, or if this is already implemented, run const checks after this pass occurs.

cc @rust-lang/wg-const-eval

@ecstatic-morse ecstatic-morse added A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-const-fn T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 25, 2019
@Centril
Copy link
Contributor

Centril commented Nov 25, 2019

This might need to consider #[non_exhaustive] as well similar to how the borrow checker needs to with #66722. cc @matthewjasper

@oli-obk
Copy link
Contributor

oli-obk commented Dec 20, 2019

Alternatively, we could maybe suppress Drop terminators entirely when a local is moved out of completely, or if this is already implemented, run const checks after this pass occurs.

So... we have three situations afaict:

  1. struct without explicit Drop impl, a !Copy field is moved out of (this is the (Vec<T>,) case)
  2. !Copy field of enum variant is moved out of (this is the Some case)
  3. the None case. Even after drop elaboriation, we still execute Drop::drop for the enum in the None case. Well, actually, even after our entire optimization pipeline, we hit bb16 in https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=152c663c007e0c9b67e733bf3d145588 which drops the value.

So... if even drop elaboration can't figure this out, I'm not sure const qualif should be finding an independent solution.

Several solutions come to mind:

  1. Introduce a Forget terminator in the spirit of
    • mem::drop == Drop terminator
    • hint::unreachable == Unreachable terminator
    • process::abort == Abort terminator
    • mem::forget == Forget terminator
  2. Add an "early drop elab" pass that detects when you match on an enum and move out in one variant and then automatically treats the other variants as moved out
    • seems dangerous to me, if we generate similar code out of other reasons, the optimization would trigger. Also affects borrowck
  3. Change mir building to not generate drops in the None-like cases
    • Undesirable because we don't want mir building to be complex

cc @eddyb

@eddyb
Copy link
Member

eddyb commented Jan 16, 2020

the None case. Even after drop elaboriation, we still execute Drop::drop for the enum in the None case.

I think there's something missing from drop elaboration's (and other analyses'?) dataflow usage:
Each edge of a SwitchInt(Discriminant(x)) can assume all other variants of x are uninitialized.

So the None arm would know Some.0 is uninitialized, and the Some arm moves out of Some.0, so the net effect would be that there's no fields to drop.

The annoying thing is that we also have to be able to check this in const-checking.


Hang on... promotion has to run before borrow-checking because it modifies the MIR.
But const-checking doesn't. Could we run const-checking after borrow-checking?

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jan 17, 2020

I'm planning to investigate eddyb's idea early next week (they gave me some instructions to follow over Zulip). This will require a small change to the dataflow analysis API which I'll probably do on top of #65672 just because I'm more familiar with it. Hopefully I can get a proof-of-concept up and running.

bors added a commit that referenced this issue Feb 27, 2020
Mark other variants as uninitialized after switch on discriminant

During drop elaboration, which builds the drop ladder that handles destruction during stack unwinding, we attempt to remove MIR `Drop` terminators that will never be reached in practice. This reduces the number of basic blocks that are passed to LLVM, which should improve performance. In #66753, a user pointed out that unreachable `Drop` terminators are common in functions like `Option::unwrap`, which move out of an `enum`. While discussing possible remedies for that issue, @eddyb suggested moving const-checking after drop elaboration. This would allow the former, which looks for `Drop` terminators and replicates a small amount of drop elaboration to determine whether a dropped local has been moved out, leverage the work done by the latter.

However, it turns out that drop elaboration is not as precise as it could be when it comes to eliminating useless drop terminators. For example, let's look at the code for `unwrap_or`.

```rust
fn unwrap_or<T>(opt: Option<T>, default: T) -> T {
    match opt {
        Some(inner) => inner,
        None => default,
    }
}
```

`opt` never needs to be dropped, since it is either moved out of (if it is `Some`) or has no drop glue (if it is `None`), and `default` only needs to be dropped if `opt` is `Some`. This is not reflected in the MIR we currently pass to codegen.

![pasted_image](https://user-images.githubusercontent.com/29463364/73384403-109a0d80-4280-11ea-8500-0637b368f2dc.png)

@eddyb also suggested the solution to this problem. When we switch on an enum discriminant, we should be marking all fields in other variants as definitely uninitialized. I implemented this on top of alongside a small optimization (split out into #68943) that suppresses drop terminators for enum variants with no fields (e.g. `Option::None`). This is the resulting MIR for `unwrap_or`.

![after](https://user-images.githubusercontent.com/29463364/73384823-e432c100-4280-11ea-84bd-d0bcc3b777b4.png)

In concert with #68943, this change speeds up many [optimized and debug builds](https://perf.rust-lang.org/compare.html?start=d55f3e9f1da631c636b54a7c22c1caccbe4bf0db&end=0077a7aa11ebc2462851676f9f464d5221b17d6a). We need to carefully investigate whether I have introduced any miscompilations before merging this. Code that never drops anything would be very fast indeed until memory is exhausted.
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Mar 2, 2020

After #68528 and #68943, the optimized MIR for unwrap no longer has any drop terminators in non-cleanup blocks. In order to make unwrap a const fn, we need to move const-checking (or at least the part of const-checking that looks for live drops) after drop elaboration.

@jyn514
Copy link
Member

jyn514 commented May 22, 2020

Silly question but - why does this need compiler support? Can't you mem::forget the option?

@rodrimati1992
Copy link
Contributor Author

rodrimati1992 commented May 22, 2020

@jyn514

Can't you mem::forget the option?

If you mean this:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=69e7f334990002dd979b81e38df3812e

#![feature(const_forget)]
#![feature(const_fn)]
#![feature(const_if_match)]
#![feature(const_panic)]

const fn unwrap<T>(opt:Option<T>)->T{
    match opt {
        Some(x)=>x,
        None=>{
            std::mem::forget(opt);
            panic!("Trying to unwrap a None")
        },
    }
}

It has no effect on the error message:

error[E0493]: destructors cannot be evaluated at compile-time
 --> src/lib.rs:6:20
  |
6 | const fn unwrap<T>(opt:Option<T>)->T{
  |                    ^^^ constant functions cannot evaluate destructors

Neither does using ManuallyDrop::new instead.

@jonas-schievink jonas-schievink added the A-destructors Area: Destructors (`Drop`, …) label May 22, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 13, 2020
…p-elab, r=oli-obk

Check for live drops in constants after drop elaboration

Resolves rust-lang#66753.

This PR splits the MIR "optimization" pass series in two and introduces a query–`mir_drops_elaborated_and_const_checked`–that holds the result of the `post_borrowck_cleanup` analyses and checks for live drops. This query is invoked in `rustc_interface` for all items requiring const-checking, which means we now do `post_borrowck_cleanup` for items even if they are unused in the crate.

As a result, we are now more precise about when drops are live. This is because drop elaboration can e.g. eliminate drops of a local when all its fields are moved from. This does not mean we are doing value-based analysis on move paths, however; Storing a `Some(CustomDropImpl)` into a field of a local will still set the qualifs for that entire local.

r? @oli-obk
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 14, 2020
…p-elab, r=oli-obk

Check for live drops in constants after drop elaboration

Resolves rust-lang#66753.

This PR splits the MIR "optimization" pass series in two and introduces a query–`mir_drops_elaborated_and_const_checked`–that holds the result of the `post_borrowck_cleanup` analyses and checks for live drops. This query is invoked in `rustc_interface` for all items requiring const-checking, which means we now do `post_borrowck_cleanup` for items even if they are unused in the crate.

As a result, we are now more precise about when drops are live. This is because drop elaboration can e.g. eliminate drops of a local when all its fields are moved from. This does not mean we are doing value-based analysis on move paths, however; Storing a `Some(CustomDropImpl)` into a field of a local will still set the qualifs for that entire local.

r? @oli-obk
RalfJung added a commit to RalfJung/rust that referenced this issue Jun 15, 2020
…p-elab, r=oli-obk

Check for live drops in constants after drop elaboration

Resolves rust-lang#66753.

This PR splits the MIR "optimization" pass series in two and introduces a query–`mir_drops_elaborated_and_const_checked`–that holds the result of the `post_borrowck_cleanup` analyses and checks for live drops. This query is invoked in `rustc_interface` for all items requiring const-checking, which means we now do `post_borrowck_cleanup` for items even if they are unused in the crate.

As a result, we are now more precise about when drops are live. This is because drop elaboration can e.g. eliminate drops of a local when all its fields are moved from. This does not mean we are doing value-based analysis on move paths, however; Storing a `Some(CustomDropImpl)` into a field of a local will still set the qualifs for that entire local.

r? @oli-obk
@bors bors closed this as completed in 5c61a8d Jun 15, 2020
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jun 15, 2020

More precise drop elaboration is now behind #![feature(const_precise_live_drops)], so Option::unwrap could be made unstably const fn with some #[cfg(bootstrap)] magic. We should probably just wait for the next bootstrap bump, however.

Manishearth added a commit to Manishearth/rust that referenced this issue Jun 27, 2020
…atch, r=oli-obk

Stabilize `#![feature(const_if_match)]`

Quoting from the [stabilization report](rust-lang#49146 (comment)):

> `if` and `match` expressions as well as the short-circuiting logic operators `&&` and `||` will become legal in all [const contexts](https://doc.rust-lang.org/reference/const_eval.html#const-context). A const context is any of the following:
>
> - The initializer of a `const`, `static`, `static mut` or enum discriminant.
> - The body of a `const fn`.
> - The value of a const generic (nightly only).
> - The length of an array type (`[u8; 3]`) or an array repeat expression (`[0u8; 3]`).
>
> Furthermore, the short-circuiting logic operators will no longer be lowered to their bitwise equivalents (`&` and `|` respectively) in `const` and `static` initializers (see rust-lang#57175). As a result, `let` bindings can be used alongside short-circuiting logic in those initializers.

Resolves rust-lang#49146.

Ideally, we would resolve 🐳 rust-lang#66753 before this lands on stable, so it might be worth pushing this back a release. Also, this means we should get the process started for rust-lang#52000, otherwise people will have no recourse except recursion for iterative `const fn`.

r? @oli-obk
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 27, 2020
…atch, r=oli-obk

Stabilize `#![feature(const_if_match)]`

Quoting from the [stabilization report](rust-lang#49146 (comment)):

> `if` and `match` expressions as well as the short-circuiting logic operators `&&` and `||` will become legal in all [const contexts](https://doc.rust-lang.org/reference/const_eval.html#const-context). A const context is any of the following:
>
> - The initializer of a `const`, `static`, `static mut` or enum discriminant.
> - The body of a `const fn`.
> - The value of a const generic (nightly only).
> - The length of an array type (`[u8; 3]`) or an array repeat expression (`[0u8; 3]`).
>
> Furthermore, the short-circuiting logic operators will no longer be lowered to their bitwise equivalents (`&` and `|` respectively) in `const` and `static` initializers (see rust-lang#57175). As a result, `let` bindings can be used alongside short-circuiting logic in those initializers.

Resolves rust-lang#49146.

Ideally, we would resolve 🐳 rust-lang#66753 before this lands on stable, so it might be worth pushing this back a release. Also, this means we should get the process started for rust-lang#52000, otherwise people will have no recourse except recursion for iterative `const fn`.

r? @oli-obk
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 28, 2020
…ch, r=oli-obk

Stabilize `#![feature(const_if_match)]`

Quoting from the [stabilization report](rust-lang#49146 (comment)):

> `if` and `match` expressions as well as the short-circuiting logic operators `&&` and `||` will become legal in all [const contexts](https://doc.rust-lang.org/reference/const_eval.html#const-context). A const context is any of the following:
>
> - The initializer of a `const`, `static`, `static mut` or enum discriminant.
> - The body of a `const fn`.
> - The value of a const generic (nightly only).
> - The length of an array type (`[u8; 3]`) or an array repeat expression (`[0u8; 3]`).
>
> Furthermore, the short-circuiting logic operators will no longer be lowered to their bitwise equivalents (`&` and `|` respectively) in `const` and `static` initializers (see rust-lang#57175). As a result, `let` bindings can be used alongside short-circuiting logic in those initializers.

Resolves rust-lang#49146.

Ideally, we would resolve 🐳 rust-lang#66753 before this lands on stable, so it might be worth pushing this back a release. Also, this means we should get the process started for rust-lang#52000, otherwise people will have no recourse except recursion for iterative `const fn`.

r? @oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-destructors Area: Destructors (`Drop`, …) B-unstable Blocker: Implemented in the nightly compiler and unstable. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants