Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ty/print: pretty-print constant aggregates (arrays, tuples and ADTs). #71232

Merged
merged 3 commits into from
Apr 20, 2020

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Apr 17, 2020

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 #71018, if this PR is merged first.


Examples of before/after:

Option<bool>
{transmute(0x01): std::option::Option<bool>}
✨ ↓↓↓ ✨
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>
✨ ↓↓↓ ✨
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 }

This PR is a prerequisite for #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.


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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 17, 2020
@RalfJung
Copy link
Member

How would {transmute(0x02): std::option::Option<bool>} be printed now?

@eddyb
Copy link
Member Author

eddyb commented Apr 17, 2020

@RalfJung Presumably std::option::Option::<bool>::None.

@RalfJung
Copy link
Member

std::option::Option::<bool>::None is not great though, this loses information.

@varkor varkor added the F-const_generics `#![feature(const_generics)]` label Apr 17, 2020
@eddyb
Copy link
Member Author

eddyb commented Apr 17, 2020

@RalfJung There's no information loss, strictly speaking (it can only be 2, anything else shouldn't make it into ty::Const - and I believe that destructure_const will ICE if it's greater than 2).

If you want the full dump of the value, we could gate that on e.g. -Zverbose. But it's not something we should expose to users who might be e.g. passing it to a const X: Option<bool> parameter.

- _3 = std::option::Option::<bool>::Some(const true,); // bb0[3]: scope 0 at $DIR/discriminant.rs:6:34: 6:44
+ _3 = const {transmute(0x01): std::option::Option<bool>}; // bb0[3]: scope 0 at $DIR/discriminant.rs:6:34: 6:44
- _3 = std::option::Option::<bool>::Some(const true); // bb0[3]: scope 0 at $DIR/discriminant.rs:6:34: 6:44
+ _3 = const std::option::Option::<bool>::Some(true); // bb0[3]: scope 0 at $DIR/discriminant.rs:6:34: 6:44
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, yeah I got confused by the double-diff here... makes sense.

@RalfJung
Copy link
Member

There's no information loss, strictly speaking (it can only be 2, anything else shouldn't make it into ty::Const - and I believe that destructure_const will ICE if it's greater than 2).

Eh, what? 2 is an invalid value for bool. It makes no sense for it to print None.
In the last PR someone, and I thought it was you (but maybe @oli-obk) argued against making it ICE on invalid representations. But it should either ICE or print without information loss.

@eddyb
Copy link
Member Author

eddyb commented Apr 17, 2020

@RalfJung You asked about Option<bool> (0->Some(false), 1->Some(true), 2->None) not bool, I don't understand why you bring up bool now.

@eddyb
Copy link
Member Author

eddyb commented Apr 17, 2020

In the last PR someone, and I thought it was you (but maybe @oli-obk) argued against making it ICE on invalid representations. But it should either ICE or print without information loss.

Ah, maybe it was me. If we could test it somehow, I'd be glad to make destructure_const return a Result and fall back to the transmute printout or w/e.

@RalfJung
Copy link
Member

Damn, somehow I thought 2 was an invalid value for this type.
I mean something like {transmute(0x10): std::option::Option<bool>}, then.

@eddyb
Copy link
Member Author

eddyb commented Apr 17, 2020

@RalfJung AFAIK, destructure_const would ICE on out of range discriminants (after all, it's implemented in miri, and previous to this PR, only used for constant value -> pattern conversion).

If we want to retain the transmute printout (which I am quite fond of), we should find some way to produce the value, though, otherwise it seems too easy to reintroduce an ICE here.

@RalfJung
Copy link
Member

I see. Miri itself won't ICE on bad discriminants, it will raise an InterpError. So in principle we would just have to forward that through the destructure_const query.

// NB: the `has_param_types_or_consts` check ensures that we can use
// the `destructure_const` query with an empty `ty::ParamEnv` without
// introducing ICEs (e.g. via `layout_of`) from missing bounds.
// E.g. `transmute([0usize; 2]): (u8, *mut T)` needs to know `T: Sized`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this happen at all? Const evaluation must already have had to know the type in order to produce a ConstValue. While the type may still be generic, in case of polymorphic const eval working out because of e.g. PhantomData<T> being a known ZST irrelevant of the type, any layout_of calls we do during destructure_const should also figure this out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*mut T has a known layout irrespective of T as long as T: Sized is in the ParamEnv, that's why I used it as an example. You can use size_of::<*mut T>() as an explicit discriminant of an enum, for example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but here we already have a ConstValue, which must have gone through some monomorphic evaluation at some point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, there is nothing requiring monomorphic const-eval, and I have this example to prove it (I made that to prove a point in #70453).

Copy link
Member Author

@eddyb eddyb Apr 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess your point is that the constant's type can't have type parameters? Sorry, I was getting turned around. const generics/array lengths shouldn't hit this.

So this could only be a problem with printing MIR after constant-folding something that uses type parameters but doesn't depend on them layout-wise.

Riiight, which is why I was thinking MIR printing could create a FmtPrinter with a ParamEnv.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 17, 2020

We can't really end up with such broken constants via the const_eval query, so I'm fine with ICEing if such constants exist, we can always fix the ICE if we end up encountering it in practice.

The PR is pretty neat. Quite verbose when compared with normal debug printing of certain types (e.g. Vec or Unique), but I guess in MIR details are more important than readability.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 17, 2020

📌 Commit eccb28e has been approved by oli-obk

@bors 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 17, 2020
@bors
Copy link
Contributor

bors commented Apr 20, 2020

⌛ Testing commit eccb28e with merge 4ca5fd2...

@bors
Copy link
Contributor

bors commented Apr 20, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 20, 2020
@bors bors merged commit 4ca5fd2 into rust-lang:master Apr 20, 2020
@eddyb eddyb deleted the print-const-adts branch April 27, 2020 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_generics `#![feature(const_generics)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants