Report const eval error inside the query #53821
Conversation
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/librustc/hir/def.rs
Outdated
| @@ -71,6 +71,7 @@ pub enum Def { | |||
| VariantCtor(DefId, CtorKind), // DefId refers to the enum variant | |||
| Method(DefId), | |||
| AssociatedConst(DefId), | |||
| Closure(hir::BodyId), | |||
There was a problem hiding this comment.
I know nothing about HIR. Who could review this part?
| LL | const B: i32 = (&A)[1]; | ||
| | ^^^^^^^^^^^^^^^-------^ | ||
| | | | ||
| | index out of bounds: the len is 0 but the index is 1 |
There was a problem hiding this comment.
Why do we error twice now for the same thing, where we only errored once before?
There was a problem hiding this comment.
I presume because the query is once called with Reveal::UserFacing and once with Reveal::All
There was a problem hiding this comment.
Is it a bug to not use the same Reveal each time? Doesn't this mean the constant is evaluated several times?
|
The part inside CTFE/miri looks good to me. I do not think I can adequately review the HIR changes though, so I'd prefer if you could get someone else to take a look at that. There are a huge amount of changes to the ui test output -- some error messages got deduplicated, but many others (and many more, I think) got duplicated. Why is that? Can you avoid that? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/librustc_mir/interpret/place.rs
Outdated
| Ok(MPlaceTy::from_aligned_ptr(ptr, layout)) | ||
| if layout.is_unsized() { | ||
| assert!(self.tcx.features().unsized_locals, "cannot alloc memory for unsized type"); | ||
| // allocate a fat pointer slot instead |
There was a problem hiding this comment.
Wait what? That seems wrong. We can have proper unsized locals, why should we have these indirections? Does that match how the MIR looks?
There was a problem hiding this comment.
I assumed we'd want to mirror rustc codegen
There was a problem hiding this comment.
I don't think so. We already have the ability to use an Operand::Indirect with a MemPlace to store the metadata -- that's like a properly typed version of a fat pointer.
| LL | bytes: [u8; std::mem::size_of::<Foo>()] | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| = note: ...which again requires const-evaluating + checking `Foo::bytes::{{constant}}`, completing the cycle | ||
| note: cycle used when processing `Foo` |
There was a problem hiding this comment.
Is it expected that "requires const-evaluating + checking Foo::bytes::{{constant}}" occurs twice at the end of this cycle?
...which requires const-evaluating + checking
Foo::bytes::{{constant}}...
...which again requires const-evaluating + checkingFoo::bytes::{{constant}}, completing the cycle
This comment has been minimized.
This comment has been minimized.
oli-obk
force-pushed
the
sanity_query
branch
from
f3dc952 to
60ccca3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
oli-obk
force-pushed
the
sanity_query
branch
from
0f52e18 to
4dd3169
Compare
This comment has been minimized.
This comment has been minimized.
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I've seen this failure before locally (like before this PR, it's not new). Rerunning the tests makes it go away. It's very frustrating. I'll open a tracking issue for it @bors retry |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 13d94ee with merge 2f4acffc0719c29ec3e74271ba8cb9db2b0b29ce... |
|
⌛ Testing commit 13d94ee with merge 10299b2bbc1302c51ae0668d542c7316f62b4382... |
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
What... was that? It started testing twice?!? Cc @rust-lang/infra @bors retry |
Report const eval error inside the query Functional changes: We no longer warn about bad constants embedded in unused types. This relied on being able to report just a warning, not a hard error on that case, which we cannot do any more now that error reporting is consistently centralized. r? @RalfJung fixes #53561
|
☀️ Test successful - status-appveyor, status-travis |
|
Perf results are mixed. Some big wins for incremental builds (as high as 27%), and some slowdowns of up to 3% for a few benchmarks, mostly |
|
We've seen these results before in the PR. attempts to track the regressions down were not very successful. I'll have another look |
Delayed CTFE backtraces This renames the env var that controls CTFE backtraces from `MIRI_BACKTRACE` to `RUST_CTFE_BACKTRACE` so that we can use `MIRI_BACKTRACE` in the miri tool to only show backtraces of the main miri execution. It also makes `RUST_CTFE_BACKTRACE` only show backtraces that actually get rendered as errors, instead of showing them eagerly when the `Err` happens. The current behavior is near useless in miri because it shows about one gazillion backtraces for errors that we later catch and do not care about. However, @oli-obk likes the current behavior for rustc CTFE work so it is still available via `RUST_CTFE_BACKTRACE=immediate`. NOTE: This is based on top of #53821. Only [the last three commits](oli-obk/rust@sanity_query...RalfJung:ctfe-backtrace) are new. Fixes #53355
Functional changes: We no longer warn about bad constants embedded in unused types. This relied on being able to report just a warning, not a hard error on that case, which we cannot do any more now that error reporting is consistently centralized.
r? @RalfJung
fixes #53561