diff --git a/actions/ql/lib/codeql/actions/test/internal/InlineExpectationsTestImpl.qll b/actions/ql/lib/codeql/actions/test/internal/InlineExpectationsTestImpl.qll index f1f275fa136a..977386a79be9 100644 --- a/actions/ql/lib/codeql/actions/test/internal/InlineExpectationsTestImpl.qll +++ b/actions/ql/lib/codeql/actions/test/internal/InlineExpectationsTestImpl.qll @@ -21,4 +21,10 @@ module Impl implements InlineExpectationsTestSig { ":" + endColumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + relativePath.regexpMatch(".*\\.ya?ml") and + result = "#" + } } diff --git a/actions/ql/test/utils/ActionsInlineExpectationsTestQuery.ql b/actions/ql/test/utils/ActionsInlineExpectationsTestQuery.ql index cf23944f553d..d6a10ff3627d 100644 --- a/actions/ql/test/utils/ActionsInlineExpectationsTestQuery.ql +++ b/actions/ql/test/utils/ActionsInlineExpectationsTestQuery.ql @@ -20,6 +20,12 @@ private module Impl implements T::InlineExpectationsTestSig { ) } + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + relativePath.regexpMatch(".*\\.ya?ml") and + result = "#" + } + class ExpectationComment extends Yaml::YamlComment { string getContents() { result = this.getText() } } diff --git a/cpp/ql/lib/utils/test/InlineExpectationsTestQuery.ql b/cpp/ql/lib/utils/test/InlineExpectationsTestQuery.ql index e71d67bc528c..d133a7c3ff88 100644 --- a/cpp/ql/lib/utils/test/InlineExpectationsTestQuery.ql +++ b/cpp/ql/lib/utils/test/InlineExpectationsTestQuery.ql @@ -1,5 +1,6 @@ /** * @kind test-postprocess + * @tags inline-expectation-test */ private import cpp diff --git a/cpp/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll b/cpp/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll index 18493c8ad19c..9cd431243e02 100644 --- a/cpp/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll +++ b/cpp/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll @@ -35,4 +35,13 @@ module Impl implements InlineExpectationsTestSig { f.getRelativePath() + ":" + startline + ":" + startcolumn + ":" + endline + ":" + endcolumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + // C/C++ databases can also contain XML (e.g. `.xml`, `.props`), whose block-comment + // syntax is not yet supported, so we only render for C/C++ sources. + relativePath + .regexpMatch(".*\\.(c|cc|cpp|cxx|cp|c\\+\\+|h|hh|hpp|hxx|h\\+\\+|inl|tcc|ipp|tpp|cu|cuh)") and + result = "//" + } } diff --git a/csharp/ql/lib/utils/test/InlineExpectationsTestQuery.ql b/csharp/ql/lib/utils/test/InlineExpectationsTestQuery.ql index 1c5be4160ae0..e968bfed133d 100644 --- a/csharp/ql/lib/utils/test/InlineExpectationsTestQuery.ql +++ b/csharp/ql/lib/utils/test/InlineExpectationsTestQuery.ql @@ -1,5 +1,6 @@ /** * @kind test-postprocess + * @tags inline-expectation-test */ private import csharp diff --git a/csharp/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll b/csharp/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll index f623d683dca6..3383fda8e566 100644 --- a/csharp/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll +++ b/csharp/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll @@ -59,4 +59,12 @@ module Impl implements InlineExpectationsTestSig { f.getRelativePath() + ":" + startline + ":" + startcolumn + ":" + endline + ":" + endcolumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + // C# databases can also contain XML (e.g. `.csproj`, `.config`) and Razor markup, whose + // comment syntaxes are not yet supported, so we only render for C# sources. + relativePath.regexpMatch(".*\\.(cs|csx)") and + result = "//" + } } diff --git a/go/ql/lib/utils/test/InlineExpectationsTestQuery.ql b/go/ql/lib/utils/test/InlineExpectationsTestQuery.ql index 0d7462c57056..b5a7cee488f4 100644 --- a/go/ql/lib/utils/test/InlineExpectationsTestQuery.ql +++ b/go/ql/lib/utils/test/InlineExpectationsTestQuery.ql @@ -1,5 +1,6 @@ /** * @kind test-postprocess + * @tags inline-expectation-test */ private import go diff --git a/go/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll b/go/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll index 3084c4846795..57ef3335087f 100644 --- a/go/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll +++ b/go/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll @@ -30,4 +30,12 @@ module Impl implements InlineExpectationsTestSig { f.getRelativePath() + ":" + startline + ":" + startcolumn + ":" + endline + ":" + endcolumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + // Go databases can also contain XML, whose block-comment syntax is not yet supported, so + // we only render for Go sources. + relativePath.matches("%.go") and + result = "//" + } } diff --git a/java/ql/lib/utils/test/InlineExpectationsTestQuery.ql b/java/ql/lib/utils/test/InlineExpectationsTestQuery.ql index 72cbb9c4148a..3cf3803950a8 100644 --- a/java/ql/lib/utils/test/InlineExpectationsTestQuery.ql +++ b/java/ql/lib/utils/test/InlineExpectationsTestQuery.ql @@ -1,5 +1,6 @@ /** * @kind test-postprocess + * @tags inline-expectation-test */ private import java diff --git a/java/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll b/java/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll index 9a51a70eda60..413a978252e0 100644 --- a/java/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll +++ b/java/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll @@ -45,4 +45,12 @@ module Impl implements InlineExpectationsTestSig { f.getRelativePath() + ":" + startline + ":" + startcolumn + ":" + endline + ":" + endcolumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + // Java databases can also contain XML; those files use a different (block) comment + // syntax that is not yet supported, so we only render for Java and Kotlin sources. + relativePath.matches(["%.java", "%.kt"]) and + result = "//" + } } diff --git a/java/ql/test-kotlin1/TestUtilities/internal/InlineExpectationsTestImpl.qll b/java/ql/test-kotlin1/TestUtilities/internal/InlineExpectationsTestImpl.qll index 646501a84c91..46d3898c44d9 100644 --- a/java/ql/test-kotlin1/TestUtilities/internal/InlineExpectationsTestImpl.qll +++ b/java/ql/test-kotlin1/TestUtilities/internal/InlineExpectationsTestImpl.qll @@ -42,4 +42,10 @@ module Impl implements InlineExpectationsTestSig { f.getRelativePath() + ":" + startline + ":" + startcolumn + ":" + endline + ":" + endcolumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + relativePath.matches(["%.java", "%.kt"]) and + result = "//" + } } diff --git a/java/ql/test-kotlin2/TestUtilities/internal/InlineExpectationsTestImpl.qll b/java/ql/test-kotlin2/TestUtilities/internal/InlineExpectationsTestImpl.qll index 646501a84c91..46d3898c44d9 100644 --- a/java/ql/test-kotlin2/TestUtilities/internal/InlineExpectationsTestImpl.qll +++ b/java/ql/test-kotlin2/TestUtilities/internal/InlineExpectationsTestImpl.qll @@ -42,4 +42,10 @@ module Impl implements InlineExpectationsTestSig { f.getRelativePath() + ":" + startline + ":" + startcolumn + ":" + endline + ":" + endcolumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + relativePath.matches(["%.java", "%.kt"]) and + result = "//" + } } diff --git a/javascript/ql/lib/utils/test/InlineExpectationsTestQuery.ql b/javascript/ql/lib/utils/test/InlineExpectationsTestQuery.ql index 068f69d0013c..80365a13fd5e 100644 --- a/javascript/ql/lib/utils/test/InlineExpectationsTestQuery.ql +++ b/javascript/ql/lib/utils/test/InlineExpectationsTestQuery.ql @@ -1,5 +1,6 @@ /** * @kind test-postprocess + * @tags inline-expectation-test */ private import javascript diff --git a/javascript/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll b/javascript/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll index d73cdcd321b5..34075a879d1c 100644 --- a/javascript/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll +++ b/javascript/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll @@ -18,6 +18,14 @@ module Impl implements InlineExpectationsTestSig { ) } + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + // JavaScript databases can also contain HTML, whose block-comment syntax is not yet + // supported, so we only render for line-comment source files. + relativePath.regexpMatch(".*\\.(js|cjs|mjs|jsx|ts|cts|mts|tsx)") and + result = "//" + } + abstract private class ExpectationCommentImpl extends Locatable { abstract string getContents(); diff --git a/python/ql/lib/utils/test/InlineExpectationsTestQuery.ql b/python/ql/lib/utils/test/InlineExpectationsTestQuery.ql index fa37afa388c0..d3051df4a707 100644 --- a/python/ql/lib/utils/test/InlineExpectationsTestQuery.ql +++ b/python/ql/lib/utils/test/InlineExpectationsTestQuery.ql @@ -1,5 +1,6 @@ /** * @kind test-postprocess + * @tags inline-expectation-test */ private import python diff --git a/python/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll b/python/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll index bc8c66cba5f3..7dfcff1bd730 100644 --- a/python/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll +++ b/python/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll @@ -19,4 +19,12 @@ module Impl implements InlineExpectationsTestSig { f.getRelativePath() + ":" + startline + ":" + startcolumn + ":" + endline + ":" + endcolumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + // Python databases can also contain XML, whose block-comment syntax is not yet supported, + // so we only render for Python sources. + relativePath.regexpMatch(".*\\.(py|pyi)") and + result = "#" + } } diff --git a/ql/ql/src/utils/test/InlineExpectationsTestQuery.ql b/ql/ql/src/utils/test/InlineExpectationsTestQuery.ql index b5c70edad177..969fe2fc3ba7 100644 --- a/ql/ql/src/utils/test/InlineExpectationsTestQuery.ql +++ b/ql/ql/src/utils/test/InlineExpectationsTestQuery.ql @@ -1,5 +1,6 @@ /** * @kind test-postprocess + * @tags inline-expectation-test */ private import ql diff --git a/ql/ql/src/utils/test/internal/InlineExpectationsTestImpl.qll b/ql/ql/src/utils/test/internal/InlineExpectationsTestImpl.qll index 88a30d0d4c7a..8e1cc8d24aee 100644 --- a/ql/ql/src/utils/test/internal/InlineExpectationsTestImpl.qll +++ b/ql/ql/src/utils/test/internal/InlineExpectationsTestImpl.qll @@ -35,4 +35,12 @@ module Impl implements InlineExpectationsTestSig { f.getRelativePath() + ":" + startline + ":" + startcolumn + ":" + endline + ":" + endcolumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + // The QL extractor can also extract YAML (e.g. `qlpack.yml`), whose `#` comment syntax + // differs, so we only render for QL sources and dbscheme files. + relativePath.regexpMatch(".*\\.(ql|qll|dbscheme)") and + result = "//" + } } diff --git a/ruby/ql/lib/utils/test/InlineExpectationsTestQuery.ql b/ruby/ql/lib/utils/test/InlineExpectationsTestQuery.ql index 08768162c173..d029c6152921 100644 --- a/ruby/ql/lib/utils/test/InlineExpectationsTestQuery.ql +++ b/ruby/ql/lib/utils/test/InlineExpectationsTestQuery.ql @@ -1,5 +1,6 @@ /** * @kind test-postprocess + * @tags inline-expectation-test */ private import ruby diff --git a/ruby/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll b/ruby/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll index d445c91d2e8f..2b7afcc62c80 100644 --- a/ruby/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll +++ b/ruby/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll @@ -46,4 +46,12 @@ module Impl implements InlineExpectationsTestSig { f.getRelativePath() + ":" + startline + ":" + startcolumn + ":" + endline + ":" + endcolumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + // Ruby databases can also contain ERB, whose comment syntax is not yet supported, so we + // only render for plain Ruby sources. + relativePath.matches("%.rb") and + result = "#" + } } diff --git a/rust/ql/lib/utils/test/InlineExpectationsTestQuery.ql b/rust/ql/lib/utils/test/InlineExpectationsTestQuery.ql index d97f49cb382f..f4916b0f3606 100644 --- a/rust/ql/lib/utils/test/InlineExpectationsTestQuery.ql +++ b/rust/ql/lib/utils/test/InlineExpectationsTestQuery.ql @@ -1,5 +1,6 @@ /** * @kind test-postprocess + * @tags inline-expectation-test */ private import rust diff --git a/rust/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll b/rust/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll index 5dc1dbaeb03d..3c7c0affcd2f 100644 --- a/rust/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll +++ b/rust/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll @@ -21,4 +21,12 @@ module Impl implements InlineExpectationsTestSig { f.getRelativePath() + ":" + startline + ":" + startcolumn + ":" + endline + ":" + endcolumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + // Rust databases can also contain YAML, whose `#` comment syntax differs, so we only + // render for Rust sources. + relativePath.matches("%.rs") and + result = "//" + } } diff --git a/shared/util/codeql/util/test/InlineExpectationsTest.qll b/shared/util/codeql/util/test/InlineExpectationsTest.qll index 21c8c3a39f64..a1d1452b5209 100644 --- a/shared/util/codeql/util/test/InlineExpectationsTest.qll +++ b/shared/util/codeql/util/test/InlineExpectationsTest.qll @@ -111,6 +111,28 @@ signature module InlineExpectationsTestSig { /** Gets the relative URL of the given location, if any. */ string getRelativeUrl(Location location); + /** + * Gets the marker that starts a line comment (for example `"//"` or `"#"`) in the source + * file with the given `relativePath`, provided that `codeql test run --learn` is able to + * render inline expectations for that file. Files for which this has no result are left + * untouched by `--learn`. + * + * This is keyed on the file rather than on the analyzed language because a single database + * may contain source files in several languages with different comment syntaxes (for + * example Java together with XML). `relativePath` is the path reported by `getRelativeUrl`. + */ + bindingset[relativePath] + string getStartCommentMarker(string relativePath); + + /** + * 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 + * inline expectations use line comments (so `--learn` renders no closing marker); + * block-comment languages can override it so that `--learn` renders a closing marker. + */ + bindingset[relativePath] + default string getEndCommentMarker(string relativePath) { none() } + /** A comment that may contain inline expectations. */ class ExpectationComment { /** Gets the contents of this comment, _excluding_ the comment indicator. */ @@ -128,6 +150,8 @@ signature module InlineExpectationsTestSig { * Module implementing inline expectations. */ module Make { + private import ExpectationParser + /** * A signature specifying the required parts of an inline expectation test. */ @@ -257,14 +281,14 @@ module Make { Impl::ExpectationComment comment, string tag, string value, string knownFailure ) { exists(TColumn column, string tags | - getAnExpectation(comment, column, _, tags, value) and + hasExpectation(comment, column, _, tags, value) and tag = tags.splitAt(",") and knownFailure = getColumnString(column) and not TestImpl::tagIsIgnored(tag) ) } or TInvalidExpectation(Impl::ExpectationComment comment, string expectation) { - getAnExpectation(comment, _, expectation, _, _) and + hasExpectation(comment, _, expectation, _, _) and not expectation.regexpMatch(expectationPattern()) } @@ -385,32 +409,6 @@ module Make { } } - private predicate getAnExpectation( - Impl::ExpectationComment comment, TColumn column, string expectation, string tags, string value - ) { - exists(string content | - content = comment.getContents().regexpCapture(expectationCommentPattern(), 1) and - ( - column = TDefaultColumn() and - exists(int end | - end = getEndOfColumnPosition(0, content) and - expectation = content.prefix(end).regexpFind(expectationPattern(), _, _).trim() - ) - or - exists(string name, int start, int end | - column = TNamedColumn(name) and - start = content.indexOf(name + ":") + name.length() + 1 and - end = getEndOfColumnPosition(start, content) and - expectation = content.substring(start, end).regexpFind(expectationPattern(), _, _).trim() - ) - ) - ) and - tags = expectation.regexpCapture(expectationPattern(), 1) and - if exists(expectation.regexpCapture(expectationPattern(), 2)) - then value = expectation.regexpCapture(expectationPattern(), 2) - else value = "" - } - /** * A module that merges two test signatures. * @@ -511,7 +509,7 @@ module Make { */ predicate hasExpectationWithValue(string tag, string value) { exists(string tags | - getAnExpectation(_, _, _, tags, value) and + hasExpectation(_, _, _, tags, value) and tag = tags.splitAt(",") ) } @@ -589,6 +587,50 @@ private string expectationPattern() { /** Gets the string `#select` or `problems`, which are equivalent result sets for a `problem` or `path-problem` query. */ private string mainResultSet() { result = ["#select", "problems"] } +/** + * Provides the low-level parse of an inline expectation comment, shared by the expectation classes + * in `Make::MakeTest` and by the `--learn` postprocessing in `TestPostProcessing`. + * + * It is a separate module so that both consumers can `private import` it without either exposing + * the parse as part of a `MakeTest` consumer's API or forcing the postprocessing to reach into + * `MakeTest`'s internals. + */ +private module ExpectationParser { + /** + * Holds if `comment` carries the expectation `expectation` (its verbatim text, for example + * `Alert[q]=v`) in `column` (`TDefaultColumn`, or a `TNamedColumn` such as `MISSING`/`SPURIOUS`), + * whose comma-separated tags are `tags` and whose expected value is `value` (`""` if none). + * + * The `--learn` postprocessing needs to see *every* parsed expectation on a comment - including + * ones the running test ignores - so it can preserve them when rewriting. + */ + predicate hasExpectation( + Impl::ExpectationComment comment, TColumn column, string expectation, string tags, string value + ) { + exists(string content | + content = comment.getContents().regexpCapture(expectationCommentPattern(), 1) and + ( + column = TDefaultColumn() and + exists(int end | + end = getEndOfColumnPosition(0, content) and + expectation = content.prefix(end).regexpFind(expectationPattern(), _, _).trim() + ) + or + exists(string name, int start, int end | + column = TNamedColumn(name) and + start = content.indexOf(name + ":") + name.length() + 1 and + end = getEndOfColumnPosition(start, content) and + expectation = content.substring(start, end).regexpFind(expectationPattern(), _, _).trim() + ) + ) + ) and + tags = expectation.regexpCapture(expectationPattern(), 1) and + if exists(expectation.regexpCapture(expectationPattern(), 2)) + then value = expectation.regexpCapture(expectationPattern(), 2) + else value = "" + } +} + /** * Provides logic for creating a `@kind test-postprocess` query that checks * inline test expectations using `$ Alert` markers. @@ -728,6 +770,16 @@ module TestPostProcessing { string getRelativeUrl(Location location) { result = location.getRelativeUrl() } + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + result = Input::getStartCommentMarker(relativePath) + } + + bindingset[relativePath] + string getEndCommentMarker(string relativePath) { + result = Input::getEndCommentMarker(relativePath) + } + final private class ExpectationCommentFinal = Input::ExpectationComment; class ExpectationComment extends ExpectationCommentFinal { @@ -736,6 +788,7 @@ module TestPostProcessing { } private import InlineExpectationsTest::Make + private import ExpectationParser /** Holds if the given locations refer to the same lines, but possibly with different column numbers. */ bindingset[loc1, loc2] @@ -1016,5 +1069,465 @@ module TestPostProcessing { Test::testFailures(_, _) and 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) + ) + } + + /** + * Holds if `comment` is one that `codeql test run --learn` may rewrite as a whole. + * + * Almost every comment is rewritable. It may carry parseable expectations; only a trailing + * note (`// note`, into which a freshly learned tag is merged to give `// $ Alert // note` + * rather than being appended as a second comment); or nothing at all (an empty `//`, into + * which a learned tag is likewise merged to give `// $ Alert`). The only comments excluded are + * one carrying an unparseable expectation, and one that mixes (in a single comma-separated + * group) a tag this test understands with a tag it ignores (for example a query ID that does + * not match the current query). The latter keeps the rewrite from having to take apart a group + * such as `Alert,Source[other-query]` whose parts this test treats differently; such comments + * are left untouched. + * + * A rewritable comment may still carry whole expectations this test ignores (for example + * `// $ Alert[other-query]`). Those are preserved verbatim by `foreignExpectation` when the + * comment is rewritten, so that a `--learn` run for one query never drops an expectation that + * 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 isRewritableComment(TestImpl2::ExpectationComment comment) { + not exists(Test::InvalidTestExpectation invalid | + invalid.getLocation() = comment.getLocation() + ) and + not exists(string tags | + hasExpectation(comment, _, _, tags, _) and + not TestInput::tagIsIgnored(tags.splitAt(",")) and + TestInput::tagIsIgnored(tags.splitAt(",")) + ) + } + + /** + * Holds if `comment` carries a whole expectation this test ignores (for example one annotated + * with a query ID that does not match the current query), whose verbatim text is `text` and + * which sits in `column` (`""` for the default column, or a named column such as `"SPURIOUS"` / + * `"MISSING"`). + * + * `codeql test run --learn` preserves such expectations unchanged when it rewrites the comment, + * because they belong to a different query that shares the same source file and this test + * cannot tell whether they still hold. + */ + private predicate foreignExpectation( + TestImpl2::ExpectationComment comment, string column, string text + ) { + exists(TColumn col, string tags | + hasExpectation(comment, col, text, tags, _) and + column = getColumnString(col) and + forall(string tag | tag = tags.splitAt(",") | TestInput::tagIsIgnored(tag)) + ) + } + + /** + * Holds if `comment` carries a trailing regular (non-interpreted) note `note`, with the note's + * own comment marker and surrounding whitespace stripped. This is either: + * + * - the text after a `//` that follows the expectations in an expectation comment (for example + * `note` in `// $ Alert // note` or `# $ Alert // note`), which the framework treats as an + * ordinary comment (see `expectationCommentPattern`); or + * - the whole content of a plain comment that carries no expectation at all (for example `note` + * in `// note` or `# note`), into which `--learn` may merge a freshly learned tag. + * + * `codeql test run --learn` keeps this note when it rewrites, deletes, or merges into the + * comment, re-wrapping it with the appropriate markers (` $ ... // note` when + * expectations remain, or ` note` when none do), so an explanatory note written next to + * code is never lost. Only `//` delimits such a note within an expectation comment, mirroring + * `expectationCommentPattern`'s `(?:[^/]|/[^/])*` expectation region, which ends only at `//`; a + * `#` never does, so `# $ Alert # note` reads `note` as a tag rather than a note. + */ + private string getTrailingNote(TestImpl2::ExpectationComment comment) { + ( + result = comment.getContents().regexpCapture("\\s*\\$ (?:[^/]|/[^/])*//(.*)", 1).trim() + or + // A plain comment with no expectation of its own: its whole content is the note. + not hasExpectation(comment, _, _, _, _) and + not exists(Test::InvalidTestExpectation invalid | + invalid.getLocation() = comment.getLocation() + ) and + result = comment.getContents().trim() + ) and + result != "" + } + + /** + * 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(string relativePath, int sl, int el, int ec | + parseLocationString(comment.getLocation().getRelativeUrl(), relativePath, 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"`). + * + * These are the expectations that survive learning: a default or `SPURIOUS:` expectation is + * kept only while it still matches a result, a `MISSING:` expectation whose result now fires is + * promoted to the default column, and a `MISSING:` expectation whose result is still absent is + * kept unchanged. + * + * This revisits expectations that are *already written* in `comment` (each + * `Test::FailureLocatable` is a parsed expectation), deciding which to keep and in which column. + * It is the counterpart of `unexpectedResultExpectation`, which instead introduces a *brand-new* + * expectation for a result that currently has none; the two feed `desiredExpectation` together. + */ + private predicate survivingExpectation( + TestImpl2::ExpectationComment comment, string column, string text + ) { + exists(Test::FailureLocatable e | + e.getLocation() = comment.getLocation() and text = e.getExpectationText() + | + // a default expectation that still matches a result is kept + e instanceof Test::GoodTestExpectation and + e = Test::getAMatchingExpectation(_, _, _, _, _) and + column = "" + or + // a `SPURIOUS:` expectation whose result still fires is kept + e instanceof Test::FalsePositiveTestExpectation and + e = Test::getAMatchingExpectation(_, _, _, _, _) and + column = "SPURIOUS" + or + // a `MISSING:` expectation whose result now fires is promoted to the default column + e instanceof Test::FalseNegativeTestExpectation and + e = Test::getAMatchingExpectation(_, _, _, _, _) and + column = "" + or + // a `MISSING:` expectation whose result is still absent is kept + e instanceof Test::FalseNegativeTestExpectation and + not e = Test::getAMatchingExpectation(_, _, _, _, _) and + column = "MISSING" + ) + } + + /** + * Holds if `--learn` should record a new expectation `text` on `endLine` of `relativePath`, + * because a non-optional actual result there has no matching expectation (an *unexpected + * result*). `text` is the fully rendered expectation, so it carries a value where the result + * has one (`Alert`, `Source`, or a custom `tag=value`). + * + * Unlike `survivingExpectation`, which revisits an expectation *already written* in a comment, + * this introduces an expectation for a result that has *no* expectation yet. It is therefore + * keyed on the result's own location (a `relativePath`/line pair) rather than on a comment + * location: there may be no comment on the line at all. Specifically it is keyed 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`). Multi-line results are deliberately + * excluded: appending a line comment at either endpoint can put it inside a string literal or + * change the meaning of the construct, so finding a safe source anchor is left to the + * block-comment/source-anchor follow-up. Whether a new single-line expectation is appended as + * a fresh comment or merged into an existing one is decided by the callers (see the append + * disjunct of `learnEdits` and `mergedNewExpectation`). + * + * `RelatedLocation` results are excluded: they are only reported when an expectation on the + * 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) { + exists(Test::ActualTestResult actualResult | + not actualResult.isOptional() and + actualResult.getTag() != "RelatedLocation" and + not exists( + Test::getAMatchingExpectation(actualResult.getLocation(), actualResult.toString(), + actualResult.getTag(), actualResult.getValue(), false) + ) and + text = actualResult.getExpectationText() and + parseLocationString(actualResult.getLocation().getRelativeUrl(), relativePath, endLine, _, + endLine, _) + ) + } + + /** + * Holds if `--learn` should merge a freshly learned expectation `text` into the existing, + * rewritable `comment` (in `column` `""`, the default), because an unexpected result fires on + * that comment's line. Merging keeps the new tag alongside the comment's existing expectations + * rather than appending a second comment to the line; when several new expectations land on the + * same line they are all merged and re-rendered together (see `renderLearnedColumn`). + */ + private predicate mergedNewExpectation( + TestImpl2::ExpectationComment comment, string column, string text + ) { + exists(string relativePath, int line | + parseLocationString(comment.getLocation().getRelativeUrl(), relativePath, line, _, _, _) and + unexpectedResultExpectation(relativePath, line, text) and + column = "" + ) + } + + /** + * Holds if, after `--learn`, `comment` should carry the expectation `text` in `column` (`""` + * for the default column, or a named column such as `"SPURIOUS"` / `"MISSING"`). + * + * This combines the surviving expectations this test understands (see `survivingExpectation`) + * with the expectations it ignores (see `foreignExpectation`), which are preserved + * verbatim so that rewriting a comment for one query never drops another query's expectation on + * the same line, and with any freshly learned expectations merged into the comment (see + * `mergedNewExpectation`). + */ + private predicate desiredExpectation( + TestImpl2::ExpectationComment comment, string column, string text + ) { + survivingExpectation(comment, column, text) + or + foreignExpectation(comment, column, text) + or + mergedNewExpectation(comment, column, text) + } + + /** + * Holds if `column` (`""` for the default column, or a named column such as `"SPURIOUS"` / + * `"MISSING"`) currently carries the expectation `text` on `comment`. This includes + * expectations this test ignores, so it can be compared against `desiredExpectation`. + */ + private predicate currentExpectation( + TestImpl2::ExpectationComment comment, string column, string text + ) { + exists(Test::FailureLocatable e | + e.getLocation() = comment.getLocation() and text = e.getExpectationText() + | + e instanceof Test::GoodTestExpectation and column = "" + or + e instanceof Test::FalsePositiveTestExpectation and column = "SPURIOUS" + or + e instanceof Test::FalseNegativeTestExpectation and column = "MISSING" + ) + or + foreignExpectation(comment, column, text) + } + + /** Holds if `--learn` should change the set of expectations carried by `comment`. */ + private predicate commentNeedsRewrite(TestImpl2::ExpectationComment comment) { + exists(string column, string text | + desiredExpectation(comment, column, text) and + not currentExpectation(comment, column, text) + ) + or + exists(string column, string text | + currentExpectation(comment, column, text) and + not desiredExpectation(comment, column, text) + ) + } + + /** Gets the rank that orders the default, `SPURIOUS:`, and `MISSING:` columns within a comment. */ + private int getColumnRank(string column) { + column = "" and result = 0 + or + column = "SPURIOUS" and result = 1 + or + column = "MISSING" and result = 2 + } + + /** + * Gets the rendered text of `column` on the learned `comment`, for example `Alert Alert[foo]` + * for the default column or `MISSING: Alert` for the `MISSING:` column, or has no result if + * that column carries no expectation after learning. Expectations within a column are ordered + * lexically, so a rewritten comment has a deterministic layout. + */ + private string renderLearnedColumn(TestImpl2::ExpectationComment comment, string column) { + desiredExpectation(comment, column, _) and + exists(string joined | + joined = + concat(string text | desiredExpectation(comment, column, text) | text, " " order by text) + | + column = "" and result = joined + or + column != "" and result = column + ": " + joined + ) + } + + /** + * Gets the fully rendered inline expectation comment (including the comment markers, but with no + * leading whitespace) that `--learn` should leave in place of `comment` in the file with the + * given `relativePath`. Has no result if no expectation survives learning (in which case the + * caller deletes the comment instead) or the file's comment syntax is unsupported. + */ + bindingset[relativePath] + private 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. + // The inner delimiter is always `//`, which the framework recognises regardless of the + // outer comment marker (so a `#`-comment file renders `# $ Alert // note`). + ( + exists(string note | + note = getTrailingNote(comment) and + trailingSuffix = " // " + note + ) + or + not exists(getTrailingNote(comment)) and trailingSuffix = "" + ) and + body = + concat(string column | + exists(renderLearnedColumn(comment, column)) + | + renderLearnedColumn(comment, column), " " order by getColumnRank(column) + ) and + result = 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 + ) { + // 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 + // rewritable comment, the new expectations are merged into it by the rewrite disjunct below + // (see `mergedNewExpectation`) rather than appended as a separate comment. + exists(string relativePath, int el, string comment | + unexpectedResultExpectation(relativePath, el, _) and + not exists(TestImpl2::ExpectationComment existing | + isRewritableComment(existing) and + 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 + ) + 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. The comment's inclusive location end is converted to the protocol's + // exclusive end boundary when it is on the same line. A multi-line location uses + // `endColumn = 0`, the engine's to-EOL convention, because some extractors (e.g. Swift) + // report a line comment as ending at column 1 of the next line. The replacement contains the + // re-rendered desired expectations (keeping any trailing regular comment), just the trailing + // regular comment when no expectation remains but a note like `// $ Alert // note` does, or + // the empty string when nothing remains. An empty to-EOL replacement also trims the + // whitespace gap left by the removed comment. + exists(TestImpl2::ExpectationComment comment, string relativePath, int sl, int sc | + isRewritableComment(comment) and + commentNeedsRewrite(comment) and + parseLocationString(comment.getLocation().getRelativeUrl(), relativePath, sl, sc, _, _) and + file = relativePath and + line = sl and + operation = "replace" and + startColumn = sc and + endColumn = getReplacementEndColumn(comment) and + ( + desiredExpectation(comment, _, _) and + text = renderReplacementComment(relativePath, 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) + ) + or + not exists(getTrailingNote(comment)) and text = "" + ) + ) + ) + } + } + + import LearnEditsImpl } } diff --git a/swift/ql/lib/utils/test/InlineExpectationsTestQuery.ql b/swift/ql/lib/utils/test/InlineExpectationsTestQuery.ql index e41d9310e0bf..bc87b5155bca 100644 --- a/swift/ql/lib/utils/test/InlineExpectationsTestQuery.ql +++ b/swift/ql/lib/utils/test/InlineExpectationsTestQuery.ql @@ -1,5 +1,6 @@ /** * @kind test-postprocess + * @tags inline-expectation-test */ private import swift diff --git a/swift/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll b/swift/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll index 718c118ef907..1e1ac85ffe6f 100644 --- a/swift/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll +++ b/swift/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll @@ -35,4 +35,11 @@ module Impl implements InlineExpectationsTestSig { f.getRelativePath() + ":" + startline + ":" + startcolumn + ":" + endline + ":" + endcolumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + // The Swift extractor only ingests Swift sources (no XML/YAML/HTML in its dbscheme), so a + // constant marker is safe; revisit if Swift ever gains extraction of another file type. + exists(relativePath) and result = "//" + } } diff --git a/unified/ql/lib/utils/test/InlineExpectationsTestQuery.ql b/unified/ql/lib/utils/test/InlineExpectationsTestQuery.ql index 96e655dbecfb..f59ccebbf201 100644 --- a/unified/ql/lib/utils/test/InlineExpectationsTestQuery.ql +++ b/unified/ql/lib/utils/test/InlineExpectationsTestQuery.ql @@ -1,5 +1,6 @@ /** * @kind test-postprocess + * @tags inline-expectation-test */ private import unified diff --git a/unified/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll b/unified/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll index 7379e1b94e66..e8cb3f922695 100644 --- a/unified/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll +++ b/unified/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll @@ -19,4 +19,12 @@ module Impl implements InlineExpectationsTestSig { f.getRelativePath() + ":" + startline + ":" + startcolumn + ":" + endline + ":" + endcolumn ) } + + bindingset[relativePath] + string getStartCommentMarker(string relativePath) { + // The unified extractor currently ingests only Swift sources, which use `//`. Gating on + // the extension keeps this correct if it gains a language with a different comment syntax. + relativePath.regexpMatch(".*\\.(swift|swiftinterface)") and + result = "//" + } }