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

Nightly ICE with non-const macro in const #65394

Closed
TomCrypto opened this issue Oct 14, 2019 · 5 comments · Fixed by #65485
Closed

Nightly ICE with non-const macro in const #65394

TomCrypto opened this issue Oct 14, 2019 · 5 comments · Fixed by #65485
Assignees
Labels
A-typesystem Area: The type system C-bug Category: This is a bug. glacier ICE tracked in rust-lang/glacier. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@TomCrypto
Copy link

Offending snippet using serde_json:

pub const JSON: serde_json::Value = serde_json::json!({
    "foo": "bar"
});

The code shouldn't build, as per the first few errors below, but currently on nightly it also causes the compiler to panic. It does not panic on stable or beta.

error[E0015]: calls in constants are limited to constant functions, tuple structs and tuple variants
 --> src/lib.rs:1:37
  |
1 |   pub const JSON: serde_json::Value = serde_json::json!({
  |  _____________________________________^
2 | |     "foo": "bar"
3 | | });
  | |__^
  |
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0017]: references in constants may only refer to immutable values
 --> src/lib.rs:1:37
  |
1 |   pub const JSON: serde_json::Value = serde_json::json!({
  |  _____________________________________^
2 | |     "foo": "bar"
3 | | });
  | |__^ constants require immutable values
  |
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0015]: calls in constants are limited to constant functions, tuple structs and tuple variants
 --> src/lib.rs:1:37
  |
1 |   pub const JSON: serde_json::Value = serde_json::json!({
  |  _____________________________________^
2 | |     "foo": "bar"
3 | | });
  | |__^
  |
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0015]: calls in constants are limited to constant functions, tuple structs and tuple variants
 --> src/lib.rs:1:37
  |
1 |   pub const JSON: serde_json::Value = serde_json::json!({
  |  _____________________________________^
2 | |     "foo": "bar"
3 | | });
  | |__^
  |
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0015]: calls in constants are limited to constant functions, tuple structs and tuple variants
 --> src/lib.rs:1:37
  |
1 |   pub const JSON: serde_json::Value = serde_json::json!({
  |  _____________________________________^
2 | |     "foo": "bar"
3 | | });
  | |__^
  |
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0015]: calls in constants are limited to constant functions, tuple structs and tuple variants
 --> src/lib.rs:1:37
  |
1 |   pub const JSON: serde_json::Value = serde_json::json!({
  |  _____________________________________^
2 | |     "foo": "bar"
3 | | });
  | |__^
  |
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0493]: destructors cannot be evaluated at compile-time
 --> src/lib.rs:1:37
  |
1 |   pub const JSON: serde_json::Value = serde_json::json!({
  |  _____________________________________^
2 | |     "foo": "bar"
3 | | });
  | |__^ constants cannot evaluate destructors
  |
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

[ERROR rustc_mir::transform::qualify_consts] old validator: [(<::serde_json::macros::json_internal macros>:120:27: 120:51, "FnCallNonConst(DefId(15:468 ~ serde_json[1ca2]::map[0]::{{impl}}[0]::new[0]))"), (<::serde_json::macros::json_internal macros>:46:11: 121:26, "MutBorrow(Mut { allow_two_phase_borrow: true })"), (<::serde_json::macros::json_internal macros>:46:30: 46:53, "FnCallNonConst(DefId(2:2220 ~ core[f8ef]::convert[0]::Into[0]::into[0]))"), (<::serde_json::macros::json_internal macros>:123:27: 123:58, "FnCallNonConst(DefId(15:1765 ~ serde_json[1ca2]::value[0]::to_value[0]))"), (<::serde_json::macros::json_internal macros>:123:27: 123:70, "FnCallNonConst(DefId(2:5319 ~ core[f8ef]::result[0]::{{impl}}[5]::unwrap[0]))"), (<::serde_json::macros::json_internal macros>:46:11: 121:26, "FnCallNonConst(DefId(15:477 ~ serde_json[1ca2]::map[0]::{{impl}}[0]::insert[0]))"), (<::serde_json::macros::json_internal macros>:46:11: 121:26, "LiveDrop")]
[ERROR rustc_mir::transform::qualify_consts] new validator: [(<::serde_json::macros::json_internal macros>:120:27: 120:51, "FnCallNonConst(DefId(15:468 ~ serde_json[1ca2]::map[0]::{{impl}}[0]::new[0]))"), (<::serde_json::macros::json_internal macros>:46:11: 121:26, "MutBorrow(Mut { allow_two_phase_borrow: true })"), (<::serde_json::macros::json_internal macros>:46:30: 46:53, "FnCallNonConst(DefId(2:2220 ~ core[f8ef]::convert[0]::Into[0]::into[0]))"), (<::serde_json::macros::json_internal macros>:123:27: 123:58, "FnCallNonConst(DefId(15:1765 ~ serde_json[1ca2]::value[0]::to_value[0]))"), (<::serde_json::macros::json_internal macros>:123:27: 123:70, "FnCallNonConst(DefId(2:5319 ~ core[f8ef]::result[0]::{{impl}}[5]::unwrap[0]))"), (<::serde_json::macros::json_internal macros>:46:11: 121:26, "FnCallNonConst(DefId(15:477 ~ serde_json[1ca2]::map[0]::{{impl}}[0]::insert[0]))"), (<::serde_json::macros::json_internal macros>:46:11: 121:26, "LiveDrop"), (<::serde_json::macros::json_internal macros>:120:14: 120:24, "LiveDrop")]
error: internal compiler error: src/librustc_mir/transform/qualify_consts.rs:1038: Disagreement between legacy and dataflow-based const validators.
    After filing an issue, use `-Zsuppress-const-validation-back-compat-ice` to compile your code.
 --> src/lib.rs:1:1
  |
