-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add memoization for const function evaluations #66294
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// build-pass | ||
|
||
// Check that the evaluation of const-functions with | ||
// zero-sized types as arguments compiles successfully | ||
|
||
struct Zst {} | ||
|
||
const fn foo(val: Zst) -> Zst { val } | ||
|
||
const FOO: Zst = foo(Zst {}); | ||
|
||
fn main() { | ||
const _: Zst = FOO; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,73 +1,21 @@ | ||
error: any use of this value will cause an error | ||
error[E0391]: cycle detected when const-evaluating `hint_unreachable` | ||
--> $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| | ||
LL | fake_type() | ||
| ^^^^^^^^^^^ | ||
| | ||
note: ...which requires const-evaluating `fake_type`... | ||
--> $DIR/uninhabited-const-issue-61744.rs:4:5 | ||
| | ||
LL | hint_unreachable() | ||
| ^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| reached the configured maximum number of stack frames | ||
| inside call to `hint_unreachable` at $DIR/uninhabited-const-issue-61744.rs:4:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:8:5 | ||
| inside call to `fake_type::<i32>` at $DIR/uninhabited-const-issue-61744.rs:12:36 | ||
... | ||
LL | const CONSTANT: i32 = unsafe { fake_type() }; | ||
| --------------------------------------------- | ||
| | ||
= note: `#[deny(const_err)]` on by default | ||
|
||
error[E0080]: erroneous constant used | ||
--> $DIR/uninhabited-const-issue-61744.rs:18:10 | ||
= note: ...which again requires const-evaluating `hint_unreachable`, completing the cycle | ||
note: cycle used when const-evaluating `fake_type` | ||
--> $DIR/uninhabited-const-issue-61744.rs:4:5 | ||
| | ||
LL | dbg!(i32::CONSTANT); | ||
| ^^^^^^^^^^^^^ referenced constant has errors | ||
LL | hint_unreachable() | ||
| ^^^^^^^^^^^^^^^^^^ | ||
|
||
error: aborting due to 2 previous errors | ||
error: aborting due to previous error | ||
|
||
For more information about this error, try `rustc --explain E0080`. | ||
For more information about this error, try `rustc --explain E0391`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does removing the use of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing the use of I've found a similar case on stable, which I've opened as a separate issue: #66332 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the constant were public, the error would persist. So this is a breaking change, where a deny by default lint was changed into an error, but only if using the constant would have errored anyway There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iirc there's a way to mark a query so that its cycles do something other than report a hard error, but I don't remember the details and whether it would help us here. I'll have a look tomorrow when I'm on a computer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great, thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry. This got lost in my todo list. So We only have cc @michaelwoerister I'm not sure if the query system can handle a query succeeding when a cycle occurs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any cycle occurring should be fatal for the compilation session. I don't think we ever quite clarified if cycles can leave incr. comp. in an invalid state and sidestep the issue by never committing such possibly invalid state. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Recovering from a query cycle is also problematic for a parallel compiler since the location we can recover from is not deterministic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like the conclusion is that we will leave this cycle error as a failure. This is technically a breaking change (move from unusable-but-valid-compile to compile-fail). It's a weird edge case that didn't seem likely to be in use in the wild; indeed no regressions were found in the crater run. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,66 +1,21 @@ | ||
error[E0080]: evaluation of constant value failed | ||
error[E0391]: cycle detected when const-evaluating `a` | ||
--> $DIR/infinite-recursion-const-fn.rs:3:25 | ||
| | ||
LL | const fn a() -> usize { b() } | ||
| ^^^ | ||
| | | ||
| reached the configured maximum number of stack frames | ||
| inside call to `b` at $DIR/infinite-recursion-const-fn.rs:3:25 | ||
| | ||
note: ...which requires const-evaluating `b`... | ||
--> $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| | ||
LL | const fn b() -> usize { a() } | ||
| --- | ||
| | | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| inside call to `a` at $DIR/infinite-recursion-const-fn.rs:4:25 | ||
| ^^^ | ||
= note: ...which again requires const-evaluating `a`, completing the cycle | ||
note: cycle used when const-evaluating `ARR::{{constant}}#0` | ||
--> $DIR/infinite-recursion-const-fn.rs:5:18 | ||
| | ||
LL | const ARR: [i32; a()] = [5; 6]; | ||
| --- inside call to `a` at $DIR/infinite-recursion-const-fn.rs:5:18 | ||
| ^^^ | ||
|
||
error: aborting due to previous error | ||
|
||
For more information about this error, try `rustc --explain E0080`. | ||
For more information about this error, try `rustc --explain E0391`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means we do that even in the Miri interpreter, when such functions are run at "run-time", right? I think that's wrong: the function, even without any arguments, can access global state and cause aliasing violations on each call. So I think this PR as-is will regress Miri's ability to find UB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to instead move memoization into const_eval's
find_fn
implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a situation where a const fn can access global state which itself is not constant? This seems like the only situation to me where aliasing may be an issue? (But I think you know much more about Miri than me! 😄)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you like such an implementation to work? Memoize the evaluation of the body and then return that from find_fn? Or memoize something else entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we might have to expand what
find_fn
can return, and the name might have to change reflecting that.Right now, probably not, but as
const fn
get more powerful and allowunsafe
code, that seems very possible to me. #66302 is a step in that direction. At that point CTFE relies on dynamic checks (at CTFE evaluation time), which Miri of course doesn't do as it runs run-time code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes - I wasn't aware we'll one day have unsafe code in const fn!
Ok. I'm going to be unavailable for the next week but will follow up on rust-lang/miri#1081 on my return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we ever allowed (without it being UB) you to mutate or read mutable state in const fn, we couldn't ever do a MIR optimization that evals a const fn call where all args are known at compile-time. Also it would allow constants to read from global mutable state in its original state.
There's no need for dynamic checks if we do post monomorphization checks for this situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We certainly shouldn't allow that during CTFE, but that's not what I talked about. I am talking about Miri-the-standalone-tool.
Now I am very confused, the dynamic checks are post-monomorphization? Both are the same thing to me in this context. Also I thought we all agreed that we should check everything dynamically so that even "Miri unleashed" is sound?
I thought we had consensus that the dynamic check described all the way at the bottom here should definitely be added. (Not that that is very relevant to this PR, but you brought it up.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I don't remember what I meant by that.