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

Switch to EarlyBinder for fn_sig query #107055

Merged
merged 6 commits into from
Jan 27, 2023
Merged

Switch to EarlyBinder for fn_sig query #107055

merged 6 commits into from
Jan 27, 2023

Conversation

kylematsuda
Copy link
Contributor

@kylematsuda kylematsuda commented Jan 19, 2023 •
edited
Loading

Part of the work to finish #105779 (also see rust-lang/types-team#78).

Several queries X have a bound_X variant that wraps the output in EarlyBinder. This adds EarlyBinder to the return type of the fn_sig query and removes bound_fn_sig.

r? @lcnr

@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 Jan 19, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 19, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@kylematsuda
Copy link
Contributor Author

While doing this, I found several places where it looks like there is something to subst in scope, but we're currently not using it. I wasn't sure so I just put subst_identity for now, but tried to call those spots out in the comments above.

lcnr
lcnr reviewed Jan 19, 2023
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

i am ignoring subst_identity in clippy because most of them should actually use subst and fixing all of them is a lot of effort for this PR.

I think we should keep subst_identity in clippy for now and open an issue or PR there that subst_identity calls need triage as many of them are wrong.

compiler/rustc_borrowck/src/diagnostics/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_cranelift/src/main_shim.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/base.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/check/compare_impl_item.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/check/compare_impl_item.rs Outdated Show resolved Hide resolved
src/tools/clippy/clippy_lints/src/default_numeric_fallback.rs Show resolved Hide resolved
src/tools/clippy/clippy_lints/src/loops/needless_range_loop.rs Show resolved Hide resolved
src/tools/clippy/clippy_lints/src/map_unit_fn.rs Show resolved Hide resolved
src/tools/clippy/clippy_lints/src/methods/expect_fun_call.rs Show resolved Hide resolved
src/tools/clippy/clippy_lints/src/methods/expect_fun_call.rs Show resolved Hide resolved
@lcnr
Copy link
Contributor

lcnr commented Jan 19, 2023

ah, somehow your comments weren't shown when reviewing the files :/ that's annoying '^^

@kylematsuda
Copy link
Contributor Author

Ah sorry about that, that's probably my fault... I think I hit "Add single comment" instead of "Start review" or something when I posted them, oops.

Anyway, thanks so much for the detailed comments! I'll try to apply them and let you know if I have questions

@rustbot
Copy link
Collaborator

rustbot commented Jan 19, 2023

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@kylematsuda
Copy link
Contributor Author

Okay, I think I applied all of the comments except the clippy ones. I just added FIXME where you pointed out more extensive changes could be made, happy to try to address those here or in a future PR but just wanted to try to check off the easy things first

@bors
Copy link
Contributor

bors commented Jan 20, 2023

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

@kylematsuda
Copy link
Contributor Author

hi @lcnr, quick q: is it ok if I resolve the discussions for the changes I've made already? (Sorry if it's a silly question, I just don't know if I'm supposed to do that or if I'm supposed to let you do that 😅)

@lcnr
Copy link
Contributor

lcnr commented Jan 25, 2023

so for review discussions i prefer the following:

  • simple nit or suggestion: close if implemented, if you disagree, comment and keep it open
  • questions: after answering keep them open for the reviewer to close if they are happy with the answer
  • more involved changes from reviews: can keep them open and mention/explain your changes in the "hey, this is ready for review again" comment.

So I guess: if you expect the reviewer to also want to look at the discussion again, keep it open, if not, close it.

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Jan 25, 2023

📌 Commit 012ef4bb1ff633c841ac5a7e4645bac7ef46e058 has been approved by lcnr

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 Jan 25, 2023
@lcnr
Copy link
Contributor

lcnr commented Jan 25, 2023

might slightly impact perf, similar to #107055

@bors rollup=never

@bors
Copy link
Contributor

bors commented Jan 25, 2023

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

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 25, 2023
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 25, 2023
@rustbot rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Jan 27, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@kylematsuda
Copy link
Contributor Author

Thanks for the tips! I appreciate the advice 😃

Just rebased this, hopefully it's good to go again. Changes due to new usages of fn_sig are in the last commit in case you want to take a look

@lcnr
Copy link
Contributor

lcnr commented Jan 27, 2023

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jan 27, 2023

📌 Commit dc1216b has been approved by lcnr

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 27, 2023
@bors
Copy link
Contributor

bors commented Jan 27, 2023

⌛ Testing commit dc1216b with merge 7919ef0...

@bors
Copy link
Contributor

bors commented Jan 27, 2023

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 7919ef0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 27, 2023
@bors bors merged commit 7919ef0 into rust-lang:master Jan 27, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 27, 2023
@bors bors mentioned this pull request Jan 27, 2023
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7919ef0): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.4% [-1.4%, -1.4%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Feb 9, 2023
@bors
Auto merge of rust-lang#107055 - kylematsuda:eb-fn-sig, r=lcnr
7ef46c8 
Switch to `EarlyBinder` for `fn_sig` query

Part of the work to finish rust-lang#105779 (also see rust-lang/types-team#78).

Several queries `X` have a `bound_X` variant that wraps the output in [`EarlyBinder`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/subst/struct.EarlyBinder.html). This adds `EarlyBinder` to the return type of the `fn_sig` query and removes `bound_fn_sig`.

r? `@lcnr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

Assignees

@lcnr lcnr

Labels
merged-by-bors This PR was explicitly merged by bors. 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.
5 participants
@kylematsuda @rustbot @lcnr @bors @rust-timer