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

minor : Deunwrap inline call #15432

Merged
merged 4 commits into from
Sep 22, 2023
Merged

Conversation

alibektas
Copy link
Member

#15398 subtask 4. There is still one instance of unwrap, which I found pretty hard to change.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 9, 2023
@alibektas alibektas changed the title Deunwrap inline call minor : Deunwrap inline call Aug 10, 2023
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.

So this I'm not okay with, this change effectively makes this entire assist non-lazy and it runs whenever the cursor is on a function call. Its not particularly cheap

crates/ide-assists/src/handlers/inline_call.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/inline_call.rs Outdated Show resolved Hide resolved
@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2023
@lnicola lnicola mentioned this pull request Sep 9, 2023
15 tasks
@alibektas
Copy link
Member Author

So while I do not claim to understand why the assist became non-lazy after this PR, I applied the requested changes hoping that these also resolve the said problem.

@@ -218,13 +219,12 @@ pub(crate) fn inline_call(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<
}

let syntax = call_info.node.syntax().clone();
let replacement = inline(&ctx.sema, file_id, function, &fn_body, &params, &call_info)?;
Copy link
Member

Choose a reason for hiding this comment

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

So while I do not claim to understand why the assist became non-lazy after this PR

Assists run in two modes: once to determine if they are available, and once to actually apply the changes, if the user picks one. Both modes use mostly the same code paths, but the |builder| closure actually determines the changes. This means that everything that happens outside that closure is performance-sensitive.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for the explanation

@lnicola
Copy link
Member

lnicola commented Sep 9, 2023

CI probably needs a cargo fmt on 1.72.

@alibektas
Copy link
Member Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 15, 2023
@Veykril
Copy link
Member

Veykril commented Sep 22, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 22, 2023

📌 Commit 893e191 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 22, 2023

⌛ Testing commit 893e191 with merge 758b5e4...

bors added a commit that referenced this pull request Sep 22, 2023
minor : Deunwrap inline call

#15398 subtask 4. There is still one instance of unwrap, which I found pretty hard to change.
@bors
Copy link
Contributor

bors commented Sep 22, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 758b5e4 to master...

@bors
Copy link
Contributor

bors commented Sep 22, 2023

👀 Test was successful, but fast-forwarding failed: 422 1 review requesting changes by reviewers with write access.

@Veykril
Copy link
Member

Veykril commented Sep 22, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 22, 2023

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: Refactor/fix clippy lints #15615

@bors
Copy link
Contributor

bors commented Sep 22, 2023

📌 Commit 893e191 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 22, 2023

⌛ Testing commit 893e191 with merge 2ededa2...

@bors
Copy link
Contributor

bors commented Sep 22, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 2ededa2 to master...

@bors bors merged commit 2ededa2 into rust-lang:master Sep 22, 2023
9 checks passed
@alibektas alibektas deleted the deunwrap/inline_call branch September 29, 2023 10:11
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.

5 participants