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.

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

add autofix comments + diagnostic on discards #1665

Merged
merged 3 commits into from
Dec 18, 2023
Merged

add autofix comments + diagnostic on discards #1665

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
87eb846
enforce a autofix comment when removing/adding discards
Techatrix Dec 10, 2023
3a9bbac
show diagnostic on discard that were added by autofix
Techatrix Dec 11, 2023
f144116
correctly detect last parameter when discarding function parameters
Techatrix Dec 11, 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
76 changes: 64 additions & 12 deletions src/features/code_actions.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,50 @@ pub const Builder = struct {
}
};

pub fn collectAutoDiscardDiagnostics(
tree: Ast,
arena: std.mem.Allocator,
diagnostics: *std.ArrayListUnmanaged(types.Diagnostic),
offset_encoding: offsets.Encoding,
) error{OutOfMemory}!void {
const token_tags = tree.tokens.items(.tag);
const token_starts = tree.tokens.items(.start);

// search for the following pattern:
// _ = some_identifier; // autofix

var i: usize = 0;
while (i < tree.tokens.len) {
const first_token: Ast.TokenIndex = @intCast(std.mem.indexOfPos(
std.zig.Token.Tag,
token_tags,
i,
&.{ .identifier, .equal, .identifier, .semicolon },
) orelse break);
defer i = first_token + 4;

const underscore_token = first_token;
const identifier_token = first_token + 2;
const semicolon_token = first_token + 3;

if (!std.mem.eql(u8, offsets.tokenToSlice(tree, underscore_token), "_")) continue;

const autofix_comment_start = std.mem.indexOfNonePos(u8, tree.source, token_starts[semicolon_token] + 1, " ") orelse continue;
if (!std.mem.startsWith(u8, tree.source[autofix_comment_start..], "//")) continue;
const autofix_str_start = std.mem.indexOfNonePos(u8, tree.source, autofix_comment_start + "//".len, " ") orelse continue;
if (!std.mem.startsWith(u8, tree.source[autofix_str_start..], "autofix")) continue;

try diagnostics.append(arena, .{
.range = offsets.tokenToRange(tree, identifier_token, offset_encoding),
.severity = .Information,
.code = null,
.source = "zls",
.message = "auto discard for unused variable",
// TODO add a relatedInformation that shows where the discarded identifier comes from
});
}
}

fn handleNonCamelcaseFunction(builder: *Builder, actions: *std.ArrayListUnmanaged(types.CodeAction), loc: offsets.Loc) !void {
const identifier_name = offsets.locToSlice(builder.handle.tree.source, loc);

Expand Down Expand Up @@ -122,15 +166,15 @@ fn handleUnusedFunctionParameter(builder: *Builder, actions: *std.ArrayListUnman
// ) void { ... }
// We have to be able to detect both cases.
const fn_proto_param = payload.get(tree).?;
const param_end = offsets.tokenToLoc(tree, ast.paramLastToken(tree, fn_proto_param)).end;
const last_param_token = ast.paramLastToken(tree, fn_proto_param);

const potential_comma_token = last_param_token + 1;
const found_comma = potential_comma_token < tree.tokens.len and tree.tokens.items(.tag)[potential_comma_token] == .comma;

const found_comma = std.mem.startsWith(
u8,
std.mem.trimLeft(u8, tree.source[param_end..], " \n"),
",",
);
const potential_r_paren_token = last_param_token + @intFromBool(found_comma) + 1;
const is_last_param = potential_r_paren_token < tree.tokens.len and tree.tokens.items(.tag)[potential_r_paren_token] == .r_paren;

const new_text = try createDiscardText(builder, identifier_name, token_starts[node_tokens[payload.func]], true, !found_comma);
const new_text = try createDiscardText(builder, identifier_name, token_starts[node_tokens[payload.func]], true, is_last_param);

const index = token_starts[node_tokens[block]] + 1;

Expand Down Expand Up @@ -413,11 +457,12 @@ fn createDiscardText(
const additional_indent = if (add_block_indentation) detectIndentation(builder.handle.tree.source) else "";

const new_text_len =
1 +
"\n".len +
indent.len +
additional_indent.len +
"_ = ;".len +
"_ = ".len +
identifier_name.len +
"; // autofix".len +
if (add_suffix_newline) 1 + indent.len else 0;
var new_text = try std.ArrayListUnmanaged(u8).initCapacity(builder.arena, new_text_len);

Expand All @@ -426,7 +471,7 @@ fn createDiscardText(
new_text.appendSliceAssumeCapacity(additional_indent);
new_text.appendSliceAssumeCapacity("_ = ");
new_text.appendSliceAssumeCapacity(identifier_name);
new_text.appendAssumeCapacity(';');
new_text.appendSliceAssumeCapacity("; // autofix");
if (add_suffix_newline) {
new_text.appendAssumeCapacity('\n');
new_text.appendSliceAssumeCapacity(indent);
Expand Down Expand Up @@ -547,13 +592,20 @@ fn getDiscardLoc(text: []const u8, loc: offsets.Loc) ?offsets.Loc {
while (i < text.len) : (i += 1) {
switch (text[i]) {
' ' => continue,
';' => break :found i + 1,
';' => break :found i,
else => return null,
}
}
return null;
};

// check if the colon is followed by the autofix comment
const autofix_comment_start = std.mem.indexOfNonePos(u8, text, colon_position + ";".len, " ") orelse return null;
if (!std.mem.startsWith(u8, text[autofix_comment_start..], "//")) return null;
const autofix_str_start = std.mem.indexOfNonePos(u8, text, autofix_comment_start + "//".len, " ") orelse return null;
if (!std.mem.startsWith(u8, text[autofix_str_start..], "autofix")) return null;
const autofix_comment_end = std.mem.indexOfNonePos(u8, text, autofix_str_start + "autofix".len, " ") orelse autofix_str_start + "autofix".len;

// check if the identifier is precede by a equal sign and then an underscore
var i: usize = loc.start - 1;
var found_equal_sign = false;
Expand Down Expand Up @@ -587,7 +639,7 @@ fn getDiscardLoc(text: []const u8, loc: offsets.Loc) ?offsets.Loc {

return offsets.Loc{
.start = start_position,
.end = colon_position,
.end = autofix_comment_end,
};
}

Expand Down
5 changes: 5 additions & 0 deletions src/features/diagnostics.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const Analyser = @import("../analysis.zig");
const ast = @import("../ast.zig");
const offsets = @import("../offsets.zig");
const URI = @import("../uri.zig");
const code_actions = @import("code_actions.zig");
const tracy = @import("../tracy.zig");

const Module = @import("../stage2/Module.zig");
Expand Down Expand Up @@ -44,6 +45,10 @@ pub fn generateDiagnostics(server: *Server, arena: std.mem.Allocator, handle: *D
try getAstCheckDiagnostics(server, arena, handle, &diagnostics);
}

if (server.config.enable_autofix) {
try code_actions.collectAutoDiscardDiagnostics(tree, arena, &diagnostics, server.offset_encoding);
}

if (server.config.warn_style) {
var node: u32 = 0;
while (node < tree.nodes.len) : (node += 1) {
Expand Down
113 changes: 90 additions & 23 deletions tests/lsp_features/code_actions.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ test "code actions - discard value" {
,
\\test {
\\ var foo = {};
\\ _ = foo;
\\ _ = foo; // autofix
\\ const bar, var baz = .{ 1, 2 };
\\ _ = baz;
\\ _ = bar;
\\ _ = baz; // autofix
\\ _ = bar; // autofix
\\}
\\
);
Expand All @@ -33,9 +33,20 @@ test "code actions - discard function parameter" {
\\
,
\\fn foo(a: void, b: void, c: void) void {
\\ _ = a;
\\ _ = b;
\\ _ = c;
\\ _ = a; // autofix
\\ _ = b; // autofix
\\ _ = c; // autofix
\\}
\\
);
try testAutofix(
\\fn foo(a: void, b: void, c: void,) void {}
\\
,
\\fn foo(a: void, b: void, c: void,) void {
\\ _ = a; // autofix
\\ _ = b; // autofix
\\ _ = c; // autofix
\\}
\\
);
Expand All @@ -56,26 +67,26 @@ test "code actions - discard captures" {
,
\\test {
\\ for (0..10, 0..10, 0..10) |i, j, k| {
\\ _ = i;
\\ _ = j;
\\ _ = k;
\\ _ = i; // autofix
\\ _ = j; // autofix
\\ _ = k; // autofix
\\ }
\\ switch (union(enum) {}{}) {
\\ inline .a => |cap, tag| {
\\ _ = cap;
\\ _ = tag;
\\ _ = cap; // autofix
\\ _ = tag; // autofix
\\ },
\\ }
\\ if (null) |x| {
\\ _ = x;
\\ _ = x; // autofix
\\ }
\\ if (null) |v| {
\\ _ = v;
\\ _ = v; // autofix
\\ } else |e| {
\\ _ = e;
\\ _ = e; // autofix
\\ }
\\ _ = null catch |e| {
\\ _ = e;
\\ _ = e; // autofix
\\ };
\\}
\\
Expand All @@ -95,7 +106,7 @@ test "code actions - discard capture with comment" {
\\ if (1 == 1) |a|
\\ //a
\\ {
\\ _ = a;
\\ _ = a; // autofix
\\ }
\\}
\\
Expand All @@ -115,7 +126,7 @@ test "code actions - discard capture - while loop with continue" {
\\ var lines: ?[]const u8 = "";
\\ var linei: usize = 0;
\\ while (lines.next()) |line| : (linei += 1) {
\\ _ = line;
\\ _ = line; // autofix
\\ }
\\}
\\
Expand All @@ -133,7 +144,7 @@ test "code actions - discard capture - while loop with continue" {
\\ var lines: ?[]const u8 = "";
\\ var linei: usize = 0;
\\ while (lines.next()) |line| : (linei += (1 * (2 + 1))) {
\\ _ = line;
\\ _ = line; // autofix
\\ }
\\}
\\
Expand All @@ -150,7 +161,7 @@ test "code actions - discard capture - while loop with continue" {
\\ var lines: ?[]const u8 = "";
\\ var linei: usize = 0;
\\ while (lines.next()) |line| : (linei += ")))".len) {
\\ _ = line;
\\ _ = line; // autofix
\\ }
\\}
\\
Expand All @@ -160,13 +171,13 @@ test "code actions - discard capture - while loop with continue" {
test "code actions - remove pointless discard" {
try testAutofix(
\\fn foo(a: u32) u32 {
\\ _ = a;
\\ _ = a; // autofix
\\ const b: ?u32 = a;
\\ _ = b;
\\ _ = b; // autofix
\\ const c = b;
\\ _ = c;
\\ _ = c; // autofix
\\ if (c) |d| {
\\ _ = d;
\\ _ = d; // autofix
\\ return d;
\\ }
\\ return 0;
Expand All @@ -185,6 +196,62 @@ test "code actions - remove pointless discard" {
);
}

test "code actions - remove discard of unknown identifier" {
try testAutofix(
\\fn foo() void {
\\ _ = a; // autofix
\\}
\\
,
\\fn foo() void {
\\}
\\
);
}

test "code actions - ignore autofix comment whitespace" {
try testAutofix(
\\fn foo() void {
\\ _ = a; // autofix
\\}
\\
,
\\fn foo() void {
\\}
\\
);
try testAutofix(
\\fn foo() void {
\\ _ = a;// autofix
\\}
\\
,
\\fn foo() void {
\\}
\\
);
try testAutofix(
\\fn foo() void {
\\ _ = a;//autofix
\\}
\\
,
\\fn foo() void {
\\}
\\
);
try testAutofix(
\\fn foo() void {
\\ _ = a; // autofix
\\}
\\
,
\\fn foo() void {
\\}
\\
);
}

fn testAutofix(before: []const u8, after: []const u8) !void {
try testAutofixOptions(before, after, true); // diagnostics come from our AstGen fork
try testAutofixOptions(before, after, false); // diagnostics come from calling zig ast-check
Expand Down