Merge pull request #1202 from travisstaloch/issue-689-discard-caps

autofix: discard captures + some multi for loop support

src/Server.zig

@@ -244,8 +244,9 @@ pub fn autofix(server: *Server, allocator: std.mem.Allocator, handle: *const Doc
     };
 
     var actions = std.ArrayListUnmanaged(types.CodeAction){};
+    var remove_capture_actions = std.AutoHashMapUnmanaged(types.Range, void){};
     for (diagnostics.items) |diagnostic| {
-        try builder.generateCodeAction(diagnostic, &actions);
+        try builder.generateCodeAction(diagnostic, &actions, &remove_capture_actions);
     }
 
     var text_edits = std.ArrayListUnmanaged(types.TextEdit){};
@@ -1189,8 +1190,9 @@ fn codeActionHandler(server: *Server, request: types.CodeActionParams) Error!?[]
     }
 
     var actions = std.ArrayListUnmanaged(types.CodeAction){};
+    var remove_capture_actions = std.AutoHashMapUnmanaged(types.Range, void){};
     for (diagnostics.items) |diagnostic| {
-        try builder.generateCodeAction(diagnostic, &actions);
+        try builder.generateCodeAction(diagnostic, &actions, &remove_capture_actions);
     }
 
     return actions.items;

src/features/code_actions.zig

