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

Make panicking in constants work with the 2021 edition #86830

Closed
wants to merge 5 commits into from
Closed

Make panicking in constants work with the 2021 edition #86830

wants to merge 5 commits into from

Conversation

jonas-schievink
Copy link
Contributor

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.

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 2, 2021
@jonas-schievink
Copy link
Contributor Author

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned estebank Jul 2, 2021
// 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 =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Abi::Rust is not an intrinsic ABI, this looks wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't parse this sentence. What does the last "which" refer to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To the impl const Display

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@RalfJung RalfJung Jul 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@RalfJung RalfJung Jul 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!("");
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!("")?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@RalfJung
Copy link
Member

RalfJung commented Jul 3, 2021

Not really sure about this approach but the basics seem to work.

Seems okay overall, except for adding the Rust to that list of intrinsic ABIs.

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.

Doesn't seem so different from the backtrace one would get at runtime?
I think the best way to improve this would be to adjust the backtrace logic. We could also possibly do strange things and pass the caller location to panic_ctfe_hook and attempt to extract the Span from that (not sure if that is possible).

@@ -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")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain that this is for the const panic machinery, and not actually implemented (I assume actually reaching this during CTFE will ICE)

@oli-obk
Copy link
Contributor

oli-obk commented Jul 4, 2021

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

@oli-obk

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Jul 4, 2021

This would have to also disable the "make sure we only evaluate const fn" check... it doesn't seem necessary for this PR so I'd rather avoid it (and maybe find a way to not disable that check: I still think we should only allow certain kinds of formatting).

@oli-obk
Copy link
Contributor

oli-obk commented Jul 4, 2021

So what about the first suggestion? We create an intrinsic for CTFE that takes one &str const generic argument for the format string, one const generic integer for the number of args and the first arg as an argument of generic type. Then const checking can check whether calls to the intrinsic in const contexts have "{}", 1 and &str for the generic parameters. All other contexts just ignore the intrinsic.

We can then either make const checking stop at the first such intrinsic it finds, ore resort to the const_select intrinsic to run different code at compile time and runtime.

@bors
Copy link
Contributor

bors commented Jul 5, 2021

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

@RalfJung
Copy link
Member

Looks like #86998 is trying to solve the same problem.

@jonas-schievink
Copy link
Contributor Author

Closing in favor of that then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants