Skip to content

Commit 00beeb9

Browse files
d10cCopilot
andcommitted
Preserve a trailing regular comment when --learn rewrites a comment
An inline expectation comment may end with a trailing regular (non-interpreted) comment after the expectations, e.g. `// $ Alert // explanatory note`. The parser (see `expectationCommentPattern`) ends the expectation region at the first `//`, so everything from there is an ordinary comment. Until now `--learn` replaced an expectation comment from its marker to the end of the line, which discarded that trailing note whenever the comment was rewritten or deleted. Add `Test::getTrailingComment`, which extracts the trailing `// ...` using the same `(?:[^/]|/[^/])*` boundary the parser uses, and thread it through the rewrite: `renderLearnedComment` re-appends it after the re-rendered expectations, and the "nothing survives" branch of `learnEdits` replaces the comment with just the trailing note (rather than the empty string) so the note is kept in place. Only `//` starts such a trailing comment -- `#` never ends the expectation region -- so in practice this benefits `//`-comment languages. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent cf74387 commit 00beeb9

1 file changed

Lines changed: 49 additions & 9 deletions

File tree

shared/util/codeql/util/test/InlineExpectationsTest.qll

Lines changed: 49 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,25 @@ module Make<InlineExpectationsTestSig Impl> {
422422
)
423423
}
424424

425+
/**
426+
* Holds if the expectation comment at `location` ends with a trailing regular (non-interpreted)
427+
* comment `text`, that is a `//` sequence after the expectations whose remainder the framework
428+
* treats as an ordinary comment (see `expectationCommentPattern`). `text` includes the trailing
429+
* comment's own `//` marker, for example `// note`.
430+
*
431+
* `codeql test run --learn` keeps this trailing comment when it rewrites or deletes the
432+
* expectation comment, so an explanatory note written next to an expectation is not lost. Only
433+
* `//` starts such a trailing comment, mirroring `expectationCommentPattern`'s
434+
* `(?:[^/]|/[^/])*` expectation region, which ends only at `//`; a `#` never does, so this is in
435+
* practice a feature of `//`-comment languages.
436+
*/
437+
predicate getTrailingComment(Impl::Location location, string text) {
438+
exists(Impl::ExpectationComment comment |
439+
comment.getLocation() = location and
440+
text = comment.getContents().regexpCapture("\\s*\\$ (?:[^/]|/[^/])*(//.*)", 1)
441+
)
442+
}
443+
425444
query predicate testFailures(FailureLocatable element, string message) {
426445
hasFailureMessage(element, message)
427446
}
@@ -1278,21 +1297,32 @@ module TestPostProcessing {
12781297
*/
12791298
bindingset[relativePath]
12801299
private string renderLearnedComment(string relativePath, TestLocation commentLoc) {
1281-
exists(string startMarker, string endMarker, string endSuffix, string body |
1300+
exists(
1301+
string startMarker, string endMarker, string endSuffix, string body, string trailingSuffix
1302+
|
12821303
startMarker = Input2::getStartCommentMarker(relativePath) and
12831304
endMarker = Input2::getEndCommentMarker(relativePath) and
12841305
(
12851306
endMarker = "" and endSuffix = ""
12861307
or
12871308
endMarker != "" and endSuffix = " " + endMarker
12881309
) and
1310+
// Preserve a trailing regular comment (e.g. `// $ Alert // note`) that sits after the
1311+
// expectations, so rewriting the expectations never drops an explanatory note.
1312+
(
1313+
exists(string trailing |
1314+
Test::getTrailingComment(commentLoc, trailing) and trailingSuffix = " " + trailing
1315+
)
1316+
or
1317+
not Test::getTrailingComment(commentLoc, _) and trailingSuffix = ""
1318+
) and
12891319
body =
12901320
concat(string column |
12911321
exists(renderLearnedColumn(commentLoc, column))
12921322
|
12931323
renderLearnedColumn(commentLoc, column), " " order by getColumnRank(column)
12941324
) and
1295-
result = startMarker + " $ " + body + endSuffix
1325+
result = startMarker + " $ " + body + trailingSuffix + endSuffix
12961326
)
12971327
}
12981328

@@ -1322,7 +1352,10 @@ module TestPostProcessing {
13221352
* `SPURIOUS:`, and `MISSING:` columns. Expectations this test ignores (for example a tag
13231353
* annotated with a different query's ID) are preserved verbatim, so the comment keeps any
13241354
* expectation belonging to a different query that shares the same source file; see
1325-
* `isRewritableComment` and `Test::getAForeignExpectation`.
1355+
* `isRewritableComment` and `Test::getAForeignExpectation`. A trailing regular comment after
1356+
* the expectations (for example the ` // note` in `// $ Alert // note`) is likewise preserved;
1357+
* see `Test::getTrailingComment`. Only `//` starts such a trailing comment, so this is in
1358+
* practice a feature of `//`-comment languages.
13261359
*/
13271360
query predicate learnEdits(
13281361
string file, int line, string operation, int startColumn, int endColumn, string text
@@ -1360,11 +1393,12 @@ module TestPostProcessing {
13601393
// This subsumes the single-expectation removal and MISSING-promotion cases and additionally
13611394
// handles comments that carry several expectations across the default, `SPURIOUS:`, and
13621395
// `MISSING:` columns. The comment is replaced from its marker to the end of the line: with
1363-
// the re-rendered desired expectations, or with the empty string when none remains (in
1364-
// which case `endColumn = 0` also trims the whitespace gap the removed comment leaves
1365-
// behind). `endColumn = 0` is the engine's "to end of line" convention, which avoids
1366-
// depending on how each extractor reports a line comment's end column (e.g. Swift reports it
1367-
// as ending at column 1 of the next line).
1396+
// the re-rendered desired expectations (keeping any trailing regular comment), with just the
1397+
// trailing regular comment when no expectation remains but a note like `// $ Alert // note`
1398+
// does, or with the empty string when nothing remains (in which case `endColumn = 0` also
1399+
// trims the whitespace gap the removed comment leaves behind). `endColumn = 0` is the
1400+
// engine's "to end of line" convention, which avoids depending on how each extractor reports
1401+
// a line comment's end column (e.g. Swift reports it as ending at column 1 of the next line).
13681402
exists(TestLocation commentLoc, string relativePath, int sl, int sc |
13691403
Test::isRewritableComment(commentLoc) and
13701404
commentNeedsRewrite(commentLoc) and
@@ -1379,7 +1413,13 @@ module TestPostProcessing {
13791413
text = renderLearnedComment(relativePath, commentLoc)
13801414
or
13811415
not exists(string column, string t | desiredExpectation(commentLoc, column, t)) and
1382-
text = ""
1416+
// No expectation survives, so drop the `$ ...` comment. If it carried a trailing regular
1417+
// comment, keep that in place of the whole comment; otherwise delete to the end of line.
1418+
(
1419+
Test::getTrailingComment(commentLoc, text)
1420+
or
1421+
not Test::getTrailingComment(commentLoc, _) and text = ""
1422+
)
13831423
)
13841424
)
13851425
}

0 commit comments

Comments
 (0)