@@ -14,7 +14,12 @@ pub const Builder = struct {
     handle: *const DocumentStore.Handle,
     offset_encoding: offsets.Encoding,
 
-    pub fn generateCodeAction(builder: *Builder, diagnostic: types.Diagnostic, actions: *std.ArrayListUnmanaged(types.CodeAction)) error{OutOfMemory}!void {
+    pub fn generateCodeAction(
+        builder: *Builder,
+        diagnostic: types.Diagnostic,
+        actions: *std.ArrayListUnmanaged(types.CodeAction),
+        remove_capture_actions: *std.AutoHashMapUnmanaged(types.Range, void),
+    ) error{OutOfMemory}!void {
         const kind = DiagnosticKind.parse(diagnostic.message) orelse return;
 
         const loc = offsets.rangeToLoc(builder.handle.text, diagnostic.range, builder.offset_encoding);
@@ -24,14 +29,12 @@ pub const Builder = struct {
                 .@"function parameter" => try handleUnusedFunctionParameter(builder, actions, loc),
                 .@"local constant" => try handleUnusedVariableOrConstant(builder, actions, loc),
                 .@"local variable" => try handleUnusedVariableOrConstant(builder, actions, loc),
-                .@"loop index capture" => try handleUnusedIndexCapture(builder, actions, loc),
+                .@"switch tag capture", .capture => try handleUnusedCapture(builder, actions, loc, remove_capture_actions),
-                .capture => try handleUnusedCapture(builder, actions, loc),
             },
             .non_camelcase_fn => try handleNonCamelcaseFunction(builder, actions, loc),
             .pointless_discard => try handlePointlessDiscard(builder, actions, loc),
             .omit_discard => |id| switch (id) {
-                .@"index capture" => try handleUnusedIndexCapture(builder, actions, loc),
+                .@"error capture" => try handleUnusedCapture(builder, actions, loc, remove_capture_actions),
-                .@"error capture" => try handleUnusedCapture(builder, actions, loc),
             },
             // the undeclared identifier may be a discard
             .undeclared_identifier => try handlePointlessDiscard(builder, actions, loc),
@@ -166,64 +169,57 @@ fn handleUnusedVariableOrConstant(builder: *Builder, actions: *std.ArrayListUnma
     });
 }
 
-fn handleUnusedIndexCapture(builder: *Builder, actions: *std.ArrayListUnmanaged(types.CodeAction), loc: offsets.Loc) !void {
+fn handleUnusedCapture(
+    builder: *Builder,
+    actions: *std.ArrayListUnmanaged(types.CodeAction),
+    loc: offsets.Loc,
+    remove_capture_actions: *std.AutoHashMapUnmanaged(types.Range, void),
+) !void {
     const tracy_zone = tracy.trace(@src());
     defer tracy_zone.end();
 
-    const capture_locs = getCaptureLoc(builder.handle.text, loc, true) orelse return;
+    const capture_loc = getCaptureLoc(builder.handle.text, loc) orelse return;
 
-    // TODO support discarding without modifying the capture
+    // look for next non-whitespace after last '|'. if its a '{' we can insert discards.
-    // by adding a discard in the block scope
+    // this means bare loop/switch captures (w/out curlies) aren't supported.
-    const is_value_discarded = std.mem.eql(u8, offsets.locToSlice(builder.handle.text, capture_locs.value), "_");
+    var block_start = capture_loc.end + 1;
-    if (is_value_discarded) {
+    while (block_start < builder.handle.text.len and
-        // |_, i| ->
+        std.ascii.isWhitespace(builder.handle.text[block_start])) : (block_start += 1)
-        // TODO fix formatting
+    {}
-        try actions.append(builder.arena, .{
+    if (builder.handle.text[block_start] != '{') {
-            .title = "remove capture",
+        return;
-            .kind = .quickfix,
-            .isPreferred = true,
-            .edit = try builder.createWorkspaceEdit(&.{builder.createTextEditLoc(capture_locs.loc, "")}),
-        });
-    } else {
-        // |v, i| -> |v|
-        // |v, _| -> |v|
-        try actions.append(builder.arena, .{
-            .title = "remove index capture",
-            .kind = .quickfix,
-            .isPreferred = true,
-            .edit = try builder.createWorkspaceEdit(&.{builder.createTextEditLoc(
-                .{ .start = capture_locs.value.end, .end = capture_locs.loc.end - 1 },
-                "",
-            )}),
-        });
     }
-}
 
-fn handleUnusedCapture(builder: *Builder, actions: *std.ArrayListUnmanaged(types.CodeAction), loc: offsets.Loc) !void {
+    const block_start_loc = offsets.Loc{ .start = block_start + 1, .end = block_start + 1 };
-    const tracy_zone = tracy.trace(@src());
+    const identifier_name = builder.handle.text[loc.start..loc.end];
-    defer tracy_zone.end();
+    const new_text = try createDiscardText(builder, identifier_name, block_start, true);
+    const action1 = .{
+        .title = "discard capture",
+        .kind = .@"source.fixAll",
+        .isPreferred = true,
+        .edit = try builder.createWorkspaceEdit(&.{builder.createTextEditLoc(block_start_loc, new_text)}),
+    };
+    const action2 = .{
+        .title = "discard capture name",
+        .kind = .quickfix,
+        .isPreferred = false,
+        .edit = try builder.createWorkspaceEdit(&.{builder.createTextEditLoc(loc, "_")}),
+    };
 
-    const capture_locs = getCaptureLoc(builder.handle.text, loc, false) orelse return;
+    // prevent adding duplicate 'remove capture' action.
-
+    // search for a matching action by comparing ranges.
-    // TODO support discarding without modifying the capture
+    const remove_cap_loc = builder.createTextEditLoc(capture_loc, "");
-    // by adding a discard in the block scope
+    const gop = try remove_capture_actions.getOrPut(builder.arena, remove_cap_loc.range);
-    if (capture_locs.index != null) {
+    if (gop.found_existing)
-        // |v, i| -> |_, i|
+        try actions.appendSlice(builder.arena, &.{ action1, action2 })
-        try actions.append(builder.arena, .{
+    else {
-            .title = "discard capture",
+        const action0 = types.CodeAction{
-            .kind = .quickfix,
-            .isPreferred = true,
-            .edit = try builder.createWorkspaceEdit(&.{builder.createTextEditLoc(capture_locs.value, "_")}),
-        });
-    } else {
-        // |v|    ->
-        // TODO fix formatting
-        try actions.append(builder.arena, .{
             .title = "remove capture",
             .kind = .quickfix,
-            .isPreferred = true,
+            .isPreferred = false,
-            .edit = try builder.createWorkspaceEdit(&.{builder.createTextEditLoc(capture_locs.loc, "")}),
+            .edit = try builder.createWorkspaceEdit(&.{remove_cap_loc}),
-        });
+        };
+        try actions.appendSlice(builder.arena, &.{ action0, action1, action2 });
     }
 }
 
@@ -374,13 +370,11 @@ const DiagnosticKind = union(enum) {
         @"function parameter",
         @"local constant",
         @"local variable",
-        @"loop index capture",
+        @"switch tag capture",
         capture,
     };
 
     const DiscardCat = enum {
-        // "discard of index capture; omit it instead"
-        @"index capture",
         // "discard of error capture; omit it instead"
         @"error capture",
     };
@@ -478,126 +472,49 @@ fn getDiscardLoc(text: []const u8, loc: offsets.Loc) ?offsets.Loc {
     };
 }
 
-const CaptureLocs = struct {
+/// takes the location of a capture ie `value` from `...|value...|...`.
-    loc: offsets.Loc,
+/// returns the location from '|' until '|'
-    value: offsets.Loc,
+fn getCaptureLoc(text: []const u8, loc: offsets.Loc) ?offsets.Loc {
-    index: ?offsets.Loc,
+    const start_pipe_position = blk: {
-};
+        var i = loc.start;
-
+        while (true) : (i -= 1) {
-/// takes the location of an identifier which is part of a payload `|value, index|`
+            if (text[i] == '|') break;
-/// and returns the location from '|' until '|' or null on failure
+            if (i == 0) return null;
-/// use `is_index_payload` to indicate whether `loc` points to `value` or `index`
-fn getCaptureLoc(text: []const u8, loc: offsets.Loc, is_index_payload: bool) ?CaptureLocs {
-    const value_end = if (!is_index_payload) loc.end else found: {
-        // move back until we find a comma
-        const comma_position = found_comma: {
-            var i = loc.start - 1;
-            while (i != 0) : (i -= 1) {
-                switch (text[i]) {
-                    ' ' => continue,
-                    ',' => break :found_comma i,
-                    else => return null,
-                }
-            } else return null;
-        };
-
-        // trim space
-        var i = comma_position - 1;
-        while (i != 0) : (i -= 1) {
-            switch (text[i]) {
-                ' ' => continue,
-                else => {
-                    if (!isSymbolChar(text[i])) return null;
-                    break :found i + 1;
-                },
-            }
-        } else return null;
-    };
-
-    const value_start = if (!is_index_payload) loc.start else found: {
-        // move back until we find a non identifier character
-        var i = value_end - 1;
-        while (i != 0) : (i -= 1) {
-            if (isSymbolChar(text[i])) continue;
-            switch (text[i]) {
-                ' ', '|', '*' => break :found i + 1,
-                else => return null,
-            }
-        } else return null;
-    };
-
-    var index: ?offsets.Loc = null;
-
-    if (is_index_payload) {
-        index = loc;
-    } else blk: {
-        // move forward until we find a comma
-        const comma_position = found_comma: {
-            var i = value_end;
-            while (i < text.len) : (i += 1) {
-                switch (text[i]) {
-                    ' ' => continue,
-                    ',' => break :found_comma i,
-                    else => break :blk,
-                }
-            }
-            break :blk;
-        };
-
-        // trim space
-        const index_start = found_start: {
-            var i = comma_position + 1;
-            while (i < text.len) : (i += 1) {
-                switch (text[i]) {
-                    ' ' => continue,
-                    else => {
-                        if (!isSymbolChar(text[i])) break :blk;
-                        break :found_start i;
-                    },
-                }
-            }
-            break :blk;
-        };
-
-        // move forward until we find a non identifier character
-        var i = index_start + 1;
-        while (i < text.len) : (i += 1) {
-            if (isSymbolChar(text[i])) continue;
-            index = offsets.Loc{
-                .start = index_start,
-                .end = i,
-            };
-            break;
         }
+        break :blk i;
+    };
+
+    const end_pipe_position = (std.mem.indexOfScalarPos(u8, text, start_pipe_position + 1, '|') orelse
+        return null) + 1;
+
+    const trimmed = std.mem.trim(u8, text[start_pipe_position + 1 .. end_pipe_position - 1], &std.ascii.whitespace);
+    if (trimmed.len == 0) return null;
+
+    return .{ .start = start_pipe_position, .end = end_pipe_position };
+}
+
+test "getCaptureLoc" {
+    {
+        const text = "|i|";
+        const caploc = getCaptureLoc(text, .{ .start = 1, .end = 2 }) orelse
+            return std.testing.expect(false);
+        const captext = text[caploc.start..caploc.end];
+        try std.testing.expectEqualStrings(text, captext);
+    }
+    {
+        const text = "|i, jjj, foobar|";
+        const caploc = getCaptureLoc(text, .{ .start = 1, .end = 17 }) orelse
+            return std.testing.expect(false);
+        const captext = text[caploc.start..caploc.end];
+        try std.testing.expectEqualStrings(text, captext);
     }
 
-    const start_pipe_position = found: {
+    try std.testing.expect(getCaptureLoc("||", .{ .start = 1, .end = 2 }) == null);
-        var i = value_start - 1;
+    try std.testing.expect(getCaptureLoc(" |", .{ .start = 1, .end = 2 }) == null);
-        while (i != 0) : (i -= 1) {
+    try std.testing.expect(getCaptureLoc("| ", .{ .start = 1, .end = 2 }) == null);
-            switch (text[i]) {
+    try std.testing.expect(getCaptureLoc("||", .{ .start = 1, .end = 1 }) == null);
-                ' ' => continue,
+    try std.testing.expect(getCaptureLoc("| |", .{ .start = 1, .end = 3 }) == null);
-                '|' => break :found i,
+    try std.testing.expect(getCaptureLoc("|    |", .{ .start = 1, .end = 6 }) == null);
-                else => return null,
-            }
-        } else return null;
-    };
-
-    const end_pipe_position = found: {
-        var i: usize = if (index) |index_loc| index_loc.end else value_end;
-        while (i < text.len) : (i += 1) {
-            switch (text[i]) {
-                ' ' => continue,
-                '|' => break :found i + 1,
-                else => return null,
-            }
-        } else return null;
-    };
-
-    return CaptureLocs{
-        .loc = .{ .start = start_pipe_position, .end = end_pipe_position },
-        .value = .{ .start = value_start, .end = value_end },
-        .index = index,
-    };
 }
 
 fn isSymbolChar(char: u8) bool {