Skip to content

Commit fdb5a2e

Browse files
committed
Make --learn record any unexpected result, not just Alert
`codeql test run --learn` learns inline expectations from a `@kind test-postprocess` query, but until now the *append* path -- the one that introduces a brand-new expectation on a line that has none, or merges one into an existing comment -- only ever recorded the built-in `Alert` tag. Column surgery on an existing comment (dropping a stale tag, promoting `MISSING:`, re-rendering siblings) was already tag-generic, but a result carrying any other tag could never be learned from scratch. That left the headline path-problem case unhandled: a path-problem reports its source with a `Source` tag on the source line (distinct from the alert line), so learning could never add the `// $ Source` a passing test needs. Replace the `Alert`-only `hasUnexpectedAlertOnLine` with a generic `learnedNewExpectation(relativePath, endLine, text)`: for every non-optional actual result with no matching expectation it records the result's fully rendered expectation text (`getExpectationText`, so a value is carried as `tag=value`), keyed on the result's end line (an expectation matches a result when its start line equals the result's end line; see `onSameLine`). `RelatedLocation` results are excluded -- they are only reported when an expectation on the line already references them, so they are never a genuinely new result to learn. Both callers become generic. The merge path (`mergedNewTag` -> `mergedNewExpectation`) feeds `desiredExpectation`, so several new tags landing on a line that already has a rewritable comment are merged and re-rendered together via `renderLearnedColumn`. The fresh-append disjunct of `learnEdits` now aggregates all expectations learned for a line into a single comment (`renderNewComment`, ordered lexically to match `renderLearnedColumn`) rather than emitting one comment per result, so a line on which two results fire grows one deterministic `// $ ...` comment instead of two. `Alert` behaviour is unchanged: an `Alert` result still renders exactly `// $ Alert` and is still merged/appended identically. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 76357e8 commit fdb5a2e

1 file changed

Lines changed: 73 additions & 40 deletions

File tree

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

Lines changed: 73 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1157,6 +1157,29 @@ module TestPostProcessing {
11571157
result = " " + renderInlineComment(relativePath, tag)
11581158
}
11591159

1160+
/**
1161+
* Gets the fully rendered inline expectation comment (including comment markers) that `--learn`
1162+
* should append to `line` of `relativePath`, carrying *every* expectation freshly learned for
1163+
* that line (see `learnedNewExpectation`) in a single comment, or has no result if that file's
1164+
* comment syntax is not supported.
1165+
*
1166+
* The expectations are ordered lexically, matching `renderLearnedColumn`, so that a line on
1167+
* which several results fire (for example a path-problem source and sink that coincide) grows
1168+
* one deterministic comment such as `// $ Sink Source` rather than several separate comments.
1169+
*/
1170+
bindingset[relativePath, line]
1171+
private string renderNewComment(string relativePath, int line) {
1172+
exists(string body |
1173+
body =
1174+
concat(string text |
1175+
learnedNewExpectation(relativePath, line, text)
1176+
|
1177+
text, " " order by text
1178+
) and
1179+
result = renderExpectationComment(relativePath, body)
1180+
)
1181+
}
1182+
11601183
/**
11611184
* Holds if, after `--learn`, the inline expectation comment at `commentLoc` should carry the
11621185
* expectation `text` in `column` (`""` for the default column, or `"SPURIOUS"` / `"MISSING"`).
@@ -1193,36 +1216,48 @@ module TestPostProcessing {
11931216
}
11941217

11951218
/**
1196-
* Holds if some non-optional `Alert` result with no matching expectation fires on `line` of
1197-
* `relativePath` -- an *unexpected result* that `--learn` should record with a new `Alert`
1198-
* expectation, either by appending a fresh comment or by merging into an existing one.
1219+
* Holds if `--learn` should record a new expectation `text` on `endLine` of `relativePath`,
1220+
* because a non-optional actual result there has no matching expectation (an *unexpected
1221+
* result*). `text` is the fully rendered expectation, so it carries a value where the result
1222+
* has one (`Alert`, `Source`, or a custom `tag=value`).
1223+
*
1224+
* The expectation is keyed on the result's *end* line, because an expectation matches a
1225+
* result when the expectation's start line equals the result's end line (see `onSameLine`).
1226+
* Whether the new expectation is appended as a fresh comment or merged into an existing one is
1227+
* decided by the callers (see the append disjunct of `learnEdits` and `mergedNewExpectation`).
1228+
*
1229+
* `RelatedLocation` results are excluded: they are only reported when an expectation on the
1230+
* line already references them (see `hasRelatedLocation`/`shouldReportRelatedLocations`), so
1231+
* they never constitute a genuinely new result that learning should introduce on its own.
11991232
*/
1200-
private predicate hasUnexpectedAlertOnLine(string relativePath, int line) {
1233+
private predicate learnedNewExpectation(string relativePath, int endLine, string text) {
12011234
exists(Test::ActualTestResult actualResult |
1202-
actualResult.getTag() = "Alert" and
1203-
actualResult.getValue() = "" and
12041235
not actualResult.isOptional() and
1236+
actualResult.getTag() != "RelatedLocation" and
12051237
not exists(
12061238
Test::getAMatchingExpectation(actualResult.getLocation(), actualResult.toString(),
12071239
actualResult.getTag(), actualResult.getValue(), false)
12081240
) and
1209-
parseLocationString(actualResult.getLocation().getRelativeUrl(), relativePath, _, _, line, _)
1241+
text = actualResult.getExpectationText() and
1242+
parseLocationString(actualResult.getLocation().getRelativeUrl(), relativePath, _, _,
1243+
endLine, _)
12101244
)
12111245
}
12121246

12131247
/**
1214-
* Holds if `--learn` should merge a freshly learned `Alert` expectation into the existing,
1248+
* Holds if `--learn` should merge a freshly learned expectation `text` into the existing,
12151249
* rewritable comment at `commentLoc` (in `column` `""`, the default), because an unexpected
1216-
* `Alert` result fires on that comment's line. Merging keeps the new tag alongside the
1217-
* comment's existing expectations rather than appending a second comment to the line.
1250+
* result fires on that comment's line. Merging keeps the new tag alongside the comment's
1251+
* existing expectations rather than appending a second comment to the line; when several new
1252+
* expectations land on the same line they are all merged and re-rendered together (see
1253+
* `renderLearnedColumn`).
12181254
*/
1219-
private predicate mergedNewTag(TestLocation commentLoc, string column, string text) {
1255+
private predicate mergedNewExpectation(TestLocation commentLoc, string column, string text) {
12201256
exists(string relativePath, int line |
12211257
Test::isRewritableComment(commentLoc) and
12221258
parseLocationString(commentLoc.getRelativeUrl(), relativePath, line, _, _, _) and
1223-
hasUnexpectedAlertOnLine(relativePath, line) and
1224-
column = "" and
1225-
text = "Alert"
1259+
learnedNewExpectation(relativePath, line, text) and
1260+
column = ""
12261261
)
12271262
}
12281263

@@ -1234,14 +1269,15 @@ module TestPostProcessing {
12341269
* This combines the surviving expectations this test understands (see `learnedExpectation`)
12351270
* with the expectations it ignores (see `Test::hasForeignExpectation`), which are preserved
12361271
* verbatim so that rewriting a comment for one query never drops another query's expectation on
1237-
* the same line, and with any freshly learned tag merged into the comment (see `mergedNewTag`).
1272+
* the same line, and with any freshly learned expectations merged into the comment (see
1273+
* `mergedNewExpectation`).
12381274
*/
12391275
private predicate desiredExpectation(TestLocation commentLoc, string column, string text) {
12401276
learnedExpectation(commentLoc, column, text)
12411277
or
12421278
Test::hasForeignExpectation(commentLoc, column, text)
12431279
or
1244-
mergedNewTag(commentLoc, column, text)
1280+
mergedNewExpectation(commentLoc, column, text)
12451281
}
12461282

12471283
/**
@@ -1359,49 +1395,46 @@ module TestPostProcessing {
13591395
*
13601396
* The following edits are emitted:
13611397
*
1362-
* - an actual result with no matching expectation records a new `Alert` expectation (an
1363-
* *unexpected result*): if the result's line already has a rewritable comment the tag is
1364-
* merged into it (see below), otherwise a fresh `// $ Alert` comment is appended; and
1398+
* - an actual result with no matching expectation records a new expectation (an *unexpected
1399+
* result*): the expectation is the result's tag, carrying a value where the result has one
1400+
* (`Alert`, a path-problem `Source`/`Sink`, or a custom `tag=value`). If the result's line
1401+
* already has a rewritable comment the expectation is merged into it (see below), otherwise a
1402+
* fresh comment carrying every expectation learned for the line is appended (for example
1403+
* `// $ Alert`, or `// $ Sink Source` when several results fire on one line); and
13651404
* - an existing rewritable expectation comment is rewritten as a whole so that it matches the
13661405
* current results: obsolete default and `// $ SPURIOUS:` expectations are dropped (a *missing
13671406
* result* or a *fixed spurious result*), a `// $ MISSING:` expectation whose result now fires
1368-
* is promoted to the default column (a *fixed missing result*), a freshly learned tag on the
1369-
* line is merged in, and the resulting expectations are re-rendered. If nothing remains, the
1370-
* comment is deleted.
1407+
* is promoted to the default column (a *fixed missing result*), any freshly learned tags on
1408+
* the line are merged in, and the resulting expectations are re-rendered. If nothing remains,
1409+
* the comment is deleted.
13711410
*
13721411
* The rewrite handles comments that carry several expectations across the default,
13731412
* `SPURIOUS:`, and `MISSING:` columns. Expectations this test ignores (for example a tag
13741413
* annotated with a different query's ID) are preserved verbatim, so the comment keeps any
13751414
* expectation belonging to a different query that shares the same source file; see
13761415
* `isRewritableComment` and `Test::hasForeignExpectation`. A trailing regular note after the
13771416
* expectations (for example the `note` in `// $ Alert // note` or `# $ Alert // note`) is
1378-
* likewise preserved, and a freshly learned tag can be merged into a plain comment that carries
1417+
* likewise preserved, and freshly learned tags can be merged into a plain comment that carries
13791418
* only such a note (`// note` -> `// $ Alert // note`); see `Test::getTrailingNote`.
13801419
*/
13811420
query predicate learnEdits(
13821421
string file, int line, string operation, int startColumn, int endColumn, string text
13831422
) {
1384-
// Unexpected result with no comment to merge into: append a new `// $ Alert` comment on the
1385-
// alert's line. The comment must go on the result's *end* line, because an expectation
1386-
// matches a result when the expectation's start line equals the result's end line (see
1387-
// `onSameLine`). For most languages a result spans a single line, but some (e.g. Rust)
1388-
// include leading trivia in the location, so the start and end lines differ. If the line
1389-
// already has a rewritable comment, the tag is merged into it by the rewrite disjunct below
1390-
// (see `mergedNewTag`) rather than appended as a separate comment.
1391-
exists(Test::ActualTestResult actualResult, string relativePath, int el, string comment |
1392-
actualResult.getTag() = "Alert" and
1393-
actualResult.getValue() = "" and
1394-
not actualResult.isOptional() and
1395-
not exists(
1396-
Test::getAMatchingExpectation(actualResult.getLocation(), actualResult.toString(),
1397-
actualResult.getTag(), actualResult.getValue(), false)
1398-
) and
1399-
parseLocationString(actualResult.getLocation().getRelativeUrl(), relativePath, _, _, el, _) and
1423+
// Unexpected result with no comment to merge into: append a fresh comment carrying every
1424+
// expectation learned for the result's line (see `learnedNewExpectation`). The comment must
1425+
// go on the result's *end* line, because an expectation matches a result when the
1426+
// expectation's start line equals the result's end line (see `onSameLine`). For most
1427+
// languages a result spans a single line, but some (e.g. Rust) include leading trivia in the
1428+
// location, so the start and end lines differ. If the line already has a rewritable comment,
1429+
// the new expectations are merged into it by the rewrite disjunct below (see
1430+
// `mergedNewExpectation`) rather than appended as a separate comment.
1431+
exists(string relativePath, int el, string comment |
1432+
learnedNewExpectation(relativePath, el, _) and
14001433
not exists(TestLocation existing |
14011434
Test::isRewritableComment(existing) and
14021435
parseLocationString(existing.getRelativeUrl(), relativePath, el, _, _, _)
14031436
) and
1404-
comment = renderExpectationComment(relativePath, "Alert") and
1437+
comment = renderNewComment(relativePath, el) and
14051438
file = relativePath and
14061439
line = el and
14071440
operation = "append" and

0 commit comments

Comments
 (0)