Support multi-tag column surgery and tag-generic learning in --learn inline expectations#22208
Draft
d10c wants to merge 9 commits into
Draft
Support multi-tag column surgery and tag-generic learning in --learn inline expectations#22208d10c wants to merge 9 commits into
--learn inline expectations#22208d10c wants to merge 9 commits into
Conversation
Add a `learnEdits` query predicate to `TestPostProcessing::Make` so that
`codeql test run --learn` can update inline `// $ Alert` expectation comments in
source files, instead of only rewriting `.expected`. The test runner consumes
this predicate and applies the edits; here we only compute them.
The predicate emits a deliberately reliable MVP subset, restricted to the plain
`Alert` tag with no value or query-id annotation:
- an actual result with no matching expectation -> append a new `// $ Alert`
comment on the result's line ("append" operation), and
- a plain `// $ Alert` comment that is the sole expectation on its line and no
longer matches any result -> remove the comment ("replace" with "").
The comment is appended on, and removed from, the result's *end* line: an
expectation matches a result when the expectation's start line equals the
result's end line (see `onSameLine`). Most results span a single line, but some
extractors include leading trivia in a location (e.g. Rust), so start and end
lines can differ. Removal deletes from the comment marker to the end of the
line ("replace" with an end column of 0, the engine's "to end of line"
convention); this avoids depending on how each extractor reports a line
comment's end column (e.g. Swift reports it as ending at column 1 of the next
line).
Cases needing sub-comment column surgery (promoting `MISSING:`, clearing
`SPURIOUS:`, or editing one tag among several) are left for a follow-up.
To render a new comment, `InputSig` gains `getStartCommentMarker(relativePath)`
and a defaulted `getEndCommentMarker(relativePath)`. These are keyed on the
source file's relative path rather than the analysed language, because one
database can mix languages with different comment syntaxes (e.g. Java + XML);
each per-language Input module returns a marker only for the file types whose
comment syntax it supports. Languages whose extractor can ingest other file
types (e.g. C# also extracts XML, JavaScript also extracts HTML) gate on the
file extension so those files are skipped; extractors that only ingest a single
line-comment language (e.g. Swift) can return a constant. Both predicates are
`bindingset[relativePath]` so they need not be materialised for all files. The
end marker is defaulted to "" so existing modules need only implement the start
marker; this leaves the door open for block-comment (XML/YAML) languages in a
later PR without another signature break.
No committed QL test accompanies this change: the predicate is only observable
through the engine's `--learn` handling (a released engine rejects the reserved
`learnEdits` name outright), so it cannot be pinned via a `.expected` file. It
is instead covered by engine-side end-to-end tests, one per language.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The C/C++ start-comment marker regex listed `.m`/`.mm` (Objective-C and Objective-C++), but the cpp extractor does not extract Objective-C, so those files never appear in a database and the entries were dead. Drop them and reword the comment to say we only render for C/C++ sources, per review feedback. Also fix the US spelling "analyzed" in the shared getStartCommentMarker doc comment, flagged by the misspelling check. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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>
`codeql test run --learn` previously handled inline expectations one expectation at a time: it could append a new `// $ Alert`, delete a comment whose *sole* expectation no longer matched, and promote a comment whose sole expectation was `// $ MISSING: Alert`. Comments that carried several expectations were left untouched, because the old code had no way to edit one expectation among several without disturbing its siblings. Generalise the rewrite to operate on a comment as a whole. For each comment the postprocess computes the set of expectations that survive learning -- a default or `SPURIOUS:` expectation is kept only while it still matches a result, a `MISSING:` expectation is promoted to the default column once its result fires (and kept otherwise) -- and re-renders the comment from that surviving set, grouped into the default / `SPURIOUS:` / `MISSING:` columns with a deterministic layout. If nothing survives, the comment is deleted (reusing the existing `endColumn = 0` delete-to-end-of-line convention, which also trims the whitespace the removed comment leaves behind). This subsumes the old single-expectation removal and MISSING-promotion disjuncts. Because a single query's postprocess only sees its own tags, the rewrite is gated on `isFullyOwnedComment`: the comment must carry at least one expectation this test understands, no expectation ignored via a foreign query ID, and no unparseable expectation. This prevents a `--learn` run for one query from silently dropping an expectation such as `// $ Alert[other-query]` that belongs to a different query sharing the same source file. Surgically editing such shared comments, and merging a freshly learned tag into an existing comment rather than appending a separate one, are handled in follow-up commits. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The whole-comment rewrite previously refused to touch any comment that also carried an expectation this test ignores -- for example `// $ Alert[other-query] Alert`, where `Alert[other-query]` is annotated with a different query's ID and so is invisible to the current query's postprocess. Rewriting such a comment from only the expectations this test understands would have silently dropped the foreign one, so the guard (`isFullyOwnedComment`) excluded it entirely, leaving even the owned, stale part unfixable. Make these comments surgically editable instead. `getAForeignExpectation` exposes each ignored expectation's verbatim text and column, and the rewrite now renders the union of the surviving owned expectations and the preserved foreign ones. So `// $ Alert[other-query] Alert` on a line that no longer fires becomes `// $ Alert[other-query]`: the owned `Alert` is dropped while the other query's expectation is kept untouched. The guard is renamed to `isRewritableComment` and relaxed accordingly: a comment is rewritable when it has at least one parseable expectation, none unparseable, and no comma-separated group that mixes an owned tag with an ignored one (such a group, e.g. `Alert,Source[other-query]`, would need to be split apart and is left untouched). Genuinely concurrent edits from two different queries' postprocess runs targeting the same comment remain out of scope; that needs the engine to reconcile edits and is not addressed here. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
When `--learn` discovers an unexpected `Alert` result on a line that already carries a rewritable expectation comment, fold the new tag into that comment instead of appending a second `// $ ...` comment beside it. This keeps one comment per line and, in particular, lets a freshly learned tag join an expectation that belongs to a different query sharing the source file (e.g. `// $ Alert[q/other]` becomes `// $ Alert Alert[q/other]`). `mergedNewTag` feeds the learned default-column tag into `desiredExpectation`, so the existing whole-comment rewrite renders it alongside the surviving and foreign expectations. The append disjunct in `learnEdits` is now gated on there being no rewritable comment on the result's line, so append and merge are mutually exclusive: a line with a rewritable comment is always merged, never double-commented. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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>
…rker `codeql test run --learn` could already keep a trailing regular note when rewriting an expectation comment (`// $ Alert // note`), but only for `//`-comment languages: the note was carried verbatim *including* its `//` marker, so deleting the last expectation from a `#`-comment file would have produced an invalid `// note` line rather than `# note`, and a plain comment carrying no expectation at all was never a merge target. Generalise trailing-note handling so it is marker-aware and works for every line-comment language: - Rename `getTrailingComment` to `getTrailingNote` and return the note's *content*, with its own comment marker and surrounding whitespace stripped. It now also matches a plain comment that carries no expectation of its own (`// note`, `# note`), whose whole content is the note. - When re-rendering a comment, the note is re-wrapped with the inner `//` delimiter the framework recognises regardless of the outer marker, so a `#`-comment file renders `# $ Alert // note`. - When the last expectation is removed but a note survives, re-wrap the note in the file's own start marker (`# note`, not the invalid `// note`). - Broaden `isRewritableComment` to accept a plain comment carrying only a note, so an unexpected result on that line merges `Alert` into the comment (`// note` -> `// $ Alert // note`) instead of appending a second comment. `isRewritableComment` is only consulted on the `--learn` path, so ordinary `codeql test run` output is unchanged.
`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>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
d10c
force-pushed
the
d10c/learn-inline-multitag
branch
from
142d460 to
fdb5a2e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #22148. This PR extends
codeql test run --learnfor inlineexpectations from single-
Alertediting to full multi-tag/multi-valuecomment surgery, and makes learning tag-generic.
Until now
--learncould only append or remove a bare// $ Alert. Acomment carrying several expectations, a foreign query's expectation, a
trailing note, a
tag=value, or a non-Alerttag such as apath-problem
Sourcewas either skipped or mishandled. The commits hereaddress that, each with per-language integration coverage:
expectation on a comment belongs to this test, re-render the comment
from the surviving expectations (drop stale/spurious, promote
MISSING:) instead of only touching a soleAlert.different query that shares the source file is kept verbatim, so
rewriting one query's comment never drops another's.
a new result on a line that already has a rewritable comment is merged
in rather than appended as a second comment.
expectations (
// $ Alert // note) survives a rewrite.whose only comment is a plain note merges into it
(
// note->// $ Alert // note), re-wrapping the note by thefile's own comment marker.
any unexpected result (a custom
query predicatetag, atag=value, or a path-problemSource/Sink), not justAlert, andaggregates several new tags on one line into a single deterministic
comment. This is what lets a path-problem learn a
// $ Sourceon thesource line, which a
Sourceexpectation on a line of its own couldnever be produced by
Alert-only learning.Alertbehaviour is unchanged, verified against the existingper-language inline-expectation tests.