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

fix: references/rename for struct_init_one, struct_init_one_comma #1903

Merged
merged 5 commits into from
May 30, 2024

Conversation

WillLillis
Copy link
Contributor

This fixes some cases where references to fields in some struct initializations aren't picked up, causing features like renaming and reference viewing to fail. Currently the fix only works one way, in that a rename invoked on the actual struct initialization will still fail, but will work when invoked on other locations such as the struct's definition or a field access. I took some time to try and fix the other direction, but I don't think I'm familiar enough with the codebase to do that yet. I added a test case for this but left it commented out as it currently fails.

This fix only covers the cases of .struct_init_one and .struct_init_one_comma. I'd really like to look into fixing the other node cases (e.g. .struct_init, .struct_init_comma, etc. if I'm understanding things properly) but wanted to cover this first case and get some feedback/ a sanity check before proceeding on. Here's a quick demo showing both renaming and references with the improvements:

zls_rename

Related Issues: #1837, #1700

Copy link
Member

@Techatrix Techatrix left a comment

Choose a reason for hiding this comment

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

It should be possible to handle all the other .struct_init_* cases by doing something like this:

var buffer: [2]Ast.Node.Index = undefined;
const struct_init = tree.fullStructInit(&buffer, node).?;
for (struct_init.ast.fields) |value_node| { // the node of `value` in `.name = value`
const name_token = tree.firstToken(value_node) - 2; // math our way two token indexes back to get the `name`
const name_loc = offsets.tokenToLoc(tree, name_token);
const name = offsets.locToSlice(tree.source, name_loc);

The following patch appears to fix the "other direction" that previously didn't work:

diff --git a/src/features/references.zig b/src/features/references.zig
index d4b665d1..09646b4d 100644
--- a/src/features/references.zig
+++ b/src/features/references.zig
@@ -443,6 +443,7 @@ pub fn referencesHandler(server: *Server, arena: std.mem.Allocator, request: Gen
             break :z null;
         },
         .label => try Analyser.getLabelGlobal(source_index, handle, name),
+        .enum_literal => try analyser.getSymbolEnumLiteral(arena, handle, source_index, name),
         else => null,
     } orelse return null;

@WillLillis
Copy link
Contributor Author

WillLillis commented May 28, 2024

It should be possible to handle all the other .struct_init_* cases by doing something like this:

Awesome, I'll take a pass at the other cases soon. Thank you! :)

Edit: Should those additions for the other cases be pushed on top of this PR, or put into a new one? That was straightforward enough I'll just assume for now it's ok to push on top of the existing PR.

@WillLillis WillLillis requested a review from Techatrix May 28, 2024 21:02
@WillLillis
Copy link
Contributor Author

Thought I ran zig fmt before pushing last night, really sorry about that!

@WillLillis
Copy link
Contributor Author

WillLillis commented May 29, 2024

Unless I'm mistaken, it looks like CI is failing now due to the recent deprecations in ziglang/zig@f97c2f2, which is fixed in #1904?

@Techatrix Techatrix force-pushed the struct_init_rename branch from a1a10c9 to db9a085 Compare May 30, 2024 20:09
Copy link
Member

@Techatrix Techatrix left a comment

Choose a reason for hiding this comment

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

I removed the initial switch case that just handled .struct_init_one.* because the other case is already capable of handling all variants. Otherwise, this looks great!

@WillLillis
Copy link
Contributor Author

I removed the initial switch case that just handled .struct_init_one.* because the other case is already capable of handling all variants. Otherwise, this looks great!

Makes sense, thanks so much for the help!

@Techatrix Techatrix merged commit d2d5f43 into zigtools:master May 30, 2024
3 checks passed
@WillLillis WillLillis deleted the struct_init_rename branch May 30, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants