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 RFC 2344, "Allow loop in constant evaluation" #52000

Closed
1 of 3 tasks
Centril opened this issue Jul 2, 2018 · 44 comments
Closed
1 of 3 tasks

Tracking issue for RFC 2344, "Allow loop in constant evaluation" #52000

Centril opened this issue Jul 2, 2018 · 44 comments
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Jul 2, 2018

This is a tracking issue for the RFC "Allow loop in constant evaluation" (rust-lang/rfcs#2344).

Steps:

Unresolved questions:

  • Should we add a true recursion check that hashes the interpreter state and detects if it has reached the same state again?
    • This will slow down const evaluation enormously and for complex iterations is essentially useless because it'll take forever (e.g. counting from 0 to u64::max_value())
@Centril Centril added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Jul 2, 2018
@oli-obk oli-obk added the A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) label Jul 2, 2018
@brunocodutra
Copy link
Contributor

brunocodutra commented Jul 21, 2018

This feature seems simple enough for me to tackle as my first attempt to contribute to rust, at least getting it to work appears to be as simple as removing the check against loops in constant functions.

I believe the real work actually goes into implementing the lint for potential infinite loops. I'm going to tackle this next and I'd really appreciate some mentorship along the way.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 21, 2018

Implementing this feature is blocked on a bunch of definitely not beginner level things. But there's ongoing preliminary work happening, so this isn't standing still even if nothing happens on the issue.

If we'd just remove the check against loops, we'd end up with amusingly broken code, because most of the const checking code only knows it needs to prevent writing multiple times to a variable... which loops can do without naming the variable twice.

Anyway, guarding against infinite loops has been partially implemented, and if you're interested in doing some work there, we're tracking one of the issues in #52475

@brunocodutra
Copy link
Contributor

Thanks for pointing me to the right direction, I'll definitely have a look for something I can help with.

@jkarns275
Copy link

Would an iteration limit be an acceptable solution to the the infinite loop problem? If the iteration limit is actually reached the code can just be compiled instead!

Though this may be deceptive since the const fns wouldn't really be const if the limit is reached.

@estebank
Copy link
Contributor

Though this may be deceptive since the const fns wouldn't really be const if the limit is reached.

It could be a compilation error, which would let the const fn continue to be const (code that fails to compile has no bearing in execution time ^_^).

@jkarns275
Copy link

jkarns275 commented Aug 23, 2018

True! Perhaps it would be worth adding a compilation option of some sort that would make it just a warning for edge cases.
Edit: reading this half a year later and i have no idea what i meant

@Centril Centril added A-const-fn A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) and removed A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) labels Dec 31, 2018
@BatmanAoD
Copy link
Member

@oli-obk

const checking code only knows it needs to prevent writing multiple times to a variable...

....sorry, I thought that compile-time evaluated code is run by MIRI; why would it matter whether a variable is written to multiple times?

Of course, any (useful) compile-time evaluation is ultimately going to result in static data in the resulting executable, so it makes sense for const values not to be written to multiple times. But shouldn't that already be guaranteed by mutability rules?

@BatmanAoD
Copy link
Member

Additionally, if the Iter trait were to be stabilized for use in const fn contexts, I think an entirely reasonable resolution to this issue would be to enable only loops that rely on Iter. This would permit many simple obviously-terminating loops (the most obvious example being those iterating over range-literals), while limiting the potential for unbounded compiler execution to problems in the implementation of the iterator used.

An even simpler and forward-compatible solution would be to only permit iteration over range-literals for now.

@BatmanAoD
Copy link
Member

^ Sorry, I thought this was an RFC PR, not a tracking-issue PR. Since this RFC explicitly states that for loops wouldn't be usable anyway, would it be worth writing a separate RFC about permitting for <var> in <range> in const fn?

@scottmcm
Copy link
Member

@BatmanAoD My assumption here is that everyone wants to be able to use traits in const fn eventually, and that for will just naturally happen along with that.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 24, 2019

sorry, I thought that compile-time evaluated code is run by MIRI; why would it matter whether a variable is written to multiple times?

