-
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
Make panicking in constants work with the 2021 edition #86830
Changes from all commits
1ede46b
948369e
8353259
80301a3
c360d6f
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 |
---|---|---|
|
@@ -309,6 +309,35 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, | |
); | ||
ecx.write_scalar(Scalar::Ptr(ptr), dest)?; | ||
} | ||
sym::panic_ctfe_hook => { | ||
// Option<&str> | ||
assert!(args.len() == 1); | ||
|
||
debug!("panic_ctfe_hook: invoked with {:?}", args[0]); | ||
|
||
let (_, idx) = ecx.read_discriminant(&args[0])?; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this unreachable? What happens when I do |
||
// `fmt::Arguments` without `impl const Display`, which we don't currently | ||
// provide. | ||
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. I can't parse this sentence. What does the last "which" refer to? 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. To the 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. So maybe ", and we currently do not provide such an But I also don't see what this has to do with 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. Actually I think my main confusion is around the term "non-literal |
||
unreachable!("non-literal `fmt::Arguments` used in const panic"); | ||
} | ||
1 => { | ||
// Some | ||
ecx.operand_downcast(&args[0], idx)? | ||
} | ||
_ => unreachable!("encountered `Option` with variant {:?}", idx), | ||
}; | ||
|
||
let ref_msg = ecx.operand_field(&some, 0)?; | ||
let msg_place = ecx.deref_operand(&ref_msg)?; | ||
let msg = Symbol::intern(ecx.read_str(&msg_place)?); | ||
let span = ecx.find_closest_untracked_caller_location(); | ||
let (file, line, col) = ecx.location_triple_for_span(span); | ||
return Err(ConstEvalErrKind::Panic { msg, file, line, col }.into()); | ||
} | ||
_ => { | ||
return Err(ConstEvalErrKind::NeedsRfc(format!( | ||
"calling intrinsic `{}`", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe 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) |
||
pub fn abort() -> !; | ||
|
||
/// Informs the optimizer that this point in the code is not reachable, | ||
|
@@ -1907,6 +1908,16 @@ extern "rust-intrinsic" { | |
/// Allocate at compile time. Should not be called at runtime. | ||
#[rustc_const_unstable(feature = "const_heap", issue = "79597")] | ||
pub fn const_allocate(size: usize, align: usize) -> *mut u8; | ||
|
||
/// Implementation detail of the `const_panic` feature. | ||
RalfJung marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// | ||
/// The panic code calls this with the result of `fmt::Arguments::as_str`. | ||
/// Since it is currently impossible to construct a non-literal | ||
/// `fmt::Arguments` in a const context, this will always be a `Some` | ||
/// containing the panic message. | ||
#[rustc_const_unstable(feature = "panic_ctfe_hook", issue = "none")] | ||
#[cfg(not(bootstrap))] | ||
pub fn panic_ctfe_hook(message: Option<&str>); | ||
} | ||
|
||
// Some functions are defined here because they accidentally got made | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe 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? const CALL_FMT_ARGS: () = {
let _ = format_args!("");
}; 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. The remaining error is from the conversion 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.
I feared this might happen; we need to fix that (so please definitely add a test).
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 |
||
|
||
fn from_string(_: String) -> Value { | ||
Value | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// edition:2021 | ||
|
||
// NB: panic macros without arguments share the 2015/2018 edition hook | ||
// We cannot annotate the expected error in the test because it point at libcore | ||
// FIXME: this is a very bad error message | ||
|
||
#![feature(const_panic)] | ||
#![allow(non_fmt_panic)] | ||
#![crate_type = "lib"] | ||
|
||
const Z: () = std::panic!("cheese"); | ||
|
||
const Z2: () = std::panic!(); | ||
//~^ ERROR evaluation of constant value failed | ||
|
||
const Y: () = std::unreachable!(); | ||
//~^ ERROR evaluation of constant value failed | ||
|
||
const X: () = std::unimplemented!(); | ||
//~^ ERROR evaluation of constant value failed | ||
|
||
const Z_CORE: () = core::panic!("cheese"); | ||
|
||
const Z2_CORE: () = core::panic!(); | ||
//~^ ERROR evaluation of constant value failed | ||
|
||
const Y_CORE: () = core::unreachable!(); | ||
//~^ ERROR evaluation of constant value failed | ||
|
||
const X_CORE: () = core::unimplemented!(); | ||
//~^ ERROR evaluation of constant value failed |
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.
Abi::Rust
is not an intrinsic ABI, this looks wrong.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.
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.
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 isNone
in libcore, despite the#[lang = "panic_impl"]
onpanic_impl
.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.
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.