Skip to content

Commit 28dd3a7

Browse files
d10cCopilot
andcommitted
Support MISSING promotion and SPURIOUS clearing in learnEdits
Extend the `learnEdits` predicate used by `codeql test run --learn` to cover two more inline-expectation cases beyond the initial add/remove subset, still restricted to the plain `Alert` tag on comments that carry a single expectation: - `// $ MISSING: Alert` whose result now appears is promoted to a plain `// $ Alert` (a *fixed missing result*), by rewriting the whole comment in place from its marker to the end of the line. - `// $ SPURIOUS: Alert` whose result no longer fires is removed (a *fixed spurious result*), reusing the same whole-comment deletion path as a now-missing plain `Alert`. Both promotion and removal operate on the comment as a whole rather than editing individual columns within it, which is reliable for a single-expectation comment; the `isSoleExpectationOnComment` guard (also excluding comments with an unparseable expectation) keeps multi-tag comments out of scope for a follow-up. The renderer is split into `renderInlineComment` (no leading space, for in-place rewrites starting at the comment marker) and `renderExpectationComment` (adds the leading separator space, for appending after code). The removal path relies on the engine's `endColumn = 0` "delete to end of line" convention, which now also trims the whitespace gap the removed trailing comment leaves behind. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 5934155 commit 28dd3a7

1 file changed

Lines changed: 83 additions & 29 deletions

File tree

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

Lines changed: 83 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1035,14 +1035,17 @@ module TestPostProcessing {
10351035
}
10361036

