Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to EarlyBinder for explicit_item_bounds #110556

Merged
merged 4 commits into from
Apr 25, 2023

Conversation

kylematsuda
Copy link
Contributor

Part of the work to finish #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 #110299 and #110498 😃)

@rustbot 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 19, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 19, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

…icit_item_bounds usages; remove bound_explicit_item_bounds query
…lyBinder; use this to simplify some EarlyBinder noise around explicit_item_bounds calls
@kylematsuda kylematsuda force-pushed the earlybinder-explicit-item-bounds branch from c7575e9 to f8a95a7 Compare April 20, 2023 19:03
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few more tiny comments, no really big problems

tcx.explicit_item_bounds(item_def_id).to_vec(),
tcx.explicit_item_bounds(item_def_id)
.subst_identity_iter_copied()
.collect::<Vec<_>>(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the turbofish necessary here? Alternatively, you could do something like subst_identity().to_vec(), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This maybe ties into a larger question (sorry for the long comment below):

Right now, EarlyBinder<T>::subst_identity has the bound T: TypeFoldable, so it's not actually available here. I was considering getting rid of the TypeFoldable bound but wasn't sure how important it is to how EarlyBinder should work -- i.e., right now you can only call subst_identity on things that you could also call subst on. (Maybe this property would come in handy in the future if doing larger refactorings?)

The lack of subst_identity is what motivated me to add subst_identity_iter_copied in this PR, as an identity version of subst_iter_copied.

Haha but now I'm worried that I'm overthinking and over-complicating this. If it's alright to do subst_identity on any inner type, then we don't have to add new methods on EarlyBinder in this PR, and every subst_identity_iter_copied would just become subst_identity().iter() (or in the case above, subst_identity().to_vec() as you mentioned). What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmmmmm.. maybe we could implement subst_identity on anything that implements TypeVisitable? We probably don't need it to be TypeFoldable, but I could potentially see us using some folder to make sure that it's actually valid to subst identity?

compiler/rustc_hir_typeck/src/generator_interior/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/context.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_transform/src/generator.rs Outdated Show resolved Hide resolved
@kylematsuda kylematsuda force-pushed the earlybinder-explicit-item-bounds branch from f8a95a7 to 5a69b5d Compare April 21, 2023 15:58
@kylematsuda
Copy link
Contributor Author

Thanks for your comments! I amended the last commit to change those skip_binder()s you pointed out.

Please see my response to your other comment, I had a question relating to it 😅

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now. Thanks!

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 25, 2023

📌 Commit 5a69b5d has been approved by compiler-errors

It is now in the queue for this repository.

@bors 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 25, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 25, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#110556 (Switch to `EarlyBinder` for `explicit_item_bounds`)
 - rust-lang#110615 (Add `impl_tag!` macro to implement `Tag` for tagged pointer easily)
 - rust-lang#110649 (Fix no_global_oom_handling build)
 - rust-lang#110671 (Consider polarity in new solver)
 - rust-lang#110783 (Fix ICE on --print=... i/o errors)
 - rust-lang#110796 (Updating Wake example to use new 'pin!' macro)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 297b222 into rust-lang:master Apr 25, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 25, 2023
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 5, 2023
…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 😃)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants