rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.63k stars 12.48k forks source link

tracking issue for `const_indexing` stabilization #29947

Closed oli-obk closed 6 years ago

oli-obk commented 8 years ago

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

debris commented 8 years ago

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: https://github.com/debris/tiny-keccak/commit/591d611c3e470d35f1f719a18aae17720c5ef690 . Hope it helps with stabilization!

oli-obk commented 8 years ago

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 commented 8 years ago

A similar regression also happened here: https://github.com/ende76/brotli-rs/issues/14

nikomatsakis commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

quadrupleslap commented 7 years ago

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 commented 7 years ago

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

quadrupleslap commented 7 years ago

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

oli-obk commented 7 years ago

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

eddyb commented 7 years ago

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

oli-obk commented 6 years ago

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 commented 6 years ago

FCP'ing since https://github.com/rust-lang/rust/pull/46882 would stabilize.

@rfcbot fcp merge

rfcbot commented 6 years ago

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 commented 6 years ago

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:

oli-obk commented 6 years ago

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

https://github.com/rust-lang/rust/blob/b1363a73ede57ae595f3a1be2bb75d308ba4f7f6/src/test/run-pass/issue-29914.rs#L12-L15

https://github.com/rust-lang/rust/blob/b1363a73ede57ae595f3a1be2bb75d308ba4f7f6/src/test/run-pass/array_const_index-1.rs#L14-L17

https://github.com/rust-lang/rust/blob/b1363a73ede57ae595f3a1be2bb75d308ba4f7f6/src/test/compile-fail/const-array-oob-arith.rs#L13-L18

https://github.com/rust-lang/rust/blob/b1363a73ede57ae595f3a1be2bb75d308ba4f7f6/src/test/compile-fail/const-err-early.rs#L18-L19

https://github.com/rust-lang/rust/blob/b1363a73ede57ae595f3a1be2bb75d308ba4f7f6/src/test/run-pass/const-fn.rs#L31-L33

https://github.com/rust-lang/rust/blob/e1cb9ba221e5cb0070ac82c6a234af11e4240680/src/test/compile-fail/const-array-oob.rs#L15-L20

https://github.com/rust-lang/rust/blob/b1363a73ede57ae595f3a1be2bb75d308ba4f7f6/src/test/compile-fail/lint-exceeding-bitshifts.rs#L72-L73

nikomatsakis commented 6 years ago

@oli-obk yep! thanks =)

nikomatsakis commented 6 years ago

@oli-obk question, looking at the existing tests, do we have one that tests this scenario that you described? I didn't immediately see it, but maybe I missed it.

[X - Y, Y - X][(X < Y) as usize]
oli-obk commented 6 years ago

I don't think so. I'll add one.

oli-obk commented 6 years ago

@nikomatsakis I added a test to the PR: https://github.com/rust-lang/rust/pull/46882/commits/83e6d3c65a7690ececc053027de10d6cfdc7dbbd

nikomatsakis commented 6 years ago

@oli-obk thanks!

@rfcbot reviewed

rfcbot commented 6 years ago

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

mrhota commented 6 years ago

@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 commented 6 years ago

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

oli-obk commented 6 years ago

@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 commented 6 years ago

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!

rfcbot commented 6 years ago

The final comment period is now complete.

nikomatsakis commented 6 years ago

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

oli-obk commented 6 years ago

Yes this is part of the miri pr