The const evaluator based on the miri-engine is what actually interprets the MIR and produces the final constant. We have an additional analysis that checks whether your constant only does things we can statically prove to not cause a certain range of errors. E.g. we currently forbid unions and transmuting via this static analysis, even though miri can evaluate them just fine. The same goes for calling trait methods. We want the const evaluator to only evaluate things that we have RFCed. The same thing is true for this issue. It's trivial for miri to evaluate loops, but we don't want to accidentally cause Cell types to end up in static variables, and the current static analyis just doesn't know how to prevent that in the presence of loops.

@BatmanAoD
Copy link
Member

@oli-obk Okay, I think I understand prohibiting transmuting and unions, since those enable operations that have effects that are invisible to the type system. I don't understand how loops would permit a Cell to be part of a static value, though; wouldn't that be part of the static value's type?

@BatmanAoD
Copy link
Member

(...or, instead of answering that specific question, feel free to point me to a more general resource I can read in order to ask more intelligent questions...)

@oli-obk
Copy link
Contributor

oli-obk commented Jan 25, 2019

feel free to point me to a more general resource I can read in order to ask more intelligent questions

I think we're severely lacking in that place. The closest I can come up with is this issue

Maybe we can kill two birds with one stone: I explain it to you, and you write docs for https://github.com/rust-lang-nursery/reference/blob/master/src/items/static-items.md, https://github.com/rust-lang-nursery/reference/blob/master/src/items/constant-items.md and https://github.com/rust-lang-nursery/reference/blob/master/src/items/associated-items.md#associated-constants

I'll review the docs, and this way we'll notice if I explained it correctly.

There's three components and we'll start with the Cell one:

static FOO: Option<Cell<u32>> = Some(Cell::new(42));

Is not legal, because we'd be able to modify the static without thread synchronization. But on the other hand

static FOO: Option<Cell<u32>> = None;

is perfectly ok, because no Cell actually exists. The same goes for

static FOO: Option<Cell<u32>> = (None, Cell::new(42)).0;

because no Cell ends up in the final constant.

Bringing us to the second point: destructors may not be called at compile-time, because Drop::drop is not a const function. This means that

const FOO: u32 = (SomeDropType, 42).1;

is not legal, because the destructor gets called during the evaluation of the constant. On the other hand

const FOO: SomeDropType = SomeDropType;

is legal, because no destructor gets called during the evaluation of the constant. Any use of it might cause a destructor to be called, but that's not a problem here.

Now we get to the third problem:

trait Bar {
    const BAR: Self;
}
trait Foo<T: Bar> {
    const FOO: u32 = (T::BAR, 42).1;
}

Is problematic exactly if T implements Drop (either manually or due to a field impl-ing Drop), because then the evaluation of FOO would call T::drop. While we could emit an error only when using trait Foo<String> or similar, that is a post-monomorphization error, which we are trying to avoid as much as possible. Post-monomorphization errors are bad, because they mean you can write broken code, and only the users of your crate will notice, not you yourself.

@RalfJung
Copy link
Member

There's also some material at https://github.com/rust-rfcs/const-eval/

@BatmanAoD
Copy link
Member

@oli-obk I would definitely be up for writing some docs!

I'm still not clear on whether there's actually a problem with Cell, though. In current, stable Rust (and, as it happens, on Nightly), the static _: Option<Cell<_>> = None declaration you've shown is not legal: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6e3f727044df4a298ab5681ba9a1a425

...so, is there a compelling reason why we would want that to be legal, even as const fn support grows?

For the second/third problem, I don't understand how const values that don't implement Copy can be used by-value at all, but clearly that is already legal in stable Rust, and in fact such a value can even be re-used multiple times: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7a580a901449868e7b0ab9434a492a8d

In any case, when compiling a trait definition, pre-monomorphization, can the compiler determine where drop calls would be inserted? If so, perhaps const fn simply shouldn't be able to use any types that aren't copy except by reference? (Not knowing hardly anything about the compiler internals: does this sound like an overly complex check?)

@mark-i-m
Copy link
Member

For the second/third problem, I don't understand how const values that don't implement Copy can be used by-value at all, but clearly that is already legal in stable Rust, and in fact such a value can even be re-used multiple times

const values are inlined at compile-time. There is no copying going on at runtime. The compiled binary contains an instance of constant for every usage.

@nikomatsakis
Copy link
Contributor

@ecstatic-morse can you update the stabilization report to not say that it is stabilizing the "instruction limit"? I think that was our consensus, and then I can kick off the FCP request.

@ecstatic-morse
Copy link
Contributor

@nikomatsakis Done.

@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

I would like to propose stabilization of permitting loops in constant evaluation, as described by the stabilization report prepared here.

@rfcbot
Copy link

rfcbot commented Jun 9, 2020

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 9, 2020
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jun 15, 2020
@rfcbot
Copy link

rfcbot commented Jun 15, 2020

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jun 15, 2020
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 25, 2020
@rfcbot
Copy link

rfcbot commented Jun 25, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

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
@jonas-schievink
Copy link
Contributor

Stabilized in #72437 🎉

foresterre added a commit to foresterre/cargo-msrv that referenced this issue Oct 6, 2021
By ignoring the lockfile, it will be regenerated for each run. The idea behind it is that we can have some compatibility for different lock file versions. However, even with a lockfile, a dependency version may be updated to a (hopefully semver compatible) version which falls within the semver requirements. If some dependency then introduces a change which breaks our MSRV, while Cargo pulls in a specified, newer, matching semantic version, we may still fail.

As an example: if we have an dependency A, with published versions 0.1 and 0.2, and our in-repo lockfile takes 0.1 while a newly generated lockfile may take 0.2 instead, and 0.2 has a higher MSRV than 0.1, then by removing the lockfile we our MSRV changes, which is a problem for MSRV verification.

In this specific case, on Rust toolchain versions below 1.46 (our MSRV is 1.42), we get the following error:

```
error[E0658]: `while` is not allowed in a `const fn`
  --> /user/.cargo/registry/src/github.com-1ecc6299db9ec823/http-0.2.5/src/header/value.rs:85:9
   |
85 | /         while i < bytes.len() {
86 | |             if !is_visible_ascii(bytes[i]) {
87 | |                 ([] as [u8; 0])[0]; // Invalid header value
88 | |             }
89 | |             i += 1;
90 | |         }
   | |_________^
   |
   = note: for more information, see rust-lang/rust#52000

```

For the `--ignore-lockfile` option itself, we'll need to figure out a strategy where we can convert between lockfile version, while keeping the versions in the Cargo.lock file the same. While in an ideal world, this error could have been prevented proper semver specifications, in the real world, such issues happen, and cargo-msrv should not overly rely on down-tree dependency specifications.
foresterre added a commit to foresterre/cargo-msrv that referenced this issue Aug 1, 2022
By ignoring the lockfile, it will be regenerated for each run. The idea behind it is that we can have some compatibility for different lock file versions. However, even with a lockfile, a dependency version may be updated to a (hopefully semver compatible) version which falls within the semver requirements. If some dependency then introduces a change which breaks our MSRV, while Cargo pulls in a specified, newer, matching semantic version, we may still fail.

As an example: if we have an dependency A, with published versions 0.1 and 0.2, and our in-repo lockfile takes 0.1 while a newly generated lockfile may take 0.2 instead, and 0.2 has a higher MSRV than 0.1, then by removing the lockfile we our MSRV changes, which is a problem for MSRV verification.

In this specific case, on Rust toolchain versions below 1.46 (our MSRV is 1.42), we get the following error:

```
error[E0658]: `while` is not allowed in a `const fn`
  --> /user/.cargo/registry/src/github.com-1ecc6299db9ec823/http-0.2.5/src/header/value.rs:85:9
   |
85 | /         while i < bytes.len() {
86 | |             if !is_visible_ascii(bytes[i]) {
87 | |                 ([] as [u8; 0])[0]; // Invalid header value
88 | |             }
89 | |             i += 1;
90 | |         }
   | |_________^
   |
   = note: for more information, see rust-lang/rust#52000

```

For the `--ignore-lockfile` option itself, we'll need to figure out a strategy where we can convert between lockfile version, while keeping the versions in the Cargo.lock file the same. While in an ideal world, this error could have been prevented proper semver specifications, in the real world, such issues happen, and cargo-msrv should not overly rely on down-tree dependency specifications.
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, ...) B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests