Clarify why InhabitedPredicate::instantiate_opt exists

[meithecatte]

At first glance, the extra casework seems pointless and needlessly error-prone. Clarify that there is a reason for it being there.

[rustbot]

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[BoxyUwU]

This really is quite error prone... It completely misses the OpaqueType variant which contains types/consts that can be instantiated 😬 Could you make the match in instantiate_opt exhaustive and bug! on OpaqueType cases? I don't think it should be reachable right now as we always wind up with opaque types being behind a GenericType variant for a type alias (which does handle instantiating correctly).

I also think the actual perf gain here isn't to do with not performing copies (those should be trivial performance wise as everything here is like 8 bytes at most since its mostly references or IDs), but more likely that we avoid re-allocating slices of predicates in cases where nothing was actually instantiated. e.g. Self::Or doesn't call into pred.or(other) unless one of the preds had parameters instantiated.

And, somewhat relatedly, could you modify the documentation on instantiate to say that it also evaluates some of the predicates to simpler forms, such as replacing Or/And of true/false with just a bare True/False predicate (I don't know why this is, perf maybe?).

[meithecatte]

I also think the actual perf gain here isn't to do with not performing copies (those should be trivial performance wise as everything here is like 8 bytes at most since its mostly references or IDs), but more likely that we avoid re-allocating slices of predicates in cases where nothing was actually instantiated. e.g. Self::Or doesn't call into pred.or(other) unless one of the preds had parameters instantiated.

Oh, that's what I meant by "recursively copying". Do you think that's not clear wording?

I don't think it should be reachable right now as we always wind up with opaque types being behind a GenericType variant for a type alias (which does handle instantiating correctly).

Hm, actually, why is that the case? I've played around with it a little and couldn't make it blow up, but I'd like to understand what's going on here.

And, somewhat relatedly, could you modify the documentation on instantiate to say that it also evaluates some of the predicates to simpler forms, such as replacing Or/And of true/false with just a bare True/False predicate (I don't know why this is, perf maybe?).

Well if substituting the generics lets you evaluate a child InhabitedPredicate to true/false, then the parent should simplify appropriately. If the predicate is built using the builders, then this doesn't happen outside of this case, so I don't think it's worth mentioning.

[meithecatte]

@rustbot review

[meithecatte]

Okay, I've figured out how to rewrite the comment and make it clearer. Still unsure about the bug! but I guess putting it in there is one way to find out whether it's actually unused.

[BoxyUwU]

Hm, actually, why is that the case? I've played around with it a little and couldn't make it blow up, but I'd like to understand what's going on here.

My understanding from looking at the users of instantiate is that its only really used when creating InhabitedPredicate corresponding to fields of ADTs that we have some generic arguments for. E.g.

struct Foo<T> { field: T }

given some Foo<u8> we'd want to check if u8 is inhabited, not some unknown type T. Afaict this happens by creating an InhabitedPredicate::GenericType(T) and instantiate T=u8.

So then the question becomes if you can write a field of a struct whose type is an opaque type (i.e. an impl Trait) so that when we make a InhabitedPredicate::OpaqueType(Opaque<T>) and then instantiate it we dont wind up instantiating T=u8.

I don't think this is possible as we don't support just writing field: impl Trait and using type_alias_impl_trait like:

type Foo<T> = impl Sized;
struct Bar<T> { field: Foo<T> }

Actually desugars to something like:

opaque type Opaque<T>: Sized;
type Foo<T> = Opaque<T>;
struct Bar<T> { field: Foo<T> }

Where the type of field isnt directly an opaque, but some type alias which is represented by InhabitedPredicate::GenericType(Foo<T>) which does handle instantiate correctly, resulting in an InhabitedPredicate::GenericType(Foo<u8>) which will later be converted to InhabitedPredicate::OpaqueType(Opaque<u8>) (note the correct generic arguments).

---

It's possible there is actually a way to get this codepath to be triggered but if it is then we should be handling it correctly instead of returning None. But I can't figure out a test case for it and wouldn't want to try and fix it "ahead of time" without a regression test. ICEing is a great way to get people to give you a test case 😅

[BoxyUwU]

@bors r+ rollup

thx for pr

[bors]

📌 Commit a3d63fb has been approved by BoxyUwU

It is now in the queue for this repository.