Stream: t-compiler

Topic: uninhabitedness check


mark-i-m (Apr 14 2020 at 17:49, on Zulip):

cc @eddyb @Nadrieril @centril

So I'm creating this to followup on https://github.com/rust-lang/rust/pull/71074#issuecomment-613423782

mark-i-m (Apr 14 2020 at 17:50, on Zulip):

That's what I have so far: master...Nadrieril:exhaustiveness-remove-tyerr. I've reduced the problem to a simpler case: now we only use tyerr in the case where a struct has private empty fields, because we don't want users outside the crate to know that the struct is uninhabited. (There's also something about non_exhaustive, but same thing here). The fix looks super easy: simply skip over those fields as if they never existed. But in practice there are so many places in the code where that would matter and I see no clean way of making sure that I get it right and that people in the future will get it right too ><. It's all the places where one of ty::Adt, Constructor::Variant/Simple or PatKind::Variant/Leaf appear, essentially. If anyone has an idea of how to get that right, I'm feeling stuck right now.

mark-i-m (Apr 14 2020 at 17:50, on Zulip):

does it make sense to wrap those structs with some other struct that forces you to decide whether to see fields or not?

centril (Apr 14 2020 at 17:54, on Zulip):

cc @varkor

Nadrieril (Apr 14 2020 at 18:00, on Zulip):

To illustrate, on this line: https://github.com/rust-lang/rust/blob/6805906fba0bca2bc77da9ad09cc9f91c3cea3eb/src/librustc_mir_build/hair/pattern/_match.rs#L2372 we have as input a pattern like SomeStruct { field2: Some(42), .. } and we need to essentially expand that into { field1: _, field2: Some(42) } while making sure to not include uninhabited private fields

Nadrieril (Apr 14 2020 at 18:02, on Zulip):

Or here https://github.com/rust-lang/rust/blob/6805906fba0bca2bc77da9ad09cc9f91c3cea3eb/src/librustc_mir_build/hair/pattern/_match.rs#L1002 we have to do the reverse: given a filtered list of patterns, we need to reconstruct a full PatKind by filling in the holes with wildcards of the correct type

Nadrieril (Apr 14 2020 at 20:20, on Zulip):

So I'm not sure what we'd have to wrap. Probably Pat ? But that would touch every single bit of the code. Maybe changing PatKind a bit could be enough, I don't know what else it's used for

mark-i-m (Apr 14 2020 at 22:53, on Zulip):

@Nadrieril So one question is: is this something all of those pieces of code should care about? if so, then maybe it actually makes sense to go change all of those lines of code.

Nadrieril (Apr 14 2020 at 23:04, on Zulip):

I don't really know how to answer that honestly. Changing all those lines, sure, but how to be sure we didn't miss any ?

Nadrieril (Apr 14 2020 at 23:07, on Zulip):

Actually, maybe PatKind::Variant could take some custom Fields struct instead of a Vec<Pat>...

Nadrieril (Apr 14 2020 at 23:08, on Zulip):

What else is Pat used for ? The scope of mir::hair::pattern looks rather limited so I might be able to change it

mark-i-m (Apr 16 2020 at 15:06, on Zulip):

Nadrieril said:

I don't really know how to answer that honestly. Changing all those lines, sure, but how to be sure we didn't miss any ?

Right, that's why I suggested changing the struct itself... it wouldn't compile until we had fixed all the places it's used... that said, I'm mostly speaking abstractly without much concrete knowledge beyond the examples you gave

mark-i-m (Apr 19 2020 at 18:15, on Zulip):

@Nadrieril Originally on the PR thread, @eddyb had suggested adding a field to Pat with the extra information. Does this end up having the same problem that we need to remember to check it?

eddyb (Apr 19 2020 at 18:16, on Zulip):

not struct Pat

eddyb (Apr 19 2020 at 18:16, on Zulip):

but the Wild variant of enum PatKind

eddyb (Apr 19 2020 at 18:16, on Zulip):

(or maybe even another variant, depending)

Nadrieril (Apr 19 2020 at 18:23, on Zulip):

I think the relevant info needs to live rather on the side where _match acts, not in Pat, so I was thinking of making ctor_wild_subpatterns be a special struct Fields rather than just a slice

mark-i-m (Apr 19 2020 at 22:52, on Zulip):

@Nadrieril Do you mean something like this:

struct Fields<'p, 'tcx> {
    fields: &'p [Pat<'tcx>],
}

impl<'p, 'tcx> Fields<'p, 'tcx> {
    /// Returns an iterator over all fields.
    pub fn iter_all(&self) -> impl Iterator<Item=Pat<'tcx>> {
        self.fields.iter()
    }

    /// Returns an iterator that hides uninhabitedness.
    pub fn iter_habited(&self) -> impl Iterator<Item=Pat<'tcx>> {
        self.fields.iter().filter(|pat| todo!())
    }
}
mark-i-m (Apr 19 2020 at 22:53, on Zulip):

And then force all users to pick one?

Nadrieril (Apr 19 2020 at 23:06, on Zulip):

Yeah something like that, but more principled so that users know when to use which

Nadrieril (Apr 19 2020 at 23:08, on Zulip):

I mean we'd find names for those methods that make it clearer what's happening

Nadrieril (Apr 19 2020 at 23:09, on Zulip):

Actually it's the reverse we want: store only the filtered list but have methods to reconstruct the full one. We need that because on Constructor::apply we only get back the filtered list

Nadrieril (Apr 19 2020 at 23:11, on Zulip):

Essentially there's a neat underlying idea to the whole file: you can split a value into a Constructor applied to some Fields. Right now we've provided Constructor and it's neat, but the other half is handled way more ad-hoc. Having Fields would fill that gap, and we'd be able to hide stuff like field filtering under that abstraction layer I hope

Nadrieril (Apr 19 2020 at 23:11, on Zulip):

I need to stare at the code some more though, all this is just off the top of my head

mark-i-m (Apr 23 2020 at 20:59, on Zulip):

@Nadrieril Hmm, I think I vaguely understand, though you might have to explain some of that to me a bit more. Is there anything I can help with?

Nadrieril (Apr 23 2020 at 22:13, on Zulip):

Thanks, I appreciate the offer :) I'm not really clear myself on what needs to be done though. I haven't had time to invest in this sadly, but when I do I'll be sure to let you know!

mark-i-m (May 01 2020 at 20:11, on Zulip):

@Nadrieril Do you mind if I take a stab at this? I would like to try to make progress on https://github.com/rust-lang/rust/pull/70551 which is blocked on de-abusing TyKind::Error... Do you mind if I ask you some questions?

Nadrieril (May 01 2020 at 20:14, on Zulip):

Sure, ask away :) I'll try to not take too long to answer

mark-i-m (May 01 2020 at 20:23, on Zulip):

Thanks @Nadrieril! Any suggestions for a starting point? So far I've define a Fields struct as above (but I will change it as you described). IIUC, I should change Constructor::apply to use Fields instead of Iterator<Pat>, and it should use the filtered list correct? Also can you point me to an example of constructing the full list so I can see how to do that?

mark-i-m (May 01 2020 at 21:44, on Zulip):

Oh, one other thing: as I've been looking at the code, I've found that sometimes we have owned Pats and sometimes we have references/slices of Pats on an arena... would it be possible to _always_ intern Pat in the arena and only deal with a thin handle (kind of like we do with ty::Ty?

mark-i-m (May 01 2020 at 22:09, on Zulip):

Looking a bit more, it looks like only the match-checking code uses an arena... would it be possible to convert the other uses of Pat to also use (a different) arena? We could also make PatKind be arena-allocated. Currently, we are using boxes...

Nadrieril (May 02 2020 at 00:51, on Zulip):

@mark-i-m About arenas, I honestly don't know. I'm guessing there aren't many places where we have a choice because any Pat we want to store in the matrix need to be arena-allocated. So anything in Fields will have to be in the arena for example. Would it simplify things if everything was in the arena ?

Nadrieril (May 02 2020 at 00:54, on Zulip):

Indeed Constructor::apply would take Fields as input. And wildcard_subpatterns would return Fields. Constructor::arity would need to be updated to take into account that we filtered some fields out

Nadrieril (May 02 2020 at 00:54, on Zulip):

I mean, start of course by just using Field in the right places and not filter out anything. We can add filtering later once we're sure everything still works

Nadrieril (May 02 2020 at 00:55, on Zulip):

Honestly I hope that's the right approach, I'm far from certain

Nadrieril (May 02 2020 at 00:57, on Zulip):

One of the trickiest bits I expected to be patterns_for_variant. That may force Fields to be a Vec<&'p Pat> instead of a slice. Or a Cow maybe

Nadrieril (May 02 2020 at 01:00, on Zulip):

Ah and I expect specialize_one_pattern to return Fields of course

Nadrieril (May 02 2020 at 01:01, on Zulip):

That's kind of the inverse of apply

Nadrieril (May 02 2020 at 01:04, on Zulip):

Ugh the comment there is a bit confused. I'm starting to think you might need to understand the actual algorithm to make this work ><

Nadrieril (May 02 2020 at 01:52, on Zulip):

I _think_ I've mentioned most of the functions that would need a change ? So maybe it's not that complicated

Nadrieril (May 02 2020 at 18:02, on Zulip):

@mark-i-m I should finally be free of stress on Tuesday. I had planned to work on this then, so if you prefer I'm happy to do it then, and then your PR can move forward

mark-i-m (May 02 2020 at 18:19, on Zulip):

@Nadrieril Thanks for the comments! I probably won't get to work a lot on this before Tuesday, so it might make more sense for you to go ahead. It would probably take me a while because I would need to go through and understand the code first. Still, I'm glad to help in any way I can.

Nadrieril (May 05 2020 at 21:20, on Zulip):

@mark-i-m https://github.com/rust-lang/rust/pull/71930

mark-i-m (May 06 2020 at 04:28, on Zulip):

Thanks @Nadrieril!

mark-i-m (May 14 2020 at 15:33, on Zulip):

@Nadrieril Btw, if you have time, would you be interested in writing up some documentation about pattern checking the rustc-dev-guide? https://rustc-dev-guide.rust-lang.org/pat-exhaustive-checking.html

Nadrieril (May 14 2020 at 19:46, on Zulip):

It's been on my todo list for a while yeah x)

Nadrieril (May 14 2020 at 19:47, on Zulip):

I'll try when I find some time

Nadrieril (May 14 2020 at 21:51, on Zulip):

I'll try when I find some time

Last update: May 29 2020 at 17:10UTC