Skip to content

Commit 86f8a8b

Browse files
TechatrixTechatrix
authored
fix code actions not working on escaped identifiers (#2492)
fixes #2453
1 parent 89e13c7 commit 86f8a8b

File tree

3 files changed

+112
-85
lines changed

3 files changed

+112
-85
lines changed

‎src/features/code_actions.zig‎

Lines changed: 45 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -332,9 +332,10 @@ fn handleUnusedFunctionParameter(builder: *Builder, loc: offsets.Loc) !void {
332332

333333
if (!builder.wantKind(.@"source.fixAll") and !builder.wantKind(.quickfix)) return;
334334

335-
const identifier_name = offsets.locToSlice(builder.handle.tree.source, loc);
336-
337335
const tree = builder.handle.tree;
336+
const identifier_token = offsets.sourceIndexToTokenIndex(tree, loc.start).pickTokenTag(.identifier, &tree) orelse return;
337+
const identifier_name = offsets.identifierTokenToNameSlice(tree, identifier_token);
338+
const identifier_full_name = offsets.tokenToSlice(tree, identifier_token);
338339

339340
const decl = (try builder.analyser.lookupSymbolGlobal(
340341
builder.handle,
@@ -373,7 +374,7 @@ fn handleUnusedFunctionParameter(builder: *Builder, loc: offsets.Loc) !void {
373374

374375
const insert_token = tree.nodeMainToken(block);
375376
const add_suffix_newline = is_last_param and tree.tokenTag(insert_token + 1) == .r_brace and tree.tokensOnSameLine(insert_token, insert_token + 1);
376-
const insert_index, const new_text = try createDiscardText(builder, identifier_name, insert_token, true, add_suffix_newline);
377+
const insert_index, const new_text = try createDiscardText(builder, identifier_full_name, insert_token, true, add_suffix_newline);
377378

378379
if (builder.wantKind(.@"source.fixAll")) {
379380
try builder.fixall_text_edits.insert(builder.arena, 0, builder.createTextEditPos(insert_index, new_text));
@@ -397,9 +398,10 @@ fn handleUnusedVariableOrConstant(builder: *Builder, loc: offsets.Loc) !void {
397398

398399
if (!builder.wantKind(.@"source.fixAll") and !builder.wantKind(.quickfix)) return;
399400

400-
const identifier_name = offsets.locToSlice(builder.handle.tree.source, loc);
401-
402401
const tree = builder.handle.tree;
402+
const identifier_token = offsets.sourceIndexToTokenIndex(tree, loc.start).pickTokenTag(.identifier, &tree) orelse return;
403+
const identifier_name = offsets.identifierTokenToNameSlice(tree, identifier_token);
404+
const identifier_full_name = offsets.tokenToSlice(tree, identifier_token);
403405

404406
const decl = (try builder.analyser.lookupSymbolGlobal(
405407
builder.handle,
@@ -418,7 +420,7 @@ fn handleUnusedVariableOrConstant(builder: *Builder, loc: offsets.Loc) !void {
418420
if (insert_token >= tree.tokens.len) return;
419421
if (tree.tokenTag(insert_token) != .semicolon) return;
420422

421-
const insert_index, const new_text = try createDiscardText(builder, identifier_name, insert_token, false, false);
423+
const insert_index, const new_text = try createDiscardText(builder, identifier_full_name, insert_token, false, false);
422424

423425
if (builder.wantKind(.@"source.fixAll")) {
424426
try builder.fixall_text_edits.append(builder.arena, builder.createTextEditPos(insert_index, new_text));
@@ -446,20 +448,16 @@ fn handleUnusedCapture(
446448
if (!builder.wantKind(.@"source.fixAll") and !builder.wantKind(.quickfix)) return;
447449

448450
const tree = builder.handle.tree;
449-
450-
const source = tree.source;
451-
452-
const identifier_token = offsets.sourceIndexToTokenIndex(tree, loc.start).pickPreferred(&.{.identifier}, &tree) orelse return;
453-
if (tree.tokenTag(identifier_token) != .identifier) return;
454-
455-
const identifier_name = offsets.locToSlice(source, loc);
451+
const identifier_token = offsets.sourceIndexToTokenIndex(tree, loc.start).pickTokenTag(.identifier, &tree) orelse return;
452+
const identifier_name = offsets.identifierTokenToNameSlice(tree, identifier_token);
453+
const identifier_full_name = offsets.tokenToSlice(tree, identifier_token);
456454

457455
// Zig can report incorrect "unused capture" errors
458456
// https://github.com/ziglang/zig/pull/22209
459457
if (std.mem.eql(u8, identifier_name, "_")) return;
460458

461459
if (builder.wantKind(.quickfix)) {
462-
const capture_loc = getCaptureLoc(source, loc) orelse return;
460+
const capture_loc = getCaptureLoc(tree.source, loc) orelse return;
463461

464462
const remove_cap_loc = builder.createTextEditLoc(capture_loc, "");
465463

@@ -531,7 +529,7 @@ fn handleUnusedCapture(
531529
// if we are on the last capture of the block, we need to add an additional newline
532530
// i.e |a, b| { ... } -> |a, b| { ... \n_ = a; \n_ = b;\n }
533531
const add_suffix_newline = is_last_capture and tree.tokenTag(insert_token + 1) == .r_brace and tree.tokensOnSameLine(insert_token, insert_token + 1);
534-
const insert_index, const new_text = try createDiscardText(builder, identifier_name, insert_token, true, add_suffix_newline);
532+
const insert_index, const new_text = try createDiscardText(builder, identifier_full_name, insert_token, true, add_suffix_newline);
535533

536534
try builder.fixall_text_edits.insert(builder.arena, 0, builder.createTextEditPos(insert_index, new_text));
537535
}
@@ -542,7 +540,7 @@ fn handlePointlessDiscard(builder: *Builder, loc: offsets.Loc) !void {
542540

543541
if (!builder.wantKind(.@"source.fixAll") and !builder.wantKind(.quickfix)) return;
544542

545-
const edit_loc = getDiscardLoc(builder.handle.tree.source, loc) orelse return;
543+
const edit_loc = getDiscardLoc(builder.handle.tree, loc) orelse return;
546544

547545
if (builder.wantKind(.@"source.fixAll")) {
548546
try builder.fixall_text_edits.append(builder.arena, builder.createTextEditLoc(edit_loc, ""));
@@ -566,23 +564,18 @@ fn handleVariableNeverMutated(builder: *Builder, loc: offsets.Loc) !void {
566564

567565
if (!builder.wantKind(.quickfix)) return;
568566

569-
const source = builder.handle.tree.source;
570-
571-
const var_keyword_end = 1 + (std.mem.lastIndexOfNone(u8, source[0..loc.start], &std.ascii.whitespace) orelse return);
572-
573-
const var_keyword_loc: offsets.Loc = .{
574-
.start = var_keyword_end -| "var".len,
575-
.end = var_keyword_end,
576-
};
577-
578-
if (!std.mem.eql(u8, offsets.locToSlice(source, var_keyword_loc), "var")) return;
567+
const tree = builder.handle.tree;
568+
const identifier_token = offsets.sourceIndexToTokenIndex(tree, loc.start).pickTokenTag(.identifier, &tree) orelse return;
569+
if (identifier_token == 0) return;
570+
const var_token = identifier_token - 1;
571+
if (tree.tokenTag(var_token) != .keyword_var) return;
579572

580573
try builder.actions.append(builder.arena, .{
581574
.title = "use 'const'",
582575
.kind = .quickfix,
583576
.isPreferred = true,
584577
.edit = try builder.createWorkspaceEdit(&.{
585-
builder.createTextEditLoc(var_keyword_loc, "const"),
578+
builder.createTextEditLoc(offsets.tokenToLoc(tree, var_token), "const"),
586579
}),
587580
});
588581
}
@@ -1157,61 +1150,39 @@ const DiagnosticKind = union(enum) {
11571150

11581151
/// takes the location of an identifier which is part of a discard `_ = location_here;`
11591152
/// and returns the location from '_' until ';' or null on failure
1160-
fn getDiscardLoc(text: []const u8, loc: offsets.Loc) ?offsets.Loc {
1161-
// check of the loc points to a valid identifier
1162-
for (offsets.locToSlice(text, loc)) |c| {
1163-
if (!Analyser.isSymbolChar(c)) return null;
1164-
}
1153+
fn getDiscardLoc(tree: Ast, loc: offsets.Loc) ?offsets.Loc {
1154+
const identifier_token = offsets.sourceIndexToTokenIndex(tree, loc.start).pickTokenTag(.identifier, &tree) orelse return null;
11651155

1166-
// check if the identifier is followed by a colon
1167-
const colon_position = found: {
1168-
var i = loc.end;
1169-
while (i < text.len) : (i += 1) {
1170-
switch (text[i]) {
1171-
' ' => continue,
1172-
';' => break :found i,
1173-
else => return null,
1174-
}
1175-
}
1176-
return null;
1177-
};
1178-
1179-
// check if the colon is followed by the autofix comment
1180-
const autofix_comment_start = std.mem.indexOfNonePos(u8, text, colon_position + ";".len, " ") orelse return null;
1181-
if (!std.mem.startsWith(u8, text[autofix_comment_start..], "//")) return null;
1182-
const autofix_str_start = std.mem.indexOfNonePos(u8, text, autofix_comment_start + "//".len, " ") orelse return null;
1183-
if (!std.mem.startsWith(u8, text[autofix_str_start..], "autofix")) return null;
1184-
const autofix_comment_end = std.mem.indexOfNonePos(u8, text, autofix_str_start + "autofix".len, " ") orelse autofix_str_start + "autofix".len;
1156+
// check if the identifier is followed by a semicolon
1157+
const semicolon_token = identifier_token + 1;
1158+
if (semicolon_token >= tree.tokens.len) return null;
1159+
if (tree.tokenTag(semicolon_token) != .semicolon) return null;
11851160

11861161
// check if the identifier is precede by a equal sign and then an underscore
1187-
var i: usize = loc.start - 1;
1188-
var found_equal_sign = false;
1189-
const underscore_position = found: {
1190-
while (true) : (i -= 1) {
1191-
if (i == 0) return null;
1192-
switch (text[i]) {
1193-
' ' => {},
1194-
'=' => {
1195-
if (found_equal_sign) return null;
1196-
found_equal_sign = true;
1197-
},
1198-
'_' => if (found_equal_sign) break :found i else return null,
1199-
else => return null,
1200-
}
1201-
}
1202-
};
1162+
if (identifier_token < 2) return null;
1163+
const equal_token = identifier_token - 1;
1164+
const underscore_token = identifier_token - 2;
1165+
if (tree.tokenTag(equal_token) != .equal) return null;
1166+
if (tree.tokenTag(underscore_token) != .identifier or !std.mem.eql(u8, offsets.tokenToSlice(tree, underscore_token), "_")) return null;
1167+
1168+
// check if the colon is followed by the autofix comment
1169+
const colon_end_index = tree.tokenStart(semicolon_token) + 1;
1170+
const autofix_comment_start = std.mem.indexOfNonePos(u8, tree.source, colon_end_index, " ") orelse return null;
1171+
if (!std.mem.startsWith(u8, tree.source[autofix_comment_start..], "//")) return null;
1172+
const autofix_str_start = std.mem.indexOfNonePos(u8, tree.source, autofix_comment_start + "//".len, " ") orelse return null;
1173+
if (!std.mem.startsWith(u8, tree.source[autofix_str_start..], "autofix")) return null;
1174+
const autofix_comment_end = std.mem.indexOfNonePos(u8, tree.source, autofix_str_start + "autofix".len, " ") orelse autofix_str_start + "autofix".len;
12031175

12041176
// move backwards until we find a newline
1205-
i = underscore_position - 1;
12061177
const start_position = found: {
1207-
while (true) : (i -= 1) {
1208-
if (i == 0) break :found underscore_position;
1209-
switch (text[i]) {
1178+
var i = tree.tokenStart(underscore_token);
1179+
while (i > 0) : (i -= 1) {
1180+
switch (tree.source[i - 1]) {
12101181
' ', '\t' => {},
1211-
'\n' => break :found i,
1212-
else => break :found underscore_position,
1182+
'\n' => break :found i - 1,
1183+
else => break :found i - 1,
12131184
}
1214-
}
1185+
} else break :found i;
12151186
};
12161187

12171188
return .{

‎src/offsets.zig‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,22 @@ pub const SourceIndexToTokenIndexResult = union(enum) {
6565
right: Ast.TokenIndex,
6666
},
6767

68+
pub fn pickTokenTag(
69+
result: SourceIndexToTokenIndexResult,
70+
wanted_token_tag: std.zig.Token.Tag,
71+
tree: *const Ast,
72+
) ?Ast.TokenIndex {
73+
switch (result) {
74+
.none => return null,
75+
.one => |token| return if (tree.tokenTag(token) == wanted_token_tag) token else null,
76+
.between => |data| {
77+
if (tree.tokenTag(data.left) == wanted_token_tag) return data.left;
78+
if (tree.tokenTag(data.right) == wanted_token_tag) return data.right;
79+
return null;
80+
},
81+
}
82+
}
83+
6884
pub fn pickPreferred(
6985
result: SourceIndexToTokenIndexResult,
7086
preferred_tags: []const std.zig.Token.Tag,

‎tests/lsp_features/code_actions.zig‎

Lines changed: 51 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,25 @@ test "discard value with comments" {
5858
);
5959
}
6060

61+
test "discard value with escaped identifier" {
62+
try testAutofix(
63+
\\test {
64+
\\ var @"struct" = {};
65+
\\ const bar, var @"union" = .{ 1, 2 };
66+
\\}
67+
\\
68+
,
69+
\\test {
70+
\\ var @"struct" = {};
71+
\\ _ = @"struct"; // autofix
72+
\\ const bar, var @"union" = .{ 1, 2 };
73+
\\ _ = @"union"; // autofix
74+
\\ _ = bar; // autofix
75+
\\}
76+
\\
77+
);
78+
}
79+
6180
test "discard function parameter" {
6281
try testAutofix(
6382
\\fn foo(a: void, b: void, c: void) void {}
@@ -111,7 +130,7 @@ test "discard function parameter with comments" {
111130
test "discard captures" {
112131
try testAutofix(
113132
\\test {
114-
\\ for (0..10, 0..10, 0..10) |i, j, k| {}
133+
\\ for (0..10, 0..10, 0..10) |i, @"test", k| {}
115134
\\ switch (union(enum) {}{}) {
116135
\\ inline .a => |cap, tag| {},
117136
\\ }
@@ -123,9 +142,9 @@ test "discard captures" {
123142
\\
124143
,
125144
\\test {
126-
\\ for (0..10, 0..10, 0..10) |i, j, k| {
145+
\\ for (0..10, 0..10, 0..10) |i, @"test", k| {
127146
\\ _ = i; // autofix
128-
\\ _ = j; // autofix
147+
\\ _ = @"test"; // autofix
129148
\\ _ = k; // autofix
130149
\\ }
131150
\\ switch (union(enum) {}{}) {
@@ -284,9 +303,9 @@ test "remove pointless discard" {
284303
try testAutofix(
285304
\\fn foo(a: u32) u32 {
286305
\\ _ = a; // autofix
287-
\\ const b: ?u32 = a;
288-
\\ _ = b; // autofix
289-
\\ const c = b;
306+
\\ const @"struct": ?u32 = a;
307+
\\ _ = @"struct"; // autofix
308+
\\ const c = @"struct";
290309
\\ _ = c; // autofix
291310
\\ if (c) |d| {
292311
\\ _ = d; // autofix
@@ -297,8 +316,8 @@ test "remove pointless discard" {
297316
\\
298317
,
299318
\\fn foo(a: u32) u32 {
300-
\\ const b: ?u32 = a;
301-
\\ const c = b;
319+
\\ const @"struct": ?u32 = a;
320+
\\ const c = @"struct";
302321
\\ if (c) |d| {
303322
\\ return d;
304323
\\ }
@@ -319,6 +338,16 @@ test "remove discard of unknown identifier" {
319338
\\}
320339
\\
321340
);
341+
try testAutofix(
342+
\\fn foo() void {
343+
\\ _ = @"struct"; // autofix
344+
\\}
345+
\\
346+
,
347+
\\fn foo() void {
348+
\\}
349+
\\
350+
);
322351
}
323352

324353
test "ignore autofix comment whitespace" {
@@ -354,7 +383,7 @@ test "ignore autofix comment whitespace" {
354383
);
355384
try testAutofix(
356385
\\fn foo() void {
357-
\\ _ = a; // autofix
386+
\\ _ = @"struct"; // autofix
358387
\\}
359388
\\
360389
,
@@ -372,7 +401,7 @@ test "remove function parameter" {
372401
, .{ .filter_title = "remove function parameter" });
373402
try testDiagnostic(
374403
\\fn foo(
375-
\\ alpha: u32,
404+
\\ @"struct": u32,
376405
\\) void {}
377406
,
378407
\\fn foo() void {}
@@ -391,6 +420,17 @@ test "variable never mutated" {
391420
\\ _ = foo;
392421
\\}
393422
, .{ .filter_title = "use 'const'" });
423+
try testDiagnostic(
424+
\\test {
425+
\\ var @"struct" = 5;
426+
\\ _ = @"struct";
427+
\\}
428+
,
429+
\\test {
430+
\\ const @"struct" = 5;
431+
\\ _ = @"struct";
432+
\\}
433+
, .{ .filter_title = "use 'const'" });
394434
}
395435

396436
test "discard capture name" {
@@ -998,5 +1038,5 @@ fn testDiagnostic(
9981038
defer allocator.free(actual);
9991039
try ctx.server.document_store.refreshLspSyncedDocument(uri, try allocator.dupeZ(u8, actual));
10001040

1001-
try std.testing.expectEqualStrings(after, handle.tree.source);
1041+
try zls.testing.expectEqualStrings(after, handle.tree.source);
10021042
}

0 commit comments

Comments
 (0)