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

feat: Create an assist to convert closure to freestanding fn #17940

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

ChayimFriedman2
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 commented Aug 22, 2024

The assist converts all captures to parameters.

Closes #17920.

This was more work than I though, since it has to handle a bunch of edge cases...

Based on #17941. Needs to merge it first.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 22, 2024
@Veykril
Copy link
Member

Veykril commented Aug 22, 2024

Can you split the PR in two if possible? One for the assist and one for all the other stuff? 2k line diff isn't that nice to review, and I also generally dread reviewing assist PRs so that doesn't help 😅

Though I guess reviewing commit by commit is also an option, though I likely won't review all of it in one sitting.

@ChayimFriedman2
Copy link
Contributor Author

@Veykril Done (#17941).

@ChayimFriedman2 ChayimFriedman2 force-pushed the closure-to-fn branch 2 times, most recently from 0f05961 to 7ff7c2b Compare August 24, 2024 20:48
bors added a commit that referenced this pull request Aug 26, 2024
Preliminary work for #17940

I split the PR as requested, and made small commits.
capture.kind(),
capture_usage.is_ref(),
)
.clone_for_update();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more a question for @Veykril, but: is there an alternative to the clone_for_update call that could be used here or do we need to wait to wait for #15710 (comment) to happen first?

Copy link
Member

Choose a reason for hiding this comment

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

There are a couple, but they are all likewise kind of annoying to do ... I imagine until we get the node tracking stuff making this not use mutable nodes is gonna be ugly

crates/hir-ty/src/infer/closure.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/convert_closure_to_fn.rs Outdated Show resolved Hide resolved
capture.kind(),
capture_usage.is_ref(),
)
.clone_for_update();
Copy link
Member

Choose a reason for hiding this comment

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

There are a couple, but they are all likewise kind of annoying to do ... I imagine until we get the node tracking stuff making this not use mutable nodes is gonna be ugly

@ChayimFriedman2
Copy link
Contributor Author

@Veykril, addressed comments.

The assist converts all captures to parameters.
@Veykril
Copy link
Member

Veykril commented Aug 29, 2024

Thanks! Gonna be a very useful assist

@bors r+

@bors
Copy link
Contributor

bors commented Aug 29, 2024

📌 Commit 0e4f4d3 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 29, 2024

⌛ Testing commit 0e4f4d3 with merge 07a66c4...

@bors
Copy link
Contributor

bors commented Aug 29, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 07a66c4 to master...

@bors bors merged commit 07a66c4 into rust-lang:master Aug 29, 2024
11 checks passed
@ChayimFriedman2 ChayimFriedman2 deleted the closure-to-fn branch October 16, 2024 05:01
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.

Assist request: convert closure to fn
5 participants