Skip to content

Fix incorrect inlining of functions that come from MBE macros #16497

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

Conversation

evertedsphere
Copy link
Contributor

@evertedsphere evertedsphere commented Feb 6, 2024

Partial fix for #16471.

As a reminder, there are two issues there:

  1. missing whitespace in parameter types (the first test)
  2. the self parameter not being replaced by this in the function body (the second test)

The first part is fixed in this PR. See this comment for the second.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 6, 2024
@evertedsphere evertedsphere changed the title fix missing whitespace in inline_call assist for macro-generated code fix missing whitespace in inline_call assist Feb 6, 2024
@evertedsphere evertedsphere force-pushed the swann/fix-inline-for-macro-generated-method branch from 98995e0 to 4c5e89e Compare February 6, 2024 00:14
@lnicola
Copy link
Member

lnicola commented Feb 6, 2024

Rebasing should fix the CI.

@evertedsphere evertedsphere force-pushed the swann/fix-inline-for-macro-generated-method branch 2 times, most recently from 088756a to 1fcac0d Compare February 6, 2024 17:21
@evertedsphere evertedsphere force-pushed the swann/fix-inline-for-macro-generated-method branch from 1fcac0d to 57a4542 Compare February 6, 2024 19:24
@evertedsphere
Copy link
Contributor Author

evertedsphere commented Feb 6, 2024

It looks like when the function body comes from a macro, usages_for_locals can't find the self parameter in it and so the renaming fails here:

if let Some(self_local) = params[0].2.as_local(sema.db) {
usages_for_locals(self_local)
.filter_map(|FileReference { name, range, .. }| match name {
FileReferenceNode::NameRef(_) => Some(body.syntax().covering_element(range)),
_ => None,
})
.for_each(|usage| {
ted::replace(usage, &this());
});
}

Specifically, in my second test (at the end of the file), we have

        self.value.$0hash2(state) // function comes from macro, fails to inline

but, as can be seen from other tests, a small change makes it pass:

        self.value.$0write2_u64(state) // function was written by hand, inlines correctly

Perhaps there is some kind of analysis that needs to be run on the function body if it comes from a macro so it is "parsed" into a syntax tree instead of being "opaque" like it seems to be here...?

@evertedsphere evertedsphere changed the title fix missing whitespace in inline_call assist Fix incorrect inlining of functions that come from MBE macros Feb 6, 2024
@evertedsphere evertedsphere marked this pull request as draft February 6, 2024 23:21
@Veykril
Copy link
Member

Veykril commented Feb 8, 2024

The usage only exists in a macro expansion so usage search can't find it. That is currently expected and non-trivial to fix as we can't look through all macro expansions that there are for obvious performance reasons. We could special case this search for locals as the scope of macro expansions is contained to the body the local belongs to, implementing that is already tricky though I believe.

@evertedsphere
Copy link
Contributor Author

@Veykril would you be okay with me removing the second test and merging the other fix for now then? I can try to do the other one separately.

@Veykril
Copy link
Member

Veykril commented Feb 9, 2024

That's fine yes

@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 Feb 9, 2024
To be added back later once we have a fix.

See rust-lang#16471 and rust-lang#16497 (comment).
@evertedsphere evertedsphere marked this pull request as ready for review February 9, 2024 21:03
@lnicola

This comment was marked as outdated.

@Veykril
Copy link
Member

Veykril commented Feb 10, 2024

That clippy issue should be fixed on master already, thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Feb 10, 2024

📌 Commit 18be556 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 10, 2024

⌛ Testing commit 18be556 with merge aa97edb...

@bors
Copy link
Contributor

bors commented Feb 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants