Support MISSING promotion and SPURIOUS clearing in --learn inline expectations#22148
Draft
d10c wants to merge 3 commits into
Draft
Support MISSING promotion and SPURIOUS clearing in --learn inline expectations#22148d10c wants to merge 3 commits into
--learn inline expectations#22148d10c wants to merge 3 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>
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
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.
Follow-up to #22140, stacked on its branch. Until #22140 merges, this PR's diff also shows that PR's commit — review only the top commit here (
Support MISSING promotion and SPURIOUS clearing in learnEdits); the diff collapses once #22140 lands.Extends the
learnEditspredicate (used bycodeql test run --learnto rewrite inline// $ Alertexpectations) to two more cases, still restricted to the plainAlerttag on comments that carry a single expectation:// $ MISSING: Alertwhose result now appears is promoted to a plain// $ Alert, by rewriting the whole comment in place from its marker to the end of the line (the whitespace gap before the comment is preserved).// $ SPURIOUS: Alertwhose result no longer fires is removed, reusing the same whole-comment deletion path as a now-missing plainAlert.Both operate on the comment as a whole rather than editing individual columns, which is reliable for a single-expectation comment; the new
isSoleExpectationOnCommentguard (which also excludes comments carrying an unparseable expectation) keeps multi-tag comments out of scope for a later follow-up. The renderer is split intorenderInlineComment(no leading space, for in-place rewrites) andrenderExpectationComment(adds the leading separator space, for appends).The removal path relies on the engine's
endColumn = 0"delete to end of line" convention, which is being updated on the engine side to also trim the whitespace gap the removed trailing comment leaves behind.End-to-end verified with
codeql test run --learnacross all per-language inline-expectation fixtures (cpp, csharp, go, java, javascript, python, ruby, rust, swift); the existingInlineExpectationsTestlibrary test is unaffected (learnEditsis invisible without--learn).