BUGFIX | Fix autofix misplacing } when resolving unused function parameters, and captures.

[alexatcanva]

What is this ?

Hi All 👋

This is my first PR contributing to zls, and it's also my first Zig PR (ever), so if at any point during this PR process I'm a little slow to pick things up please forgive me 🙏

This PR is aimed at resolving issue #1543

The issue occurs when you use the autofix functionality of zls to automatically provide discarded variable usages inside your function block, or inside your capture block.

An example can be seen like so:

pub fn format(a: u8, b: u8, c: u8) !void {
}

// Then after you save:


pub fn format(a: u8, b: u8, c: u8) !void {
    _ = c;
    _ = b;
    _ = a;}

This PR addresses this issue so the output of the autofix should now be like:

pub fn format(a: u8, b: u8, c: u8) !void {
    _ = a;
    _ = b;
    _ = c;
}

This behaviour can also be noted for the capture discards:

// old

pub fn format(a: u8, b: u8, c: u8) !void {
    _ = c;
    _ = b;
    _ = a;

    for (0..10) |ix| {
        _ = ix;}
}

// new

pub fn format(a: u8, b: u8, c: u8) !void {
    _ = a;
    _ = b;
    _ = c;

    for (0..10) |ix| {
        _ = ix;
    }
}

Let me know if I've missed anything here! Thanks!

[Techatrix]

Hi @alexatcanva 👋

I've had some comment but those are mostly code simplification or formatting and nothing critical. Overall you did a good job.

The most important thing is the missing test coverage. The current code action test were formatting the source code before comparing so there was no way to check for correctly formatted output.

The following patch should remove the formatting pass (and adjust one test case).
Most code action tests fail after this patch but not after your PR which shows that you produce code that is formatted just like zig fmt.

diff --git a/tests/lsp_features/code_actions.zig b/tests/lsp_features/code_actions.zig
index 5f9ae04..8d9796c 100644
--- a/tests/lsp_features/code_actions.zig
+++ b/tests/lsp_features/code_actions.zig
@@ -85,10 +85,11 @@ test "code actions - discard captures" {
 test "code actions - discard capture with comment" {
     try testAutofix(
         \\test {
-        \\  if (1 == 1) |a|
-        \\      //a
-        \\      {}
+        \\    if (1 == 1) |a|
+        \\    //a
+        \\    {}
         \\}
+        \\
     ,
         \\test {
         \\    if (1 == 1) |a|
@@ -174,9 +175,5 @@ fn testAutofix(before: []const u8, after: []const u8) !void {
     defer allocator.free(actual);
     try ctx.server.document_store.refreshDocument(uri, try allocator.dupeZ(u8, actual));
 
-    try std.testing.expect(handle.tree.errors.len == 0);
-    const formatted_actual = try handle.tree.render(allocator);
-    defer allocator.free(formatted_actual);
-
-    try std.testing.expectEqualStrings(after, formatted_actual);
+    try std.testing.expectEqualStrings(after, handle.tree.source);
 }

[alexatcanva]

Wow! @Techatrix thank you so much for taking the time to provide such an in-depth review! 🙏

I really appreciate the effort you have spent here to clearly outline and explain the improvements that I could have made in here. This type of feedback is invaluable while I am trying to learn the ins-and-outs of Zig and zls!

I have made the relevant adjustments to this PR as per your review comments, please let me know if there's anything else I can do to assist further!

Thanks again! 🚀

[Techatrix]

👍