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

Allow `loop` in constant evaluation #2344

Merged
merged 5 commits into from Jul 2, 2018

Conversation

@oli-obk
Copy link
Contributor

commented Feb 22, 2018

@Centril Centril added the T-lang label Feb 22, 2018

@Centril
Copy link
Member

left a comment

All for this change 👍
I have some mostly minor, hopefully helpful, thoughts =)


Allow the use of `loop`, `while` and `while let` during constant evaluation.
`for` loops are technically allowed, too, but can't be used in practice because
each iteration calls `iterator.next()`, which is not a `const fn` and thus can't

This comment has been minimized.

Copy link
@Centril

Centril Feb 22, 2018

Member

Linking #2237 which (or a modified version of which) will fix that and let you use iter.next() in const fn.

# Motivation
[motivation]: #motivation

Any iteration is expressible as a recursion. Since we already allow recursion

This comment has been minimized.

Copy link
@Centril

Centril Feb 22, 2018

Member

Small possible improvement: s/a recursion/with recursion.


Any iteration is expressible as a recursion. Since we already allow recursion
via const fn and termination of said recursion via `if` or `match`, all code
enabled by const recursion is already legal now. Writing loops with recursion is

This comment has been minimized.

Copy link
@Centril

Centril Feb 22, 2018

Member

Does this hold given recursion depth limits?

This comment has been minimized.

Copy link
@oli-obk

oli-obk Feb 22, 2018

Author Contributor

yea, if the loop takes too many iterations you'll hit the limit, but that's not much of an issue imo, since you can always work around that with unrolling or similar tricks.

This comment has been minimized.

Copy link
@Centril

Centril Feb 22, 2018

Member

Ah, thanks for explaining =)

Still... might it be good to allow users to raise the limit from the default so that it is not so fixed and so that you can fix cases wherein you know that termination is guaranteed but rustc complains nonetheless?

via const fn and termination of said recursion via `if` or `match`, all code
enabled by const recursion is already legal now. Writing loops with recursion is
very tedious and can quickly become unreadable, while regular loops are much
more natural in Rust.

This comment has been minimized.

Copy link
@Centril

Centril Feb 22, 2018

Member

This feels like a somewhat bold assertion that seems to suggest that functional languages are less readable, while I'd argue that the opposite is the case, recursion is often more readable than loops - and iteration combinators using map, filter, fold even more so (tho combinators have nothing to do with recursion).

