This used to be EarlyBinder<Result<T, E>> in the return type of bound_return_position_impl_trait_in_trait_tys, but I changed it to Result<EarlyBinder<T>, E> to be more consistent with some of the other queries (like impl_trait_ref). Does it seem alright like this?

It doesn't matter. Whichever one's easier to use.

Not super happy with having to skip_binder here and then add it back in the decoding stage (cstore_impl.rs), but I had some trouble storing the EarlyBinder directly in the metadata table and this seemed like the simplest alternative

Yeah, I don't like this either. What trouble exactly were you having here?

Yeah it's during the encoding stage (record! call above). The query returns an EarlyBinder<&'tcx HashMap<..>>, but the type would be stored in the table as LazyValue<EarlyBinder<HashMap<..>>> (without the borrow inside the EarlyBinder). The record! macro above works automatically if the EarlyBinder isn't there, but if it is, it gives a type error about getting EarlyBinder<&HashMap> instead of EarlyBinder<HashMap>.

I'll play around with it a bit, I might just be missing an easy way to do this.

Ah, it's due to this signature of fn lazy<T: ParameterizedOverTcx, B: Borrow<T::Value<'tcx>>>(..). When the EarlyBinder isn't there, we have no trouble encoding HashMap<K, V> using a &HashMap<K, V>, but the early binder is messing up the deref I think...?

Anyways, maybe it's actually better to store the EarlyBinder inside of the values of the hashmap, i.e. make it a HashMap<DefId, EarlyBinder<Ty<'tcx>>>.

Thanks, that makes sense!

Sounds good, I'll try putting the EarlyBinder around the Ty. I was actually considering that initially, just wasn't sure if it was intentionally around the whole HashMap before. Do those two things convey slightly different meanings? EarlyBinder<HashMap<..>> suggests to me that there is one substs for all of the hashmap values, while HashMap<DefId, EarlyBinder<Ty>> suggests that each Ty could have a different substs. (But I'm not sure if that distinction actually matters in practice...)

Hmm could it maybe also work as Result<&'tcx EarlyBinder<HashMap<DefId, Ty<'tcx>>>, Error> instead of Result<EarlyBinder<&'tcx HashMap<DefId, Ty>>, Error>?

I think a HashMap<DefId, EarlyBinder<Ty<'tcx>>> is the best representation of the data here, and it probably simplifies the indexing that we do in project.rs if the values are wrapped in early binders instead of the whole hashmap.

Do those two things convey slightly different meanings?

Uh, it could I guess? It doesn't really matter in practice, yeah.

In my head, wrapping the whole HashMap in an EarlyBinder signifies that the whole hashmap should be substituted all at once, where we never do that in practice.

Ah okay thanks! I'll change it to that then 👍

You could probably just push this subst call into the "Ok" branch of the map_or_else, rather than wrapping the ty::Error in a binder.