Make panicking in constants work with the 2021 edition #86830
Conversation
|
Some changes occured to the CTFE / Miri engine cc @rust-lang/miri |
|
r? @estebank (rust-highfive has picked a reviewer for you, use r? to override) |
|
rust-highfive
added
the
S-waiting-on-review
|
r? @oli-obk |
| // Intrinsics use `rustc_const_{un,}stable` attributes to indicate constness. | ||
| // `panic_impl` also does in order to pass const checks (it is never evaluated at compile | ||
| // time). | ||
| if let Abi::Rust | Abi::RustIntrinsic | Abi::PlatformIntrinsic = |
There was a problem hiding this comment.
Abi::Rust is not an intrinsic ABI, this looks wrong.
There was a problem hiding this comment.
Ah I assume this is for the panic_impl?
This is a rather bad hack, I'd strongly prefer it we can avoid it. I have no idea if it has adverse side-effects.
There was a problem hiding this comment.
I tried to special-case the panic_impl lang item only, but it is None in libcore, despite the #[lang = "panic_impl"] on panic_impl.
There was a problem hiding this comment.
Hm... but now that you are allowing a stable ABI here (as in, an ABI that can be used by stable code), the ABI check just does not make any sense any more this way; we might as well remove it entirely and always call lookup_const_stability.
And then we need to somehow be sure that this cannot be exploited by user code, and that is the part I am unsure about.
| // None | ||
| // For now, this is unreachable code - you cannot construct a non-literal | ||
| // `fmt::Arguments` without `impl const Display`, which we don't currently | ||
| // provide. |
There was a problem hiding this comment.
I can't parse this sentence. What does the last "which" refer to?
There was a problem hiding this comment.
To the impl const Display
There was a problem hiding this comment.
So maybe ", and we currently do not provide such an impl"?
But I also don't see what this has to do with impl const Display...
There was a problem hiding this comment.
Actually I think my main confusion is around the term "non-literal fmt::Arguments". You also use it in the panic_ctfe_hook doc comment, but I do not know what you mean by that term.
| let some = match idx.index() { | ||
| 0 => { | ||
| // None | ||
| // For now, this is unreachable code - you cannot construct a non-literal |
There was a problem hiding this comment.
Why is this unreachable? What happens when I do panic!("{}", "hello") in a const fn in the new edition?
| @@ -3,7 +3,6 @@ struct Value; | |||
|
|
|||
| static settings_dir: String = format!(""); | |||
| //~^ ERROR calls in statics are limited to constant functions | |||
| //~| ERROR calls in statics are limited to constant functions | |||
There was a problem hiding this comment.
So there's one error less here... which function is the remaining error from?
Can you add a format_args!, just to make sure that that is still rejected inside const? Probably something like
const CALL_FMT_ARGS: () = {
let _ = format_args!("");
};There was a problem hiding this comment.
The remaining error is from the conversion of fmt::Arguments to String.
format_args!("") does in fact compile now, since panic!("") expands to that. Is there some way to make it only const when used through panic!("")?
There was a problem hiding this comment.
format_args!("") does in fact compile now
I feared this might happen; we need to fix that (so please definitely add a test).
Is there some way to make it only const when used through panic!("")?
const checking happens after macro expansion, so this will be a bit tricky, but I still think adding a special hack in const checking is the best way here. @oli-obk would have a better idea for how to best to that; I hope they are back soon. :D
Seems okay overall, except for adding the
Doesn't seem so different from the backtrace one would get at runtime? |
| @@ -719,6 +719,7 @@ extern "rust-intrinsic" { | |||
| /// | |||
| /// A more user-friendly and stable version of this operation is | |||
| /// [`std::process::abort`](../../std/process/fn.abort.html). | |||
| #[rustc_const_unstable(feature = "const_abort", issue = "none")] | |||
There was a problem hiding this comment.
Please explain that this is for the const panic machinery, and not actually implemented (I assume actually reaching this during CTFE will ICE)
|
this works, and i see nothing wrong with it except for the fact that it increases complexity and adds lots of specialized logic. could we instead add a intrinsic call directly in the panic macro? if we added something like rust-lang/const-eval#7 (comment) we could "just" write two separate paths. one for ctfe, one for runtime, all in lib code instead of compiler magic |
This comment has been minimized.
This comment has been minimized.
|
This would have to also disable the "make sure we only evaluate |
|
So what about the first suggestion? We create an intrinsic for CTFE that takes one We can then either make const checking stop at the first such intrinsic it finds, ore resort to the |
|
☔ The latest upstream changes (presumably #86891) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Looks like #86998 is trying to solve the same problem. |
|
Closing in favor of that then |
Not really sure about this approach but the basics seem to work.
Since we call an intrinsic, this produces very poor error messages. I'd appreciate any help for improving them, but I believe it requires some hairy changes to how const eval reports errors.