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

const_panic: ICE on non-&str panic payload #66693

Closed
RalfJung opened this issue Nov 24, 2019 · 18 comments · Fixed by #80734
Closed

const_panic: ICE on non-&str panic payload #66693

RalfJung opened this issue Nov 24, 2019 · 18 comments · Fixed by #80734
Labels
A-const-eval Area: constant evaluation (mir interpretation) C-bug Category: This is a bug. glacier ICE tracked in rust-lang/glacier. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

The panic payload of the std::panic! macro (as passed on to begin_panic) is generic, but the CTFE panic support assumes it will always be an &str. This leads to ICEs:

#![feature(const_panic)]

const C: () = {
    panic!(1234);
};

Cc @oli-obk

@jonas-schievink jonas-schievink added A-const-eval Area: constant evaluation (mir interpretation) C-bug Category: This is a bug. requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ labels Nov 24, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Nov 24, 2019

I guess we could support this, but we'd have to find some way to render the errors. Maybe we need to wait for const traits so we can require const Debug?

@RalfJung
Copy link
Member Author

Alternatively constck could require the argument to have type &str.

@memoryruins
Copy link
Contributor

core::panic! also requires a &str

#![feature(const_panic)]
const _: () = core::panic!(1234);
error[E0308]: mismatched types
 --> src/lib.rs:2:28
  |
2 | const _: () = core::panic!(1234);
  |                            ^^^^ expected `&str`, found integer

@RalfJung
Copy link
Member Author

Yeah, the libcore panic machinery does not support non-format-string panics.

@memoryruins
Copy link
Contributor

Aye, left out the latter of my message. &str is likely the most common argument to panic!, and it's not unusual with e.g. core::panic!; so I agree that waiting for const traits before supporting any other arguments is reasonable.

@DutchGhost
Copy link
Contributor

Im hitting this ICE on stable without feature flag:

struct Bug([u8; panic!(1)]);

@RalfJung
Copy link
Member Author

Hm indeed, that is interesting:

error[E0658]: panicking in constants is unstable
 --> src/lib.rs:1:17
  |
1 | struct Bug([u8; panic!(1)]);
  |                 ^^^^^^^^^
  |
  = note: see issue #51999 <https://github.com/rust-lang/rust/issues/51999> for more information
  = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

thread 'rustc' panicked at '`ref_to_mplace` called on non-ptr type', src/librustc_mir/interpret/place.rs:292:47
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: internal compiler error: unexpected panic

@rust-lang/wg-const-eval any idea why we go on here after seeing an error? Does this kind of behavior mean any part of CTFE is reachable on stable somehow?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 28, 2020

Does this kind of behavior mean any part of CTFE is reachable on stable somehow?

yes. We're not gating CTFE on whether there were stability (or any other static checks) failing. I've been abusing this since 1.0 to demo things on the stable compiler and never got around to fixing it. I don't even know if we have a tracking issue for it.

@RalfJung

This comment has been minimized.

@oli-obk

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 29, 2020

On second thought, let's leave this issue for the ICE, and make a new one for "we can trigger nightly-only const code on stable because we don't abort compilation early enough": #76064.

@sffc
Copy link

sffc commented Oct 13, 2020

Given that this ICE is pre-existing, as shown by #66693 (comment), can we un-block #51999? I'd really like to see const_panic stabilized, and this seems like a minor issue to be blocking on.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 13, 2020

Given that this ICE is pre-existing, as shown by #66693 (comment), can we un-block #51999? I'd really like to see const_panic stabilized, and this seems like a minor issue to be blocking on.

That comment is an example of accidentally partially exposing an unstable feature on stable -- a separate bug tracked in #76064. I don't see how that's an argument for shipping that feature on stable when it is broken. In particular, #76064 applies to all unstable const features.

Fixing this bug here is not even that hard I think: somewhere around here

if is_lang_panic_fn(tcx, callee) {

there needs to be a check that the argument has type &str.

Re: #51999, AFAIK that one is also blocked on figuring out how the diagnostics should look like, and a further concern (though maybe not blocking) is this problem.

@davidhewitt
Copy link
Contributor

I guess that #3007 might make this a non-issue, where it is proposed std::panic! also would require &str as first argument.

@CDirkx
Copy link
Contributor

CDirkx commented Dec 22, 2020

I guess that #3007 might make this a non-issue, where it is proposed std::panic! also would require &str as first argument.

For reference: rust-lang/rfcs#3007, which was recently merged. The plan is to fix the behaviour of panic in the Rust 2021 edition.

Given that this ICE is listed as a blocker for const_err, does this mean that stabilizing that will have to wait for the next edition?

@abonander
Copy link
Contributor

No, because breaking changes are allowed to be made to unstable features without moving to a new edition; that's kinda the whole idea. The 2021 edition will effectively just extend the same restriction to all panic!() invocations, const context or not.

@abonander
Copy link
Contributor

there needs to be a check that the argument has type &str.

@RalfJung I'd like to tackle this but I'm not very experienced with the CTFE code. I get that we need to check args[0] from the Terminator::Call that we've matched on in this branch and we need to match on the Operand enum, but:

match &args[0] {
    Operand::Copy(place) | Operand::Move(place) => // how to get the type of `Place` here?
    Operand::Constant(constant) => // compare constant.ty, but against what?
}

For the first question, I see Place::iter_projections() and that we probably want to check that the PlaceElem type of the tuple and that there's variants of ProjectionElem that are Ty, but do we stop the iteration when we get one that is a type? Is the projection list always guaranteed to terminate in a variant of ProjectionElem that has a Ty? And once we have an instance of Ty, do we just check that there's any projection in the list that has a Ty::kind() == TyKind::Str?

@RalfJung
Copy link
Member Author

RalfJung commented Jan 5, 2021

@abonander you can use Operand::ty to get the type of the Operand. Then you can test its kind() to be Str.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) C-bug Category: This is a bug. glacier ICE tracked in rust-lang/glacier. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants