cleanup and dedupe CTFE and Miri error reporting #104317

RalfJung · 2022-11-12T12:40:49Z

It looks like most of the time, this error raised from const_prop_lint is just redundant -- it duplicates the error reported when evaluating the const-eval query. This lets us make ConstEvalErr private to the const_eval module which I think is a good step.

The Miri change mostly replaces a match by if let, and dedupes the "this error is impossible in Miri" checks.

r? @oli-obk
Fixes #75461

rustbot · 2022-11-12T12:40:55Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rustbot · 2022-11-12T15:26:54Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

oli-obk · 2022-11-14T10:11:34Z

src/test/ui/consts/const-err-late.rs

    black_box((S::<i32>::FOO, S::<u32>::FOO));
-    //~^ ERROR erroneous constant
-    //~| ERROR erroneous constant


This is basically the reason the sometimes-duplicate error was added. Without it, it becomes unclear what caused the error

There is still an error pointing at FOO. But the problem is the missing instantiation site?

This is a general problem with our const-eval errors -- they don't show where the const was used. Not sure what the best way is to add that information. After all the query is only evaluated once, without knowing the use sites. A separate lint is kind of a hack. Making this part of const_prop_lint is also a hack, this is an entirely separate concern. (Seems fine to keep it there I guess but there should be a comment explaining all this.)

compiler/rustc_mir_transform/src/const_prop_lint.rs

RalfJung · 2022-11-14T20:07:49Z

All right, attempt #2. :D

RalfJung · 2022-11-14T20:08:46Z

src/test/ui/associated-consts/issue-88599-ref-self.stderr

+   |
+LL |     const CONST: usize = <&Self>::CONST;
+   |                          ^^^^^^^^^^^^^^
+


This is strange, the build actually succeeds despite this error...

oli-obk · 2022-11-15T08:57:57Z

compiler/rustc_mir_transform/src/const_prop_lint.rs

