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: Support for GOTO def from *inside* files included with include! macro #16439

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

wasd96040501
Copy link
Contributor

@wasd96040501 wasd96040501 commented Jan 27, 2024

close #14937
Try to implement goto def from inside files included with include! macro.
This implementation has two limitations:

  1. Only one file which calls include! will be tracked. (I think multiple file be included is a rare case and we may let it go for now)
  2. Mapping token from included file to macro call file (semantics.rs:646~658) works fine but I am not sure is this the correct way to implement.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 27, 2024
@cmyr
Copy link

cmyr commented Jan 29, 2024

Thanks for looking into this! I've played around with it a bit in a test project, where it's working fine, and also in my real project where I haven't been able to get it to work. (edit: after clearing some caches and restarting again it looks like it's working here too!)

Can you clarify what you mean by "Only one file which calls include! will be tracked."? Does this mean that we only track only track one include! for a given target file, or do you mean something else (like in a given project, only a single file can have include! statements that are tracked?)

In my test project it looks like this is working for multiple includes (e.g. A includes A' and B includes B' and I can navigate from both the included files.)

If you're curious to see the original project that motivated this issue it's available https://github.com/googlefonts/fontations/; specifically both the read-fonts and write-fonts crates use a pattern where there is a generated/ folder containing a bunch of code that is included from corresponding files in the src/tables directory.

@wasd96040501
Copy link
Contributor Author

@cmyr Thanks for your positive feedback!
"Only one file which calls include! will be tracked." means, for example, a file A is included both by B and C, the navigation in A will respect to only one of B and C.
Here is a simple code
file A.rs

fn eat() {
    apple();
}

file B.rs

fn apple() {
}
include!("A.rs");

file C.rs

fn apple() {
}
include!("A.rs");

When we goto def in apple() of A, we will only navigate to one of B and C, not both.

@cmyr
Copy link

cmyr commented Jan 30, 2024

okay yes I think that is an intuitive and reasonable restriction that A) indicates that the user probably has more serious problems and B) I forget what I was going to say for B).

For what it's worth I would probably phrase this as, "A given file can only be tracked via include! to a single location." Or something, I found the original phrasing a bit ambiguous.

Ultimately unimportant nitpicking though, thank you for the patch, this will significantly improve our quality of life. :)

@Veykril
Copy link
Member

Veykril commented Jan 30, 2024

Thanks!
Only supporting a file once per include is fine, we can fix that rather easily but I don't think we gain much from that right now.

The mapping logic looks correct to me.

The main issue here is that non-item level includes won't work with this, but this was known from the start given the lazy nature of rust-analyzer so that will most likely never work.

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 30, 2024

📌 Commit b22e772 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jan 30, 2024

⌛ Testing commit b22e772 with merge 22b6f96...

@bors
Copy link
Collaborator

bors commented Jan 30, 2024

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

@bors bors merged commit 22b6f96 into rust-lang:master Jan 30, 2024
10 checks passed
bors added a commit that referenced this pull request Jan 31, 2024
internal: Remove unnecessary usages of ExpansionInfo

And some follow up simplifications to #16439
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.

Support for GOTO def from *inside* files included with include! macro?
5 participants