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

tracking issue for const_indexing stabilization #29947

Closed
oli-obk opened this issue Nov 20, 2015 · 31 comments · Fixed by #46882
Closed

tracking issue for const_indexing stabilization #29947

oli-obk opened this issue Nov 20, 2015 · 31 comments · Fixed by #46882
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Nov 20, 2015

The feature called const_indexing allows the indexing operation to be performed while calculating a true constant like an array length.

See the implementation at #25570

oli-obk added a commit to oli-obk/rust that referenced this issue Nov 20, 2015
@debris
Copy link
Contributor

debris commented Dec 3, 2015

I've updated rust nightly today and i've stumbled upon const_indexing issue in my lib.

const indexing is an unstable feature

The code was compiling fine on 1.4.0, beta and old nightly, so I assume it might be the issue with new feature itself, not my code. After quick investigation I worked around the issue in this commit: debris/tiny-keccak@591d611 . Hope it helps with stabilization!

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 3, 2015

oh... not good. I know what's going on. You are using the index operation on the rhs of a bitshift operation. So during check_const the rhs is const_evaled, which triggers this... Fixing this makes the code rather ugly. Basically I need to make sure that during compiler checks no feature gate errors are emitted but simply const eval fails...

@frewsxcv
Copy link
Member

frewsxcv commented Dec 5, 2015

A similar regression also happened here: ende76/brotli-rs#14

bors added a commit that referenced this issue Dec 7, 2015
@huonw huonw added B-unstable Blocker: Implemented in the nightly compiler and unstable. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jan 6, 2016
@nikomatsakis
Copy link
Contributor

@eddyb do you see any reason not to stabilize this? Seems like we're going to wind up with a model that supports this sort of thing, at least in some cases.

@eddyb
Copy link
Member

eddyb commented Nov 11, 2016

The actual addresses would be different... probably fine though, we already do the same thing in trans.
EDIT: ignore me, this only matters in trans. If qualify_consts passes then miri likely can eval it.
@oli-obk @solson Any objections regarding miri? Doubt it but just in case.

@solson
Copy link
Member

solson commented Nov 11, 2016

Indexing will force allocations in miri, unlike most things supported by legacy const eval, but it wouldn't be the only thing (e.g. I think *&42 is valid in consts today). It sounds fine.

@quadrupleslap
Copy link

Is this going to be stabilised soon? I tried it on stable, and it works without any issues, but it's feature-gated only on nightly. Why does it seem to work on stable?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 25, 2017

@quadrupleslap: that sounds buggy, can you give an example code exhibiting that behaviour?

@quadrupleslap
Copy link

Okay, my bad, I was using them in different ways. I think this behaviour is buggy, though:

const HELLO: &'static [u8; 5] = b"hello";
const H: u8 = HELLO[0];

fn main() {
    println!("'{:?}' starts with '{:?}'!", HELLO, H);
    
    // Comment the let and match out and everything works.
    // It looks like constants aren't guaranteed to be constant.
    let n = 3;
    match n {
        H => {
            println!("Why does it only error if it's used in a constant position?");
        },
        _ => ()
    }
    
    // I think if indexing in constants is unstable, rustc should error
    // even if the constant isn't used in a constant position.
}

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 25, 2017

That's just a symptom of our 3 separate const evaluator implementations. (HIR folding, trans MIR interpretation, pattern creation)

@quadrupleslap
Copy link

I... what? That's terrifying, but okay, I guess. Thanks for the read!

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 25, 2017

Don't worry, there's work being done to merge them into a single one.

@eddyb
Copy link
Member

eddyb commented Jan 25, 2017

Well, I think there's only two? But the HIR one can work both before type-checking and after (differently).

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jul 22, 2017
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 28, 2017

I would like to stabilize const indexing in the process of moving from old ctfe to miri. While the feature gate could be kept, it would require some work which would simply be useless if we're going to stabilize const indexing anyway.

@cramertj
Copy link
Member

FCP'ing since #46882 would stabilize.

@rfcbot fcp merge

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 17, 2018
@rfcbot
Copy link

rfcbot commented Jan 17, 2018

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

No concerns currently listed.

Once these reviewers reach consensus, 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.

@nikomatsakis
Copy link
Contributor

I'm going to use this feature as a kind of "proving ground" -- I feel frustrated that we often don't have a lot of documentaiton on precisely what is being stabilized and what the current behavior is. Can somebody -- perhaps @oli-obk -- write-up something that says:

  • What the new behavior is and what some of the interesting edge cases or limits are
  • What are some test cases that show the new behavior and those limits
    • these ought to be pointers to tests in the code base, so that we can be sure that behavior won't regress

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 18, 2018

Did you have something in mind like this?

New behaviour and edge cases

The new behaviour is that similar to accessing a tuple field, you can now access an array element. The only difference is that you can compute which element you want instead of having to write it out literally.

Similar to tuples, the array is computed entirely beforehand and the indexing only happens if all elements of the array are computed correctly.

Similar to tuples again, accessing an an out of bounds element will produce a compiler error. Unlike tuples, since the index depends on a computation, you can cause the indexing to fail depending on the platform. E.g. [1, 2, 3, 4, 5][std::size_of::<usize>()] will produce a compile-time error on 64 bit systems, but not on 32 bit systems. But that is nothing new, since you can already produce an error with 0xFFFFFFFF - std::usize::MAX

Emulating if with arrays

While you can emulate branching with arrays and bool casting, you won't gain much out of it, because all branches are evaluated, irrelevant of the condition. The following will produce an error unless X and Y are both 0, because either X - Y will overflow or Y - X will overflow (assuming unsigned types for X and Y)

[X - Y, Y - X][(X < Y) as usize]

So this differs from the assumed potential future support for if which would not error in

if X < Y { Y - X } else { X - Y }

Test cases

const ARR: [usize; 5] = [5, 4, 3, 2, 1];
fn main() {
assert_eq!(3, ARR[ARR[3]]);

const ARR: [i32; 6] = [42, 43, 44, 45, 46, 47];
const IDX: usize = 3;
const VAL: i32 = ARR[IDX];
const BLUB: [i32; (ARR[0] - 41) as usize] = [5];

const ARR: [i32; 6] = [42, 43, 44, 45, 46, 47];
const IDX: usize = 3;
const VAL: i32 = ARR[IDX];
const BONG: [i32; (ARR[0] - 41) as usize] = [5];
const BLUB: [i32; (ARR[0] - 40) as usize] = [5]; //~ ERROR: mismatched types
const BOO: [i32; (ARR[0] - 41) as usize] = [5, 99]; //~ ERROR: mismatched types

pub const E: u8 = [5u8][1];
//~^ ERROR index out of bounds: the len is 1 but the index is 1

const fn generic_arr<T: Copy>(t: [T; 1]) -> T {
t[0]
}

const FOO: [usize; 3] = [1, 2, 3];
const BAR: usize = FOO[5]; // no error, because the error below occurs before regular const eval
const BLUB: [u32; FOO[4]] = [5, 6];
//~^ ERROR constant evaluation error [E0080]
//~| index out of bounds: the len is 3 but the index is 4

let n = 1i64 >> [63][0];
let n = 1i64 >> [64][0]; //~ ERROR: bitshift exceeds the type's number of bits

bors added a commit that referenced this issue Jan 18, 2018
Replace all const evaluation with miri

* error reporting in constants prints a stacktrace through all called const fns
* Trivial constant propagation and folding in MIR (always active, irrelevant of the optimization level)
* can now use floating constants in patterns (previously only floating point literals were allowed)
    * the future compat lint is still produced for both cases
* can index into constant arrays during const eval (previously feature gated)
* can create a constant union value with field `a` and read from field `b`
* can dereference references into constants
* can create references inside constants (`const X: &u32 = &22`)
* Tuple struct constructors can be used in constants
* regression in const eval errors spans (some of these need improvements in mir debug info)
* can cast floats to ints and vice versa (in constants, and even nan/inf constants)
* Mir dump prints false/true instead of 0u8/1u8
* `1i8 >> [8][0]` does not lint about exceeding bitshifts anymore.
    * Needs const propagation across projections
* `foo[I]` produces a const eval lint if `foo: [T; N]` and `N < I`
    * Essentially all builtin panics produce lints if they can be statically proven to trigger at runtime. This is on a best effort basis, so there might be some complex cases that don't trigger. (The runtime panic stays there, irrelevant of whether the lint is produced or not)

fixes #34997 (stack overflow with many constants)
fixes #25574 (deref byte strings in patterns)
fixes #27918 (broken mir ICE)
fixes #46114 (ICE on struct constructors in patterns)
fixes #37448 (`SomeStruct { foo } as SomeStruct`)
fixes #43754 (`return` in const fn)
fixes #41898 (tuple struct constructors)
fixes #31364 (infinite recursion with const fn, fixed by miri's recursion limit)
closes #29947 (const indexing stabilization)

r? @eddyb
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 25, 2018

@nikomatsakis I added a test to the PR: 83e6d3c

@nikomatsakis
Copy link
Contributor

@oli-obk thanks!

@rfcbot reviewed

@rfcbot
Copy link

rfcbot commented Jan 26, 2018

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

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 26, 2018
@mrhota
Copy link
Contributor

mrhota commented Jan 28, 2018

@oli-obk @nikomatsakis in order to stabilize, don't we need at least an open PR on rust-lang-nursery/reference documenting the to-be-stabilized feature? The Unstable Book hardly even has anything about this.

@mrhota
Copy link
Contributor

mrhota commented Jan 28, 2018

you also need a PR on the unstable book removing the page about this feature

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 29, 2018

@mrhota: while discussion around the semantics were quite extensive, the feature itself is not more special than any of the mathematical operations. They all have error modes (e.g. overflow or index out of bounds), all input arguments are validated entirely before the result is computed, even if not all of them are needed (e.g. {5u8 - 6; 42} + 3 will error even though the 5u8 - 6 will never show up in the result.

Additionally, the reference already mentions array indexing in https://doc.rust-lang.org/nightly/reference/expressions.html#constant-expressions

@nikomatsakis
Copy link
Contributor

I can believe that the existing documentation is sufficient -- I've not looked closely, but I doubt that the precise borders of what constitutes a legal constant expression are too well-defined.

But at minimum removing the "unstable page" makes sense!

bors added a commit that referenced this issue Jan 30, 2018
Replace all const evaluation with miri

* error reporting in constants prints a stacktrace through all called const fns
* Trivial constant propagation and folding in MIR (always active, irrelevant of the optimization level)
* can now use floating constants in patterns (previously only floating point literals were allowed)
    * the future compat lint is still produced for both cases
* can index into constant arrays during const eval (previously feature gated)
* can create a constant union value with field `a` and read from field `b`
* can dereference references into constants
* can create references inside constants (`const X: &u32 = &22`)
* Tuple struct constructors can be used in constants
* regression in const eval errors spans (some of these need improvements in mir debug info)
* can cast floats to ints and vice versa (in constants, and even nan/inf constants)
* Mir dump prints false/true instead of 0u8/1u8
* `1i8 >> [8][0]` does not lint about exceeding bitshifts anymore.
    * Needs const propagation across projections
* `foo[I]` produces a const eval lint if `foo: [T; N]` and `N < I`
    * Essentially all builtin panics produce lints if they can be statically proven to trigger at runtime. This is on a best effort basis, so there might be some complex cases that don't trigger. (The runtime panic stays there, irrelevant of whether the lint is produced or not)

fixes #34997 (stack overflow with many constants)
fixes #25574 (deref byte strings in patterns)
fixes #27918 (broken mir ICE)
fixes #46114 (ICE on struct constructors in patterns)
fixes #37448 (`SomeStruct { foo } as SomeStruct`)
fixes #43754 (`return` in const fn)
fixes #41898 (tuple struct constructors)
fixes #31364 (infinite recursion with const fn, fixed by miri's recursion limit)
closes #29947 (const indexing stabilization)
fixes #45044 (pattern matching repeat expressions)

r? @eddyb
@rfcbot
Copy link

rfcbot commented Feb 5, 2018

The final comment period is now complete.

bors added a commit that referenced this issue Feb 6, 2018
Replace all const evaluation with miri

* error reporting in constants prints a stacktrace through all called const fns
* Trivial constant propagation and folding in MIR (always active, irrelevant of the optimization level)
* can now use floating constants in patterns (previously only floating point literals were allowed)
    * the future compat lint is still produced for both cases
* can index into constant arrays during const eval (previously feature gated)
* can create a constant union value with field `a` and read from field `b`
* can dereference references into constants
* can create references inside constants (`const X: &u32 = &22`)
* Tuple struct constructors can be used in constants
* regression in const eval errors spans (some of these need improvements in mir debug info)
* can cast floats to ints and vice versa (in constants, and even nan/inf constants)
* Mir dump prints false/true instead of 0u8/1u8
* `1i8 >> [8][0]` does not lint about exceeding bitshifts anymore.
    * Needs const propagation across projections
* `foo[I]` produces a const eval lint if `foo: [T; N]` and `N < I`
    * Essentially all builtin panics produce lints if they can be statically proven to trigger at runtime. This is on a best effort basis, so there might be some complex cases that don't trigger. (The runtime panic stays there, irrelevant of whether the lint is produced or not)
* can use `union`s to implement `transmute` for `Copy` types in constants without a feature gate. With all the greatness and nasal demons that come with this.

fixes #34997 (stack overflow with many constants)
fixes #25574 (deref byte strings in patterns)
fixes #27918 (broken mir ICE)
fixes #46114 (ICE on struct constructors in patterns)
fixes #37448 (`SomeStruct { foo } as SomeStruct`)
fixes #43754 (`return` in const fn)
fixes #41898 (tuple struct constructors)
fixes #31364 (infinite recursion with const fn, fixed by miri's recursion limit)
closes #29947 (const indexing stabilization)
fixes #45044 (pattern matching repeat expressions)
fixes #47971 (ICE on const fn + references)

r? @eddyb
@nikomatsakis
Copy link
Contributor

@oli-obk is there a PR for stabilizing this work? I guess landing miri would probably do it...?

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 6, 2018

Yes this is part of the miri pr

bors added a commit that referenced this issue Mar 7, 2018
Replace all const evaluation with miri

* error reporting in constants prints a stacktrace through all called const fns
* Trivial constant propagation and folding in MIR (always active, irrelevant of the optimization level)
* can now use floating constants in patterns (previously only floating point literals were allowed)
    * the future compat lint is still produced for both cases
* can index into constant arrays during const eval (previously feature gated)
* can create a constant union value with field `a` and read from field `b`
* can dereference references into constants
* can create references inside constants (`const X: &u32 = &22`)
* Tuple struct constructors can be used in constants
* regression in const eval errors spans (some of these need improvements in mir debug info)
* can cast floats to ints and vice versa (in constants, and even nan/inf constants)
* Mir dump prints false/true instead of 0u8/1u8
* `1i8 >> [8][0]` does not lint about exceeding bitshifts anymore.
    * Needs const propagation across projections
* `foo[I]` produces a const eval lint if `foo: [T; N]` and `N < I`
    * Essentially all builtin panics produce lints if they can be statically proven to trigger at runtime. This is on a best effort basis, so there might be some complex cases that don't trigger. (The runtime panic stays there, irrelevant of whether the lint is produced or not)
* can use `union`s to implement `transmute` for `Copy` types in constants without a feature gate. With all the greatness and nasal demons that come with this.
* can convert integers to `&'static T` in constants (useful for embedded)

fixes #34997 (stack overflow with many constants)
fixes #25574 (deref byte strings in patterns)
fixes #27918 (broken mir ICE)
fixes #46114 (ICE on struct constructors in patterns)
fixes #37448 (`SomeStruct { foo } as SomeStruct`)
fixes #43754 (`return` in const fn)
fixes #41898 (tuple struct constructors)
fixes #31364 (infinite recursion with const fn, fixed by miri's recursion limit)
closes #29947 (const indexing stabilization)
fixes #45044 (pattern matching repeat expressions)
fixes #47971 (ICE on const fn + references)
fixes #48081 (ICE on cyclic assoc const error)
fixes #48746 (nonhelpful error message with unions)

r? @eddyb

even though 1k loc are added in tests, this PR reduces the loc in this repository by 700
bors added a commit that referenced this issue Mar 8, 2018
Replace all const evaluation with miri

* error reporting in constants prints a stacktrace through all called const fns
* Trivial constant propagation and folding in MIR (always active, irrelevant of the optimization level)
* can now use floating constants in patterns (previously only floating point literals were allowed)
    * the future compat lint is still produced for both cases
* can index into constant arrays during const eval (previously feature gated)
* can create a constant union value with field `a` and read from field `b`
* can dereference references into constants
* can create references inside constants (`const X: &u32 = &22`)
* Tuple struct constructors can be used in constants
* regression in const eval errors spans (some of these need improvements in mir debug info)
* can cast floats to ints and vice versa (in constants, and even nan/inf constants)
* Mir dump prints false/true instead of 0u8/1u8
* `1i8 >> [8][0]` does not lint about exceeding bitshifts anymore.
    * Needs const propagation across projections
* `foo[I]` produces a const eval lint if `foo: [T; N]` and `N < I`
    * Essentially all builtin panics produce lints if they can be statically proven to trigger at runtime. This is on a best effort basis, so there might be some complex cases that don't trigger. (The runtime panic stays there, irrelevant of whether the lint is produced or not)
* can use `union`s to implement `transmute` for `Copy` types in constants without a feature gate. With all the greatness and nasal demons that come with this.
* can convert integers to `&'static T` in constants (useful for embedded)

fixes #34997 (stack overflow with many constants)
fixes #25574 (deref byte strings in patterns)
fixes #27918 (broken mir ICE)
fixes #46114 (ICE on struct constructors in patterns)
fixes #37448 (`SomeStruct { foo } as SomeStruct`)
fixes #43754 (`return` in const fn)
fixes #41898 (tuple struct constructors)
fixes #31364 (infinite recursion with const fn, fixed by miri's recursion limit)
closes #29947 (const indexing stabilization)
fixes #45044 (pattern matching repeat expressions)
fixes #47971 (ICE on const fn + references)
fixes #48081 (ICE on cyclic assoc const error)
fixes #48746 (nonhelpful error message with unions)

r? @eddyb

even though 1k loc are added in tests, this PR reduces the loc in this repository by 700
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.