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.

Sign up for GitHub

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

Jump to bottom

Make (try_)subst_and_normalize_erasing_regions take EarlyBinder #110297

Merged
merged 4 commits into from
May 8, 2023
Merged

Make (try_)subst_and_normalize_erasing_regions take EarlyBinder #110297

merged 4 commits into from
May 8, 2023

Conversation

kylematsuda
Copy link
Contributor

@kylematsuda kylematsuda commented Apr 13, 2023 •
edited
Loading

Changes subst_and_normalize_erasing_regions and try_subst_and_normalize_erasing_regions to take EarlyBinder<T> instead of T.

(related to #105779)

This was suggested by @BoxyUwU in #107753 (comment). After changing type_of to return EarlyBinder, there were several places where the binder was immediately skipped to call tcx.subst_and_normalize_erasing_regions, only for the binder to be reconstructed inside of that method.

r? @BoxyUwU

@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. labels Apr 13, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 13, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

jackh726
jackh726 reviewed Apr 14, 2023
View reviewed changes
src/tools/clippy/clippy_lints/src/methods/unnecessary_to_owned.rs
@@ -435,7 +435,7 @@ fn can_change_type<'a>(cx: &LateContext<'a>, mut expr: &'a Expr<'a>, mut ty: Ty<
let output_ty = fn_sig.output();
if output_ty.contains(*param_ty) {
if let Ok(new_ty) = cx.tcx.try_subst_and_normalize_erasing_regions(
new_subst, cx.param_env, output_ty) {
new_subst, cx.param_env, EarlyBinder(output_ty)) {
Copy link
Member

Choose a reason for hiding this comment

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

So, seems like the subst_identity().skip_binder() call to create this isn't correct...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yeah I agree that this looks weird now. Would it be better as skip_binder().skip_binder()? Or are you envisioning a larger change here?

Copy link
Member

Choose a reason for hiding this comment

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

I have to look a bit more at this. But likely the easiest thing is to either just leave it as-is with a comment, or map/skip_binder above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay thanks! I changed it to skip_binder() above in the most recent commit, but happy to try to improve it further.

Yeah, I also thought about mapping it, it seems like we always skip the inner Binder there but want to keep the EarlyBinder around, so could do something like

let fn_sig = cx.tcx.fn_sig(def_id).map_bound(|t| t.skip_binder());

(and then replace the calls to .inputs() with .skip_binder().inputs())

but I wasn't sure if this was more or less readable than how it is now

Copy link
Member

Choose a reason for hiding this comment

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

I think this PR doesn't necessarily need to fix this, at minimum we should probably open a clippy issue about it and add a FIXME before this PR lands

compiler/rustc_middle/src/ty/instance.rs Outdated Show resolved Hide resolved
@rustbot
Copy link
Collaborator

rustbot commented Apr 14, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@bors
Copy link
Contributor

bors commented Apr 21, 2023

☔ The latest upstream changes (presumably #96840) made this pull request unmergeable. Please resolve the merge conflicts.

@BoxyUwU
Copy link
Member

BoxyUwU commented May 6, 2023 •
edited
Loading

I will r+ this once its been rebased again, that subst_identity() has been changed to no_bound_vars().unwrap(), and a FIXME added to the sus subst_identity() in clippy. sorry for taking so long to look over this, thanks for doing this. I am happy to leave the bubbling of EarlyBinder with where it is in this PR and leave changing other places to future PRs

@kylematsuda
Copy link
Contributor Author

Hi @BoxyUwU, thanks for taking a look! Sorry, I've also been busy recently and sort of forgot about this PR 😅

I rebased and added the FIXME as you suggested, the no_bound_vars().unwrap() doesn't seem to work though unfortunately (see comment above)

@BoxyUwU
Copy link
Member

BoxyUwU commented May 7, 2023

@bors r+

@bors
Copy link
Contributor

bors commented May 7, 2023

📌 Commit d27f401 has been approved by BoxyUwU

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 May 7, 2023
@Dylan-DPC Dylan-DPC mentioned this pull request May 8, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 8, 2023
@bors
Auto merge of rust-lang#111342 - Dylan-DPC:rollup-b5p6wzy, r=Dylan-DPC
c86e7fb 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#110297 (Make `(try_)subst_and_normalize_erasing_regions` take `EarlyBinder`)
 - rust-lang#110827 (Fix lifetime suggestion for type aliases with objects in them)
 - rust-lang#111022 (Use smaller ints for bitflags)
 - rust-lang#111056 (Fix some suggestions where a `Box<T>` is expected.)
 - rust-lang#111262 (Further normalize msvc-non-utf8-ouput)
 - rust-lang#111265 (Make generics_of has_self on RPITITs delegate to the opaque)
 - rust-lang#111323 (Give a more helpful error when running the rustc shim directly)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 71a1ac2 into rust-lang:master May 8, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 8, 2023
@kylematsuda kylematsuda deleted the earlybinder_tcx_subst branch May 9, 2023 21:44
@kylematsuda kylematsuda mentioned this pull request May 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackh726 jackh726 jackh726 left review comments

@BoxyUwU BoxyUwU BoxyUwU left review comments

Assignees

@BoxyUwU BoxyUwU

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.
Projects
None yet
Milestone
1.71.0
Development

Successfully merging this pull request may close these issues.
5 participants
@kylematsuda @rustbot @bors @BoxyUwU @jackh726