I agree that regular loops are more used in Rust, but I believe this has more to do with the lack of guaranteed TCO (Tail Call Optimization, https://en.wikipedia.org/wiki/Tail_call, https://stackoverflow.com/questions/310974/what-is-tail-call-optimization) which RFC #1888 attempts to address.

I don't think any of these assertions are necessary to motivate loops in const fn - feature parity alone and the feeling that the language is uniform when you switch from fn -> const fn should be enough.

[guide-level-explanation]: #guide-level-explanation

If you previously had to write functional code inside constants, you can now
change it to imperative code. For example if you wrote a fibonacci like

This comment has been minimized.

Copy link
@Centril

Centril Feb 22, 2018

Member

Who doesn't love a fibonacci example =P This section feels very nicely written!
Stylistically I'ld do: s/like/like: (and before the other sections as well..)

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

A loop in MIR is a cyclic graph of BasicBlocks. Evaluating such a loop is no

This comment has been minimized.

Copy link
@Centril

Centril Feb 22, 2018

Member

s/BasicBlocks/BasicBlocks

different from evaluating a linear sequence of BasicBlocks, except that
termination is not guaranteed. To ensure that the compiler never hangs
indefinitely, we count the number of terminators processed and once we reach a
fixed limit, we report an error mentioning that we aborted constant evaluation,

This comment has been minimized.

Copy link
@Centril

Centril Feb 22, 2018

Member

Perhaps a word about if and how you can control that fixed limit? (I assume the #[recursion_limit = "<limit>"] attribute?)

This comment has been minimized.

Copy link
@oli-obk

oli-obk Feb 22, 2018

Author Contributor

there's no such attribute

This comment has been minimized.

Copy link
@oli-obk

oli-obk Feb 22, 2018

Author Contributor

That's a different recursion. Recursing in const eval does not actually cause recursion in the compiler, just more elements in the virtual stack Vec.

# Drawbacks
[drawbacks]: #drawbacks

* Loops are not guaranteed to terminate

This comment has been minimized.

Copy link
@Centril

Centril Feb 22, 2018

Member

Nicely written.

In the far future it might be interesting to have a total effect in the language with a totality checker that guarantees termination.

This comment has been minimized.

Copy link
@joshtriplett

joshtriplett Feb 23, 2018

Member

Do we have a mechanism to adjust the amount of compile-time evaluation allowed, similar to the macro limits?

This comment has been minimized.

Copy link
@oli-obk

oli-obk Feb 23, 2018

Author Contributor

Nope, but it's trivial to add (a single line next to the recursion limit attr code)

@scottmcm

This comment has been minimized.

Copy link
Member

commented Feb 24, 2018

Just counting all terminators worries me slightly that a tweak to MIR optimizations or HIR->MIR lowering will result in code that happened to be right on the edge breaking in an update.

Is there some tweak to the rule that's more resilient to otherwise-invisible changes, and where it's easier to understand by looking at your code how many of whatever it's counting you're taking? Like maybe only counting backedges taken. (Though even that changes for loop unrolling, I guess.)

Come to think of it, I wonder if the recursion limit means that the MIR inliner should add a new marker statement to the MIR so that heuristic changes don't change how many times a function recurses...

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2018

Well.... the current limit is riddiculously high. 1 million terminators take a while to evaluate.

We could also hash the entire state at every terminator and detect true loops this way. We could even only do that once we reach the limit so we don't slow down the regular evaluations.

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2018

We could emit a warning for long evaluations so ppl know rustc isn't hanging, just busy

@burdges

This comment has been minimized.

Copy link

commented Feb 24, 2018

Also test builds could report how much time was spent in rustc itself vs const evaluation, so slow crates get blamed quickly.

@Centril

This comment has been minimized.

Copy link
Member

commented Feb 24, 2018

Well.... the current limit is riddiculously high. 1 million terminators take a while to evaluate.

Do you have some guess as to how much a while might be? Just so we're on the same page =)

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2018

While that depends on the code you're executing, experience with when the limit was on statements was several minutes

Edit: with the hash checker we could set it to seconds so ppl get feedback fast

@nagisa

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2018

We should not have any error on constant evaluation by default. Not time and not terminator count based. Instead, there should exist a lint of-sorts that would warn every so often that such-and-such constant item is still being evaluated, and that’s it. People who want to make sure stuff doesn’t get out of hand can then #[deny] it and people who know they gonna do heavy compile-time computations can #[allow] it so the compiler doesn’t nag at them too much.

So basically what this says.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Feb 24, 2018

@nagisa I'd be concerned about people running automated builds and finding that they take forever, in that case. CI, for instance. Having an overridable stall detector on by default seems like a feature.

@nagisa

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2018

@joshtriplett Sure, hence #[deny]. Of course we still would need to provide a way to configure actual limit. Most importantly, with this approach breaking code which uses #[deny(long_const_eval)] would be about as breaking as breaking code which specifies #[deny(warnings)] (we do not consider it to be a breaking change).

@clarfon

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2018

Considering how Rust's type system is turing complete, infinite compile loops are already possible. This just makes them a bit easier.

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2018

To the best of my knowledge you can't do infinite loops in the type system without hitting the recursion limit. True recursions would trigger an error of the form "needed X to compute X"

@clarfon

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2018

That makes sense. Perhaps the an iteration limit could be equivalent to the recursion limit?

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2018

They are different, because now there can be repetitive recursions. I think going with the warning and no limit is the way to go. I'll change the RFC accordingly

bors added a commit to rust-lang/rust that referenced this pull request Apr 15, 2018

Auto merge of #49947 - oli-obk:turing_complete_const_eval, r=nagisa
Don't abort const eval due to long running evals, just warn

one check-box of #49930

r? @nagisa (rust-lang/rfcs#2344 (comment))
@oli-obk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

Long running const eval doesn't cause errors anymore: rust-lang/rust#49980

# Drawbacks
[drawbacks]: #drawbacks

* Infinite loops will hang the compiler if the lint is not denied

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix May 14, 2018

Member

Nit: Even just a #[warn] on the lint will cause warning output to occur (and repeat ad nauseaum) in response to an infinite loop, right? Usually when I hear "hang" I think of "no output visible"... so I would personally have written "if the lint is #[allow]'ed" here. But that might just be my own personal interpretation.

indefinitely, we count the number of terminators processed and whenever we reach
a fixed limit, we report a lint mentioning that we cannot guarantee that the
evaluation will terminate and reset the counter to zero. This lint should recur
in a non-annoying amount of time (e.g. at least 30 seconds between occurrences).

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix May 14, 2018

Member

So just to be clear: the const-eval state will include both a terminator-counter and a timestamp; When the terminator-counter both 1. exceeds the (here unspecified) fixed limit, and 2. the current time delta from the timestamp is non-annoying, then we report the lint, reset the terminator-counter and timestamp, and finally resume const-eval execution?

(I'm just checking that there's not a typo here and that we are planning on using both pieces of state for the filter here, something of which I am in favor.)

This comment has been minimized.

Copy link
@oli-obk

oli-obk May 14, 2018

Author Contributor

The current implementation just has a MIR terminator counter for the warning. There's no host time check. Shouldn't be too hard to add.

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix May 14, 2018

Member

I just want to clarify the end-intent. The current implementation isn't really something that concerns me all that much. :)

@pnkfelix

This comment has been minimized.

Copy link
Member

commented May 14, 2018

@rfcbot fcp merge

(I usually construct a comment thread summary before issuing the rfcbot command, but in this case I think all of the relevant comments were addressed, usually via incorporation into the RFC text, so I'm able to just recommend that people read the RFC.)

@rfcbot

This comment has been minimized.

Copy link

commented May 14, 2018

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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

This comment has been minimized.

Copy link

commented Jun 22, 2018

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

@rfcbot

This comment has been minimized.

Copy link

commented Jul 2, 2018

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

@Centril Centril merged commit c794c75 into rust-lang:master Jul 2, 2018

@Centril

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

Huzzah! This RFC is now merged!

Tracking issue: rust-lang/rust#52000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.