Replace const eval limit by a lint and add an exponential backoff warning

[oli-obk]

The lint triggers at the first power of 2 that comes after 1 million function calls or traversed back-edges (takes less than a second on usual programs). After the first emission, an unsilenceable warning is repeated at every following power of 2 terminators, causing it to get reported less and less the longer the evaluation runs.

cc @rust-lang/wg-const-eval

fixes #93481
closes #67217

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @fee1-dead (or someone else) soon.

Please see the contribution instructions for more information.

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[fee1-dead]

Welcome to Rust, oli! :)

[oli-obk]

Nominating for T-lang discussion. Are y'all on board with having no limit on the amount of time/steps constant evaluation can take, and instead just linting on long running evaluations to keep the users aware that the compiler is still busy doing things? It does mean const FOO: () = loop {}; will just make the the compiler never finish (though emit a warning every now and then).

If the lint is denyd, the evaluation stops at the first time the warning is hit. We could also make the lint deny by default, to require users to opt-in to long running const eval on a per-crate, module or even constant level via the regular lint mechanics.

Note that if a dependency has an infinite loop, this lint will not be shown by cargo and the dependency will just be locked forever if it allowed/warned the lint.

[inquisitivecrystal]

Nominating for T-lang discussion. Are y'all on board with having no limit on the amount of time/steps constant evaluation can take, and instead just linting on long running evaluations to keep the users aware that the compiler is still busy doing things? It does mean const FOO: () = loop {}; will just make the the compiler never finish (though emit a warning every now and then).

Why don't we just write a program to figure out if constant evaluation will eventually halt? It can't be that hard (this is a joke).

A more serious comment: I'd find it a bit surprising as a user if the compiler kept on going forever. Would it be possible to set some sort of limit that's so ridiculously high we'd expect it to never be hit in a valid program (I'm thinking like, the equivalent of 30 seconds or one minute of execution time, but you could even go longer)? I know it's not as conceptually nice as this approach, but I don't think most people are prepared to wait ten minutes for the compiler to stop running. At least that would avoid CI continuing until it timed out, for instance.

If we want to avoid hard limits, this could be a deny by default lint, but one with a higher threshold than the warn by default lint. I guess the point I'm gesturing at here is that the point at which we want to start warning people and the point at which we want to error out might not be the same: they might actually be quite far apart.

[Nilstrieb]

Suggested change

	if new_steps > 1_000_000 && new_steps.count_ones() == 1 {
	if new_steps > 1_000_000 && new_steps.is_power_of_two() {

That should be just as efficient and a little nicer

[RalfJung]

FWIW the -Zmiri-report-progress flag also uses 1_000_000 as default value for how many blocks to wait before reporting progress. So 👍 on that default value.

However the next power of two will be pretty close, won't it? So the second report comes rather quick after the first...
EDIT: Oh it doesn't report at 1 million, only at the next power of 2 after 1 million. Makes sense, but doesn't match the PR description. :)

[RalfJung]

A more serious comment: I'd find it a bit surprising as a user if the compiler kept on going forever. Would it be possible to set some sort of limit that's so ridiculously high we'd expect it to never be hit in a valid program (I'm thinking like, the equivalent of 30 seconds or one minute of execution time, but you could even go longer)? I know it's not as conceptually nice as this approach, but I don't think most people are prepared to wait ten minutes for the compiler to stop running. At least that would avoid CI continuing until it timed out, for instance.

If we set it to 30_000_000, which might be around 30s, I guarantee someone will report a bug "but I need just 2 million steps beyond that".^^ So then we need to still let users increase the limit. And worse someone might be using 29_999_999 steps and then MIR building changes and that puts their code above the threshold -- making the MIR building change a breaking change which is quite bad. That's exactly what just happened.

[marmeladema]

Should that limit be user configurable then?

[est31]

Some analysis, through experimentation I found that the change that caused #103814 decreased the highest compiling const eval limit from 166_665 to 124_999. This corresponds to a 33% change in costs to the user on this particular benchmark (comparing nightlies 4b8f43199 2022-10-19 with 11ebe6512 2022-11-01).

