Closed lcnr closed 1 year ago
@rustbot claim
Hi @lcnr, I was able to check this out a bit and I think I understand what needs to be done here. Just to be sure: the queries that we want to change correspond to all of the bound_X
methods on TyCtx
?
I played around with this a bit on the fn_sig
query and was able to get x.py check
working again for the whole repo again after changing fn_sig
's return type to EarlyBinder<..>
, so I think I have an idea of how to get started on the other queries.
For some of the queries (namely fn_sig
, type_of
, impl_trait_ref
, and [explicit_]predicates_of
), it looks like the non-bound version is used in many more places than the bound version. I just wanted to double check that these queries should also be changed?
Yes that sounds correct to me.
There are quite a bit of places where we currently implicitly instantiate_identity
so this code adds some additional noise. I still think that it is worth it as it clarifies the intention here and sill requires people to quickly think whether they're in the right context.
When accessing parts of the return type of these queries which don't access generic parameters, e.g. the number of arguments or c-variadic for fn_sig
, and we're also not in the identity context, please use skip_binder
instead of instantiate_identity
. It's fine to get that wrong at some places as we should catch it during review. E.g. the following should use skip_binder()
as its talking about a fn_sig
from outside of the current item. Alternatively, add a method fn abi(self)
to EarlyBinder<ty::PolyFnSig<'tcx>>
.
Ok great, thanks for all the tips! I'll work on this and let you know if I get confused.
Hi, I’ve been playing with this and have been able to implement the changes to the queries for some of the most used ones (fn_sig
, type_of
, impl_trait_ref
, predicates_of
, …). I've checked that ui tests are passing.
I want to clean some things up and then open a PR, but I have a few questions:
EarlyBinder
represents an item that might have generics, but before early parameters are substituted. For example, consider a function fn foo<T>() { ... }
. If we are looking at a call to foo
like let x = foo<i32>()
, the type of T
is known to be i32
. In this case, we call EarlyBinder::subst
to set T = i32
. If instead we’re inside of the declaration of foo
, T
could be anything, so we substitute it with a placeholder type T
by using EarlyBinder::subst_identity
. When we make this “identity substitution”, we are in the identity context. In other words, we're in the identity context when we're trying to "look at" the item definition, not at any usage/call/instantiation. Is this roughly correct?EarlyBinder::skip_binder
would be implemented the same way as EarlyBinder::subst_identity
, but means something different: “I am explicitly ignoring the types of generic parameters” vs “I am substituting placeholder types for the generic parameters”. (Side note: can we call this skip_early_binder
instead of skip_binder
? Wondering if it is confusing with Binder::skip_binder
.)EarlyBinder
to the predicates_of
query? Currently, predicates_of
returns a GenericPredicates
, and GenericPredicates::instantiate_X
is almost always immediately called. From what I can tell, the instantiate_X
methods are doing conceptually the same thing as EarlyBinder::subst
(and even call it internally). After adding EarlyBinder
, we start getting things that look like tcx.predicates_of(..).subst_identity().instantiate_identity()
, or even tcx.predicates_of(..).subst_identity().instantiate(tcx, subst)
, which looks contradictory to me. Could predicates_of
be left as-is? We could also consider removing bound_predicates_of
, which is only used in 2 places in the code (although I don’t really understand what’s happening at either of those places).rustc_metadata
also be changed to use EarlyBinder
? I assume that the metadata is supposed to stay synced with the corresponding TyCtxt
queries, which would mean that these should be changed, but I wanted to check.EarlyBinder::subst_identity
, EarlyBinder::skip_early_binder
, etcX
to bound_X
, and add the necessary subst_identity
or skip_early_binder
callsX
return type to have EarlyBinder
; rename all usages of bound_X
to X
; update the types stored in the metadatabound_X
method from TyCtxt
Sorry in advance for so many questions :sweat_smile:. If this general plan sounds alright, I'll open a PR. Thanks!
sorry for not replying earlier. I have been on vacation.
1.i. mostly correct. EarlyBinder
is used for all generics of items except for late-bound lifetimes on functions, to ensure that we don't forget to provide generic arguments when using these items. In rustc
, we use ty::Param
(ty::ConstKind::Param
/ty::ReEarlyBound
) to represent both the universally bound variables, of for<T> fn foo<T>
whenever we use foo
, and the universal placeholder when we're inside of foo
, e.g. when typechecking that function.
This may not help too much, but ty::Param
is equivalent to either ty::Bound
or ty::Placeholder
depending on whether we're "outside" or "inside" of a given item. EarlyBinder
is used when we're outside of an item, and subst_identity
can be used while we're inside.
1.ii. yes, as ty::Param
is used to represent generic parameters, both when used as bound variables and when used as placeholders, subst_identity
is just a noop and there is only a conceptual difference between it and skip_binder
. EarlyBinder
and Binder
should ideally be the same thing, so I prefer using skip_binder
for both as it conceptually represents the same operation both times.
GenericPredicates
is interesting. In the past, EarlyBinder
was always implicit, so GenericPredicates
ended up having the same concept in instantiate
. Having EarlyBinder<GenericPredicates>
isn't really meaningful unless you directly access the predicates
field. It may make sense to keep using GenericPredicates
but change predicates
to be &'tcx [EarlyBinder<(Predicate<'tcx>, Span)>],
. Alternatively it's fine to ignore that query for now.
:thinking: yes, we should probably move those to EarlyBinder<T>
as well. This should not change the runtime behavior though as EarlyBinder
itself doesn't contain any data. You may have to implement some additional traits there or whatever, would have to check :grin:
iii. and iv. can be done in the same commit. Apart from that this seems like the best approach :+1:
Thanks for working on this :heart: it feels difficult to explain 1. so I hope that it made at least some sense. Feel free to reach out about that on zulip if you have any questions. Explaining it in sync is easier :grin:
Thanks so much for the detailed response! This helps a lot, I really appreciate it. (And sorry to have asked so many questions right before/during the holidays...:sweat_smile:). I should have some time this week to work on this :+1:
This has been implemented for const_param_default
and impl_trait_ref
by #106696. There are still other queries which need the same work.
@kylematsuda are you still working on this? going to unassign you but definitely don't hesitate to reassign yourself if you intend to continue working on it.
Thanks a lot for your good work here ❤️
Ah sorry @lcnr! I got a bit busy and dropped the ball on this... but I should have time to work on it more now. Thanks for the reminder :sweat_smile:
Looking in the nightly docs, here are the bound_X
queries that remain:
bound_abstract_const
(non-bound version is thir_abstract_const
)bound_explicit_item_bounds
bound_impl_subject
bound_return_position_impl_trait_in_trait_tys
(non-bound version is collect_return_position_impl_trait_in_trait_tys
)Also I think at some point @BoxyUwU had suggested changing subst_and_normalize_erasing_regions
to take EarlyBinder<T>
instead of T
, so I'll plan to do that too.
I have some questions about a few of these but I might just try and knock out the more straightforward-looking ones first.
@rustbot claim
After merging #111410, all of the existing bound_X
methods on tcx
have been removed. Thank you to everyone who helped review PRs, and especially @lcnr for mentoring this issue!
There is a bit of follow-up work that can be done (I'm happy to work on some of these if it would be helpful):
subst
instead of subst_identity
, and suggested opening an issue on ClippyEarlyBinder
: We started this a bit in #110297, but there might be more interfaces that should have EarlyBinder
added. Also, there might be other queries that logically should return EarlyBinder
, but don't have a bound_
version, so haven't been converted yetfor clippy I am not sure how helpful opening an issue like "hey, some of the subst_identity
calls may be wrong" would be. Might be good to add a bunch of FIXME
s to the code instead (or directly fix them when doing so). I don't have the time to do so myself atm.
Further bubbling up EarlyBinder
is good but probably can't be tracked in this issue
I think it would also be valuable to try to make the value in EarlyBinder
private and require users to explicitly use skip_binder
whenever they access the inner value.
implement https://github.com/rust-lang/types-team/issues/78