Skip to content

Commit 84b3d68

Browse files
mattnitemattnite
authored
Linter: remind contributors to attach issue links with TODO comments (ZigEmbeddedGroup#783)
1 parent 5dfb8ea commit 84b3d68

File tree

2 files changed

+182
-28
lines changed

2 files changed

+182
-28
lines changed

‎.github/workflows/lint.yml‎

Lines changed: 124 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -36,14 +36,109 @@ jobs:
3636
echo "Changed files: $FILES"
3737
3838
if [ -n "$FILES" ]; then
39-
echo "$FILES" | xargs ./tools/linter/zig-out/bin/linter > lint_results.json
39+
echo "$FILES" | xargs ./tools/linter/zig-out/bin/linter > lint_results_raw.json
4040
else
41-
echo "[]" > lint_results.json
41+
echo "[]" > lint_results_raw.json
4242
fi
4343
4444
# Debug output
45-
echo "Lint results:"
46-
cat lint_results.json
45+
echo "Raw lint results:"
46+
cat lint_results_raw.json
47+
48+
- name: Filter lint results to changed lines
49+
uses: actions/github-script@v7
50+
with:
51+
script: |
52+
const fs = require('fs');
53+
const { execSync } = require('child_process');
54+
55+
if (!fs.existsSync('lint_results_raw.json')) {
56+
fs.writeFileSync('lint_results.json', '[]');
57+
console.log('No raw lint results found');
58+
return;
59+
}
60+
61+
const rawIssues = JSON.parse(fs.readFileSync('lint_results_raw.json', 'utf8'));
62+
63+
if (rawIssues.length === 0) {
64+
fs.writeFileSync('lint_results.json', '[]');
65+
console.log('No lint issues found');
66+
return;
67+
}
68+
69+
console.log(`Found ${rawIssues.length} total lint issue(s)`);
70+
71+
// Get diff with line numbers
72+
const baseSha = '${{ github.event.pull_request.base.sha }}';
73+
const headSha = '${{ github.event.pull_request.head.sha }}';
74+
75+
// Parse diff to get changed line numbers per file
76+
const changedLines = {};
77+
78+
for (const issue of rawIssues) {
79+
const file = issue.file;
80+
81+
if (!changedLines[file]) {
82+
try {
83+
// Get the diff for this specific file
84+
const diff = execSync(
85+
`git diff --unified=0 ${baseSha} ${headSha} -- "${file}"`,
86+
{ encoding: 'utf8' }
87+
);
88+
89+
// Parse the diff to extract changed line numbers
90+
const lines = new Set();
91+
const diffLines = diff.split('\n');
92+
93+
for (const line of diffLines) {
94+
// Match the @@ -start,count +start,count @@ format
95+
const match = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/);
96+
if (match) {
97+
const startLine = parseInt(match[1]);
98+
const count = match[2] ? parseInt(match[2]) : 1;
99+
100+
// Add all changed lines in this hunk
101+
for (let i = 0; i < count; i++) {
102+
lines.add(startLine + i);
103+
}
104+
}
105+
}
106+
107+
changedLines[file] = lines;
108+
console.log(`File ${file}: ${lines.size} changed line(s)`);
109+
} catch (error) {
110+
console.log(`Could not get diff for ${file}: ${error.message}`);
111+
changedLines[file] = new Set();
112+
}
113+
}
114+
}
115+
116+
// Filter issues to only those on changed lines
117+
const filteredIssues = rawIssues.filter(issue => {
118+
const fileLines = changedLines[issue.file] || new Set();
119+
const isOnChangedLine = fileLines.has(issue.line);
120+
if (!isOnChangedLine) {
121+
console.log(`Filtered out: ${issue.file}:${issue.line} (not on changed line)`);
122+
}
123+
return isOnChangedLine;
124+
});
125+
126+
// Save filtered issues
127+
fs.writeFileSync('lint_results.json', JSON.stringify(filteredIssues, null, 2));
128+
129+
// Create summary of filtered issues
130+
const filtered = rawIssues.length - filteredIssues.length;
131+
if (filtered > 0) {
132+
console.log(`Filtered out ${filtered} issue(s) on unchanged lines`);
133+
134+
const filteredOut = rawIssues.filter(issue => !filteredIssues.includes(issue));
135+
const summary = filteredOut.map(i => `${i.file}:${i.line}: ${i.message}`).join('\n');
136+
fs.writeFileSync('filtered_issues.txt',
137+
`The following ${filtered} issue(s) exist but are not on lines changed in this PR:\n\n${summary}`
138+
);
139+
}
140+
141+
console.log(`Kept ${filteredIssues.length} issue(s) on changed lines for inline comments`);
47142
48143
- name: Post review with grouped comments
49144
uses: actions/github-script@v7
@@ -150,7 +245,11 @@ jobs:
150245
review.body && review.body.includes('<!-- lint-review -->')
151246
);
152247
153-
if (!content || content === '[]') {
248+
// Check if there are filtered issues
249+
const hasFilteredIssues = fs.existsSync('filtered_issues.txt');
250+
const filteredContent = hasFilteredIssues ? fs.readFileSync('filtered_issues.txt', 'utf8') : '';
251+
252+
if ((!content || content === '[]') && !hasFilteredIssues) {
154253
console.log('No lint issues found');
155254
156255
// Dismiss all existing bot reviews and resolve their conversations
@@ -171,11 +270,11 @@ jobs:
171270
return;
172271
}
173272
174-
const issues = JSON.parse(content);
273+
const issues = content && content !== '[]' ? JSON.parse(content) : [];
175274
176275
// Create hash of current issues to check if review needs updating
177276
const issuesHash = crypto.createHash('md5')
178-
.update(JSON.stringify(issues.map(i => `${i.file}:${i.line}:${i.message}`)))
277+
.update(JSON.stringify(issues.map(i => `${i.file}:${i.line}:${i.message}`)) + filteredContent)
179278
.digest('hex')
180279
.substring(0, 8);
181280
@@ -204,7 +303,7 @@ jobs:
204303
}
205304
}
206305
207-
// Prepare review comments
306+
// Prepare review comments (only for issues on changed lines)
208307
const reviewComments = [];
209308
const issuesByFile = {};
210309
@@ -226,26 +325,37 @@ jobs:
226325
const fileCount = Object.keys(issuesByFile).length;
227326
228327
let reviewBody = `## 🔍 Lint Results\n\n`;
229-
reviewBody += `Found **${totalIssues}** issue${totalIssues !== 1 ? 's' : ''} in **${fileCount}** file${fileCount !== 1 ? 's' : ''}:\n\n`;
230328
231-
for (const [file, fileIssues] of Object.entries(issuesByFile)) {
232-
reviewBody += `- **${file}**: ${fileIssues.length} issue${fileIssues.length !== 1 ? 's' : ''}\n`;
329+
if (totalIssues > 0) {
330+
reviewBody += `Found **${totalIssues}** issue${totalIssues !== 1 ? 's' : ''} on changed lines in **${fileCount}** file${fileCount !== 1 ? 's' : ''}:\n\n`;
331+
332+
for (const [file, fileIssues] of Object.entries(issuesByFile)) {
333+
reviewBody += `- **${file}**: ${fileIssues.length} issue${fileIssues.length !== 1 ? 's' : ''}\n`;
334+
}
335+
}
336+
337+
// Add filtered issues summary if exists
338+
if (hasFilteredIssues) {
339+
reviewBody += `\n<details>\n<summary>ℹ️ Additional issues on unchanged lines</summary>\n\n\`\`\`\n${filteredContent}\n\`\`\`\n\n</details>\n`;
233340
}
234341
235342
reviewBody += `\n<!-- lint-review -->\n<!-- lint-hash:${issuesHash} -->`;
236343
344+
// Determine review event type
345+
const reviewEvent = totalIssues > 0 ? 'REQUEST_CHANGES' : 'COMMENT';
346+
237347
try {
238348
const review = await github.rest.pulls.createReview({
239349
owner: context.repo.owner,
240350
repo: context.repo.repo,
241351
pull_number: context.issue.number,
242352
commit_id: context.payload.pull_request.head.sha,
243353
body: reviewBody,
244-
event: 'REQUEST_CHANGES',
354+
event: reviewEvent,
245355
comments: reviewComments
246356
});
247357
248-
console.log(`Created review with ${reviewComments.length} comments`);
358+
console.log(`Created review with ${reviewComments.length} inline comment(s)`);
249359
} catch (error) {
250360
console.error(`Failed to create review:`, error.message);
251361
@@ -257,7 +367,7 @@ jobs:
257367
pull_number: context.issue.number,
258368
commit_id: context.payload.pull_request.head.sha,
259369
body: reviewBody + '\n\n⚠️ Could not attach inline comments due to an error.',
260-
event: 'REQUEST_CHANGES'
370+
event: reviewEvent
261371
});
262372
console.log('Created review without inline comments as fallback');
263373
} catch (fallbackError) {

‎tools/linter/src/main.zig‎

Lines changed: 58 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@ const std = @import("std");
22
const assert = std.debug.assert;
33
const Allocator = std.mem.Allocator;
44

5+
const Token = std.zig.Token;
6+
const TokenIndex = std.zig.Ast.TokenIndex;
7+
58
const Issue = struct {
69
file: []const u8,
710
line: u32,
@@ -20,8 +23,8 @@ pub fn main() !void {
2023
const args = try std.process.argsAlloc(allocator);
2124
defer std.process.argsFree(allocator, args);
2225

23-
var issues: std.array_list.Managed(Issue) = .init(allocator);
24-
defer issues.deinit();
26+
var issues: std.ArrayListUnmanaged(Issue) = .{};
27+
defer issues.deinit(allocator);
2528

2629
for (args[1..]) |path| {
2730
const source = std.fs.cwd().readFileAllocOptions(allocator, path, 100 * 1024 * 1024, null, .@"1", 0) catch |err| {
@@ -32,6 +35,8 @@ pub fn main() !void {
3235

3336
var ast = try std.zig.Ast.parse(allocator, source, .zig);
3437
defer ast.deinit(allocator);
38+
39+
try check_todos(allocator, path, source, &issues);
3540
for (ast.nodes.items(.tag), ast.nodes.items(.main_token)) |node_tag, main_tok_idx| {
3641
switch (node_tag) {
3742
.fn_proto_simple,
@@ -48,7 +53,7 @@ pub fn main() !void {
4853
snake_case,
4954
});
5055

51-
try issues.append(.{
56+
try issues.append(allocator, .{
5257
.line = @intCast(location.line + 1),
5358
.message = message,
5459
.file = path,
@@ -78,9 +83,6 @@ pub fn main() !void {
7883
try writer.flush();
7984
}
8085

81-
const Token = std.zig.Token;
82-
const TokenIndex = std.zig.Ast.TokenIndex;
83-
8486
fn find_first_token_tag(ast: std.zig.Ast, tag: Token.Tag, start_idx: TokenIndex) TokenIndex {
8587
return for (ast.tokens.items(.tag)[start_idx..], start_idx..) |token_tag, token_idx| {
8688
if (token_tag == tag)
@@ -118,24 +120,66 @@ fn camel_to_snake(arena: Allocator, str: []const u8) ![]const u8 {
118120
if (str.len == 0)
119121
return str;
120122

121-
var ret = std.array_list.Managed(u8).init(arena);
122-
errdefer ret.deinit();
123+
var ret = std.ArrayListUnmanaged(u8){};
124+
errdefer ret.deinit(arena);
123125

124126
if (std.ascii.isUpper(str[0])) {
125-
try ret.append(std.ascii.toLower(str[0]));
127+
try ret.append(arena, std.ascii.toLower(str[0]));
126128
} else {
127-
try ret.append(str[0]);
129+
try ret.append(arena, str[0]);
128130
}
129131

130132
for (str[1..]) |c| {
131133
if (std.ascii.isUpper(c)) {
132134
// Add underscore before uppercase letters
133-
try ret.append('_');
134-
try ret.append(std.ascii.toLower(c));
135+
try ret.append(arena, '_');
136+
try ret.append(arena, std.ascii.toLower(c));
135137
} else {
136-
try ret.append(c);
138+
try ret.append(arena, c);
137139
}
138140
}
139141

140-
return ret.toOwnedSlice();
142+
return ret.toOwnedSlice(arena);
143+
}
144+
145+
const uppercase_todo_keywords: []const []const u8 = &.{
146+
"TODO",
147+
"HACK",
148+
"FIXME",
149+
"XXX",
150+
"BUG",
151+
"NOTE",
152+
};
153+
154+
pub fn check_todos(
155+
allocator: Allocator,
156+
path: []const u8,
157+
source: []const u8,
158+
issues: *std.ArrayListUnmanaged(Issue),
159+
) !void {
160+
var line_num: u32 = 1;
161+
var it = std.mem.splitScalar(u8, source, '\n');
162+
while (it.next()) |line| : (line_num += 1) {
163+
const comment_start = std.mem.indexOf(u8, line, "//") orelse continue;
164+
const comment = line[comment_start..];
165+
if (contains_todo_keyword(comment) and !contains_link(comment)) {
166+
try issues.append(allocator, .{
167+
.file = path,
168+
.line = line_num,
169+
.message = "TODO style comments need to have a linked microzig issue on the same line.",
170+
});
171+
}
172+
}
173+
}
174+
175+
/// Checks if a string contains a link (URL).
176+
fn contains_link(text: []const u8) bool {
177+
return std.mem.indexOf(u8, text, "https://github.com/ZigEmbeddedGroup/microzig/issues") != null;
178+
}
179+
180+
fn contains_todo_keyword(text: []const u8) bool {
181+
return for (uppercase_todo_keywords) |keyword| {
182+
if (std.mem.indexOf(u8, text, keyword) != null)
183+
break true;
184+
} else false;
141185
}

0 commit comments

Comments
 (0)