I think not having a limit, even if that limit is not perfect, removes control from users in a bad fashion. With this PR, if you allow the lint, because you do a lot of computation, you lose the protections from infinite loops that rustc gives you: It's an either-or descision without much flexibility. Furthermore, the limit is still there, it's just hardcoded, so you still can write code that skirts around the limit (like in the instance of #103814) and then it issues warnings down the line.

I'm also not sure about the UX impact of receiving 4 warnings for a const evaluation that runs for 10 seconds.

My proposal:

• Have two lints instead of exponential backoff, one which is warn by default (long_running_const_eval), and a second that is deny by default (very_long_running_const_eval).
• The second lint would trigger after 4 times the limit of the first lint was hit. That gives enough space for breaking changes to be noticed before they show up as errors. The second lint would exist so that CI jobs terminate. Personally I never really look at still-running CI, most times it's because of a limited number of github actions or something like that.
• The limit would still be configurable. It would be bound to MIR emitting changes, changes in how e.g. the len function on slices is implemented in std, or some other const fn's are implemented in upstream crates. None of these changes alone should blow past the limit with a factor of 4. The limit would still be unstable, yes, but as we are talking about lints instead of errors now, this is less of a problem: lints are allowed to be unstable.
• The user-exposed limit might be given in multiples of thousands, and if there is some change in MIR generation that issues the lint for many people, that multiplier could be adjusted, e.g. to 1200, etc. Of course changes affect code snippets in hugely different ways, depending on which code they use.
• For further protection, if the lint is emitted, it would provide a suggested limit value that is 50% above what is actually needed.

One can also think about AST based cost systems which might be more robust, as AST changes less regularly than MIR, so you only have the noise from upstream crates that you need to worry about. The only problem is that it is probably a huge pain to move information on "how much does this operation cost" through the compiler. MIR optimizations are another issue.

Edit: my proposal is the proposed end state, one could start with something simpler, like just having long_running_const_eval and no configurability. Also the design needs to do something about users allowing the warn-by-default lint and then hitting the deny-by-default one. Maybe have the very_long_* lint check the long_* lint's level at the span before firing?

[RalfJung]

I don't think user-defined limits are worth the complexity. These numbers have no absolute meaning and the relative scale changes over time. I think people just acknowledging "yes this const is slow please eval it anyway" is about all the flexibility we need. Crucially since this is a lint, MIR building changes can never break a dependency. I'm not even sure if we need the exponential back-off vs just a simple deny-by-default lint.

[bors]

☔ The latest upstream changes (presumably #103954) made this pull request unmergeable. Please resolve the merge conflicts.

[pnkfelix]

The lang team discussed this a bit yesterday.

Executive summary:

• In the long term, we would prefer a more stable metric to be employed here.
  • @nikomatsakis has suggested sum of counting back-edges ("an edge in CFG where end dominates start") + procedure calls (where the counter is incremented even for mir-inlined calls).
  • @pnkfelix privately mused to @nikomatsakis whether counting "mems" would be more intuitive metric for end-users; but they agree that @nikomatsakis 's proposal of back-edges + calls should be more stable in the face of hypothetical future MIR optimizations.
  • (@nikomatsakis did privately note that even "back edges" is tricky, considering e.g. constructs like .await that expand into a loop)
• @nikomatsakis also expressed dismay over removing a supposed "guarantee" that every rustc invocation would terminate, but the team argued that we already lost that with proc-macros.
• We didn't make any decision about this specific PR, but the intution of @pnkfelix is that, even with the wrong metric, just moving to some lint is a step in the right direction toward making this not a hazard for people seeking to upgrade their copy of rustc.

[RalfJung]

@nikomatsakis also expressed dismay over removing a supposed "guarantee" that every rustc invocation would terminate, but the team argued that we already lost that with proc-macros.

We can always make this into a hard error after 2^48 basic blocks or so if you want to still have that guarantee. ;)

[pnkfelix]

yes niko did make some side comment about heat death of universe while mulling over all of this

[jruderman]

Please keep some way to set a low limit, so that fuzzing (and regression tests for iloops) can continue to run quickly. It could be a -Z flag instead of a crate attribute, so programmers who aren't trying to test the compiler are less likely to create future-incompatible code.

[RalfJung]

Fuzzing of regular Rust code works with timeouts when executing the code. Fuzzing of const eval can do the same, can't it? Anyway if you deny this lint you have basically the same limit as today.

[jruderman]

I'm trying to avoid timeout-kills because libfuzzer takes a long time to restart (at least the way I'm using it).

[oli-obk]

@rustbot ready

[fee1-dead]

@bors r+ rollup=never

[bors]

📌 Commit 05eae08 has been approved by fee1-dead

It is now in the queue for this repository.

[bors]

⌛ Testing commit 05eae08 with merge 23f93a1...

[bors]

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing 23f93a1 to master...

[rust-timer]

Finished benchmarking commit (23f93a1): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	-2.1%	[-2.1%, -2.1%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 644.818s -> 642.933s (-0.29%)