Skip to content

autofix: discard captures + some multi for loop support #1202

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.

Sign up for GitHub

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

Merged
leecannon merged 3 commits into from
May 27, 2023
Merged

autofix: discard captures + some multi for loop support #1202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
49b679e
autofix: discard captures + some multi for loop support
travisstaloch May 27, 2023
32dc6c3
autofix: clean up dupe 'remove capture' action detection
travisstaloch May 27, 2023
58bc245
autofix: tighten up getCaptureLoc()
travisstaloch May 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/Server.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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){};
Expand Down Expand Up @@ -1200,8 +1201,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;
Expand Down
257 changes: 87 additions & 170 deletions src/features/code_actions.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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),
.capture => try handleUnusedCapture(builder, actions, loc),
.@"switch tag capture", .capture => try handleUnusedCapture(builder, actions, loc, remove_capture_actions),
},
.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),
.@"error capture" => try handleUnusedCapture(builder, actions, loc, remove_capture_actions),
},
// the undeclared identifier may be a discard
.undeclared_identifier => try handlePointlessDiscard(builder, actions, loc),
Expand Down Expand Up @@ -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
// by adding a discard in the block scope
const is_value_discarded = std.mem.eql(u8, offsets.locToSlice(builder.handle.text, capture_locs.value), "_");
if (is_value_discarded) {
// |_, i| ->
// TODO fix formatting
try actions.append(builder.arena, .{
.title = "remove capture",
.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 },
"",
)}),
});
// look for next non-whitespace after last '|'. if its a '{' we can insert discards.
// this means bare loop/switch captures (w/out curlies) aren't supported.
var block_start = capture_loc.end + 1;
while (block_start < builder.handle.text.len and
std.ascii.isWhitespace(builder.handle.text[block_start])) : (block_start += 1)
{}
if (builder.handle.text[block_start] != '{') {
return;
}
}

fn handleUnusedCapture(builder: *Builder, actions: *std.ArrayListUnmanaged(types.CodeAction), loc: offsets.Loc) !void {
const tracy_zone = tracy.trace(@src());
defer tracy_zone.end();

const capture_locs = getCaptureLoc(builder.handle.text, loc, false) orelse return;
const block_start_loc = offsets.Loc{ .start = block_start + 1, .end = block_start + 1 };
const identifier_name = builder.handle.text[loc.start..loc.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, "_")}),
};

// TODO support discarding without modifying the capture
// by adding a discard in the block scope
if (capture_locs.index != null) {
// |v, i| -> |_, i|
try actions.append(builder.arena, .{
.title = "discard capture",
.kind = .quickfix,
.isPreferred = true,
.edit = try builder.createWorkspaceEdit(&.{builder.createTextEditLoc(capture_locs.value, "_")}),
});
} else {
// |v| ->
// TODO fix formatting
try actions.append(builder.arena, .{
// prevent adding duplicate 'remove capture' action.
// search for a matching action by comparing ranges.
const remove_cap_loc = builder.createTextEditLoc(capture_loc, "");
const gop = try remove_capture_actions.getOrPut(builder.arena, remove_cap_loc.range);
if (gop.found_existing)
try actions.appendSlice(builder.arena, &.{ action1, action2 })
else {
const action0 = types.CodeAction{
.title = "remove capture",
.kind = .quickfix,
.isPreferred = true,
.edit = try builder.createWorkspaceEdit(&.{builder.createTextEditLoc(capture_locs.loc, "")}),
});
.isPreferred = false,
.edit = try builder.createWorkspaceEdit(&.{remove_cap_loc}),
};
try actions.appendSlice(builder.arena, &.{ action0, action1, action2 });
}
}

Expand Down Expand Up @@ -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",
};
Expand Down Expand Up @@ -478,126 +472,49 @@ fn getDiscardLoc(text: []const u8, loc: offsets.Loc) ?offsets.Loc {
};
}

const CaptureLocs = struct {
loc: offsets.Loc,
value: offsets.Loc,
index: ?offsets.Loc,
};

/// takes the location of an identifier which is part of a payload `|value, index|`
/// and returns the location from '|' until '|' or null on failure
/// 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;
/// takes the location of a capture ie `value` from `...|value...|...`.
/// returns the location from '|' until '|'
fn getCaptureLoc(text: []const u8, loc: offsets.Loc) ?offsets.Loc {
const start_pipe_position = blk: {
var i = loc.start;
while (true) : (i -= 1) {
if (text[i] == '|') break;
if (i == 0) return null;
}
break :blk i;
};

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;
};
const end_pipe_position = (std.mem.indexOfScalarPos(u8, text, start_pipe_position + 1, '|') orelse
return null) + 1;

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;
};
const trimmed = std.mem.trim(u8, text[start_pipe_position + 1 .. end_pipe_position - 1], &std.ascii.whitespace);
if (trimmed.len == 0) return null;

// 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;
};
return .{ .start = start_pipe_position, .end = end_pipe_position };
}

// 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;
}
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: {
var i = value_start - 1;
while (i != 0) : (i -= 1) {
switch (text[i]) {
' ' => continue,
'|' => break :found i,
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,
};
try std.testing.expect(getCaptureLoc("||", .{ .start = 1, .end = 2 }) == null);
try std.testing.expect(getCaptureLoc(" |", .{ .start = 1, .end = 2 }) == null);
try std.testing.expect(getCaptureLoc("| ", .{ .start = 1, .end = 2 }) == null);
try std.testing.expect(getCaptureLoc("||", .{ .start = 1, .end = 1 }) == null);
try std.testing.expect(getCaptureLoc("| |", .{ .start = 1, .end = 3 }) == null);
try std.testing.expect(getCaptureLoc("| |", .{ .start = 1, .end = 6 }) == null);
}

fn isSymbolChar(char: u8) bool {
Expand Down