-                err.report_as_error(tcx, "erroneous constant used");
-                None
+        let res = self.use_ecx(source_info, |this| this.ecx.const_to_op(&c.literal, None));
+        if res.is_none() {


we do need the error to avoid emitting the note in case of TooGeneric and similar "nonfatal const prop failures"

Urgh but that's a terrible abstraction violation. :/

it's what report_as_error was doing before. So you could add a maybe_report_erroneous_const() method to the error type and call it where we called report_as_error before

RalfJung · 2022-11-15T09:22:19Z

I think what we really want is a const_eval query where we pass in a span that points to where the constant is used, so that the error message can incorporate that. Unfortunately we cannot do this on the query level, because then evaluating the same constant at different locations would re-evaluate the constant each time since an input changes.

So the 2nd best thing I can think of is some function that wraps the query. It first calls the query and then if that errors also emits a span for the usage location.

I don't quite understand how we propagate error information out of the query though... looks like all we get there is ErrorHandled. const_prop_lint currently circumvents this by calling const_to_op which does a bunch of checks before invoking the query.

So maybe const_to_op should be that wrapper function? Maybe instead of raising an InterpError it should emit a diagnostic itself, complementing the query it fired off? That would also make things more consistent between consts used in const-context vs runtime-context. It feels strange to have this wrapper be inside InterpCx though, this does not really need a full interpreter.

oli-obk · 2022-11-15T09:49:38Z

Unfortunately we cannot do this on the query level, because then evaluating the same constant at different locations would re-evaluate the constant each time since an input changes.

We could do that by having an Option<Span> argument in the query key that we only set to Some if we are erroring. So basically invoke it once with None, causing no error to be emitted, but an error to be returned, if you get back an error, invoke again with Some(span) and now the query can error inside.

RalfJung · 2022-11-15T10:14:00Z

Hm that sounds like it would make the error code more complicated again because it'd have to suppress the error the first time around. I'll leave that to later, feel free to open an issue tracking that -- it's a nice idea!

For now I think I am going with the 2nd variant I described above.

rustbot · 2022-11-15T11:13:26Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in const_evaluatable.rs

cc @lcnr

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung · 2022-11-15T11:21:07Z

src/test/ui/consts/const-err-late.stderr

+  --> $DIR/const-err-late.rs:19:16
+   |
+LL |     black_box((S::<i32>::FOO, S::<u32>::FOO));
+   |                ^^^^^^^^^^^^^


I don't know why errors are shown multiple times. But in production we deduplicate so users should see this only once.

oli-obk · 2022-11-15T11:26:22Z

@bors r+

oli-obk · 2022-11-15T16:48:02Z

@bors r+

bors · 2022-11-15T16:48:07Z

📌 Commit e7d9981eb83cdc7b6a2dc33194e016d99e7ef34b has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-11-15T21:37:43Z

☔ The latest upstream changes (presumably #104054) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2022-11-16T09:59:31Z

@bors r=oli-obk

bors · 2022-11-16T09:59:34Z

📌 Commit 1115ec6 has been approved by oli-obk

It is now in the queue for this repository.

RalfJung · 2022-11-16T10:00:13Z

@bors rollup=maybe

…li-obk cleanup and dedupe CTFE and Miri error reporting It looks like most of the time, this error raised from const_prop_lint is just redundant -- it duplicates the error reported when evaluating the const-eval query. This lets us make `ConstEvalErr` private to the const_eval module which I think is a good step. The Miri change mostly replaces a `match` by `if let`, and dedupes the "this error is impossible in Miri" checks. r? `@oli-obk` Fixes rust-lang#75461

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#103750 (Fix some misleading target feature aliases) - rust-lang#104137 (Issue error when -C link-self-contained option is used on unsupported platforms) - rust-lang#104317 (cleanup and dedupe CTFE and Miri error reporting) - rust-lang#104335 (Only do parser recovery on retried macro matching) - rust-lang#104394 (various cleanups to try to reduce the use of spans inside method resolution) - rust-lang#104459 (rustdoc: remove unused JS IIFE from main.js) - rust-lang#104462 (rustdoc: remove pointless CSS `.rightside { padding-right: 2px }`) - rust-lang#104466 (rustdoc: remove no-op CSS `#crate-search-div { display: inline-block }`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

lcnr · 2022-11-17T15:52:07Z

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

-                write!(f, "encountered constants with type errors, stopping evaluation")
+                write!(
+                    f,
+                    "an error has already been reported elsewhere (this sould not usually be printed)"


s/sould/should

…li-obk cleanup and dedupe CTFE and Miri error reporting It looks like most of the time, this error raised from const_prop_lint is just redundant -- it duplicates the error reported when evaluating the const-eval query. This lets us make `ConstEvalErr` private to the const_eval module which I think is a good step. The Miri change mostly replaces a `match` by `if let`, and dedupes the "this error is impossible in Miri" checks. r? ``@oli-obk`` Fixes rust-lang#75461

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#103750 (Fix some misleading target feature aliases) - rust-lang#104137 (Issue error when -C link-self-contained option is used on unsupported platforms) - rust-lang#104317 (cleanup and dedupe CTFE and Miri error reporting) - rust-lang#104335 (Only do parser recovery on retried macro matching) - rust-lang#104394 (various cleanups to try to reduce the use of spans inside method resolution) - rust-lang#104459 (rustdoc: remove unused JS IIFE from main.js) - rust-lang#104462 (rustdoc: remove pointless CSS `.rightside { padding-right: 2px }`) - rust-lang#104466 (rustdoc: remove no-op CSS `#crate-search-div { display: inline-block }`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned oli-obk Nov 12, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 12, 2022

This comment has been minimized.

Sign in to view

RalfJung force-pushed the ctfe-error-reporting branch 3 times, most recently from 5618924 to 2068b3b Compare November 12, 2022 15:26

oli-obk reviewed Nov 14, 2022

View reviewed changes

compiler/rustc_mir_transform/src/const_prop_lint.rs Show resolved Hide resolved

RalfJung force-pushed the ctfe-error-reporting branch from 2068b3b to bcb08fb Compare November 14, 2022 20:07

RalfJung commented Nov 14, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the ctfe-error-reporting branch from bcb08fb to e58fdfc Compare November 14, 2022 20:31

This comment has been minimized.

Sign in to view

oli-obk reviewed Nov 15, 2022

View reviewed changes

RalfJung force-pushed the ctfe-error-reporting branch from e58fdfc to 97068b6 Compare November 15, 2022 09:06

This comment has been minimized.

Sign in to view

RalfJung mentioned this pull request Nov 15, 2022

Use only ty::Unevaluated<'tcx, ()> in type system #98588

Merged

oli-obk mentioned this pull request Nov 15, 2022

Feed a span into the const eval queries in case of an error #104438

Open

RalfJung force-pushed the ctfe-error-reporting branch from 97068b6 to 30e095d Compare November 15, 2022 11:13

RalfJung force-pushed the ctfe-error-reporting branch from 30e095d to e7d9981 Compare November 15, 2022 11:19

RalfJung commented Nov 15, 2022

View reviewed changes

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 15, 2022

RalfJung force-pushed the ctfe-error-reporting branch 3 times, most recently from 8cb93db to 5787421 Compare November 16, 2022 08:05

cleanup and dedupe CTFE and Miri error reporting

1115ec6

RalfJung force-pushed the ctfe-error-reporting branch from 5787421 to 1115ec6 Compare November 16, 2022 09:13

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 16, 2022

matthiaskrgr mentioned this pull request Nov 16, 2022

Rollup of 9 pull requests #104489

Closed

matthiaskrgr mentioned this pull request Nov 16, 2022

Rollup of 8 pull requests #104492

Merged

bors merged commit 353b915 into rust-lang:master Nov 16, 2022

rustbot added this to the 1.67.0 milestone Nov 16, 2022

RalfJung deleted the ctfe-error-reporting branch November 17, 2022 08:00

lcnr reviewed Nov 17, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup and dedupe CTFE and Miri error reporting #104317

cleanup and dedupe CTFE and Miri error reporting #104317

RalfJung commented Nov 12, 2022

rustbot commented Nov 12, 2022

This comment has been minimized.

rustbot commented Nov 12, 2022

oli-obk Nov 14, 2022

RalfJung Nov 14, 2022

RalfJung commented Nov 14, 2022

RalfJung Nov 14, 2022

This comment has been minimized.

This comment has been minimized.

oli-obk Nov 15, 2022

RalfJung Nov 15, 2022

oli-obk Nov 15, 2022

RalfJung commented Nov 15, 2022

This comment has been minimized.

oli-obk commented Nov 15, 2022

RalfJung commented Nov 15, 2022

rustbot commented Nov 15, 2022

RalfJung Nov 15, 2022

oli-obk commented Nov 15, 2022

oli-obk commented Nov 15, 2022

bors commented Nov 15, 2022

bors commented Nov 15, 2022

RalfJung commented Nov 16, 2022

bors commented Nov 16, 2022

RalfJung commented Nov 16, 2022

lcnr Nov 17, 2022

cleanup and dedupe CTFE and Miri error reporting #104317

cleanup and dedupe CTFE and Miri error reporting #104317

Conversation

RalfJung commented Nov 12, 2022

rustbot commented Nov 12, 2022

This comment has been minimized.

rustbot commented Nov 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Nov 14, 2022

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Nov 15, 2022

This comment has been minimized.

oli-obk commented Nov 15, 2022

RalfJung commented Nov 15, 2022

rustbot commented Nov 15, 2022

Choose a reason for hiding this comment

oli-obk commented Nov 15, 2022

oli-obk commented Nov 15, 2022

bors commented Nov 15, 2022

bors commented Nov 15, 2022

RalfJung commented Nov 16, 2022

bors commented Nov 16, 2022

RalfJung commented Nov 16, 2022

Choose a reason for hiding this comment