Skip to content

feat: Compute closure captures#14470

Merged
bors merged 1 commit intorust-lang:masterfrom
HKalbasi:mir
Apr 10, 2023
Merged

feat: Compute closure captures#14470
bors merged 1 commit intorust-lang:masterfrom
HKalbasi:mir

Conversation

@HKalbasi
Copy link
Member

@HKalbasi HKalbasi commented Apr 2, 2023 •
edited
Loading

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
Veykril
Veykril reviewed Apr 6, 2023
View reviewed changes
Copy link
Member

@Veykril Veykril left a comment •
edited
Loading

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

@HKalbasi HKalbasi mentioned this pull request Apr 6, 2023
Merged
@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 •
edited
Loading

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
@bors
Auto merge of #14511 - HKalbasi:dev, r=Veykril
e739c99 
Always reborrow mutable reference receiver in methods

Dependency of #14470
@HKalbasi HKalbasi mentioned this pull request Apr 6, 2023
Merged
bors added a commit that referenced this pull request Apr 7, 2023
@bors
Auto merge of #14521 - HKalbasi:dev, r=Veykril
937ef97 
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 HKalbasi mentioned this pull request Apr 10, 2023
Closed
@HKalbasi
Copy link
Member Author

HKalbasi commented Apr 10, 2023

@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
@bors bors mentioned this pull request Apr 10, 2023
Merged
@Veykril
Copy link
Member

Veykril commented Apr 11, 2023 •
edited
Loading

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

@HKalbasi
Copy link
Member Author

HKalbasi commented Apr 11, 2023

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

@Veykril
Copy link
Member

Veykril commented Apr 11, 2023 •
edited
Loading

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

HKalbasi commented Apr 11, 2023

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

HKalbasi commented Apr 11, 2023

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.

@HKalbasi HKalbasi mentioned this pull request Apr 11, 2023
Merged
bors added a commit that referenced this pull request Apr 11, 2023
@bors
Auto merge of #14550 - HKalbasi:mir, r=HKalbasi
600283f 
Fix inference in nested closures

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

HKalbasi commented Apr 11, 2023

@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 •
edited
Loading

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
@HKalbasi HKalbasi mentioned this pull request Apr 13, 2023
Closed
@HKalbasi HKalbasi mentioned this pull request May 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Veykril Veykril Veykril left review comments

Assignees

No one assigned

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@HKalbasi @bors @lowr @Veykril @rustbot