diff --git a/README.md b/README.md index 1859337..42d49fe 100644 --- a/README.md +++ b/README.md @@ -39,8 +39,9 @@ steps: repository. If set to an empty string, the action will not write a SARIF file. The SARIF is always generated and printed to the workflow log. - `comment`: Set to true to comment on the PR with the issues. If set to false or ommitted, the action will not comment on the PR. Issues that carry a fix are - commented as [suggested changes](#suggested-changes). An issue is commented on only if every line it spans is part of the pull request's diff, as GitHub - rejects comments anchored outside it. + commented as [suggested changes](#suggested-changes). An issue is commented on only if the pull request's diff adds at least one of the lines it spans, as + GitHub rejects comments anchored outside the diff. An issue whose range reaches beyond what the diff shows is anchored on the part of it that the diff does + show, and its fix is then rendered as plain text rather than as a suggestion, since applying it would rewrite lines the pull request does not show. - `summary`: True by default - generates a markdown summary for the job. If set to false, the action will not generate a markdown summary. @@ -210,6 +211,11 @@ commented as a suggestion. To have one commented, widen the replacement to cover The fixes Bugalint writes out always use the second form, so a fix survives being read back from a SARIF file that Bugalint itself generated. +A fix is offered as a suggestion only when the pull request's diff shows every line the issue spans. A formatter reformatting a whole statement because one of +its lines changed reports a range reaching past the three context lines the diff carries around that change, and GitHub rejects a comment anchored outside the +diff. Such an issue is still commented on, anchored on the lines of its range that the diff does show, with its fix rendered as a plain code block and a note +naming the lines it covers, since applying it in one click would rewrite lines the pull request does not show. + ### Example With Custom Regex This is an example of how this action can be used to parse the output of a hypothetical custom linter called `mylinter`, which outputs issues in the following diff --git a/__tests__/bugalint.test.ts b/__tests__/bugalint.test.ts index 32d8386..39b1fb2 100644 --- a/__tests__/bugalint.test.ts +++ b/__tests__/bugalint.test.ts @@ -7,7 +7,7 @@ import { getRegexParser, parseDiffLines, isNewIssue, - isCommentableIssue, + getCommentAnchor, failOnIssues, filterNewIssues, _testExports, @@ -75,11 +75,26 @@ index 1111111..2222222 100644 expect(isNewIssue({ path: 'A/B/test.py' }, diffLines, '.')).toBe(false) }) - it('comments only on issues whose whole range is in the diff', () => { - expect(isCommentableIssue({ path: 'A/B/test.py', line: 1, eline: 3 }, diffLines, '.')).toBe(true) - expect(isCommentableIssue({ path: 'A/B/test.py', line: 3, eline: 4 }, diffLines, '.')).toBe(false) - expect(isCommentableIssue({ path: 'A/B/other.py', line: 1 }, diffLines, '.')).toBe(false) - expect(isCommentableIssue({ path: 'A/B/test.py' }, diffLines, '.')).toBe(false) + it('anchors a comment on the whole range when the diff shows all of it', () => { + const anchor = { path: 'A/B/test.py', line: 1, eline: 3, partial: false } + expect(getCommentAnchor({ path: 'A/B/test.py', line: 1, eline: 3 }, diffLines, '.')).toStrictEqual(anchor) + }) + + it('anchors a comment on the shown part of a range the diff cuts off', () => { + const anchor = { path: 'A/B/test.py', line: 3, eline: 3, partial: true } + expect(getCommentAnchor({ path: 'A/B/test.py', line: 3, eline: 4 }, diffLines, '.')).toStrictEqual(anchor) + }) + + it('relativizes the anchor path to the analysis path', () => { + const anchor = { path: 'A/B/test.py', line: 3, eline: 3, partial: false } + expect(getCommentAnchor({ path: 'test.py', line: 3 }, diffLines, 'A\\B')).toStrictEqual(anchor) + }) + + it('anchors nothing on an issue the diff does not add', () => { + expect(getCommentAnchor({ path: 'A/B/test.py', line: 1 }, diffLines, '.')).toBeUndefined() + expect(getCommentAnchor({ path: 'A/B/other.py', line: 1 }, diffLines, '.')).toBeUndefined() + expect(getCommentAnchor({ path: 'A/B/test.py' }, diffLines, '.')).toBeUndefined() + expect(getCommentAnchor({ line: 2 }, diffLines, '.')).toBeUndefined() }) describe('failOnIssues', () => { @@ -152,6 +167,46 @@ describe('commentBody', () => { }) }) +describe('fixBeyondDiff', () => { + const diff = [ + 'diff --git a/A.cpp b/A.cpp', + 'index 1111111..2222222 100644', + '--- a/A.cpp', + '+++ b/A.cpp', + '@@ -874,7 +874,7 @@', + ' &obja,', + ' &iosb,', + ' nullptr,', + '- FLAG_A | FLAG_B | FLAG_C,', + '+ FLAG_A | FLAG_B,', + ' nullptr,', + ' 0,', + ' CreateFileTypeNone,', + '' + ].join('\n') + const diffLines = parseDiffLines(diff) + const issue = { level: 'warning' as const, path: 'A.cpp', line: 868, eline: 883, fix: ['status = f(a,', ' b);'] } + const tag = '' + + it('anchors on the lines the hunk shows when the fix reaches past both of its ends', () => { + expect(getCommentAnchor(issue, diffLines, '.')).toStrictEqual({ path: 'A.cpp', line: 874, eline: 880, partial: true }) + }) + + it('offers the fix as plain text rather than a suggestion, naming the lines it covers', () => { + expect(_testExports.buildCommentBody(tag, 'clang-format', issue, true)).toBe( + `${tag}\n[warning:clang-format]\n` + + 'The pull request diff does not show all of lines 868-883, so this replacement cannot be offered as a suggestion:\n' + + '```\nstatus = f(a,\n b);\n```' + ) + }) + + it('still offers a suggestion once the diff shows the whole fix', () => { + expect(_testExports.buildCommentBody(tag, 'clang-format', issue)).toBe( + `${tag}\n[warning:clang-format]\n\`\`\`suggestion\nstatus = f(a,\n b);\n\`\`\`` + ) + }) +}) + describe('sarifFix', () => { const fixOf = (region: Region, deletedRegion: Region, text: string): string[] | undefined => { const log = { @@ -304,8 +359,8 @@ describe('invertedRange', () => { it('never reaches a comment anchor whose start follows its end', () => { const diffLines = parseDiffLines('diff --git a/t.py b/t.py\n--- a/t.py\n+++ b/t.py\n@@ -1,1 +1,3 @@\n x\n+y\n+z\n') - expect(isCommentableIssue({ path: 't.py', line: 2, eline: 3 }, diffLines, '.')).toBe(true) - expect(isCommentableIssue({ path: 't.py', line: 3, eline: 2 }, diffLines, '.')).toBe(false) + expect(getCommentAnchor({ path: 't.py', line: 2, eline: 3 }, diffLines, '.')).toStrictEqual({ path: 't.py', line: 2, eline: 3, partial: false }) + expect(getCommentAnchor({ path: 't.py', line: 3, eline: 2 }, diffLines, '.')).toBeUndefined() }) }) diff --git a/dist/index.js b/dist/index.js index 85d8ff2..8f8f9c2 100644 --- a/dist/index.js +++ b/dist/index.js @@ -29940,7 +29940,7 @@ exports.addComments = addComments; exports.getPrDiff = getPrDiff; exports.parseDiffLines = parseDiffLines; exports.isNewIssue = isNewIssue; -exports.isCommentableIssue = isCommentableIssue; +exports.getCommentAnchor = getCommentAnchor; exports.filterNewIssues = filterNewIssues; exports.failOnIssues = failOnIssues; exports.createSummary = createSummary; @@ -30201,14 +30201,19 @@ function getKnownParser(identifier, message) { function getRegexParser(regex, message, levelMap) { return (input) => appendMessage(parseRegex(input, regex, levelMap), message); } -function buildCommentBody(commentTag, identifier, issue) { +function buildCommentBody(commentTag, identifier, issue, partial = false) { const identifiers = `[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]`; const body = `${commentTag}\n${[issue.msg != null && issue.msg !== '' ? `**${issue.msg}**` : undefined, identifiers].filter((n) => n).join('\n')}`; if (issue.fix == null || (issue.fix.length === 1 && issue.fix[0] === '')) { return body; } const fence = '`'.repeat(Math.max(3, ...Array.from(issue.fix.join('\n').matchAll(/`+/g), (m) => m[0].length + 1))); - return `${body}\n${fence}suggestion\n${issue.fix.map((line) => `${line}\n`).join('')}${fence}`; + const fix = issue.fix.map((line) => `${line}\n`).join(''); + if (!partial) { + return `${body}\n${fence}suggestion\n${fix}${fence}`; + } + const note = `The pull request diff does not show all of lines ${issue.line}-${issue.eline ?? issue.line}, so this replacement cannot be offered as a suggestion:`; + return `${body}\n${note}\n${fence}\n${fix}${fence}`; } async function addComments(issues, prDiff, githubToken, identifier, owner, repo, prNumber, analysisPath) { /* eslint camelcase: ["error", {allow: ['^pull_number$', '^comment_id$', '^start_side$', '^start_line$']}] */ @@ -30227,22 +30232,22 @@ async function addComments(issues, prDiff, githubToken, identifier, owner, repo, const comments = []; for (const issue of issues) { (0, core_1.debug)(`Processing issue on ${issue.path}:${issue.line}`); - if (!isCommentableIssue(issue, diffLines, analysisPath)) { - (0, core_1.debug)(`Skipping issue on ${issue.path}:${issue.line} because it is not on lines the pull request diff shows`); + const anchor = getCommentAnchor(issue, diffLines, analysisPath); + if (anchor == null) { + (0, core_1.debug)(`Skipping issue on ${issue.path}:${issue.line} because it is not on lines the pull request adds`); continue; } if (comments.length >= 50) { (0, core_1.warning)('More than 50 comments detected. Only the first 50 will be posted.'); break; } - const endLine = issue.eline ?? issue.line; const args = { - path: normalizePath(issue.path, analysisPath), + path: anchor.path, side: 'RIGHT', start_side: 'RIGHT', - line: endLine, - start_line: endLine === issue.line ? undefined : issue.line, - body: buildCommentBody(commentTag, identifier, issue) + line: anchor.eline, + start_line: anchor.eline === anchor.line ? undefined : anchor.line, + body: buildCommentBody(commentTag, identifier, issue, anchor.partial) }; (0, core_1.debug)(`Generating comment ${JSON.stringify(args)}`); comments.push(args); @@ -30304,12 +30309,30 @@ function isNewIssue(issue, diffLines, analysisPath) { const lines = diffLines[normalizePath(issue.path, analysisPath)]; return issueLines(issue.line, issue.eline).some((line) => lines?.[line] ?? false); } -function isCommentableIssue(issue, diffLines, analysisPath) { - if (!isNewIssue(issue, diffLines, analysisPath)) { - return false; +function getCommentAnchor(issue, diffLines, analysisPath) { + if (issue.path == null || issue.line == null) { + return undefined; } - const lines = diffLines[normalizePath(issue.path, analysisPath)]; - return issueLines(issue.line, issue.eline).every((line) => lines?.[line] != null); + const commentPath = normalizePath(issue.path, analysisPath); + const lines = diffLines[commentPath]; + const eline = issue.eline ?? issue.line; + const range = issueLines(issue.line, eline); + const added = range.find((line) => lines?.[line] ?? false); + if (added == null) { + return undefined; + } + if (range.every((line) => lines?.[line] != null)) { + return { path: commentPath, line: issue.line, eline, partial: false }; + } + let start = added; + let end = added; + while (start > issue.line && lines?.[start - 1] != null) { + start--; + } + while (end < eline && lines?.[end + 1] != null) { + end++; + } + return { path: commentPath, line: start, eline: end, partial: true }; } function filterNewIssues(issues, prDiff, analysisPath) { const diffLines = parseDiffLines(prDiff); diff --git a/src/bugalint.ts b/src/bugalint.ts index e59f08e..a8b0d59 100644 --- a/src/bugalint.ts +++ b/src/bugalint.ts @@ -297,14 +297,19 @@ export function getRegexParser(regex: RegExp, message: string, levelMap?: Record return (input: string) => appendMessage(parseRegex(input, regex, levelMap), message) } -function buildCommentBody(commentTag: string, identifier: string, issue: Issue): string { +function buildCommentBody(commentTag: string, identifier: string, issue: Issue, partial = false): string { const identifiers = `[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]` const body = `${commentTag}\n${[issue.msg != null && issue.msg !== '' ? `**${issue.msg}**` : undefined, identifiers].filter((n) => n).join('\n')}` if (issue.fix == null || (issue.fix.length === 1 && issue.fix[0] === '')) { return body } const fence = '`'.repeat(Math.max(3, ...Array.from(issue.fix.join('\n').matchAll(/`+/g), (m) => m[0].length + 1))) - return `${body}\n${fence}suggestion\n${issue.fix.map((line) => `${line}\n`).join('')}${fence}` + const fix = issue.fix.map((line) => `${line}\n`).join('') + if (!partial) { + return `${body}\n${fence}suggestion\n${fix}${fence}` + } + const note = `The pull request diff does not show all of lines ${issue.line}-${issue.eline ?? issue.line}, so this replacement cannot be offered as a suggestion:` + return `${body}\n${note}\n${fence}\n${fix}${fence}` } export async function addComments( @@ -336,8 +341,9 @@ export async function addComments( const comments = [] for (const issue of issues) { debug(`Processing issue on ${issue.path}:${issue.line}`) - if (!isCommentableIssue(issue, diffLines, analysisPath)) { - debug(`Skipping issue on ${issue.path}:${issue.line} because it is not on lines the pull request diff shows`) + const anchor = getCommentAnchor(issue, diffLines, analysisPath) + if (anchor == null) { + debug(`Skipping issue on ${issue.path}:${issue.line} because it is not on lines the pull request adds`) continue } if (comments.length >= 50) { @@ -345,14 +351,13 @@ export async function addComments( break } - const endLine = issue.eline ?? issue.line const args = { - path: normalizePath(issue.path, analysisPath), + path: anchor.path, side: 'RIGHT', start_side: 'RIGHT', - line: endLine, - start_line: endLine === issue.line ? undefined : issue.line, - body: buildCommentBody(commentTag, identifier, issue) + line: anchor.eline, + start_line: anchor.eline === anchor.line ? undefined : anchor.line, + body: buildCommentBody(commentTag, identifier, issue, anchor.partial) } debug(`Generating comment ${JSON.stringify(args)}`) comments.push(args) @@ -421,12 +426,37 @@ export function isNewIssue(issue: Issue, diffLines: DiffLines, analysisPath: str return issueLines(issue.line, issue.eline).some((line) => lines?.[line] ?? false) } -export function isCommentableIssue(issue: Issue, diffLines: DiffLines, analysisPath: string): issue is Issue & Required> { - if (!isNewIssue(issue, diffLines, analysisPath)) { - return false +export interface CommentAnchor { + path: string + line: number + eline: number + partial: boolean +} + +export function getCommentAnchor(issue: Issue, diffLines: DiffLines, analysisPath: string): CommentAnchor | undefined { + if (issue.path == null || issue.line == null) { + return undefined } - const lines: Record | undefined = diffLines[normalizePath(issue.path, analysisPath)] - return issueLines(issue.line, issue.eline).every((line) => lines?.[line] != null) + const commentPath = normalizePath(issue.path, analysisPath) + const lines: Record | undefined = diffLines[commentPath] + const eline = issue.eline ?? issue.line + const range = issueLines(issue.line, eline) + const added = range.find((line) => lines?.[line] ?? false) + if (added == null) { + return undefined + } + if (range.every((line) => lines?.[line] != null)) { + return { path: commentPath, line: issue.line, eline, partial: false } + } + let start = added + let end = added + while (start > issue.line && lines?.[start - 1] != null) { + start-- + } + while (end < eline && lines?.[end + 1] != null) { + end++ + } + return { path: commentPath, line: start, eline: end, partial: true } } export function filterNewIssues(issues: Iterable, prDiff: string, analysisPath: string): Issue[] {