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

Errors for if and match in a const are not descriptive #66125

Closed
ecstatic-morse opened this issue Nov 5, 2019 · 1 comment · Fixed by #66170
Closed

Errors for if and match in a const are not descriptive #66125

ecstatic-morse opened this issue Nov 5, 2019 · 1 comment · Fixed by #66170
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-diagnostics Area: Messages for errors, warnings, and lints D-papercut Diagnostics: An error or lint that needs small tweaks. D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Nov 5, 2019

Consider the following code (Playground):

const _: i32 = match 1 {
    2 => 3,
    4 => 5,
    _ => 0,
};

const _: i32 = if true {
    4
} else {
    5
};

This is not valid today, since if and match are forbidden in constants. That restriction can be lifted soon on nightly, so we would like to emit good errors that point the user to the offending code and suggest a feature gate if it would help.

However, the errors emitted by the nightly compiler are not great:

error[E0019]: constant contains unimplemented expression type
 --> src/lib.rs:1:22
  |
1 | const _: i32 = match 1 {
  |                      ^

error[E0019]: constant contains unimplemented expression type
 --> src/lib.rs:2:5
  |
2 |     2 => 3,
  |     ^

error[E0019]: constant contains unimplemented expression type
 --> src/lib.rs:7:19
  |
7 | const _: i32 = if true {
  |                   ^^^^

error[E0019]: constant contains unimplemented expression type
  --> src/lib.rs:7:16
   |
7  |   const _: i32 = if true {
   |  ________________^
8  | |     4
9  | | } else {
10 | |     5
11 | | };
   | |_^

error: aborting due to 4 previous errors

At present, each const has two errors associated with it. One is triggered by the FakeRead emitted for the borrow checker and points to the value being branched on (1 and true) respectively. The other is triggered by the SwitchInt terminator itself. While the span of this error is good for the if (it points to the whole if-else construct), it is very confusing for the match, where it points to the pattern in the first match arm despite the SwitchInt handling all arms simultaneously.

I've thought a bit about how to improve this, but I don't have a great solution. I'm hoping someone can help. I don't think we want to rely only on the presence of FakeRead to reject branchy code in a const; SwitchInt seems like the natural candidate (maybe with a fallback to FakeRead to detect single-arm match statements). However, I don't know how to improve the span for the SwitchInt for the match, or if doing so would cause other diagnostics to regress.

I think the ideal approach would be to check for this at the HIR or AST level. This would give us very precise spans, and we could continue checking for SwitchInt in the MIR to make sure the high-level check didn't miss anything. Is this feasible?

@ecstatic-morse ecstatic-morse added the A-diagnostics Area: Messages for errors, warnings, and lints label Nov 5, 2019
@estebank estebank added A-const-eval Area: constant evaluation (mir interpretation) D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. labels Nov 5, 2019
@Centril
Copy link
Contributor

Centril commented Nov 6, 2019

I think the ideal approach would be to check for this at the HIR or AST level. This would give us very precise spans, and we could continue checking for SwitchInt in the MIR to make sure the high-level check didn't miss anything. Is this feasible?

You could use .delay_span_bug in the MIR check and use a simple HIR visitor to reject certain ExprKind::s, e.g. ::Match(...) and ::Loop(...) should be pretty all-encompassing.

bors added a commit that referenced this issue Nov 11, 2019
Add a HIR pass to check consts for `if`, `loop`, etc.

Resolves #66125.

This PR adds a HIR pass to check for high-level control flow constructs that are forbidden in a const-context. The MIR const-checker is unable to provide good spans for these since they are lowered to control flow primitives (e.g., `Goto` and `SwitchInt`), and these often don't map back to the underlying statement as a whole. This PR is intended only to improve diagnostics once `if` and `match` become commonplace in constants (behind a feature flag). The MIR const-checker will continue to operate unchanged, and will catch anything this check might miss.

In this implementation, the HIR const-checking pass is run much earlier than the MIR one, so it will supersede any errors from the latter. I will need some mentoring if we wish to change this, since I'm not familiar with the diagnostics system. Moving this pass into the same phase as the MIR const-checker could also help keep backwards compatibility for items like `const _: () = loop { break; };`, which are currently (erroneously?) accepted by the MIR const-checker (see #62272).

r? @Centril
cc @eddyb (since they filed #62272)
@bors bors closed this as completed in ded5ee0 Nov 13, 2019
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) A-diagnostics Area: Messages for errors, warnings, and lints D-papercut Diagnostics: An error or lint that needs small tweaks. D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. 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.

3 participants