UX improvements around const usage and error lints

[golddranks]

This code produces errors and warnings that I consider suboptimal:

#[warn(const_err)]
const TEST: bool = [true][1];
const TEST_2: bool = TEST;

fn main() {}

(Playground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=20e9f735947348f61b83d9331c36f538 )

The errors and warnings are:

   Compiling playground v0.0.1 (/playground)
warning: constant item is never used: `TEST`
 --> src/main.rs:2:1
  |
2 | const TEST: bool = [true][1];
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

warning: constant item is never used: `TEST_2`
 --> src/main.rs:3:1
  |
3 | const TEST_2: bool = TEST;
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: any use of this value will cause an error
 --> src/main.rs:2:20
  |
2 | const TEST: bool = [true][1];
  | -------------------^^^^^^^^^-
  |                    |
  |                    index out of bounds: the len is 1 but the index is 1
  |
note: the lint level is defined here
 --> src/main.rs:1:8
  |
1 | #[warn(const_err)]
  |        ^^^^^^^^^

error: any use of this value will cause an error
 --> src/main.rs:3:22
  |
3 | const TEST_2: bool = TEST;
  | ---------------------^^^^-
  |                      |
  |                      referenced constant has errors
  |
  = note: `#[deny(const_err)]` on by default

error: aborting due to previous error

error: could not compile `playground`.

To learn more, run the command again with --verbose.

What could be better about this error message?

• By setting the const_err lint to warn, it shows error message "warning: any use of this value will cause an error". Indeed, it errors when TEST is used in the definition of TEST_2. However, it also warns that TEST isn't used; so it has a conflicting definition of what counts as "use".
  • Extra verbosity not being great, we could get rid of the "not used" warning here and resolve the conflict of definitions in one go.
• The error message "any use of this value will cause an error" gets repeated. I think the word choice "any" fits well to the warning at definition site, but on the usage site better would be: "error: use of ill-defined/erroneous/something? constant" Maybe also the secondary span could be shortened not to include the item definition; as for me, it muddles the distinction of definition and usage even more. (Might be just here, though, as the usage and definition site are quite similar in this example?)
• The message "note: #[deny(const_err)] on by default" is confusing: yes, the default setting for const_err is deny, but it isn't actually deny here. Is the default setting what you wanted to tell about? How is it relevant, especially as it would be better conveyed in the earlier "note: the lint level is defined here" warning?
  • Edit: I understood a bit more about the "note: #[deny(const_err)] on by default" message. #[deny(const_err)] is a per-item attribute, so it notes that it's on by default... on this item. The message could be better.

Meta

Reproduces on Rust version: 1.43.1 && latest nightly 2020-05-26.

[golddranks]

Ping @estebank, I'm going to try and tackle this!

[golddranks]

Note: found about these error messages originally here: #51999 I think that improving these will also enhance the "panicking in const" experience.

[golddranks]

More notes: found some info about const_err lint here: #71800 It seems that the momentum is to move towards it turning a future-compat lint and eventually a hard error? So need to re-evaluate the error messages here from the viewpoint of const_err hardly every being anything other than `deny. and be careful not to do needless work.

[golddranks]

Basically all the points in the original post seem to stay relevant, but the last one (improving the note about lint level) changes to the question whether we should just get rid of it.

[RalfJung]

Note that improving this will be hard until we resolve #71800. Since const_err can be allowed, we basically have to show errors both at the const def site and the const use site.

Cc @rust-lang/wg-const-eval

[RalfJung]

Edit: I understood a bit more about the "note: #[deny(const_err)] on by default" message. #[deny(const_err)] is a per-item attribute, so it notes that it's on by default... on this item. The message could be better.

Every attribute in Rust is per-item/expression/whatever-it-is-attached-to. If you want to set const_err to "warn" globally, do #![warn(const_err)]. Then the note should change accordingly.

What do you think a better message would look like? Notice that your code is equivalent to something like

const TEST_2: bool = TEST;

// 1000 lines of code.

#[warn(const_err)]
const TEST: bool = [true][1];

So it seems to me like the fact that an unrelated item has warn(const_err) should not affect the message for TEST_2.

[golddranks]

So it seems to me like the fact that an unrelated item has warn(const_err) should not affect the message for TEST_2.

So, it seems that I had a misunderstanding what the error below was trying to tell:

error: any use of this value will cause an error
 --> src/main.rs:3:22
  |
3 | const TEST_2: bool = TEST;
  | ---------------------^^^^-
  |                      |
  |                      referenced constant has errors
  |
  = note: `#[deny(const_err)]` on by default

I thought it was that the use of TEST here is an error (because of I thought the word "this value" pointed to TEST highlighted with ^^^^), but the case was that the definition of TEST_2 is the error being reported here.

So, here's a rethought example of what I consider better errors, with this code: (note, removed the warn level as UX with deny is going to be priority if the lint is turning eventually to a hard error.)

const TEST: bool = [true][1];
const TEST_2: bool = TEST;
fn main() {}

Here's what it currently produces:

   Compiling playground v0.0.1 (/playground)
warning: constant item is never used: `TEST`
 --> src/main.rs:1:1
  |
1 | const TEST: bool = [true][1];
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

warning: constant item is never used: `TEST_2`
 --> src/main.rs:2:1
  |
2 | const TEST_2: bool = TEST;
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: any use of this value will cause an error
 --> src/main.rs:1:20
  |
1 | const TEST: bool = [true][1];
  | -------------------^^^^^^^^^-
  |                    |
  |                    index out of bounds: the len is 1 but the index is 1
  |
  = note: `#[deny(const_err)]` on by default

error: any use of this value will cause an error
 --> src/main.rs:2:22
  |
2 | const TEST_2: bool = TEST;
  | ---------------------^^^^-
  |                      |
  |                      referenced constant has errors

error: aborting due to 2 previous errors

error: could not compile `playground`.

To learn more, run the command again with --verbose.

Here's what I find better errors, updated to reflect my new understanding:

• hide the unused warning for TEST
• hide the lint level note
• change the error message

   Compiling playground v0.0.1 (/playground)
warning: constant item is never used: `TEST_2`
 --> src/main.rs:2:1
  |
2 | const TEST_2: bool = TEST;
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: cannot define `TEST` as an erroneous value
 --> src/main.rs:1:20
  |
1 | const TEST: bool = [true][1];
  | -------------------^^^^^^^^^-
  |                    |
  |                    index out of bounds: the len is 1 but the index is 1

error: cannot define `TEST_2` as an erroneous value
 --> src/main.rs:2:22
  |
2 | const TEST_2: bool = TEST;
  | ---------------------^^^^-
  |                      |
  |                      referenced constant has errors

error: aborting due to 2 previous errors

error: could not compile `playground`.

To learn more, run the command again with --verbose.

[golddranks]

Note: The unused lint warns separately for each unused item even for items that have been used inside another unused item. The linting for consts is in line with that, so it's not relevant to this issue, and shouldn't be unclear if the error message itself is improved.

[estebank]

In my eyes the ideal output here would be:

warning: constant item is never used: `TEST_2`
 --> src/main.rs:2:1
  |
2 | const TEST_2: bool = TEST;
  |       ^^^^^^

error: using `TEST` will cause "index out of bound" error
 --> src/main.rs:1:20
  |
1 | const TEST: bool = [true][1];
  |       ----         ^^^^^^^^^ index out of bounds: the len is 1 but the index is 1
  |       |
  |       using this will cause "index out of bounds" error
  |
  = note: `#[deny(const_err)]` on by default

error: aborting due to previous error

Making the definition of TEST_2 be considered when considering whether TEST is used should be straightforward.

Hiding the lint level note is in my eyes a regression, because it hides information from the user. We present it in the first place so that people know that this is 1) a lint and 2) how to disable it if they wish.

Hiding the "cannot define TEST_2 as an erroneous value" lint error would be harder to do, but it might be possible to check whether it is another const, and if so, turn the lint/error call delay_as_bug on it so that we avoid a regression where we compile without alerting by accident, but we no longer display it, as it is redundant information. We should probably also check that this is a local const, otherwise we would let you refer to a bad constant from another crate that disables the lint without any warning, and that would be surprising, I think.

[golddranks]

@estebank I'm currently reading and tinkering with the error handling code in the const eval system + miri to see how it works, trying to eventually get only single error instead of two.

About hiding the lint level: on the other thread ( #71800 ) there seemed to be significant momentum towards deprecating that lint. Because of backwards compatibility, we can't get rid of it, but I think that if it's deprecated, hiding it makes sense.

[golddranks]

@estebank There is another error around const panicking that wasn't talked about in this issue yet, but I'm eager to hear your opinion about it:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=52b473fae95235bfa6efd3b3fe653932

Here, the error looks like this:

warning: constant is never used: `A`
 --> src/main.rs:4:1
  |
4 | const A: () = panic!("panic while evaluating const");
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

error: any use of this value will cause an error
 --> src/main.rs:4:15
  |
4 | const A: () = panic!("panic while evaluating const");
  | --------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
  |               |
  |               the evaluated program panicked at 'panic while evaluating const', src/main.rs:4:15
  |
  = note: `#[deny(const_err)]` on by default
  = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

What do you think about the note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) part? This note used to be shown in situations like this, where it's actually useful:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=f677256fe5205f771e0f01efa9b7120b

However, here, with panic! I'm not sure whether it's useful. Seems like dead weight. However, making an exception for panic! sounds like a complication internally. What do you think?

[estebank]

However, here, with panic! I'm not sure whether it's useful. Seems like dead weight. However, making an exception for panic! sounds like a complication internally. What do you think?

It would be nice if we could have some kind of annotation similar to #[must_use] that would tell rustc not to talk about the macro-backtrace. We could also instead check against macros that come from the stdlib. I'm not sure how hard it would be to manage that, but it certainly seems like a possible improvement that can be tackled on its own.

[e2-71828]

Another problem is that constants that depend on type parameters don’t report the value of those type parameters when they error. For an example, see https://users.rust-lang.org/t/how-to-get-context-for-associated-const-errors/44478

When an error occurs in one monomorphization but not others, it’s important to be able to tell where the error occurred.