1 | / pub const JSON: serde_json::Value = serde_json::json!({
2 | |     "foo": "bar"
3 | | });
  | |___^

thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:876:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.40.0-nightly (1721c9685 2019-10-12) running on x86_64-unknown-linux-gnu

note: compiler flags: -C codegen-units=1 -C debuginfo=2 --crate-type lib

note: some of the compiler flags provided by cargo are hidden

Version: 1.40.0-nightly (2019-10-12 1721c96)
Playground: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=5558c5ebe1ea55f66650422dffcf6d88

Quite possibly a duplicate of #65348 just buried inside the macro expansion but I'm not familiar enough with Rust compiler internals to make that determination.

@Centril Centril added I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-typesystem Area: The type system labels Oct 14, 2019
@Centril
Copy link
Contributor

Centril commented Oct 14, 2019

cc @ecstatic-morse

@JohnTitor JohnTitor added the C-bug Category: This is a bug. label Oct 14, 2019
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Oct 14, 2019

I've minimized this to the following:

const _: Vec<i32> = {
    let x = Vec::<i32>::new();
    let r = &mut x;
    let y = x;
    y 
};

A mutable reference is required in all variants of this, so this particular mismatch does not represent a true backwards compatibility hazard: Creating a reference that would allow mutation is forbidden in all const contexts. We could simply stop triggering the mismatch ICE when both lists contain the same MutBorrow error.

The underlying issue, however, is that IndirectlyMutableLocals is being too conservative once again. As soon as a mutable reference is taken to x, we use type-based qualification instead of looking at the NeedsDrop qualif which gets cleared when x is moved from. Increasing the precision here would require a DefinitelyMovedFrom dataflow analysis. This would get checked first whenever NeedsDrop was queried (much like how we currently check IndirectlyMutableLocals first). I believe this will be necessary when we do allow &mut in const contexts, otherwise writing code would be quite painful.

I could hack together a solution for this particular case that doesn't require another dataflow analysis. Basically, during validation, we could keep track of which Locals have been moved out of in the current basic block and use that as a conservative approximation of DefinitelyMovedFrom. This would fail if the local was moved from in one basic block and then dropped in a successor of that block. It seems relatively hard to write rust code that gets translated to that MIR, however. For example, the following does not do it, but I think it should be possible.

struct NeedsDrop(i32);

impl NeedsDrop {
    const fn add_one(self)-> Self {
        NeedsDrop(self.0 + 1)
    }
}

impl Drop for NeedsDrop {
    fn drop(&mut self) {}
}

const _: NeedsDrop = {
    let x = NeedsDrop(0);
    let r = &mut x;
    let y = x.add_one();
    y
};

@Centril Centril removed the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Oct 14, 2019
@Centril
Copy link
Contributor

Centril commented Oct 14, 2019

cc @oli-obk @RalfJung @eddyb

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Oct 17, 2019

Checking for moves in the same block fails pretty often. I've implemented the first fix described above, but we probably want to use a MaybeInitializedLocals analysis in the long term.

@pnkfelix
Copy link
Member

triage: P-medium. Assigning to @ecstatic-morse . Removing nomination label.

@pnkfelix pnkfelix added P-medium Medium priority and removed I-nominated labels Oct 17, 2019
Centril added a commit to Centril/rust that referenced this issue Oct 19, 2019
…match-ugliness, r=eddyb

Suppress ICE when validators disagree on `LiveDrop`s in presence of `&mut`

Resolves rust-lang#65394.

This hack disables the validator mismatch ICE in cases where a `MutBorrow` error has been emitted by both validators, but they don't agree on the number of `LiveDrop` errors.

The new validator is more conservative about whether a value is moved from in the presence of mutable borrows. For example, the new validator will emit a `LiveDrop` error on the following code.

```rust
const _: Vec<i32> = {
    let mut x = Vec::new();
    let px = &mut x as *mut _;
    let y = x;
    unsafe { ptr::write(px, Vec::new()); }
    y
};
```

This code is not UB AFAIK (it passes MIRI at least). The current validator does not emit a `LiveDrop` error for `x` upon exit from the initializer. `x` is not actually dropped, so I think this is correct? A proper fix for this would require a new `MaybeInitializedLocals` dataflow analysis or maybe a relaxation of the existing `IndirectlyMutableLocals` one.

r? @RalfJung
Centril added a commit to Centril/rust that referenced this issue Oct 19, 2019
…match-ugliness, r=eddyb

Suppress ICE when validators disagree on `LiveDrop`s in presence of `&mut`

Resolves rust-lang#65394.

This hack disables the validator mismatch ICE in cases where a `MutBorrow` error has been emitted by both validators, but they don't agree on the number of `LiveDrop` errors.

The new validator is more conservative about whether a value is moved from in the presence of mutable borrows. For example, the new validator will emit a `LiveDrop` error on the following code.

```rust
const _: Vec<i32> = {
    let mut x = Vec::new();
    let px = &mut x as *mut _;
    let y = x;
    unsafe { ptr::write(px, Vec::new()); }
    y
};
```

This code is not UB AFAIK (it passes MIRI at least). The current validator does not emit a `LiveDrop` error for `x` upon exit from the initializer. `x` is not actually dropped, so I think this is correct? A proper fix for this would require a new `MaybeInitializedLocals` dataflow analysis or maybe a relaxation of the existing `IndirectlyMutableLocals` one.

r? @RalfJung
@bors bors closed this as completed in 27f8c79 Oct 19, 2019
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Sep 15, 2024
Stabilize `&mut` (and `*mut`) as well as `&Cell` (and `*const Cell`) in const

This stabilizes `const_mut_refs` and `const_refs_to_cell`. That allows a bunch of new things in const contexts:
- Mentioning `&mut` types
- Creating `&mut` and `*mut` values
- Creating `&T` and `*const T` values where `T` contains interior mutability
- Dereferencing `&mut` and `*mut` values (both for reads and writes)

The same rules as at runtime apply: mutating immutable data is UB. This includes mutation through pointers derived from shared references; the following is diagnosed with a hard error:
```rust
#[allow(invalid_reference_casting)]
const _: () = {
    let mut val = 15;
    let ptr = &val as *const i32 as *mut i32;
    unsafe { *ptr = 16; }
};
```

The main limitation that is enforced is that the final value of a const (or non-`mut` static) may not contain `&mut` values nor interior mutable `&` values. This is necessary because the memory those references point to becomes *read-only* when the constant is done computing, so (interior) mutable references to such memory would be pretty dangerous. We take a multi-layered approach here to ensuring no mutable references escape the initializer expression:
- A static analysis rejects (interior) mutable references when the referee looks like it may outlive the current MIR body.
- To be extra sure, this static check is complemented by a "safety net" of dynamic checks. ("Dynamic" in the sense of "running during/after const-evaluation, e.g. at runtime of this code" -- in contrast to "static" which works entirely by looking at the MIR without evaluating it.)
  - After the final value is computed, we do a type-driven traversal of the entire value, and if we find any `&mut` or interior-mutable `&` we error out.
  - However, the type-driven traversal cannot traverse `union` or raw pointers, so there is a second dynamic check where if the final value of the const contains any pointer that was not derived from a shared reference, we complain. This is currently a future-compat lint, but will become an ICE in rust-lang#128543. On the off-chance that it's actually possible to trigger this lint on stable, I'd prefer if we could make it an ICE before stabilizing const_mut_refs, but it's not a hard blocker. This part of the "safety net" is only active for mutable references since with shared references, it has false positives.

Altogether this should prevent people from leaking (interior) mutable references out of the const initializer.

While updating the tests I learned that surprisingly, this code gets rejected:
```rust
const _: Vec<i32> = {
    let mut x = Vec::<i32>::new(); //~ ERROR destructor of `Vec<i32>` cannot be evaluated at compile-time
    let r = &mut x;
    let y = x;
    y
};
```
The analysis that rejects destructors in `const` is very conservative when it sees an `&mut` being created to `x`, and then considers `x` to be always live. See [here](rust-lang#65394 (comment)) for a longer explanation. `const_precise_live_drops` will solve this, so I consider this problem to be tracked by rust-lang#73255.

Cc `@rust-lang/wg-const-eval` `@rust-lang/lang`
Cc rust-lang#57349
Cc rust-lang#80384
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 15, 2024
Rollup merge of rust-lang#129195 - RalfJung:const-mut-refs, r=fee1-dead

Stabilize `&mut` (and `*mut`) as well as `&Cell` (and `*const Cell`) in const

This stabilizes `const_mut_refs` and `const_refs_to_cell`. That allows a bunch of new things in const contexts:
- Mentioning `&mut` types
- Creating `&mut` and `*mut` values
- Creating `&T` and `*const T` values where `T` contains interior mutability
- Dereferencing `&mut` and `*mut` values (both for reads and writes)

The same rules as at runtime apply: mutating immutable data is UB. This includes mutation through pointers derived from shared references; the following is diagnosed with a hard error:
```rust
#[allow(invalid_reference_casting)]
const _: () = {
    let mut val = 15;
    let ptr = &val as *const i32 as *mut i32;
    unsafe { *ptr = 16; }
};
```

The main limitation that is enforced is that the final value of a const (or non-`mut` static) may not contain `&mut` values nor interior mutable `&` values. This is necessary because the memory those references point to becomes *read-only* when the constant is done computing, so (interior) mutable references to such memory would be pretty dangerous. We take a multi-layered approach here to ensuring no mutable references escape the initializer expression:
- A static analysis rejects (interior) mutable references when the referee looks like it may outlive the current MIR body.
- To be extra sure, this static check is complemented by a "safety net" of dynamic checks. ("Dynamic" in the sense of "running during/after const-evaluation, e.g. at runtime of this code" -- in contrast to "static" which works entirely by looking at the MIR without evaluating it.)
  - After the final value is computed, we do a type-driven traversal of the entire value, and if we find any `&mut` or interior-mutable `&` we error out.
  - However, the type-driven traversal cannot traverse `union` or raw pointers, so there is a second dynamic check where if the final value of the const contains any pointer that was not derived from a shared reference, we complain. This is currently a future-compat lint, but will become an ICE in rust-lang#128543. On the off-chance that it's actually possible to trigger this lint on stable, I'd prefer if we could make it an ICE before stabilizing const_mut_refs, but it's not a hard blocker. This part of the "safety net" is only active for mutable references since with shared references, it has false positives.

Altogether this should prevent people from leaking (interior) mutable references out of the const initializer.

While updating the tests I learned that surprisingly, this code gets rejected:
```rust
const _: Vec<i32> = {
    let mut x = Vec::<i32>::new(); //~ ERROR destructor of `Vec<i32>` cannot be evaluated at compile-time
    let r = &mut x;
    let y = x;
    y
};
```
The analysis that rejects destructors in `const` is very conservative when it sees an `&mut` being created to `x`, and then considers `x` to be always live. See [here](rust-lang#65394 (comment)) for a longer explanation. `const_precise_live_drops` will solve this, so I consider this problem to be tracked by rust-lang#73255.

Cc `@rust-lang/wg-const-eval` `@rust-lang/lang`
Cc rust-lang#57349
Cc rust-lang#80384
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-typesystem Area: The type system C-bug Category: This is a bug. glacier ICE tracked in rust-lang/glacier. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. 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.

6 participants