Skip to content

feat: Compute closure captures #14470

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

Merged
merged 1 commit into from
Apr 10, 2023
Merged

feat: Compute closure captures #14470

merged 1 commit into from
Apr 10, 2023

Conversation

HKalbasi
Copy link
Member

@HKalbasi HKalbasi commented Apr 2, 2023

This PR:

  • Computes closure captures and the trait it implements (Fn, FnMut or FnOnce)
  • Computes data layout of closures
  • Adds support for closure MIR lowering
  • Changes the closure type display from |arg1: ty1, arg2: ty| -> ret to impl FnX(arg1: ty1, arg2: ty2) -> ret

fix #12297

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 2, 2023
@HKalbasi HKalbasi force-pushed the mir branch 3 times, most recently from e47135b to a611b26 Compare April 2, 2023 20:49
@HKalbasi
Copy link
Member Author

HKalbasi commented Apr 3, 2023

Is there a way in salsa to get a query result only if it is calculated before? Currently, the display code for closure calls db.infer, which works, but if we try to use it in the middle of infer (for example for print debugging, or due some future bug) we will get cycle panic, which is worse than fallback to a simpler display.

@bors
Copy link
Contributor

bors commented Apr 5, 2023

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

@HKalbasi HKalbasi force-pushed the mir branch 2 times, most recently from 136f1cd to ca56f2c Compare April 5, 2023 23:20
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

Haven't looked over everything yet, but as this is a rather big PR again, could you try splitting out the derive macro expansion change as its own PR? And if possible also the Forces reborrow of mutable references in method receiver, part

@bors
Copy link
Contributor

bors commented Apr 6, 2023

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

@lowr
Copy link
Contributor

lowr commented Apr 6, 2023

I think #12297 should be linked?

I don't have too strong opinion here, but I'd rather have inlay hints rendering for closures configurable. I pretty much agree with this comment in that I don't usually care what Fn* traits closures implement. Current pseudo-Rust notation |T, U| -> R is not valid Rust, but I find it concise which I value for stuffs shown in my editor all the time.

On the other hand, I'd love to see the information on hover!

@HKalbasi
Copy link
Member Author

HKalbasi commented Apr 6, 2023

Ah, I forgot that it was controversial last time. Will make it configurable.

bors added a commit that referenced this pull request Apr 6, 2023
Always reborrow mutable reference receiver in methods

Dependency of #14470
bors added a commit that referenced this pull request Apr 7, 2023
Add bounds for fields in derive macro

Dependency of #14470
@HKalbasi HKalbasi force-pushed the mir branch 3 times, most recently from 070a734 to 7bb6683 Compare April 7, 2023 22:33
@HKalbasi
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Apr 10, 2023

📌 Commit 59b6f2d has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 10, 2023

⌛ Testing commit 59b6f2d with merge 44cf8ef...

@bors
Copy link
Contributor

bors commented Apr 10, 2023

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing 44cf8ef to master...

@bors bors merged commit 44cf8ef into rust-lang:master Apr 10, 2023
@Veykril
Copy link
Member

Veykril commented Apr 11, 2023

This absolutely ruined type inference on self (unknown types)
image

@HKalbasi
Copy link
Member Author

Ah sorry, I should check these always. Will look at it.

@Veykril
Copy link
Member

Veykril commented Apr 11, 2023

Ah I think this is in part due to the dangling temporary Expr::Missing we are allocating now, though we do have some changes to the pattern types as well

@HKalbasi
Copy link
Member Author

Type inference for nested closures is broken it seems. Still looking.

@Veykril
Copy link
Member

Veykril commented Apr 11, 2023

Should we revert this PR for now (so you don't have the stress of having to fix this immediately?), and maybe just pick the hir-def pieces and what not that don't break things yet

@HKalbasi
Copy link
Member Author

Since this is big, and I have some uncommited changes on top of it, reverting it makes some git headache for me. I will file a revert tomorrow if the fix is not trivial.

bors added a commit that referenced this pull request Apr 11, 2023
Fix inference in nested closures

fix #14470 (comment)
@HKalbasi
Copy link
Member Author

@Veykril it looks like metric is now back, but I didn't change anything related to the dangling Missing. Was it just a guess, or did you see something about it?

@Veykril
Copy link
Member

Veykril commented Apr 11, 2023

Just an assumption thoug I mightve misunderstood parts of the code. Do note thoug that we are not fully back to prior this PR in terms on unknown types, so it would be nice to figure out what else regressed here.

@lnicola lnicola changed the title Compute closure captures feat: Compute closure captures Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

impl Fn, impl FnMut, ... as inlay hint for closures
5 participants