handle ConstValue::ByRef in relate #71018
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
eddyb
reviewed
Apr 11, 2020
eddyb
reviewed
Apr 11, 2020
Comment on lines
15
to
16
| = note: expected struct `Const<ByRef { alloc: Allocation { bytes: [3, 0, 0, 0, 0, 0, 0, 0], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [255], len: Size { raw: 8 } }, size: Size { raw: 8 }, align: Align { pow2: 3 }, mutability: Not, extra: () }, offset: Size { raw: 0 } }: [usize; 1]>` | ||
| found struct `Const<ByRef { alloc: Allocation { bytes: [4, 0, 0, 0, 0, 0, 0, 0], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [255], len: Size { raw: 8 } }, size: Size { raw: 8 }, align: Align { pow2: 3 }, mutability: Not, extra: () }, offset: Size { raw: 0 } }: [usize; 1]>` |
There was a problem hiding this comment.
I have a WIP branch where I make this print the actual values, I think we should wait for that, so that we can actually see it in action.
There was a problem hiding this comment.
🤷♂️ don't think that waiting is worth it if this is otherwise ready.
There was a problem hiding this comment.
It's not ready, because we can't read the error message.
There was a problem hiding this comment.
We already emit this error message rn. This PR only prevents correct programs from failing with it.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
That branch I mentioned is now a PR: #71232.
eddyb
reviewed
Apr 11, 2020
src/librustc_middle/ty/relate.rs
Outdated
Comment on lines
567
to
574
| let a_destructured = tcx.destructure_const(param_env.and(a)); | ||
| let b_destructured = tcx.destructure_const(param_env.and(b)); | ||
|
|
||
| // Both the variant and each field have to be equal. | ||
| if a_destructured.variant == b_destructured.variant { | ||
| for (a_field, b_field) in | ||
| a_destructured.fields.iter().zip(b_destructured.fields.iter()) | ||
| { | ||
| relation.consts(a_field, b_field)?; |
There was a problem hiding this comment.
@oli-obk @matthewjasper What do you think of this use of destructure_const?
There was a problem hiding this comment.
destructure_const panics if the type isn't an array, adt or tuple, so it looks incorrect to not have a check on the type.
There was a problem hiding this comment.
it is. breaks with #63322
currently working on this
There was a problem hiding this comment.
It should allow closures as well, IMO, although I'm not sure closures should be considered structurally matcheable...
There was a problem hiding this comment.
Should be fixed, don't know if there are other types which have ConstValue::ByRef and should work rn.
There was a problem hiding this comment.
Oh yeah you can't have closures in a const type, so you can mark closures as not structurally matcheable.
There was a problem hiding this comment.
Wait, hang on, it can't be ty::Closure, maybe you were still talking about dyn Trait.
There was a problem hiding this comment.
both examples in #63322 end up relying on &dyn Trait afaict
lcnr
force-pushed
the
custom-const-param
branch
3 times, most recently
from
56c3ec0
to
cb4ffda
Compare
eddyb
reviewed
Apr 11, 2020
lcnr
force-pushed
the
custom-const-param
branch
from
cb4ffda
to
c1757a8
Compare
eddyb
reviewed
Apr 11, 2020
eddyb
reviewed
Apr 11, 2020
src/librustc_middle/ty/relate.rs
Outdated
Comment on lines
582
to
584
| // FIXME(const_generics): There are probably some `TyKind`s | ||
| // which should be handled here. | ||
| _ => Err(TypeError::ConstMismatch(expected_found(relation, &a, &b))), |
There was a problem hiding this comment.
I can't think of any, at least out of the types we allow. You should use at least delay_span_bug, because returning Err indicates that they're definitely not equal (which may allow e.g. overlapping impls).
There was a problem hiding this comment.
👌 should relate_consts only be called for types which are structural match?
Is structural match implemented for &?
i.e. should this work
#![feature(const_generics)]
struct A<const N: &'static ()>;
fn main() {
let _ = A::<{&()}>;
}There was a problem hiding this comment.
Yes, it should, but it's blocked on one of @oli-obk's PRs.
eddyb
added
the
S-blocked
|
Blocking on #71018 (comment). |
lcnr
force-pushed
the
custom-const-param
branch
from
c1757a8
to
ebacb34
Compare
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 20, 2020
ty/print: pretty-print constant aggregates (arrays, tuples and ADTs). Oddly enough, we don't have any UI tests showing this off in types, only `mir-opt` tests. However, the pretty form should show up in the test output diff of rust-lang#71018, if this PR is merged first. <hr/> Examples of before/after: |`Option<bool>`| |:-:| |`{transmute(0x01): std::option::Option<bool>}`| | :sparkles: ↓↓↓ :sparkles: | |`std::option::Option::<bool>::Some(true)`| | `RawVec<u32>` | |:-:| | `ByRef { alloc: Allocation { bytes: [4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [65535], len: Size { raw: 16 } }, size: Size { raw: 16 }, align: Align { pow2: 3 }, mutability: Not, extra: () }, offset: Size { raw: 0 } }: alloc::raw_vec::RawVec::<u32>`| | :sparkles: ↓↓↓ :sparkles: | |`alloc::raw_vec::RawVec::<u32> { ptr: std::ptr::Unique::<u32> { pointer: {0x4 as *const u32}, _marker: std::marker::PhantomData::<u32> }, cap: 0usize, alloc: std::alloc::Global }`| <hr/> This PR is a prerequisite for rust-lang#61486, *sort of*, in that we need to be able to pretty-print values in order to even consider how we might mangle them. We still don't have pretty-printing for constants of reference types, @oli-obk has the necessary support logic in a PR but I didn't want to interfere with that. <hr/> Each commit should be reviewed separately, as I've fixed a couple deficiencies along the way. r? @oli-obk cc @rust-lang/wg-mir-opt @varkor @yodaldevoid
crlf0710
added
S-waiting-on-author
|
I think this is unblocked now since the fore-mentioned PR has been merged, maybe this will need a rebase? |
lcnr
force-pushed
the
custom-const-param
branch
from
ebacb34
to
8b0c6c4
Compare
|
Your PR failed (pretty log, 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 |
lcnr
force-pushed
the
custom-const-param
branch
from
8b0c6c4
to
b026d05
Compare
|
Should be ready for merge ✨ |
oli-obk
approved these changes
Apr 26, 2020
oli-obk
reviewed
Apr 26, 2020
| --> $DIR/different_byref.rs:8:9 | ||
| | | ||
| LL | x = Const::<{ [4] }> {}; | ||
| | ^^^^^^^^^^^^^^^^^^^ expected `3usize`, found `4usize` |
There was a problem hiding this comment.
Only mildly related to this PR
@rust-lang/wg-const-eval we could consider collecting all spans that lead to a value during const eval and thus allow each byte in the final constant to have its own spans for diagnostics. Then we can report a sort of backtrace for values. Though I fear that will cause quite some overhead, even if we only need to do it in the current crate and not serialize it to metadata (so ClearCrossCrate).
There was a problem hiding this comment.
That would be more advanced than what we do for type aliases, heh.
|
☔ The latest upstream changes (presumably #71707) made this pull request unmergeable. Please resolve the merge conflicts. |
lcnr
force-pushed
the
custom-const-param
branch
from
b026d05
to
a08bccb
Compare
oli-obk
added
S-waiting-on-review
eddyb
approved these changes
May 1, 2020
|
@bors r+ |
|
📌 Commit a08bccb has been approved by |
bors
added
S-waiting-on-bors
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 1, 2020
handle ConstValue::ByRef in relate fixes rust-lang#68615 r? @eddyb
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 1, 2020
handle ConstValue::ByRef in relate fixes rust-lang#68615 r? @eddyb
|
⌛ Testing commit a08bccb with merge b6e7b292fff458a6728a49e39a114a5b00678970... |
|
@bors retry yield |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 2, 2020
Rollup of 5 pull requests Successful merges: - rust-lang#71018 (handle ConstValue::ByRef in relate) - rust-lang#71758 (Remove leftover chalk types) - rust-lang#71760 (Document unsafety for `*const T` and `*mut T`) - rust-lang#71761 (doc: reference does not exist, probably a typo) - rust-lang#71762 (doc: this resulted in a link pointing to a non-existent target) Failed merges: - rust-lang#71726 (Suggest deref when coercing `ty::Ref` to `ty::RawPtr` with arbitrary mutability) r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #68615
r? @eddyb