10371037
/**
1038-
* Gets the fully rendered inline expectation comment (including the comment markers) that
1039-
* `--learn` should insert for a new `tag` expectation in the file with the given
1040-
* `relativePath`, or has no result if that file's comment syntax is not supported.
1038+
* Gets the inline expectation comment (including the comment markers, but with no leading
1039+
* whitespace) that renders a plain `tag` expectation in the file with the given `relativePath`,
1040+
* or has no result if that file's comment syntax is not supported. For example `// $ Alert` for
1041+
* a language whose line comments start with `//`.
10411042
*
1042-
* The leading space separates the comment from any existing content on the line.
1043+
* This form is used when rewriting an existing comment in place, starting at its comment
1044+
* marker, so it must not carry the leading separator space that `renderExpectationComment`
1045+
* adds for appending after code.
10431046
*/
10441047
bindingset[relativePath, tag]
1045-
private string renderExpectationComment(string relativePath, string tag) {
1048+
private string renderInlineComment(string relativePath, string tag) {
10461049
exists(string startMarker, string endMarker, string endSuffix |
10471050
startMarker = Input2::getStartCommentMarker(relativePath) and
10481051
endMarker = Input2::getEndCommentMarker(relativePath) and
@@ -1051,7 +1054,39 @@ module TestPostProcessing {
10511054
or
10521055
endMarker != "" and endSuffix = " " + endMarker
10531056
) and
1054-
result = " " + startMarker + " $ " + tag + endSuffix
1057+
result = startMarker + " $ " + tag + endSuffix
1058+
)
1059+
}
1060+
1061+
/**
1062+
* Gets the fully rendered inline expectation comment (including the comment markers) that
1063+
* `--learn` should append for a new `tag` expectation in the file with the given
1064+
* `relativePath`, or has no result if that file's comment syntax is not supported.
1065+
*
1066+
* The leading space separates the comment from any existing content on the line.
1067+
*/
1068+
bindingset[relativePath, tag]
1069+
private string renderExpectationComment(string relativePath, string tag) {
1070+
result = " " + renderInlineComment(relativePath, tag)
1071+
}
1072+
1073+
/**
1074+
* Holds if `expectation` is the only inline expectation carried by its comment, so `--learn`
1075+
* can rewrite or delete the comment as a whole without disturbing a sibling expectation.
1076+
* Comments that additionally carry an invalid (unparseable) expectation are excluded, to avoid
1077+
* corrupting text the library could not fully understand.
1078+
*/
1079+
private predicate isSoleExpectationOnComment(Test::FailureLocatable expectation) {
1080+
count(Test::FailureLocatable other |
1081+
other.getLocation() = expectation.getLocation() and
1082+
(
1083+
other instanceof Test::GoodTestExpectation or
1084+
other instanceof Test::FalsePositiveTestExpectation or
1085+
other instanceof Test::FalseNegativeTestExpectation
1086+
)
1087+
) = 1 and
1088+
not exists(Test::InvalidTestExpectation invalid |
1089+
invalid.getLocation() = expectation.getLocation()
10551090
)
10561091
}
10571092

@@ -1066,15 +1101,18 @@ module TestPostProcessing {
10661101
* `text` (the empty string deletes the range).
10671102
*
10681103
* Only a deliberately reliable subset is emitted so far, restricted to the plain `Alert`
1069-
* tag with no value or query-id annotation:
1104+
* tag with no value or query-id annotation, on comments that carry a single expectation:
10701105
*
10711106
* - an actual result with no matching expectation gets a new `// $ Alert` comment appended
1072-
* (an *unexpected result*), and
1073-
* - a plain `// $ Alert` comment whose result is now missing, and which is the only
1074-
* expectation on that comment, is removed (a *missing result*).
1107+
* (an *unexpected result*);
1108+
* - a comment whose sole expectation is a plain `// $ Alert` whose result is now missing, or a
1109+
* `// $ SPURIOUS: Alert` whose result no longer fires, is removed (a *missing result* or a
1110+
* *fixed spurious result*); and
1111+
* - a comment whose sole expectation is `// $ MISSING: Alert` whose result now appears is
1112+
* promoted to a plain `// $ Alert` (a *fixed missing result*).
10751113
*
1076-
* Cases that need sub-comment column information (promoting `MISSING:`/clearing `SPURIOUS:`,
1077-
* or editing one tag among several on a line) are intentionally left for a follow-up.
1114+
* Cases that need sub-comment column information (editing one tag among several on a comment
1115+
* that carries multiple expectations) are intentionally left for a follow-up.
10781116
*/
10791117
query predicate learnEdits(
10801118
string file, int line, string operation, int startColumn, int endColumn, string text
@@ -1102,36 +1140,52 @@ module TestPostProcessing {
11021140
text = comment
11031141
)
11041142
or
1105-
// Missing result: remove a plain `// $ Alert` comment that is the comment's sole
1106-
// expectation and no longer matches any actual result.
1107-
exists(Test::GoodTestExpectation expectation, string relativePath, int sl, int sc |
1143+
// Obsolete expectation: remove a comment whose sole expectation is either a plain
1144+
// `// $ Alert` that no longer matches any result (a *missing result*), or a
1145+
// `// $ SPURIOUS: Alert` whose result no longer fires, so the known false positive is fixed
1146+
// (a *fixed spurious result*). In both cases the whole comment is deleted.
1147+
exists(Test::FailureLocatable expectation, string relativePath, int sl, int sc |
1148+
(
1149+
expectation instanceof Test::GoodTestExpectation or
1150+
expectation instanceof Test::FalsePositiveTestExpectation
1151+
) and
11081152
expectation.getTag() = "Alert" and
11091153
expectation.getValue() = "" and
11101154
not expectation = Test::getAMatchingExpectation(_, _, _, _, _) and
1111-
// the comment carries exactly this one expectation, so removing it wholesale is safe
1112-
count(Test::FailureLocatable other |
1113-
other.getLocation() = expectation.getLocation() and
1114-
(
1115-
other instanceof Test::GoodTestExpectation or
1116-
other instanceof Test::FalsePositiveTestExpectation or
1117-
other instanceof Test::FalseNegativeTestExpectation
1118-
)
1119-
) = 1 and
1120-
not exists(Test::InvalidTestExpectation invalid |
1121-
invalid.getLocation() = expectation.getLocation()
1122-
) and
1155+
isSoleExpectationOnComment(expectation) and
11231156
parseLocationString(expectation.getLocation().getRelativeUrl(), relativePath, sl, sc, _, _) and
11241157
file = relativePath and
11251158
line = sl and
11261159
operation = "replace" and
11271160
startColumn = sc and
11281161
// A trailing inline expectation comment always runs to the end of its line, so delete from
11291162
// the comment marker to the end of the line. `endColumn = 0` is the engine's "to end of
1130-
// line" convention; using it avoids depending on how each extractor reports a line
1131-
// comment's end column (e.g. Swift reports it as ending at column 1 of the next line).
1163+
// line" convention (it also trims the whitespace gap the removed comment leaves behind);
1164+
// using it avoids depending on how each extractor reports a line comment's end column
1165+
// (e.g. Swift reports it as ending at column 1 of the next line).
11321166
endColumn = 0 and
11331167
text = ""
11341168
)
1169+
or
1170+
// Fixed missing result: a comment whose sole expectation is `// $ MISSING: Alert` now has a
1171+
// matching result, so promote it to a plain `// $ Alert`. The whole comment is rewritten in
1172+
// place, from its comment marker to the end of the line, with a freshly rendered plain
1173+
// comment. Rewriting wholesale is reliable for a single-expectation comment and avoids
1174+
// editing individual columns within the comment. The replacement text is non-empty, so
1175+
// unlike the removal case the whitespace before the comment is preserved.
1176+
exists(Test::FalseNegativeTestExpectation expectation, string relativePath, int sl, int sc |
1177+
expectation.getTag() = "Alert" and
1178+
expectation.getValue() = "" and
1179+
expectation = Test::getAMatchingExpectation(_, _, _, _, _) and
1180+
isSoleExpectationOnComment(expectation) and
1181+
parseLocationString(expectation.getLocation().getRelativeUrl(), relativePath, sl, sc, _, _) and
1182+
file = relativePath and
1183+
line = sl and
1184+
operation = "replace" and
1185+
startColumn = sc and
1186+
endColumn = 0 and
1187+
text = renderInlineComment(relativePath, "Alert")
1188+
)
11351189
}
11361190
}
11371191
}

0 commit comments

Comments
 (0)