suppress errors in const eval during selection #81339

lcnr · 2021-01-24T14:52:46Z

needs MCP but was surprisingly simple to implement.

Do not emit errors during const eval when checking constants inside of the trait system.
We do this by adding a Reveal::Selection which behaves similar to Reveal::UserFacing but
supresses errors during const eval.
This allows coherence to work properly with feature(const_evaluatable_checked) and
improves the error location from abysmal to somewhat acceptable.

r? @oli-obk

rust-highfive · 2021-01-24T14:52:48Z

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

compiler/rustc_codegen_cranelift/src/constant.rs

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

bjorn3 · 2021-01-24T15:25:53Z

src/test/ui/const-generics/const_evaluatable_checked/from-sig-fail.stderr

   |
 LL | fn test<const N: usize>() -> [u8; N - 1] {
-   |                                   ^^^^^ attempt to compute `0_usize - 1_usize`, which would overflow
+   |                                   ----- required by this bound in `test`


This is an error quality regression.

I don't think so. We previously didn't even mention that the errors was caused by the test::<0>() call which is definitely an improvement.

We probably want to reevaluate the constant when emitting the selection error to actually emit why the evaluation failed. so that we get both error messages.

compiler/rustc_mir/src/const_eval/eval_queries.rs

lcnr · 2021-01-25T12:14:42Z

@bors try @rust-timer queue

rust-timer · 2021-01-25T12:14:43Z

Awaiting bors try build completion.

lcnr · 2021-01-25T12:14:48Z

@bors try-

edit: nm, we already use the Reveal::Selection path on stable with this PR, thought I had to first enable it

bors · 2021-01-25T12:14:56Z

⌛ Testing commit 33ee18a with merge 182a10535327b82daa53b34fcb510489ec7b7632...

bors · 2021-01-25T12:42:13Z

💔 Test failed - checks-actions

rust-log-analyzer · 2021-01-25T12:42:49Z

The job dist-x86_64-linux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking crates-io v0.31.1 (/tmp/cargo/crates/crates-io)
error[E0283]: type annotations needed
   --> src/cargo/util/config/de.rs:530:63
    |
530 |                 seed.deserialize(Tuple2Deserializer(1i32, env.as_ref()))
    |                                                           |   |
    |                                                           |   |
    |                                                           |   cannot infer type for type parameter `T` declared on the trait `AsRef`
    |                                                           this method call resolves to `&T`
    |
    = note: cannot satisfy `std::string::String: AsRef<_>`
help: use the fully qualified path for the potential candidates
    |
530 |                 seed.deserialize(Tuple2Deserializer(1i32, <std::string::String as AsRef<OsStr>>::as_ref(env)))
    |                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
530 |                 seed.deserialize(Tuple2Deserializer(1i32, <std::string::String as AsRef<std::path::Path>>::as_ref(env)))
    |                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
530 |                 seed.deserialize(Tuple2Deserializer(1i32, <std::string::String as AsRef<str>>::as_ref(env)))
    |                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
530 |                 seed.deserialize(Tuple2Deserializer(1i32, <std::string::String as AsRef<[u8]>>::as_ref(env)))

error: aborting due to previous error

For more information about this error, try `rustc --explain E0283`.

oli-obk · 2021-01-26T10:59:55Z

@bors try @rust-timer queue

rust-timer · 2021-01-26T10:59:56Z

Awaiting bors try build completion.

bors · 2021-01-26T11:00:06Z

⌛ Trying commit 33ee18a with merge 16305df2fb3846267c4f2537ae4d0f678e170259...

bors · 2021-01-26T12:00:44Z

☀️ Try build successful - checks-actions
Build commit: 16305df2fb3846267c4f2537ae4d0f678e170259 (16305df2fb3846267c4f2537ae4d0f678e170259)

rust-timer · 2021-01-26T12:00:45Z

Queued 16305df2fb3846267c4f2537ae4d0f678e170259 with parent a8f7075, future comparison URL.

@rustbot label: +S-waiting-on-perf

rust-timer · 2021-01-26T13:39:58Z

Finished benchmarking try commit (16305df2fb3846267c4f2537ae4d0f678e170259): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

bjorn3 · 2021-01-26T13:54:41Z

Regressions of up to 2.4% on ctfe-stress incr-full and up to 1.7% otherwise. For full the max regression is 0.3%, for incr-full excluding ctfe-stress 1.0%, for incr-unchanged up to 1.2% and up to 1.7% for incr-patched.

RalfJung · 2021-01-26T13:55:18Z

compiler/rustc_mir/src/const_eval/eval_queries.rs

+            let mut key = key;
+            key.param_env = key.param_env.with_reveal_selection();
+            match tcx.eval_to_const_value_raw(key) {
+                // try again with reveal all as requested


This will try again with reveal "user facing" though, won't it?

yeah, didn't update all the comments yet. Opened this case I wanted other people to look at this PR in case there's a fundamental issue I've missed.

RalfJung · 2021-01-26T13:56:39Z

compiler/rustc_mir/src/const_eval/eval_queries.rs

-        let mut key = key;
-        key.param_env = key.param_env.with_user_facing();
-        match tcx.eval_to_allocation_raw(key) {
-            // try again with reveal all as requested


You lost this comment.

Also, the Reveal logic here is an exact duplicate of the one above, isn't it? By now it looks non-trivial enough to justify doing some refactoring to share it.

RalfJung · 2021-01-26T13:57:10Z

compiler/rustc_mir/src/const_eval/eval_queries.rs

+                match error.kind {
+                    err_inval!(Layout(LayoutError::Unknown(_)))
+                    | err_inval!(TooGeneric)
+                    | err_inval!(AlreadyReported(_)) => {}


Why are those exact three treated differently here?

That will cause some new issues though -- it will be much harder to avoid printing the same const error multiple times.

when const eval prints an error the InterpError is AlreadyReported. So we don't return Silent here to prevent emitting the error twice.

err_inval!(Layout(LayoutError::Unknown(_))) | err_inval!(TooGeneric) because these result in ErrorHandled::TooGeneric which we still want to emit

Probably needs a comment as well ^^

You are quoting me entirely out of context here.^^ In that quote I was referring to "have the query return a Result".

when const eval prints an error the InterpError is AlreadyReported. So we don't return Silent here to prevent emitting the error twice.

Well but it means we hit the code path below for reporting it? Or is report_as_lint et al silent for AlreadyReported?

err_inval!(Layout(LayoutError::Unknown(_))) | err_inval!(TooGeneric) because these result in ErrorHandled::TooGeneric which we still want to emit

Oh we do? I thought TooGeneric should never be emitted, it means "please re-evaluate with RevealAll?

RalfJung · 2021-01-26T14:06:37Z

If we stick the actual error into the Silent variant, then we don't have to re-run. Not sure if this rerunning has an impact on perf

Yeah, that would definitely be the much more sensible approach: have the query return a Result. The alternative of evaluating twice but being silent the first time is... rather silly, to be honest.^^

The question is if we can find a nice error type to put into that Result. Using literally InterpResult is... painful at best. Is there some stringified representation we can use? We need not just an error message though, but also the backtrace.

RalfJung · 2021-01-26T14:07:31Z

That (have the query return a Result) will cause some new issues though -- it will be much harder to avoid printing the same const error multiple times.

lcnr · 2021-01-26T14:20:45Z

The alternative of evaluating twice but being silent the first time is... rather silly, to be honest.^^

I think that's fine as we only have to evaluate twice when failing the compilation (ignoring that we sometimes only lint instead of emitting an error rn) which tbh seems fine and it does avoid having to deal with "Using literally InterpResult is... painful at best."

oli-obk · 2021-01-26T14:37:19Z

Exactly 50% more executions of eval_to_const_value_raw, which totally makes sense if lots of the executions are with ParamEnv::RevealAll, since we now do a third execution of this query, even if the actual execution time is deduplicated sometimes (not so sure about the last part, since we also have 50% actual runtime regression on the query itself).

All in all, the perf regression is absolutely benign, mostly below 1% and only for the CTFE stress test we reach 2.4%.

But I think there are multiple ways we can think about improving the situation. One thing we need to look into is the metadata encoding of the queries. Since most of the time the content is the same, we need to check whether we only encode the content once.

The other thing is figuring out more ways to ensure we don't re-run a build if not absolutely necessary. Multiple questions here:

Do we need to have access to these query calls from other crates?
Can we forward the errors from just the Reveal::Selection but not from others?
At this point, should we just have separate queries? or do we need the param env for transitivity

RalfJung · 2021-01-26T14:48:40Z

I think that's fine as we only have to evaluate twice when failing the compilation

Yeah, that's fair... though having 1 query turn into 3 nested queries is still pretty heavy. Queries are rather expensive. And doesn't also triple the cache?

lcnr · 2021-02-13T09:50:32Z

Do we need to have access to these query calls from other crates?

think so yeah, we have to try evaluating consts from other crates with const_evaluatable_checked.

Can we forward the errors from just the Reveal::Selection but not from others?

Not exactly sure what you mean here. I guess this is about returning an optional InterpError from calls with Reveal::Selection and just emitting the error in Reveal::UserFacing without reevaluating the constant.

At this point, should we just have separate queries? or do we need the param env for transitivity

I am not sure if having separate queries would be useful. We still need to pass in the param env so that we can normalize projections during const eval. And even if we were to not add Reveal::Selection and instead add a separate query for that we still want to first try that query before running const eval with Reveal::UserFacing.

Yeah, that's fair... though having 1 query turn into 3 nested queries is still pretty heavy. Queries are rather expensive. And doesn't also triple the cache?

Since most of the time the content is the same, we need to check whether we only encode the content once.

That would be good i think, don't know how easy this is to implement. Opened a zulip topic for this: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/deduplicate.20query.20cache

oli-obk · 2021-02-13T12:46:56Z

Not exactly sure what you mean here. I guess this is about returning an optional InterpError from calls with Reveal::Selection and just emitting the error in Reveal::UserFacing without reevaluating the constant.

yes, exactly that.

Do we need to have access to these query calls from other crates?

think so yeah, we have to try evaluating consts from other crates with const_evaluatable_checked.

Many times we'll be using different generics to do that, so we'd be recomputing anyway. Of course if that constant is very expensive to compute, then that's a different story... Oh god, we could make the decision for whether to cache the result depend on the evaluation's instruction counter

const_evaluatable_checked: Stop eagerly erroring in `is_const_evaluatable` Fixes rust-lang#82279 We don't want to be emitting errors inside of is_const_evaluatable because we may call this during selection where it should be able to fail silently There were two errors being emitted in `is_const_evaluatable`. The one causing the compile error in rust-lang#82279 was inside the match arm for `FailureKind::MentionsParam` but I moved the other error being emitted too since it made things cleaner imo The `NotConstEvaluatable` enum \*should\* have a fourth variant for when we fail to evaluate a concrete const, e.g. `0 - 1` but that cant happen until rust-lang#81339 cc `@oli-obk` `@lcnr` r? `@nikomatsakis`

lcnr · 2021-07-08T20:11:37Z

don't have the capacity to work on this rn

lcnr added 2 commits January 24, 2021 15:48

add new Reveal for const eval during selection

93a73da

update tests

9ee0d80

rust-highfive assigned oli-obk Jan 24, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2021

lcnr added A-const-generics Area: const generics (parameters and arguments) F-generic_const_exprs `#![feature(generic_const_exprs)]` labels Jan 24, 2021

bjorn3 reviewed Jan 24, 2021

View reviewed changes

compiler/rustc_codegen_cranelift/src/constant.rs Outdated Show resolved Hide resolved

typo

33ee18a

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

bjorn3 reviewed Jan 24, 2021

View reviewed changes

oli-obk reviewed Jan 25, 2021

View reviewed changes

compiler/rustc_mir/src/const_eval/eval_queries.rs Show resolved Hide resolved

bors mentioned this pull request Jan 25, 2021

Make more traits of the From/Into family diagnostic items #81277

Merged

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 26, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 26, 2021

RalfJung reviewed Jan 26, 2021

View reviewed changes

lcnr changed the title ~~const eval sfinae xd~~ surpress errors in const eval during selection Jan 26, 2021

oli-obk changed the title ~~surpress errors in const eval during selection~~ supress errors in const eval during selection Feb 13, 2021

oli-obk changed the title ~~supress errors in const eval during selection~~ suppress errors in const eval during selection Feb 13, 2021

oli-obk mentioned this pull request Feb 22, 2021

Perform WF-check on types with no type parameters #69741

Closed

BoxyUwU mentioned this pull request Mar 2, 2021

const_evaluatable_checked: Stop eagerly erroring in is_const_evaluatable #82707

Merged

lcnr closed this Jul 8, 2021

lcnr mentioned this pull request Jul 9, 2021

don't eagerly emit errors when evaluating constants during selection #87001

Open

b-naber mentioned this pull request Jan 7, 2022

Suppress errors in const eval during trait selection #92653

Closed

lcnr mentioned this pull request Jan 25, 2022

Suppress errors in const eval during trait selection v2 #92674

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

suppress errors in const eval during selection #81339

suppress errors in const eval during selection #81339

lcnr commented Jan 24, 2021 •

edited

Loading

rust-highfive commented Jan 24, 2021

bjorn3 Jan 24, 2021

lcnr Jan 24, 2021 •

edited

Loading

lcnr commented Jan 25, 2021

rust-timer commented Jan 25, 2021

lcnr commented Jan 25, 2021 •

edited

Loading

bors commented Jan 25, 2021

bors commented Jan 25, 2021

rust-log-analyzer commented Jan 25, 2021

oli-obk commented Jan 26, 2021

rust-timer commented Jan 26, 2021

bors commented Jan 26, 2021

bors commented Jan 26, 2021

rust-timer commented Jan 26, 2021

rust-timer commented Jan 26, 2021

bjorn3 commented Jan 26, 2021

RalfJung Jan 26, 2021

lcnr Jan 26, 2021

RalfJung Jan 26, 2021

RalfJung Jan 26, 2021

lcnr Jan 26, 2021

RalfJung Jan 26, 2021

RalfJung Jan 26, 2021

RalfJung commented Jan 26, 2021

RalfJung commented Jan 26, 2021 •

edited

Loading

lcnr commented Jan 26, 2021 •

edited

Loading

oli-obk commented Jan 26, 2021

RalfJung commented Jan 26, 2021

lcnr commented Feb 13, 2021

oli-obk commented Feb 13, 2021

lcnr commented Jul 8, 2021

suppress errors in const eval during selection #81339

suppress errors in const eval during selection #81339

Conversation

lcnr commented Jan 24, 2021 • edited Loading

rust-highfive commented Jan 24, 2021

Choose a reason for hiding this comment

lcnr Jan 24, 2021 • edited Loading

Choose a reason for hiding this comment

lcnr commented Jan 25, 2021

rust-timer commented Jan 25, 2021

lcnr commented Jan 25, 2021 • edited Loading

bors commented Jan 25, 2021

bors commented Jan 25, 2021

rust-log-analyzer commented Jan 25, 2021

oli-obk commented Jan 26, 2021

rust-timer commented Jan 26, 2021

bors commented Jan 26, 2021

bors commented Jan 26, 2021

rust-timer commented Jan 26, 2021

rust-timer commented Jan 26, 2021

bjorn3 commented Jan 26, 2021

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

RalfJung commented Jan 26, 2021

RalfJung commented Jan 26, 2021 • edited Loading

lcnr commented Jan 26, 2021 • edited Loading

oli-obk commented Jan 26, 2021

RalfJung commented Jan 26, 2021

lcnr commented Feb 13, 2021

oli-obk commented Feb 13, 2021

lcnr commented Jul 8, 2021

lcnr commented Jan 24, 2021 •

edited

Loading

lcnr Jan 24, 2021 •

edited

Loading

lcnr commented Jan 25, 2021 •

edited

Loading

RalfJung commented Jan 26, 2021 •

edited

Loading

lcnr commented Jan 26, 2021 •

edited

Loading