Add `ConstKind::Error` and convert `ErrorHandled::Reported` to it. #71049

eddyb · 2020-04-12T03:40:36Z

By replicating the ty::Error approach to encoding "an error has occurred", all of the mechanisms that skip redundant/downstream errors are engaged and help out (see the reduction in test output).

This PR also adds ErrorHandled::Linted for the lint case because using ErrorHandled::Reported without having emitted an error that is guaranteed to stop compilation, is incorrect now.

r? @oli-obk cc @rust-lang/wg-const-eval @varkor @yodaldevoid

src/librustc_mir/interpret/memory.rs

RalfJung · 2020-04-12T07:11:08Z

This PR also adds ErrorHandled::Linted for the lint case because using ErrorHandled::Reported without having emitted an error that is guaranteed to stop compilation, is incorrect now.

Should there be a strongly worded comment somewhere to ensure this? What is the failure mode if we get this wrong (i.e., do we have a safety net that makes this ICE instead of silently doing the wrong thing)?

eddyb · 2020-04-12T11:37:49Z

Should there be a strongly worded comment somewhere to ensure this? What is the failure mode if we get this wrong (i.e., do we have a safety net that makes this ICE instead of silently doing the wrong thing)?

@mark-i-m has a series of PRs where using tcx.types.err is replaced with a wrapper that uses delay_span_bug and they're finding all sorts of places in the compiler that abused error types.

And if we do #69426, we can have ErrorHandled::Reported hold an ErrorReported token.
Actually, let me do that now, so it's not forgotten.

eddyb · 2020-04-12T15:58:59Z

@RalfJung Implemented the more explicit route and added a comment, let me know if it's better.

@varkor I won't remove it but F-const_generics may be misleading, this is useful with regular array lengths too (in fact, most of the test that have improved error output are like that).

varkor · 2020-04-12T16:00:40Z

@eddyb: that label is simply used to track what PRs have made progress on any issue related to const generics, even if the primary purpose is not necessarily for const generics. At least, that's how I've been using it :)

mark-i-m · 2020-04-12T20:21:28Z

cc #70866

src/librustc_infer/infer/resolve.rs

Centril · 2020-04-13T01:10:20Z

src/librustc_middle/ty/relate.rs

        if !x.val.needs_infer() {
            return x.eval(tcx, relation.param_env()).val;
        }
        x.val
    };

+    // FIXME(eddyb) doesn't look like everything below checks that `a.ty == b.ty`.
+    // We could probably always assert it early, as `const` generic parameters
+    // are not allowed to depend on other generic parameters, i.e. are concrete.


I think this temporary and won't always remain so, so let's not bake that assumption too deeply into the compiler.

We have already, this is nothing compared to everything else. It's only "temporary" in the sense that the first thing we stabilize won't have generic parameters in const parameter types.

Part of the problem is we'd need to stabilize something like StructuralMatch (some deep trait, unlike StructuralPartialEq/StructuralEq), because we can't just allow unrestricted types.
But even with that also it's a PITA to implement correctly, far more so than just concrete types.

Sure I understand all of this; I'm just saying we should avoid too deep of an architectural dependence on the fact that const params can't depend on type params so that it doesn't take a year to dig out of.

src/librustc_middle/ty/flags.rs

Centril · 2020-04-13T01:16:49Z

src/librustc_middle/mir/interpret/error.rs

-            err_inval!(TypeckError) => return Err(ErrorHandled::Reported),
+            err_inval!(TypeckError(error_reported)) => {
+                return Err(ErrorHandled::Reported(error_reported));
+            }
            // We must *always* hard error on these, even if the caller wants just a lint.
            err_inval!(Layout(LayoutError::SizeOverflow(_))) => true,


Hmm, this case doesn't early return so it results in Ok(_) and then report_as_lint will turn it into ErrorReported::Linted, which seems wrong?

cc @oli-obk @RalfJung

This function unfortunately became a mess over time. :/ I get the feeling we are trying to shove too many different things into one code path here.

I also do not have the slightest clue what that replace_span_with business in report_as_lint is even doing.

But SizeOverflow will be a hard error even when report_as_lint is called, so always returning Linted there seems wrong, superficially.

This function unfortunately became a mess over time. :/ I get the feeling we are trying to shove too many different things into one code path here.

