Switch to `EarlyBinder` for `collect_return_position_impl_trait_in_trait_tys` #110498

kylematsuda · 2023-04-18T18:22:02Z

Part of the work to finish #105779.

This PR adds EarlyBinder to the return type of the collect_return_position_impl_trait_in_trait_tys query and removes bound_return_position_impl_trait_in_trait_tys.

r? @lcnr

kylematsuda · 2023-04-18T18:25:04Z

compiler/rustc_hir_analysis/src/check/compare_impl_item.rs

@@ -579,7 +579,7 @@ fn compare_asyncness<'tcx>(
 pub(super) fn collect_return_position_impl_trait_in_trait_tys<'tcx>(
    tcx: TyCtxt<'tcx>,
    impl_m_def_id: LocalDefId,
-) -> Result<&'tcx FxHashMap<DefId, Ty<'tcx>>, ErrorGuaranteed> {
+) -> Result<ty::EarlyBinder<&'tcx FxHashMap<DefId, Ty<'tcx>>>, ErrorGuaranteed> {


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.

kylematsuda · 2023-04-18T18:28:25Z

compiler/rustc_metadata/src/rmeta/encoder.rs

@@ -1328,7 +1328,7 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
            if tcx.impl_method_has_trait_impl_trait_tys(def_id)
                && let Ok(table) = self.tcx.collect_return_position_impl_trait_in_trait_tys(def_id)
            {
-                record!(self.tables.trait_impl_trait_tys[def_id] <- table);
+                record!(self.tables.trait_impl_trait_tys[def_id] <- table.skip_binder());


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 👍

compiler-errors · 2023-04-18T20:05:33Z

compiler/rustc_trait_selection/src/traits/project.rs

+            .map_or_else(
+                |guar| ty::EarlyBinder(tcx.ty_error(guar)),
+                |tys| tys.map_bound(|tys| tys[&obligation.predicate.def_id]),
+            )
            .subst(tcx, impl_fn_substs),


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.

compiler-errors · 2023-04-18T22:27:39Z

@kylematsuda can you squash this into one commit? We don't need so many commits, especially for example 5c49e1e243cc745c86316ec39817f60de9f69c53 which undoes a commit that's lower in the stack.

After that I can approve. Thanks!

…in_trait_tys query; remove bound_X version

kylematsuda · 2023-04-18T23:10:01Z

Okay, squashed. Thanks!!

compiler-errors · 2023-04-18T23:11:13Z

r? @compiler-errors @bors r+ rollup

bors · 2023-04-18T23:11:15Z

📌 Commit 522bc5f has been approved by compiler-errors

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#110432 (Report more detailed reason why `Index` impl is not satisfied) - rust-lang#110451 (Minor changes to `IndexVec::ensure_contains_elem` & related methods) - rust-lang#110476 (Delay a good path bug on drop for `TypeErrCtxt` (instead of a regular delayed bug)) - rust-lang#110498 (Switch to `EarlyBinder` for `collect_return_position_impl_trait_in_trait_tys`) - rust-lang#110507 (boostrap: print output during building tools) - rust-lang#110510 (Fix ICE for transmutability in candidate assembly) - rust-lang#110513 (make `non_upper_case_globals` lint not report trait impls) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…tem-bounds, r=compiler-errors Switch to `EarlyBinder` for `explicit_item_bounds` Part of the work to finish rust-lang#105779. This PR adds `EarlyBinder` to the return type of the `explicit_item_bounds` query and removes `bound_explicit_item_bounds`. r? `@compiler-errors` (hope it's okay to request you, since you reviewed rust-lang#110299 and rust-lang#110498 😃)

rustbot assigned lcnr Apr 18, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 18, 2023

kylematsuda commented Apr 18, 2023

View reviewed changes

compiler-errors reviewed Apr 18, 2023

View reviewed changes

rustbot added the A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) label Apr 18, 2023

add EarlyBinder to return type of collect_return_position_impl_trait_…

522bc5f

…in_trait_tys query; remove bound_X version

kylematsuda force-pushed the earlybinder-rpitit-tys branch from 5c49e1e to 522bc5f Compare April 18, 2023 22:34

rustbot assigned compiler-errors and unassigned lcnr Apr 18, 2023

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 18, 2023

matthiaskrgr mentioned this pull request Apr 19, 2023

Rollup of 7 pull requests #110522

Merged

bors merged commit 9f0b16b into rust-lang:master Apr 19, 2023

rustbot added this to the 1.71.0 milestone Apr 19, 2023

kylematsuda deleted the earlybinder-rpitit-tys branch April 19, 2023 15:55

kylematsuda mentioned this pull request Apr 19, 2023

Switch to EarlyBinder for explicit_item_bounds #110556

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to `EarlyBinder` for `collect_return_position_impl_trait_in_trait_tys` #110498

Switch to `EarlyBinder` for `collect_return_position_impl_trait_in_trait_tys` #110498

kylematsuda commented Apr 18, 2023

kylematsuda Apr 18, 2023

compiler-errors Apr 18, 2023

kylematsuda Apr 18, 2023

compiler-errors Apr 18, 2023

kylematsuda Apr 18, 2023

compiler-errors Apr 18, 2023

kylematsuda Apr 18, 2023

kylematsuda Apr 18, 2023

compiler-errors Apr 18, 2023

compiler-errors Apr 18, 2023

kylematsuda Apr 18, 2023

compiler-errors Apr 18, 2023

compiler-errors commented Apr 18, 2023

kylematsuda commented Apr 18, 2023

compiler-errors commented Apr 18, 2023

bors commented Apr 18, 2023

Switch to EarlyBinder for collect_return_position_impl_trait_in_trait_tys #110498

Switch to EarlyBinder for collect_return_position_impl_trait_in_trait_tys #110498

Conversation

kylematsuda commented Apr 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Apr 18, 2023

kylematsuda commented Apr 18, 2023

compiler-errors commented Apr 18, 2023

bors commented Apr 18, 2023

Switch to `EarlyBinder` for `collect_return_position_impl_trait_in_trait_tys` #110498

Switch to `EarlyBinder` for `collect_return_position_impl_trait_in_trait_tys` #110498