From 0728c0a3ea17c8889010ba059d385b1a0a4e57a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Tue, 3 Nov 2026 18:02:04 +0100 Subject: [PATCH 1/2] Add direct MakeTest inline learning Expose the learnEdits protocol from direct MakeTest queries without relying on query metadata or @id. Share edit emission with qlref postprocessing while restricting direct reconciliation to safe line-comment syntax and leaving unsupported forms for verification. Add Python and Java fixtures covering custom expected tags, foreign tags, standard tags, and missing query IDs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e860263a-9e7b-4451-8b4a-3b5d119a962c --- .../learn-edits/Test.expected | 0 .../learn-edits/Test.java | 3 + .../learn-edits/Test.ql | 19 + .../learn-edits/Test.expected | 0 .../learn-edits/Test.ql | 25 + .../learn-edits/test.py | 1 + .../util/test/InlineExpectationsTest.qll | 508 +++++++++++------- 7 files changed, 365 insertions(+), 191 deletions(-) create mode 100644 java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.expected create mode 100644 java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.java create mode 100644 java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql create mode 100644 python/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.expected create mode 100644 python/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql create mode 100644 python/ql/test/library-tests/InlineExpectationsTest/learn-edits/test.py diff --git a/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.expected b/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.java b/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.java new file mode 100644 index 000000000000..0d313a741236 --- /dev/null +++ b/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.java @@ -0,0 +1,3 @@ +class Test { + void target() {} // $ java-direct +} diff --git a/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql b/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql new file mode 100644 index 000000000000..e10d98139905 --- /dev/null +++ b/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql @@ -0,0 +1,19 @@ +import java +import utils.test.InlineExpectationsTest + +module DirectTest implements TestSig { + string getARelevantTag() { result = "java-direct" } + + predicate hasActualResult(Location location, string element, string tag, string value) { + exists(Method method | + method.getName() = "target" and + method.fromSource() and + location = method.getLocation() and + element = method.toString() and + tag = "java-direct" and + value = "" + ) + } +} + +import MakeTest diff --git a/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.expected b/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql b/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql new file mode 100644 index 000000000000..5be03368d83b --- /dev/null +++ b/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql @@ -0,0 +1,25 @@ +import python +import utils.test.InlineExpectationsTest + +module DirectTest implements TestSig { + string getARelevantTag() { result = "direct" } + + string getARelevantExpectedTag() { result = ["direct", "direct[alias]"] } + + bindingset[expectedTag, actualTag] + predicate tagMatches(string expectedTag, string actualTag) { + expectedTag = ["direct", "direct[alias]"] and actualTag = "direct" + } + + predicate hasActualResult(Location location, string element, string tag, string value) { + exists(Name name | + name.getId() = "target" and + location = name.getLocation() and + element = name.toString() and + tag = "direct" and + value = "" + ) + } +} + +import MakeTest diff --git a/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/test.py b/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/test.py new file mode 100644 index 000000000000..b56884865e80 --- /dev/null +++ b/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/test.py @@ -0,0 +1 @@ +target = 1 # $ direct[alias] foreign diff --git a/shared/util/codeql/util/test/InlineExpectationsTest.qll b/shared/util/codeql/util/test/InlineExpectationsTest.qll index 789cc6133bb6..9550eb79c4d1 100644 --- a/shared/util/codeql/util/test/InlineExpectationsTest.qll +++ b/shared/util/codeql/util/test/InlineExpectationsTest.qll @@ -149,6 +149,15 @@ signature module InlineExpectationsTestSig { } } +bindingset[loc] +private predicate parseRelativeUrl(string loc, string relativePath, int sl, int sc, int el, int ec) { + relativePath = loc.splitAt(":", 0) and + sl = loc.splitAt(":", 1).toInt() and + sc = loc.splitAt(":", 2).toInt() and + el = loc.splitAt(":", 3).toInt() and + ec = loc.splitAt(":", 4).toInt() +} + /** * Module implementing inline expectations. */ @@ -176,6 +185,15 @@ module Make { bindingset[expectedTag, actualTag] default predicate tagMatches(string expectedTag, string actualTag) { expectedTag = actualTag } + /** + * Gets an expected tag that belongs to this test. + * + * This should be overridden when `tagMatches` accepts aliases other than the tags returned by + * `getARelevantTag`. + */ + bindingset[result] + default string getARelevantExpectedTag() { result = getARelevantTag() } + /** Holds if expectations marked with `expectedTag` are ignored. */ bindingset[expectedTag] default predicate tagIsIgnored(string expectedTag) { none() } @@ -217,6 +235,118 @@ module Make { } } + /** + * Grammar-local operations shared by direct tests and postprocessing queries when rendering edits. + */ + module CommentRenderer { + bindingset[relativePath, content] + string wrapInComment(string relativePath, string content) { + exists(string startMarker | + startMarker = Impl::getStartCommentMarker(relativePath) and + if exists(Impl::getEndCommentMarker(relativePath)) + then result = startMarker + " " + content + " " + Impl::getEndCommentMarker(relativePath) + else result = startMarker + " " + content + ) + } + + /** + * Gets the exclusive end column for replacing `comment`. Same-line locations use inclusive end + * columns, while a multi-line comment location uses the engine's replace-to-EOL sentinel. + */ + int getReplacementEndColumn(Impl::ExpectationComment comment) { + exists(int sl, int el, int ec | + parseRelativeUrl(Impl::getRelativeUrl(comment.getLocation()), _, sl, _, el, ec) and + if sl = el then result = ec + 1 else result = 0 + ) + } + } + + /** + * The reconciliation decisions needed by the shared edit emitter. + * + * Direct tests and postprocessing queries implement this differently, but both use the same edit + * protocol and grammar-local comment rendering. + */ + signature module LearningPlanSig { + predicate unexpectedResultExpectation(string relativePath, int line, string text); + + predicate canRewriteComment(Impl::ExpectationComment comment); + + predicate commentNeedsRewrite(Impl::ExpectationComment comment); + + predicate hasDesiredExpectation(Impl::ExpectationComment comment); + + bindingset[relativePath, comment] + string renderReplacementComment(string relativePath, Impl::ExpectationComment comment); + + bindingset[relativePath, comment] + predicate renderWithoutExpectations( + string relativePath, Impl::ExpectationComment comment, string text, int endColumn + ); + } + + /** + * Emits the engine's `learnEdits` protocol from a mode-specific reconciliation plan. + * + * Each row requests either: + * + * - `append`, with both columns set to 0, to add `text` at the end of `line`; or + * - `replace`, with a 1-based half-open column range. An `endColumn` of 0 replaces through the end + * of the line. + */ + module LearnEdits { + bindingset[relativePath, line] + private string renderAppendedComment(string relativePath, int line) { + exists(string body | + Plan::unexpectedResultExpectation(relativePath, line, _) and + body = + concat(string text | + Plan::unexpectedResultExpectation(relativePath, line, text) + | + text, " " order by text + ) and + result = " " + CommentRenderer::wrapInComment(relativePath, "$ " + body) + ) + } + + query predicate learnEdits( + string file, int line, string operation, int startColumn, int endColumn, string text + ) { + exists(string relativePath, int resultLine | + Plan::unexpectedResultExpectation(relativePath, resultLine, _) and + not exists(Impl::ExpectationComment comment | + parseRelativeUrl(Impl::getRelativeUrl(comment.getLocation()), relativePath, resultLine, _, + _, _) + ) and + file = relativePath and + line = resultLine and + operation = "append" and + startColumn = 0 and + endColumn = 0 and + text = renderAppendedComment(relativePath, resultLine) + ) + or + exists(Impl::ExpectationComment comment, string relativePath, int sl, int sc | + Plan::canRewriteComment(comment) and + Plan::commentNeedsRewrite(comment) and + parseRelativeUrl(Impl::getRelativeUrl(comment.getLocation()), relativePath, sl, sc, _, _) and + exists(Impl::getStartCommentMarker(relativePath)) and + file = relativePath and + line = sl and + operation = "replace" and + startColumn = sc and + ( + Plan::hasDesiredExpectation(comment) and + text = Plan::renderReplacementComment(relativePath, comment) and + endColumn = CommentRenderer::getReplacementEndColumn(comment) + or + not Plan::hasDesiredExpectation(comment) and + Plan::renderWithoutExpectations(relativePath, comment, text, endColumn) + ) + ) + } + } + /** * The module for tests with inline expectations. The test implements the signature to provide * the actual results of the query, which are then compared with the expected results in comments @@ -410,6 +540,148 @@ module Make { query predicate testFailures(FailureLocatable element, string message) { hasFailureMessage(element, message) } + + /** + * Implements direct-query learning for the deliberately narrow syntax that can be reconciled + * without query metadata: + * + * - line comments containing default-column expectations separated by ASCII spaces; + * - arbitrary tags declared by `getARelevantExpectedTag`, with unrelated tags preserved; and + * - single-line actual-result locations. + * + * Tabs between expectation atoms, named columns, comma groups, trailing notes, block comments, + * and multi-line results are left untouched. If one of those forms needs an edit, the + * verification run reports the remaining test failure instead of risking source corruption. The + * runner must also reject directories with more than one direct producer, because this module + * does not reconcile edits from independent queries. + */ + private module DirectLearningPlan implements LearningPlanSig { + bindingset[tag] + private predicate tagIsForeign(string tag) { + TestImpl::tagIsIgnored(tag) + or + not tag = TestImpl::getARelevantExpectedTag() + } + + private string directExpectationPattern() { + exists(string tag | + tag = "[A-Za-z_-][A-Za-z0-9_-]*(?:\\[[^\\[\\]]+\\])?" and + result = tag + "(?:=\\S*)?" + ) + } + + predicate canRewriteComment(Impl::ExpectationComment comment) { + comment.isLineComment() and + hasExpectation(comment, _, _, _, _) and + not exists(InvalidTestExpectation invalid | invalid.getLocation() = comment.getLocation()) and + not hasExpectation(comment, TNamedColumn(_), _, _, _) and + not exists(string tags | + hasExpectation(comment, TDefaultColumn(), _, tags, _) and tags.matches("%,%") + ) and + not exists(comment.getContents().regexpCapture(expectationCommentPattern(), 2)) and + exists(string content | + content = comment.getContents().regexpCapture(expectationCommentPattern(), 1).trim() and + content != "" and + not content.matches("%\t%") and + forall(string token | token = content.splitAt(" ") and token != "" | + token.regexpMatch(directExpectationPattern()) + ) + ) and + not exists(ActualTestResult actualResult, string relativePath, int sl, int el | + not actualResult.isOptional() and + actualResult.getTag() = TestImpl::getARelevantTag() and + not exists( + getAMatchingExpectation(actualResult.getLocation(), actualResult.toString(), + actualResult.getTag(), actualResult.getValue(), false) + ) and + parseRelativeUrl(Impl::getRelativeUrl(actualResult.getLocation()), relativePath, sl, _, + el, _) and + sl != el and + parseRelativeUrl(Impl::getRelativeUrl(comment.getLocation()), relativePath, el, _, _, _) + ) + } + + private predicate foreignExpectation(Impl::ExpectationComment comment, string text) { + exists(string tags | + hasExpectation(comment, TDefaultColumn(), text, tags, _) and + forall(string tag | tag = tags.splitAt(",") | tagIsForeign(tag)) + ) + } + + private predicate survivingExpectation(Impl::ExpectationComment comment, string text) { + exists(GoodTestExpectation expectation | + expectation.getLocation() = comment.getLocation() and + expectation = getAMatchingExpectation(_, _, _, _, _) and + text = expectation.getExpectationText() + ) + } + + predicate unexpectedResultExpectation(string relativePath, int line, string text) { + exists(ActualTestResult actualResult | + not actualResult.isOptional() and + actualResult.getTag() = TestImpl::getARelevantTag() and + not exists( + getAMatchingExpectation(actualResult.getLocation(), actualResult.toString(), + actualResult.getTag(), actualResult.getValue(), false) + ) and + parseRelativeUrl(Impl::getRelativeUrl(actualResult.getLocation()), relativePath, line, _, + line, _) and + text = actualResult.getExpectationText() + ) + } + + private predicate desiredExpectation(Impl::ExpectationComment comment, string text) { + survivingExpectation(comment, text) + or + foreignExpectation(comment, text) + or + exists(string relativePath, int line | + parseRelativeUrl(Impl::getRelativeUrl(comment.getLocation()), relativePath, line, _, _, _) and + unexpectedResultExpectation(relativePath, line, text) + ) + } + + private predicate currentExpectation(Impl::ExpectationComment comment, string text) { + foreignExpectation(comment, text) + or + exists(string tags, string value | + hasExpectation(comment, TDefaultColumn(), _, tags, value) and + not tagIsForeign(tags) and + if value = "" then text = tags else text = tags + "=" + value + ) + } + + predicate commentNeedsRewrite(Impl::ExpectationComment comment) { + exists(string text | + desiredExpectation(comment, text) and not currentExpectation(comment, text) + ) + or + exists(string text | + currentExpectation(comment, text) and not desiredExpectation(comment, text) + ) + } + + predicate hasDesiredExpectation(Impl::ExpectationComment comment) { + desiredExpectation(comment, _) + } + + bindingset[relativePath, comment] + string renderReplacementComment(string relativePath, Impl::ExpectationComment comment) { + exists(string body | + body = concat(string text | desiredExpectation(comment, text) | text, " " order by text) and + result = CommentRenderer::wrapInComment(relativePath, "$ " + body) + ) + } + + bindingset[relativePath, comment] + predicate renderWithoutExpectations( + string relativePath, Impl::ExpectationComment comment, string text, int endColumn + ) { + exists(relativePath) and exists(comment) and text = "" and endColumn = 0 + } + } + + import LearnEdits } /** @@ -434,6 +706,13 @@ module Make { result = TestImpl1::getARelevantTag() or result = TestImpl2::getARelevantTag() } + bindingset[result] + string getARelevantExpectedTag() { + result = TestImpl1::getARelevantExpectedTag() + or + result = TestImpl2::getARelevantExpectedTag() + } + bindingset[expectedTag] predicate tagIsIgnored(string expectedTag) { TestImpl1::tagIsIgnored(expectedTag) or @@ -461,6 +740,8 @@ module Make { predicate getARelevantTag = M::getARelevantTag/0; + predicate getARelevantExpectedTag = M::getARelevantExpectedTag/0; + predicate tagIsIgnored = M::tagIsIgnored/1; predicate hasActualResult = M::hasActualResult/4; @@ -478,6 +759,8 @@ module Make { predicate getARelevantTag = M::getARelevantTag/0; + predicate getARelevantExpectedTag = M::getARelevantExpectedTag/0; + predicate tagIsIgnored = M::tagIsIgnored/1; predicate hasActualResult = M::hasActualResult/4; @@ -497,6 +780,8 @@ module Make { predicate getARelevantTag = M::getARelevantTag/0; + predicate getARelevantExpectedTag = M::getARelevantExpectedTag/0; + predicate tagIsIgnored = M::tagIsIgnored/1; predicate hasActualResult = M::hasActualResult/4; @@ -710,22 +995,11 @@ module TestPostProcessing { module Make { private import InlineExpectationsTest as InlineExpectationsTest - bindingset[loc] - private predicate parseLocationString( - string loc, string relativePath, int sl, int sc, int el, int ec - ) { - relativePath = loc.splitAt(":", 0) and - sl = loc.splitAt(":", 1).toInt() and - sc = loc.splitAt(":", 2).toInt() and - el = loc.splitAt(":", 3).toInt() and - ec = loc.splitAt(":", 4).toInt() - } - pragma[nomagic] private string getRelativePathTo(string absolutePath) { exists(Input::Location loc | loc.hasLocationInfo(absolutePath, _, _, _, _) and - parseLocationString(Input::getRelativeUrl(loc), result, _, _, _, _) + parseRelativeUrl(Input::getRelativeUrl(loc), result, _, _, _, _) ) } @@ -734,7 +1008,7 @@ module TestPostProcessing { MkResultLocation(string relativePath, int sl, int sc, int el, int ec) { exists(string data | queryResults(_, _, _, data) and - parseLocationString(data, relativePath, sl, sc, el, ec) and + parseRelativeUrl(data, relativePath, sl, sc, el, ec) and not Input::getRelativeUrl(_) = data // avoid duplicate locations ) } @@ -1110,58 +1384,8 @@ module TestPostProcessing { relation = "testFailures" } - /** - * The implementation of `--learn`'s inline-expectation editing. Every helper is grouped in this - * private module so that only the public `query predicate learnEdits` leaks into the enclosing - * scope (re-exposed by the `import LearnEditsImpl` below); all the supporting predicates stay - * private to this module. - */ - private module LearnEditsImpl { - /** - * Gets `content` wrapped in the comment markers of the file with the given `relativePath`, for - * example `// content` for a line-comment file or `` for a block-comment file, - * or has no result if that file's comment syntax is not supported. - * - * This renders the comment markers only; the caller supplies whatever `content` goes between - * them, whether a ` $ `-prefixed inline expectation or a plain trailing note kept when the last - * expectation is removed. No leading whitespace is added: the append path adds its own separator - * from the preceding code, while the replace path substitutes the comment in place. - */ - bindingset[relativePath, content] - private string wrapInComment(string relativePath, string content) { - exists(string startMarker | - startMarker = Input::getStartCommentMarker(relativePath) and - if exists(Input::getEndCommentMarker(relativePath)) - then result = startMarker + " " + content + " " + Input::getEndCommentMarker(relativePath) - else result = startMarker + " " + content - ) - } - - /** - * Gets the fully rendered inline expectation comment (including the leading separator and the - * comment markers) that `--learn` should *append* to `line` of `relativePath`, carrying *every* - * expectation freshly learned for that line (see `unexpectedResultExpectation`) in a single - * comment, or has no result if that file's comment syntax is not supported. - * - * The leading space separates the appended comment from the existing code on the line (for a - * `//`-comment file this yields ` // $ Alert`). The expectations are ordered lexically, matching - * `renderLearnedColumn`, so that a line on which several results fire (for example a path-problem - * source and sink that coincide) grows one deterministic comment such as `// $ Sink Source` - * rather than several separate comments. - */ - bindingset[relativePath, line] - private string renderAppendedComment(string relativePath, int line) { - exists(string body | - body = - concat(string text | - unexpectedResultExpectation(relativePath, line, text) - | - text, " " order by text - ) and - result = " " + wrapInComment(relativePath, "$ " + body) - ) - } - + /** The qlref reconciliation policy used by the shared edit emitter. */ + private module PostprocessingLearningPlan implements LearningPlanSig { /** * Holds if `comment` is one that `codeql test run --learn` may rewrite as a whole. * @@ -1183,7 +1407,7 @@ module TestPostProcessing { * belongs to a different query sharing the same source file. A comment carrying *only* such * ignored expectations (or nothing to change) is therefore rewritable but is left as is. */ - private predicate canRewriteComment(TestImpl2::ExpectationComment comment) { + predicate canRewriteComment(TestImpl2::ExpectationComment comment) { comment.isLineComment() and not exists(Test::InvalidTestExpectation invalid | invalid.getLocation() = comment.getLocation() @@ -1286,7 +1510,7 @@ module TestPostProcessing { string relativePath, int line, string tag, string value, string queryId ) { exists(TestImpl2::ExpectationComment comment, string full, string base, string qualifier | - parseLocationString(comment.getLocation().getRelativeUrl(), relativePath, line, _, _, _) and + parseRelativeUrl(comment.getLocation().getRelativeUrl(), relativePath, line, _, _, _) and hasExpectation(comment, TDefaultColumn(), _, full, value) and not full.matches("%,%") and parseQualifiedTag(full, base, qualifier) and @@ -1304,8 +1528,8 @@ module TestPostProcessing { exists(Test::ActualTestResult actualResult | not actualResult.isOptional() and actualResult.getTag() != "RelatedLocation" and - parseLocationString(actualResult.getLocation().getRelativeUrl(), relativePath, _, _, - endLine, _) and + parseRelativeUrl(actualResult.getLocation().getRelativeUrl(), relativePath, _, _, endLine, + _) and tag = actualResult.getTag() and value = actualResult.getValue() ) @@ -1388,8 +1612,7 @@ module TestPostProcessing { private string renderSurvivingDefaultExpectation(Test::FailureLocatable expectation) { exists(string relativePath, int line, string base, string queryId | parseQualifiedTag(expectation.getTag(), base, queryId) and - parseLocationString(expectation.getLocation().getRelativeUrl(), relativePath, line, _, _, - _) and + parseRelativeUrl(expectation.getLocation().getRelativeUrl(), relativePath, line, _, _, _) and ( inCohortMode() and (queryId = "" or queryId = getQueryId()) and @@ -1400,19 +1623,6 @@ module TestPostProcessing { ) } - /** - * Gets the exclusive end column for replacing `comment`. Same-line CodeQL locations use - * inclusive end columns, so their end boundary is one column later. A comment whose location - * ends on another line uses 0, the engine's replace-to-EOL sentinel; this covers extractors - * that report a line comment as ending at column 1 of the next line. - */ - private int getReplacementEndColumn(TestImpl2::ExpectationComment comment) { - exists(int sl, int el, int ec | - parseLocationString(comment.getLocation().getRelativeUrl(), _, sl, _, el, ec) and - if sl = el then result = ec + 1 else result = 0 - ) - } - /** * Holds if, after `--learn`, `comment` should carry the expectation `text` in `column` (`""` * for the default column, or `"SPURIOUS"` / `"MISSING"`). @@ -1479,7 +1689,7 @@ module TestPostProcessing { * line already references them (see `hasRelatedLocation`/`shouldReportRelatedLocations`), so * they never constitute a genuinely new result that learning should introduce on its own. */ - private predicate unexpectedResultExpectation(string relativePath, int endLine, string text) { + predicate unexpectedResultExpectation(string relativePath, int endLine, string text) { exists(Test::ActualTestResult actualResult | not actualResult.isOptional() and actualResult.getTag() != "RelatedLocation" and @@ -1487,7 +1697,7 @@ module TestPostProcessing { Test::getAMatchingExpectation(actualResult.getLocation(), actualResult.toString(), actualResult.getTag(), actualResult.getValue(), false) ) and - parseLocationString(actualResult.getLocation().getRelativeUrl(), relativePath, endLine, _, + parseRelativeUrl(actualResult.getLocation().getRelativeUrl(), relativePath, endLine, _, endLine, _) and text = renderCohortAtom(relativePath, endLine, actualResult.getTag(), actualResult.getValue()) @@ -1505,7 +1715,7 @@ module TestPostProcessing { TestImpl2::ExpectationComment comment, string column, string text ) { exists(string relativePath, int line | - parseLocationString(comment.getLocation().getRelativeUrl(), relativePath, line, _, _, _) and + parseRelativeUrl(comment.getLocation().getRelativeUrl(), relativePath, line, _, _, _) and unexpectedResultExpectation(relativePath, line, text) and column = "" ) @@ -1600,7 +1810,7 @@ module TestPostProcessing { or exists(string relativePath, int line, string tag, string value | column = "" and - parseLocationString(comment.getLocation().getRelativeUrl(), relativePath, line, _, _, _) and + parseRelativeUrl(comment.getLocation().getRelativeUrl(), relativePath, line, _, _, _) and isCollapsedAtom(relativePath, line, tag, value) and hasCompleteQualifiedRepresentation(comment, tag, value) and text = renderAtom(tag, value) @@ -1621,14 +1831,14 @@ module TestPostProcessing { hasExpectation(comment, TDefaultColumn(), _, full, value) and parseQualifiedTag(full, tag, queryId) and cohortMember(queryId) and - parseLocationString(comment.getLocation().getRelativeUrl(), relativePath, line, _, _, _) and + parseRelativeUrl(comment.getLocation().getRelativeUrl(), relativePath, line, _, _, _) and isCollapsedAtom(relativePath, line, tag, value) and hasCompleteQualifiedRepresentation(comment, tag, value) ) } /** Holds if `--learn` should change the set of expectations carried by `comment`. */ - private predicate commentNeedsRewrite(TestImpl2::ExpectationComment comment) { + predicate commentNeedsRewrite(TestImpl2::ExpectationComment comment) { exists(string column, string text | desiredExpectation(comment, column, text) and not desiredExpectationIsRepresented(comment, column, text) @@ -1674,9 +1884,7 @@ module TestPostProcessing { * caller deletes the comment instead) or the file's comment syntax is unsupported. */ bindingset[relativePath] - private string renderReplacementComment( - string relativePath, TestImpl2::ExpectationComment comment - ) { + string renderReplacementComment(string relativePath, TestImpl2::ExpectationComment comment) { exists(string body, string trailingSuffix | // Preserve a trailing regular note (e.g. the `note` in `// $ Alert // note`) that sits // after the expectations, so rewriting the expectations never drops an explanatory note. @@ -1696,112 +1904,30 @@ module TestPostProcessing { | renderLearnedColumn(comment, column), " " order by getColumnRank(column) ) and - result = wrapInComment(relativePath, "$ " + body + trailingSuffix) + result = CommentRenderer::wrapInComment(relativePath, "$ " + body + trailingSuffix) ) } - /** - * Holds if `codeql test run --learn` should edit the source file `file` so that its inline - * expectations match the current query results. Each row asks the test runner to change - * `line`, where `operation` is either: - * - * - `"append"`: add `text` (a fully rendered comment) at the end of the line; `startColumn` - * and `endColumn` are both 0. - * - `"replace"`: replace the 1-based half-open column range `[startColumn, endColumn)` with - * `text`. Equal boundaries insert `text`; the empty string deletes a non-empty range. - * `endColumn = 0` extends the range to the end of the line. - * - * The following edits are emitted: - * - * - an actual result with no matching expectation records a new expectation (an *unexpected - * result*): the expectation is the result's tag, carrying a value where the result has one - * (`Alert`, a path-problem `Source`/`Sink`, or a custom `tag=value`). If the result's line - * already has a rewritable comment the expectation is merged into it (see below), otherwise a - * fresh comment carrying every expectation learned for the line is appended (for example - * `// $ Alert`, or `// $ Sink Source` when several results fire on one line); and - * - an existing rewritable expectation comment is rewritten as a whole so that it matches the - * current results: obsolete default and `// $ SPURIOUS:` expectations are dropped (a *missing - * result* or a *fixed spurious result*), a `// $ MISSING:` expectation whose result now fires - * is promoted to the default column (a *fixed missing result*), any freshly learned tags on - * the line are merged in, and the resulting expectations are re-rendered. If nothing remains, - * the comment is deleted. - * - * The rewrite handles comments that carry several expectations across the default, - * `SPURIOUS:`, and `MISSING:` columns. Expectations this test ignores (for example a tag - * annotated with a different query's ID) are preserved verbatim, so the comment keeps any - * expectation belonging to a different query that shares the same source file; see - * `isRewritableComment` and `foreignExpectation`. A trailing regular note after the - * expectations (for example the `note` in `// $ Alert // note` or `# $ Alert // note`) is - * likewise preserved, and freshly learned tags can be merged into a plain comment that carries - * only such a note (`// note` -> `// $ Alert // note`); see `getTrailingNote`. - */ - query predicate learnEdits( - string file, int line, string operation, int startColumn, int endColumn, string text + predicate hasDesiredExpectation(TestImpl2::ExpectationComment comment) { + desiredExpectation(comment, _, _) + } + + bindingset[relativePath, comment] + predicate renderWithoutExpectations( + string relativePath, TestImpl2::ExpectationComment comment, string text, int endColumn ) { - // Unexpected result with no comment to merge into: append a fresh comment carrying every - // expectation learned for the result's line (see `unexpectedResultExpectation`). The comment must - // go on the result's *end* line, because an expectation matches a result when the - // expectation's start line equals the result's end line (see `onSameLine`). - // `unexpectedResultExpectation` deliberately has no result for a multi-line location because - // appending at either endpoint is not generally source-safe. If the line already has a - // comment, appending would place the new marker inside that comment. A rewritable comment - // is merged by the rewrite disjunct below (see `mergedNewExpectation`); an excluded comment - // is left untouched so the verification run reports the residual mismatch. - exists(string relativePath, int el, string comment | - unexpectedResultExpectation(relativePath, el, _) and - not exists(TestImpl2::ExpectationComment existing | - parseLocationString(existing.getLocation().getRelativeUrl(), relativePath, el, _, _, _) - ) and - comment = renderAppendedComment(relativePath, el) and - file = relativePath and - line = el and - operation = "append" and - startColumn = 0 and - endColumn = 0 and - text = comment + exists(string note | + note = getTrailingNote(comment) and + text = CommentRenderer::wrapInComment(relativePath, note) and + endColumn = CommentRenderer::getReplacementEndColumn(comment) ) or - // Rewrite an existing expectation comment as a whole so that it matches the current results. - // This subsumes the single-expectation removal and MISSING-promotion cases and additionally - // handles comments that carry several expectations across the default, `SPURIOUS:`, and - // `MISSING:` columns. A replacement that leaves a comment uses the protocol's exclusive end - // boundary, converted from the comment's inclusive same-line location. Deleting the whole - // comment uses `endColumn = 0`, the engine's to-EOL convention, so the whitespace gap before - // the removed comment is trimmed. Multi-line locations also use the to-EOL convention because - // some extractors (e.g. Swift) report a line comment as ending at column 1 of the next line. - exists(TestImpl2::ExpectationComment comment, string relativePath, int sl, int sc | - canRewriteComment(comment) and - commentNeedsRewrite(comment) and - parseLocationString(comment.getLocation().getRelativeUrl(), relativePath, sl, sc, _, _) and - exists(Input::getStartCommentMarker(relativePath)) and - file = relativePath and - line = sl and - operation = "replace" and - startColumn = sc and - ( - desiredExpectation(comment, _, _) and - text = renderReplacementComment(relativePath, comment) and - endColumn = getReplacementEndColumn(comment) - or - not desiredExpectation(comment, _, _) and - // No expectation survives, so drop the `$ ...` part of the comment. If it carried a - // trailing regular note, keep that note re-wrapped in the file's own comment markers (so a - // `#`-comment file yields `# note`, not an invalid `// note`, and a block-comment file - // yields ``); otherwise delete to the end of line. - ( - exists(string note | - note = getTrailingNote(comment) and - text = wrapInComment(relativePath, note) and - endColumn = getReplacementEndColumn(comment) - ) - or - not exists(getTrailingNote(comment)) and text = "" and endColumn = 0 - ) - ) - ) + not exists(getTrailingNote(comment)) and + text = "" and + endColumn = 0 } } - import LearnEditsImpl + import LearnEdits } } From ae1a0d732244eb9c4f8a69719c151c0358e004bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Wed, 4 Nov 2026 14:10:58 +0100 Subject: [PATCH 2/2] Reject unsafe direct inline expectation edits Require generated expectation atoms to round-trip through the inline grammar before emitting an edit. Let language implementations reject unsafe end-of-line appends, and conservatively disable Python appends where explicit continuation lines cannot be distinguished at this seam. Cover unsafe hash, whitespace, newline, and Python continuation cases through the public learnEdits relation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1adfa307-c48e-43f5-94e3-8286e4a8c083 --- .../learn-edits/Test.expected | 2 + .../learn-edits/Test.java | 5 ++ .../learn-edits/Test.ql | 24 +++++++-- .../internal/InlineExpectationsTestImpl.qll | 4 ++ .../learn-edits/Test.ql | 8 ++- .../learn-edits/test.py | 2 + .../util/test/InlineExpectationsTest.qll | 53 ++++++++++++++++--- 7 files changed, 85 insertions(+), 13 deletions(-) diff --git a/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.expected b/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.expected index e69de29bb2d1..3c28015823fa 100644 --- a/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.expected +++ b/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.expected @@ -0,0 +1,2 @@ +| Test.java | 3 | append | 0 | 0 | // $ java-direct=safe | +| Test.java | 4 | replace | 25 | 37 | // $ foreign java-direct=safe | diff --git a/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.java b/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.java index 0d313a741236..fe0f85783bc8 100644 --- a/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.java +++ b/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.java @@ -1,3 +1,8 @@ class Test { void target() {} // $ java-direct + void safeAppend() {} + void safeReplace() {} // $ foreign + void unsafeHash() {} + void unsafeSpace() {} // $ foreign + void unsafeNewline() {} // $ foreign } diff --git a/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql b/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql index e10d98139905..89847164aa98 100644 --- a/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql +++ b/java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql @@ -4,16 +4,34 @@ import utils.test.InlineExpectationsTest module DirectTest implements TestSig { string getARelevantTag() { result = "java-direct" } + predicate tagIsIgnored(string expectedTag) { expectedTag = "foreign" } + predicate hasActualResult(Location location, string element, string tag, string value) { exists(Method method | - method.getName() = "target" and + method.getName() = + ["target", "safeAppend", "safeReplace", "unsafeHash", "unsafeSpace", "unsafeNewline"] and method.fromSource() and location = method.getLocation() and element = method.toString() and tag = "java-direct" and - value = "" + if method.getName() = "unsafeHash" + then value = "a#b" + else + if method.getName() = "unsafeSpace" + then value = "hello world" + else + if method.getName() = "unsafeNewline" + then value = "hello\nworld" + else + if method.getName() = ["safeAppend", "safeReplace"] + then value = "safe" + else value = "" ) } } -import MakeTest +private module Test = MakeTest; + +from string file, int line, string operation, int startColumn, int endColumn, string text +where Test::learnEdits(file, line, operation, startColumn, endColumn, text) +select file, line, operation, startColumn, endColumn, text diff --git a/python/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll b/python/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll index 48d4b5f7fd74..09600e50a502 100644 --- a/python/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll +++ b/python/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll @@ -30,4 +30,8 @@ module Impl implements InlineExpectationsTestSig { relativePath.regexpMatch(".*\\.(py|pyi)") and result = "#" } + + // Python source lines are not available here, so an explicit `\` continuation cannot be excluded. + bindingset[relativePath, line] + predicate canAppendCommentAtEndOfLine(string relativePath, int line) { none() } } diff --git a/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql b/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql index 5be03368d83b..d5f2c14b8286 100644 --- a/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql +++ b/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql @@ -13,7 +13,7 @@ module DirectTest implements TestSig { predicate hasActualResult(Location location, string element, string tag, string value) { exists(Name name | - name.getId() = "target" and + name.getId() = ["target", "target_continuation"] and location = name.getLocation() and element = name.toString() and tag = "direct" and @@ -22,4 +22,8 @@ module DirectTest implements TestSig { } } -import MakeTest +private module Test = MakeTest; + +from string file, int line, string operation, int startColumn, int endColumn, string text +where Test::learnEdits(file, line, operation, startColumn, endColumn, text) +select file, line, operation, startColumn, endColumn, text diff --git a/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/test.py b/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/test.py index b56884865e80..c6514e112787 100644 --- a/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/test.py +++ b/python/ql/test/library-tests/InlineExpectationsTest/learn-edits/test.py @@ -1 +1,3 @@ target = 1 # $ direct[alias] foreign +target_continuation = 1 \ + + 1 diff --git a/shared/util/codeql/util/test/InlineExpectationsTest.qll b/shared/util/codeql/util/test/InlineExpectationsTest.qll index 9550eb79c4d1..0a6174a2249b 100644 --- a/shared/util/codeql/util/test/InlineExpectationsTest.qll +++ b/shared/util/codeql/util/test/InlineExpectationsTest.qll @@ -124,6 +124,12 @@ signature module InlineExpectationsTestSig { bindingset[relativePath] string getStartCommentMarker(string relativePath); + /** + * Holds if a comment can safely be appended at the end of `line` in `relativePath`. + */ + bindingset[relativePath, line] + default predicate canAppendCommentAtEndOfLine(string relativePath, int line) { any() } + /** * Gets the marker that ends a comment (for example `"-->"`) in the source file with the * given `relativePath`. Has no result by default, which is correct for languages whose @@ -318,6 +324,7 @@ module Make { parseRelativeUrl(Impl::getRelativeUrl(comment.getLocation()), relativePath, resultLine, _, _, _) ) and + Impl::canAppendCommentAtEndOfLine(relativePath, resultLine) and file = relativePath and line = resultLine and operation = "append" and @@ -626,7 +633,8 @@ module Make { ) and parseRelativeUrl(Impl::getRelativeUrl(actualResult.getLocation()), relativePath, line, _, line, _) and - text = actualResult.getExpectationText() + text = actualResult.getExpectationText() and + expectationAtomRoundTrips(actualResult.getTag(), actualResult.getValue(), text) ) } @@ -919,6 +927,23 @@ private module ExpectationParser { } } +bindingset[tag, value, text] +private predicate expectationAtomRoundTrips(string tag, string value, string text) { + exists(string content, string parsedTag, string parsedValue | + content = ("$ " + text).regexpCapture(expectationCommentPattern(), 1).trim() and + content = text and + content.regexpMatch(expectationPattern()) and + parsedTag = content.regexpCapture(expectationPattern(), 1) and + parsedTag = tag and + ( + parsedValue = content.regexpCapture(expectationPattern(), 2) + or + not exists(content.regexpCapture(expectationPattern(), 2)) and parsedValue = "" + ) and + parsedValue = value + ) +} + /** * Provides logic for creating a `@kind test-postprocess` query that checks * inline test expectations using `$ Alert` markers. @@ -1564,21 +1589,29 @@ module TestPostProcessing { } /** - * Renders a current-query atom in its canonical cohort form: bare when every member produces - * it, otherwise qualified with the current query ID. + * Renders a current-query tag in its canonical cohort form: bare when every member produces + * the atom, otherwise qualified with the current query ID. */ bindingset[relativePath, line, tag, value] - private string renderCohortAtom(string relativePath, int line, string tag, string value) { + private string renderCohortTag(string relativePath, int line, string tag, string value) { ( not inCohortMode() or isCollapsedAtom(relativePath, line, tag, value) ) and - result = renderAtom(tag, value) + result = tag or inCohortMode() and not isCollapsedAtom(relativePath, line, tag, value) and - result = renderAtom(tag + "[" + getQueryId() + "]", value) + result = tag + "[" + getQueryId() + "]" + } + + /** + * Renders a current-query atom in its canonical cohort form. + */ + bindingset[relativePath, line, tag, value] + private string renderCohortAtom(string relativePath, int line, string tag, string value) { + result = renderAtom(renderCohortTag(relativePath, line, tag, value), value) } /** @@ -1699,8 +1732,12 @@ module TestPostProcessing { ) and parseRelativeUrl(actualResult.getLocation().getRelativeUrl(), relativePath, endLine, _, endLine, _) and - text = - renderCohortAtom(relativePath, endLine, actualResult.getTag(), actualResult.getValue()) + exists(string renderedTag | + renderedTag = + renderCohortTag(relativePath, endLine, actualResult.getTag(), actualResult.getValue()) and + text = renderAtom(renderedTag, actualResult.getValue()) and + expectationAtomRoundTrips(renderedTag, actualResult.getValue(), text) + ) ) }