My thoughts exactly.

Yea, so the must_error variable should be eliminated in favour of early returning here by calling report_as_error

I kept must_error but replaced Result<(), ErrorHandled> in the return type with ErrorHandled, let me know if that's enough.

that works, too, lgtm

Avoiding that boolean could have made the code cleaner... but maybe not, and anyway we can leave that to later.

src/librustc_middle/ty/context.rs

src/librustc_trait_selection/traits/error_reporting/mod.rs

RalfJung · 2020-04-13T09:06:57Z

src/librustc_middle/mir/interpret/error.rs

@@ -90,7 +81,7 @@ impl<'tcx> ConstEvalErr<'tcx> {

    pub fn report_as_error(&self, tcx: TyCtxtAt<'tcx>, message: &str) -> ErrorHandled {
        match self.struct_error(tcx, message, |mut e| e.emit()) {
-            Ok(_) => ErrorHandled::Reported,
+            Ok(_) => ErrorHandled::Reported(ErrorReported),


Is still seems rather easy to just say ErrorReported when the compiler says that that is the type you need. Maybe the only way to construct this should be something like ErrorReported::assert_error_reported_to_user or so? Could that constructor do delay_bug just to be extra safe or does that not make sense?

@RalfJung We're working on that but it's not ready yet because of some abuse in the compiler (of ty::Error which we wanted to add an ErrorReported to).

I tried to make this PR not blocked on that, but write ErrorReported or error_reported everywhere relevant so e.g. @mark-i-m can find it later.

This is mostly because I only did this PR as a prerequisite for #70970.
I am fine with blocking this PR on @mark-i-m's work if fixing #70942 is not a priority.

@eddyb sure, if improving this is on your list then that's fine for me. I was just pointing out that as the final solution this still seems suboptimal.

I was just pointing out that as the final solution this still seems suboptimal.

Indeed. Also, I should've pointed at #69426.

Having this PR will definitely make my next PR easier.

bors · 2020-04-15T00:52:44Z

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

oli-obk · 2020-04-15T16:15:47Z

r=me after a rebase

This (partially) reverts commit f47c4ff.

eddyb · 2020-04-16T16:03:39Z

@bors r=oli-obk rollup=never

bors · 2020-04-16T16:03:40Z

📌 Commit 77f38dc has been approved by oli-obk

Dylan-DPC-zz · 2020-04-16T18:02:51Z

@bors p=1

bors · 2020-04-17T11:52:08Z

⌛ Testing commit 77f38dc with merge 8d67f57...

bors · 2020-04-17T15:03:18Z

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 8d67f57 to master...

…-obk Detect mistyped associated consts in `Instance::resolve`. *Based on rust-lang#71049 to prevent redundant/misleading downstream errors.* Fixes rust-lang#70942 by refusing to resolve an associated `const` if it doesn't have the same type in the `impl` that it does in the `trait` (which we assume had errored, and `delay_span_bug` guards against bugs).

rust-highfive assigned oli-obk Apr 12, 2020

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

eddyb mentioned this pull request Apr 12, 2020

Detect mistyped associated consts in Instance::resolve. #70970

Merged

This comment has been minimized.

Sign in to view

eddyb force-pushed the const-err branch from fa31aa5 to 6deddda Compare April 12, 2020 04:17

RalfJung reviewed Apr 12, 2020

View reviewed changes

src/librustc_mir/interpret/memory.rs Show resolved Hide resolved

varkor added the F-const_generics `#![feature(const_generics)]` label Apr 12, 2020

eddyb force-pushed the const-err branch from 6deddda to 2f129cc Compare April 12, 2020 15:57

Centril reviewed Apr 13, 2020

View reviewed changes

src/librustc_infer/infer/resolve.rs Show resolved Hide resolved

Centril reviewed Apr 13, 2020

View reviewed changes

src/librustc_middle/ty/flags.rs Show resolved Hide resolved

Centril reviewed Apr 13, 2020

View reviewed changes

src/librustc_middle/ty/context.rs Show resolved Hide resolved

Centril reviewed Apr 13, 2020

View reviewed changes

src/librustc_trait_selection/traits/error_reporting/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Apr 13, 2020

View reviewed changes

eddyb force-pushed the const-err branch from 2f129cc to 5a977f8 Compare April 14, 2020 20:37

eddyb mentioned this pull request Apr 16, 2020

Do not ICE in the face of invalid enum discriminant #69903

Merged

eddyb force-pushed the const-err branch from 5a977f8 to 1e6f8c1 Compare April 16, 2020 15:13

eddyb added 4 commits April 16, 2020 19:00

ty: add ty::ConstKind::Error to replace tcx.consts.err.

2deb39d

Partially revert "Do not ICE in the face of invalid enum discriminant"

e22d479

This (partially) reverts commit f47c4ff.

mir/interpret: only use ErrorHandled::Reported for ErrorReported.

d7c4081

ty: convert ErrorHandled::Reported to ConstKind::Error.

77f38dc

eddyb force-pushed the const-err branch from 1e6f8c1 to 77f38dc Compare April 16, 2020 16:00

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 Apr 16, 2020

bors mentioned this pull request Apr 17, 2020

perf: Remove inline attribute from into_vec() #71204

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 17, 2020

bors merged commit 8d67f57 into rust-lang:master Apr 17, 2020

This was referenced Apr 17, 2020

Support const args in type dependent paths #71154

Closed

Make TLS accesses explicit in MIR #71192

Merged

Dogfood more or_patterns in the compiler #71231

Merged

eddyb deleted the const-err branch April 18, 2020 15:38

JohnTitor mentioned this pull request Apr 18, 2020

const generics: rustc panicked at 'already borrowed: BorrowMutError' #69654

Closed

eddyb mentioned this pull request Jun 18, 2020

Allow inference regions when relating consts #73225

Merged

varkor mentioned this pull request Jul 22, 2020

Tracking issue for const generics (RFC 2000) #44580

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ConstKind::Error` and convert `ErrorHandled::Reported` to it. #71049

Add `ConstKind::Error` and convert `ErrorHandled::Reported` to it. #71049

eddyb commented Apr 12, 2020 •

edited

Loading

This comment has been minimized.

RalfJung commented Apr 12, 2020

eddyb commented Apr 12, 2020

eddyb commented Apr 12, 2020

varkor commented Apr 12, 2020

mark-i-m commented Apr 12, 2020

Centril Apr 13, 2020

eddyb Apr 13, 2020

eddyb Apr 13, 2020

Centril Apr 13, 2020

Centril Apr 13, 2020 •

edited

Loading

eddyb Apr 13, 2020

RalfJung Apr 13, 2020

RalfJung Apr 13, 2020

Centril Apr 13, 2020

oli-obk Apr 14, 2020

eddyb Apr 14, 2020

oli-obk Apr 15, 2020

RalfJung Apr 15, 2020

RalfJung Apr 13, 2020

eddyb Apr 13, 2020

RalfJung Apr 13, 2020

eddyb Apr 13, 2020

mark-i-m Apr 13, 2020

bors commented Apr 15, 2020

oli-obk commented Apr 15, 2020

eddyb commented Apr 16, 2020

bors commented Apr 16, 2020

Dylan-DPC-zz commented Apr 16, 2020

bors commented Apr 17, 2020

bors commented Apr 17, 2020

Add ConstKind::Error and convert ErrorHandled::Reported to it. #71049

Add ConstKind::Error and convert ErrorHandled::Reported to it. #71049

Conversation

eddyb commented Apr 12, 2020 • edited Loading

This comment has been minimized.

RalfJung commented Apr 12, 2020

eddyb commented Apr 12, 2020

eddyb commented Apr 12, 2020

varkor commented Apr 12, 2020

mark-i-m commented Apr 12, 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

Centril Apr 13, 2020 • edited 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

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

bors commented Apr 15, 2020

oli-obk commented Apr 15, 2020

eddyb commented Apr 16, 2020

bors commented Apr 16, 2020

Dylan-DPC-zz commented Apr 16, 2020

bors commented Apr 17, 2020

bors commented Apr 17, 2020

Add `ConstKind::Error` and convert `ErrorHandled::Reported` to it. #71049

Add `ConstKind::Error` and convert `ErrorHandled::Reported` to it. #71049

eddyb commented Apr 12, 2020 •

edited

Loading

Centril Apr 13, 2020 •

edited

Loading