[const_panic] Report const_panic diagnostics identically to compiler_error invocations #79695

mehcode · 2020-12-04T08:44:44Z

Given

const _ = panic!("Hello");

Before

error: any use of this value will cause an error
 --> const_panic.rs:3:15
  |
3 | const _: () = panic!("hello");
  | --------------^^^^^^^^^^^^^^^-
  |               |
  |               the evaluated program panicked at 'hello', const_panic.rs:3:15
  |

After

error: hello
 --> const_panic.rs:3:15
  |
3 | const _: () = panic!("hello");
  | --------------^^^^^^^^^^^^^^^-
  |

Ref: #51999 (comment)

If this is accepted, I believe that along with the recently merged #78069, the const_panic feature should be unblocked and could be stabilized.

I'm not super happy with how the implementation of this was done; any pointers on how to clean that up would be appreciated.

rust-highfive · 2020-12-04T08:44:47Z

r? @matthewjasper

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

oli-obk · 2020-12-04T11:34:13Z

cc @rust-lang/wg-const-eval

r? @oli-obk

oli-obk · 2020-12-04T11:42:28Z

compiler/rustc_mir/src/const_eval/error.rs

 // `ConstEvalErr` (in `librustc_middle/mir/interpret/error.rs`) knows to
 // handle these.
 impl<'tcx> Into<InterpErrorInfo<'tcx>> for ConstEvalErrKind {
    fn into(self) -> InterpErrorInfo<'tcx> {
-        err_machine_stop!(self.to_string()).into()
+        match self {
+            ConstEvalErrKind::Panic { msg, .. } => InterpError::Panic(msg).into(),


I think we can mirror what miri does here. Instead of having an impl MachineStopType for String, we create an

enum TerminationInfo { Generic(String), Panic(String), }

Then you can downcast to TerminationInfo below and handle it just there.

oli-obk · 2020-12-04T11:48:14Z

compiler/rustc_mir/src/const_eval/error.rs

@@ -193,7 +197,13 @@ impl<'tcx> ConstEvalErr<'tcx> {
                    rustc_session::lint::builtin::CONST_ERR,
                    hir_id,
                    tcx.span,
-                    |lint| finish(lint.build(message), Some(err_msg)),
+                    |lint| {
+                        if is_panic {


I think something similar needs to be done for the Report as hard error case below. Maybe the match &self.error above could produce a message and an Option<String> for the note, and then you can set the appropriatly once?

RalfJung · 2020-12-05T13:56:08Z

compiler/rustc_middle/src/mir/interpret/error.rs

@@ -439,6 +439,8 @@ pub enum InterpError<'tcx> {
    /// The program exhausted the interpreter's resources (stack/heap too big,
    /// execution takes too long, ...).
    ResourceExhaustion(ResourceExhaustionInfo),
+    /// The program terminated immediately from a panic.
+    Panic(Symbol),


This is a CTFE-only error, I don't think it should be added here. This is more of a case for the MachineStopType.

RalfJung · 2020-12-05T13:59:11Z

While I agree that the old rendering of panics is not great, I wonder -- what is the justification for rendering panics so differently from all the other kinds of CTFE errors? Why should everything have "any use of this value will cause an error" but only panics do not?

oli-obk · 2020-12-07T08:21:23Z

While I agree that the old rendering of panics is not great, I wonder -- what is the justification for rendering panics so differently from all the other kinds of CTFE errors? Why should everything have "any use of this value will cause an error" but only panics do not?

That's just because we still haven't made const_err a hard error. If you do the same thing in a static, you get

error[E0080]: could not evaluate static initializer
 --> src/lib.rs:2:19
  |
2 | static FOO: i32 = panic!("Hello");
  |                   ^^^^^^^^^^^^^^^ the evaluated program panicked at 'Hello', src/lib.rs:2:19

The main reason I see to render panics during CTFE differently is to make them essentially be the way in which users can define their own errors.

RalfJung · 2020-12-07T08:31:19Z

If you do the same thing in a static, you get

That looks like a bug though if the same error is rendered very differently for statics vs consts.

The main reason I see to render panics during CTFE differently is to make them essentially be the way in which users can define their own errors.

I'd say these are still user-defined CTFE errors and hence should be recognized as such. It doesn't have to say "the evaluated program panicked", but I think it should say something about const-evaluation somewhere the same way other CTFE errors do.

For example. I would be completely on-board with

error[E0080]: could not evaluate static initializer
 --> src/lib.rs:2:19
  |
2 | static FOO: i32 = panic!("Hello");
  |                   ^^^^^^^^^^^^^^^ Hello

Though this does look somewhat redundant since the same text is also shown in the source, but whatever.^^

mehcode · 2020-12-07T09:12:36Z

In my perspective, the useful point of this is to enable macro authors to produce errors that look like compiler errors and are able to be evaluated in the regular context of the language within the type system.

As a couple of concrete examples (taken from SQLx) that I want to see possible:

error: within the query, syntax error at or near `,`
 --> some_query.rs:3:15
  |
3 | sqlx::query!("SELECT 5,");
  | ------^^^^^^^^^^^^^^^^^^^-
  |

struct Foo { foo: String }

error: mismatched types; Rust type `String` (as SQL type `TEXT`) is not compatible with SQL type `INTEGER`
 --> some_query.rs:3:15
  |
3 | sqlx::query_as!(Foo, "SELECT 10 as foo");
  | ------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
  |

For context, we (SQLx) can achieve the above today in nightly (with the recent awesome ability to do impl const _), it's just the error message printing we're looking to improve.

@RalfJung would you perhaps be okay with uniformly inverting the diagnostic? From your example:

error[E0080]: Hello
 --> src/lib.rs:2:19
  |
2 | static FOO: i32 = panic!("Hello");
  |                   ^^^^^^^^^^^^^^^ could not evaluate static initializer

And a non-panic example:

error: attempt to divide `1_usize` by zero
 --> src/lib.rs:1:18
  |
1 | const x: usize = 1 / 0;
  | -----------------^^^^^-
  |                  |
  |                  any use of this value will cause an error

RalfJung · 2020-12-07T09:29:57Z

The second one is rather odd:

error: attempt to divide `1_usize` by zero
 --> src/lib.rs:1:18
  |
1 | const x: usize = 1 / 0;
  | -----------------^^^^^-
  |                  |
  |                  any use of this value will cause an error

The "this value" points at the division, but the "value" it references is x.

error: within the query, syntax error at or near `,`
 --> some_query.rs:3:15
  |
3 | sqlx::query!("SELECT 5,");
  | ------^^^^^^^^^^^^^^^^^^^-
  |

I see, so you are also relying on macros to hide the underlying source of the error? Isn't this rather fragile, where if you move the panic! call into a const fn helper function, the rendering won't be so nice any more?

What would be so bad about

error: could not evaluate constant
 --> some_query.rs:3:15
  |
3 | sqlx::query!("SELECT 5,");
  | ------^^^^^^^^^^^^^^^^^^^-
  |       |
  |       within the query, syntax error at or near `,`
  |

I am not sure what our usual policy is for the error label vs the note-at-the-span, but it seems to me that the label should be the "general kind of error" and the note-at-the-span should be more specific than that.

RalfJung · 2020-12-07T09:33:03Z

That's just because we still haven't made const_err a hard error. If you do the same thing in a static, you get

Does it even make sense to fine-tune this if it will all change again when const_err becomes a hard error?

Maybe we should first have a PR that makes const_err a future-incompatibility lint and that makes it look the way we'd want the hard error to look later. Right now it all seems to be a bit too much up in the air.

oli-obk · 2020-12-07T09:34:09Z

cc @rust-lang/wg-diagnostics

I am not sure what our usual policy is for the error label vs the note-at-the-span, but it seems to me that the label should be the "general kind of error" and the note-at-the-span should be more specific than that.

Yes. I still believe this is a special situation, just like

fn main() {
    compile_error!("foomp");
}

will produce

error: foomp
 --> src/main.rs:2:5
  |
2 |     compile_error!("foomp");
  |     ^^^^^^^^^^^^^^^^^^^^^^^^

without any additional information. To me it feels like panicking during CTFE is just a more controllable variant of compile_error!.

mehcode · 2020-12-07T09:40:18Z

What would be so bad about

error: could not evaluate constant
--> some_query.rs:3:15
   |
 3 | sqlx::query!("SELECT 5,");
   | ------^^^^^^^^^^^^^^^^^^^-
   |       |
   |       within the query, syntax error at or near `,`
   |

From my experience, newer developers will stop reading as soon as possible when looking at an error message. In that lens, the more important or relevant information to them in order to fix the error, should be directly next to error: where its visually scannable.

Additionally, consider how this would look with --message-format=short:

src/some_query.rs:10:1 error: could not evaluate constant
src/some_query.rs:10:8 error: could not evaluate constant
src/some_query.rs:11:1 error: could not evaluate constant
src/some_query.rs:12:1 error: could not evaluate constant
src/some_query.rs:14:1 error: could not evaluate constant
src/some_query.rs:19:1 error: could not evaluate constant

It's not possible to guess what's going on from that.

RalfJung · 2020-12-07T09:42:56Z

To me it feels like panicking during CTFE is just a more controllable variant of compile_error!.

I don't understand why one would rather want to control the label than the note-at-the-span -- honestly, between the two query! renderings shown above, I think I'd prefer the one I gave.^^ So I'd say compile_error! is the "weird" one here, and that should rather become

error: user-defined compilation error
 --> src/main.rs:2:5
  |
2 |     compile_error!("foomp");
  |     ^^^^^^^^^^^^^^^^^^^^^^^^ foomp

But if everyone else thinks that's fine, then whatever. However, the rendering should definitely be the same no matter whether the error arises in a const or a static.

From my experience, newer developers will stop reading as soon as possible when looking at an error message. In that lens, the more important or relevant information to them in order to fix the error, should be directly next to error: where its visually scannable.

That sounds like it is quite the problem in general since we usually put very useful specific information in the note. I am not convinced hacking around this in individual diagnostics is a good fix.

Additionally, consider how this would look with --message-format=short:

Interesting, I didn't know about this flag. Are error labels usually useful enough to make that flag any good? If so, then again this seems like something we should fix for all CTFE errors, not just for panics.

mehcode · 2020-12-07T09:50:46Z

Interesting, I didn't know about this flag. Are error labels usually useful enough to make that flag any good? If so, then again this seems like something we should fix for all CTFE errors, not just for panics.

It seems that when --message-format=short was added, there was at least a fair amount of interest in improving the single-line error messages so they were understandable.

#49546 (comment)

#49546

oli-obk · 2020-12-07T10:23:48Z

But if everyone else thinks that's fine, then whatever. However, the rendering should definitely be the same no matter whether the error arises in a const or a static.

Definitely... we can't really do this in general until const_err is a hard error, but at least the decision on what is the note and what is the main message should be the same for both. We could even decide to make CTFE panics a hard error, because there's no way for users to be relying on it being a lint right now.

RalfJung · 2020-12-07T10:30:23Z

we can't really do this in general until const_err is a hard error

Why can't we make the wording of the error already be the same as what it will be when it becomes a hard error? In particular for future incompat lints (which this one really should be), isn't that what we usually do?

abonander · 2020-12-31T00:01:39Z

@RalfJung can you clarify what the wording should be then? Also I agree with @oli-obk's point that panics should just be hard errors as that will make integrating in SQLx easier (const _: () = panic!(....)).

RalfJung · 2021-01-02T14:14:02Z

With the current style, I'd make it something like could not evaluate constant. I understand the proposal here is to make the "short message" more informative than that; I think the ones proposing this change should figure out that wording. :) All I am saying is, panics are not special -- if we decide that the short message should be informative, we should make it informative for all const_err. Likely, the internal rustc type representing these errors should then have a short_msg and a span_msg method, so that for each error type we can separately control what is printed in the short message and what is printed next to the span. That is assuming we have something useful to add at the span... some const-errors have more information than should be put into the short message, but maybe those should be extra notes, not attached to the span? I have no idea how rustc diagnostics usually distribute information between the title and the span.

Then I'd propose to make the short title something like could not evaluate constant: $SHORT_MSG.

JohnCSimon · 2021-01-19T05:47:59Z

Ping from triage - @mehcode
can you please resolve the build failure and the reviewer notes? thank you

@rustbot label: -S-waiting-on-review +S-waiting-on-author

JohnCSimon · 2021-02-14T15:23:32Z

Triage:
@mehcode - unfortunately I'm closing this as inactive. Thanks for contributing. If you're willing to continue on this, please open a new PR.

rust-highfive assigned matthewjasper Dec 4, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 4, 2020

report const_panic diagnostics identically to compiler_error invocations

dc820e0

mehcode force-pushed the rl-const-panic-diagnostic branch from 3ecd5e0 to dc820e0 Compare December 4, 2020 10:02

oli-obk added the A-const-eval Area: Constant evaluation (MIR interpretation) label Dec 4, 2020

rust-highfive assigned oli-obk and unassigned matthewjasper Dec 4, 2020

oli-obk reviewed Dec 4, 2020

View reviewed changes

RalfJung reviewed Dec 5, 2020

View reviewed changes

RalfJung mentioned this pull request Dec 26, 2020

Suboptimal diagnostic if const eval error site is in a different crate #80380

Open

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 19, 2021

oli-obk closed this Feb 14, 2021

RalfJung mentioned this pull request Mar 4, 2021

Tracking issue for RFC 2345, "Allow panicking in constants" (const_panic) #51999

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[const_panic] Report const_panic diagnostics identically to compiler_error invocations #79695

[const_panic] Report const_panic diagnostics identically to compiler_error invocations #79695

mehcode commented Dec 4, 2020

rust-highfive commented Dec 4, 2020

oli-obk commented Dec 4, 2020

oli-obk Dec 4, 2020

oli-obk Dec 4, 2020

RalfJung Dec 5, 2020

RalfJung commented Dec 5, 2020

oli-obk commented Dec 7, 2020

RalfJung commented Dec 7, 2020 •

edited

Loading

mehcode commented Dec 7, 2020 •

edited

Loading

RalfJung commented Dec 7, 2020 •

edited

Loading

RalfJung commented Dec 7, 2020

oli-obk commented Dec 7, 2020

mehcode commented Dec 7, 2020 •

edited

Loading

RalfJung commented Dec 7, 2020

mehcode commented Dec 7, 2020

oli-obk commented Dec 7, 2020

RalfJung commented Dec 7, 2020

abonander commented Dec 31, 2020 •

edited

Loading

RalfJung commented Jan 2, 2021

JohnCSimon commented Jan 19, 2021

JohnCSimon commented Feb 14, 2021

[const_panic] Report const_panic diagnostics identically to compiler_error invocations #79695

[const_panic] Report const_panic diagnostics identically to compiler_error invocations #79695

Conversation

mehcode commented Dec 4, 2020

rust-highfive commented Dec 4, 2020

oli-obk commented Dec 4, 2020

oli-obk Dec 4, 2020

Choose a reason for hiding this comment

oli-obk Dec 4, 2020

Choose a reason for hiding this comment

RalfJung Dec 5, 2020

Choose a reason for hiding this comment

RalfJung commented Dec 5, 2020

oli-obk commented Dec 7, 2020

RalfJung commented Dec 7, 2020 • edited Loading

mehcode commented Dec 7, 2020 • edited Loading

RalfJung commented Dec 7, 2020 • edited Loading

RalfJung commented Dec 7, 2020

oli-obk commented Dec 7, 2020

mehcode commented Dec 7, 2020 • edited Loading

RalfJung commented Dec 7, 2020

mehcode commented Dec 7, 2020

oli-obk commented Dec 7, 2020

RalfJung commented Dec 7, 2020

abonander commented Dec 31, 2020 • edited Loading

RalfJung commented Jan 2, 2021

JohnCSimon commented Jan 19, 2021

JohnCSimon commented Feb 14, 2021

RalfJung commented Dec 7, 2020 •

edited

Loading

mehcode commented Dec 7, 2020 •

edited

Loading

RalfJung commented Dec 7, 2020 •

edited

Loading

mehcode commented Dec 7, 2020 •

edited

Loading

abonander commented Dec 31, 2020 •

edited

Loading