Overhaul const-checking diagnostics #77354

ecstatic-morse · 2020-09-30T02:16:43Z

The primary purpose of this PR was to remove NonConstOp::STOPS_CONST_CHECKING, which causes any additional errors found by the const-checker to be silenced. I used this flag to preserve diagnostic parity with qualify_min_const_fn.rs, which has since been removed.

However, simply removing the flag caused a deluge of errors in some cases, since an error would be emitted any time a local or temporary had a wrong type. To remedy this, I added an alternative system (DiagnosticImportance) to silence additional error messages that were likely to distract the user from the underlying issue. When an error of the highest importance occurs, all less important errors are silenced. When no error of the highest importance occurs, all less important errors are emitted after checking is complete. Following the suggestions from the important error is usually enough to fix the less important errors, so this should lead to better UX most of the time.

There's also some unrelated diagnostics improvements in this PR isolated in their own commits. Splitting them out would be possible, but a bit of a pain. This isn't as tidy as some of my other PRs, but it should only affect diagnostics, never whether or not something passes const-checking. Note that there are a few trivial exceptions to this, like banning Yield in all const-contexts, not just const fn.

As always, meant to be reviewed commit-by-commit.

r? @oli-obk

This doesn't change any UI test output

This errors during AST lowering. Any errors we emit here are just noise.

This ensures that `emit_error` will actually cause compilation to fail.

The "otherwise" note is printed before the suggestion currently.

This helper function was meant to reduce code duplication between const-checking pre- and post-drop-elaboration. Most of the functionality is only relevant for the pre-drop-elaboration pass.

ecstatic-morse · 2020-09-30T02:26:04Z

src/test/ui/consts/const-eval/assign-to-static-within-other-static-2.rs

-#![feature(const_raw_ptr_deref)]
+#![feature(const_raw_ptr_deref, const_mut_refs)]



This test was only failing pre-monomorphization due to a missing feature gate. It seemed like we wanted to exhaustively test this, especially since there is another test without the feature gate modified in this commit. This led to #77353.

Now all structured errors must have their own error code

Inline assembly is now the only user of E0019. What is it doing that E0015 is not?

oli-obk · 2020-09-30T09:01:51Z

cc @rust-lang/wg-diagnostics

oli-obk · 2020-09-30T09:03:59Z

compiler/rustc_mir/src/transform/check_consts/ops.rs

    }

    fn emit_error(&self, ccx: &ConstCx<'_, '_>, span: Span) {
-        mcf_emit_error(ccx, span, "const fn generators are unstable");
+        ccx.tcx.sess.struct_span_err(span, "Generators and `async` functions cannot be `const`").emit();


Since this is already checked in the syntax based checks, couldn't this also be a delay_span_bug?

I couldn't see a way to make a DiagnosticBuilder that gets emitted as a delay_span_bug. I could either inline the delay_span_bug, since the structured error is only emitted in one place, or wait until I change to Result<ErrorReported, DiagnosticBuilder>.

You invoke delay_as_bug instead of emit: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/struct.DiagnosticBuilder.html#method.delay_as_bug

oli-obk · 2020-09-30T09:05:11Z

compiler/rustc_mir/src/transform/check_consts/ops.rs

@@ -49,7 +49,9 @@ pub fn non_const<O: NonConstOp>(ccx: &ConstCx<'_, '_>, op: O, span: Span) -> boo
        return false;
    }

-    op.emit_error(ccx, span);
+    let mut err = op.build_error(ccx, span);


The "usual" way we do this is to return an ErrorReported instance

Ah, nevermind, this is for the primary/secondary system. One possibility would be not to have the importance function, but instead return an Result<ErrorReported, DiagnosticBuilder<'tcx>> from build_error. Then this could internally decide whether to immediately report or whether to return the builder for later use.

oli-obk · 2020-09-30T09:06:54Z

compiler/rustc_mir/src/transform/check_consts/ops.rs

@@ -33,7 +33,12 @@ pub fn non_const<O: NonConstOp>(ccx: &ConstCx<'_, '_>, op: O, span: Span) -> boo
                        concat!(r#"#[rustc_const_unstable(feature = "...", issue = "...")]"#, '\n').to_owned(),
                        Applicability::HasPlaceholders,
                    )
-                    .note("otherwise `#[allow_internal_unstable]` can be used to bypass stability checks")
+                    .span_suggestion(
+                        ccx.body.span,


this should only be ccx.body.span.lo() I think? Or we may even the `ccx.tcx.def_span(ccx.def_id.to_def_id()) so it's inserted before the item and not as part of the body.

Yeah, the def_span one seems most correct.

def_span seems to point before the block as well. I ended up using fn_sig().span.shrink_to_lo(), which should do the right thing.

compiler/rustc_mir/src/transform/check_consts/ops.rs

oli-obk · 2020-09-30T09:14:04Z

compiler/rustc_mir/src/transform/check_consts/validation.rs

@@ -301,7 +318,15 @@ impl Validator<'mir, 'tcx> {

        let mut err = op.build_error(self.ccx, span);
        assert!(err.is_error());
-        err.emit();
+
+        match op.importance() {


As mentioned elsewhere, I think build_error should return a Result<ErrorReported, DiagnosticBuilder<'tcx>>. Additionally the error_emitted field could then be an Option<ErrorReported>

Hmm... I just noticed that emit does not return Option<ErrorReported>.. sorry I thought we already had done all that. Let's carry on with this PR then and fix that together with making emit create instances of ErrorReported

Sounds good. I like the ErrorReported approach.

compiler/rustc_error_codes/src/error_codes/E0019.md

ecstatic-morse · 2020-09-30T16:28:02Z

@oli-obk I want to just do the Result<ErrorReported, ...> thing now. Does that work for you? I'll avoid rebasing for now and we can squash it before this gets merged.

oli-obk · 2020-09-30T16:45:49Z

Sure, I have no problem with that being done immediately in this PR, I just didn't want to make it a requirement for getting merged

`def_span` has the same issues as `body.span`, so do it this way instead.

ecstatic-morse · 2020-09-30T18:36:15Z

I tried doing the Result<...> refactor and liked it less than the current approach, since most every emit_error function has to emit and then return an Ok. I'll try it in a follow-up PR and we can see if you feel the same. Do you want me to delay_span_bug on Generator MIR as part of this PR? It doesn't fit into the build_error framework since errors are emited unconditionally, so I'll have to circumvent check_op. I will probably use that error for the case in #77361, so I think it should remain inside the framework for now.

Other review comments should be addressed.

oli-obk · 2020-10-01T06:44:26Z

@bors r+

yea, I was a bit worried about that, especially since emit doesn't return Option<ErrorReported> yet. One thing that could be done (and would probably need to be done even with emit returning Option<ErrorReported>) is to add a helper trait that has an emit_err() method that returns Ok(ErrorReported), so you can just invoke that instead of emit. Once emit returns Option<ErrorReported> we can then make the body of emit_err() be Ok(diag.emit().unwrap()), since we know for sure we're not emitting lints. This way only the code that actually returns Err will actually need special handling.

bors · 2020-10-01T06:44:27Z

📌 Commit 1301f43 has been approved by oli-obk

bors · 2020-10-01T07:38:53Z

⌛ Testing commit 1301f43 with merge fc42fb8...

bors · 2020-10-01T09:52:50Z

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing fc42fb8 to master...

ecstatic-morse added 3 commits September 29, 2020 17:38

Useful derives on mir::LocalKind

4d343a5

Continue const-checking after errors when easy

325b7d4

This doesn't change any UI test output

Continue after impl Trait in const fn

25c7753

rust-highfive assigned oli-obk Sep 30, 2020

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

ecstatic-morse force-pushed the const-checking-moar-errors branch from c85f2cd to 97dc865 Compare September 30, 2020 02:17

ecstatic-morse added 15 commits September 29, 2020 19:20

delay_span_bug if const-checking an async function

c38aca0

This errors during AST lowering. Any errors we emit here are just noise.

Forbid generator-specific MIR in all const-contexts

20e07e7

Return a DiagnosticBuilder from structured errors

782a595

This ensures that `emit_error` will actually cause compilation to fail.

Fix "unstable in stable" error

ce50939

The "otherwise" note is printed before the suggestion currently.

Remove ops::non_const

de35c42

This helper function was meant to reduce code duplication between const-checking pre- and post-drop-elaboration. Most of the functionality is only relevant for the pre-drop-elaboration pass.

Give MutDeref a real error message

b518ccb

Bless mut tests

a23297f

Priority levels

5b31455

Don't emit duplicate errors for the return place

b400871

Bless tests

51fbd55

Emit multiple function pointer errors from const-checker

37f37dc

Bless output

879d379

Don't stop const-checking after erroneous trait bound

e02ea83

Bless tests

4bbc79c

Remove machinery for halting error output

287993c

ecstatic-morse force-pushed the const-checking-moar-errors branch from 97dc865 to 287993c Compare September 30, 2020 02:20

ecstatic-morse commented Sep 30, 2020

View reviewed changes

Update compile-fail test

bed7b29

ecstatic-morse force-pushed the const-checking-moar-errors branch 2 times, most recently from e89a2c7 to 6533e40 Compare September 30, 2020 04:10

ecstatic-morse added 2 commits September 29, 2020 21:17

Remove default build_error impl

1513904

Now all structured errors must have their own error code

Rewrite E0019 example

7c6d685

Inline assembly is now the only user of E0019. What is it doing that E0015 is not?

ecstatic-morse force-pushed the const-checking-moar-errors branch from 6533e40 to 7c6d685 Compare September 30, 2020 04:17

oli-obk reviewed Sep 30, 2020

View reviewed changes

compiler/rustc_mir/src/transform/check_consts/ops.rs Show resolved Hide resolved

oli-obk reviewed Sep 30, 2020

View reviewed changes

compiler/rustc_error_codes/src/error_codes/E0019.md Outdated Show resolved Hide resolved

ecstatic-morse added 2 commits September 30, 2020 11:29

Better span for attribute suggestions

0c26144

`def_span` has the same issues as `body.span`, so do it this way instead.

Remove E0019, use E0015 for inline assembly in a const

1301f43

ecstatic-morse force-pushed the const-checking-moar-errors branch from 0d3960e to 1301f43 Compare September 30, 2020 18:29

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 1, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 1, 2020

bors merged commit fc42fb8 into rust-lang:master Oct 1, 2020

rustbot added this to the 1.48.0 milestone Oct 1, 2020

ecstatic-morse deleted the const-checking-moar-errors branch October 6, 2020 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul const-checking diagnostics #77354

Overhaul const-checking diagnostics #77354

ecstatic-morse commented Sep 30, 2020 •

edited

Loading

ecstatic-morse Sep 30, 2020

oli-obk commented Sep 30, 2020

oli-obk Sep 30, 2020

ecstatic-morse Sep 30, 2020

oli-obk Sep 30, 2020

oli-obk Sep 30, 2020

oli-obk Sep 30, 2020 •

edited by ecstatic-morse

Loading

oli-obk Sep 30, 2020

ecstatic-morse Sep 30, 2020

ecstatic-morse Sep 30, 2020

oli-obk Sep 30, 2020

oli-obk Sep 30, 2020

ecstatic-morse Sep 30, 2020

ecstatic-morse commented Sep 30, 2020 •

edited

Loading

oli-obk commented Sep 30, 2020

ecstatic-morse commented Sep 30, 2020 •

edited

Loading

oli-obk commented Oct 1, 2020

bors commented Oct 1, 2020

bors commented Oct 1, 2020

bors commented Oct 1, 2020

		#![feature(const_raw_ptr_deref)]
		#![feature(const_raw_ptr_deref, const_mut_refs)]

Overhaul const-checking diagnostics #77354

Overhaul const-checking diagnostics #77354

Conversation

ecstatic-morse commented Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

oli-obk commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk Sep 30, 2020 • edited by ecstatic-morse Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse commented Sep 30, 2020 • edited Loading

oli-obk commented Sep 30, 2020

ecstatic-morse commented Sep 30, 2020 • edited Loading

oli-obk commented Oct 1, 2020

bors commented Oct 1, 2020

bors commented Oct 1, 2020

bors commented Oct 1, 2020

ecstatic-morse commented Sep 30, 2020 •

edited

Loading

oli-obk Sep 30, 2020 •

edited by ecstatic-morse

Loading

ecstatic-morse commented Sep 30, 2020 •

edited

Loading

ecstatic-morse commented Sep 30, 2020 •

edited

Loading