From a1c3dca5de4e74f26b8674e0a7f82f49f3cfe0c8 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 11 Feb 2025 12:55:23 +0100 Subject: [PATCH 1/8] JS: Convert OK-style to $-style expectations in one test --- .../DuplicateDependency/duplicates.js | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/duplicates.js b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/duplicates.js index 8d1f68cfa6f2..528178ada019 100644 --- a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/duplicates.js +++ b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/duplicates.js @@ -1,18 +1,18 @@ (function(){ function f(){} - f.$inject = ['dup5', 'dup5']; // NOT OK + f.$inject = ['dup5', 'dup5']; // $ Alert angular.module('myModule', []) - .run(['dup1a', 'dup1a', function(dup1a, dup1a){}]) // OK (flagged by js/duplicate-parameter-name) - .run(['dup2a', 'dup2a', function(dup2a, dup2b){}]) // NOT OK - .run(['dup3b', 'dup3b', function(dup3a, dup3b){}]) // NOT OK - .run(['dup4', 'notDup4A', 'dup4', function(notDup4B, dup4, notDup4C){}]) // NOT OK + .run(['dup1a', 'dup1a', function(dup1a, dup1a){}]) // OK - flagged by js/duplicate-parameter-name + .run(['dup2a', 'dup2a', function(dup2a, dup2b){}]) // $ Alert + .run(['dup3b', 'dup3b', function(dup3a, dup3b){}]) // $ Alert + .run(['dup4', 'notDup4A', 'dup4', function(notDup4B, dup4, notDup4C){}]) // $ Alert .run(f) - .run(function(dup6, dup6){})// OK (flagged by js/duplicate-parameter-name) - .run(function(notDup7a, notDup7b){}) // OK - .run(['notDup8a', 'notDup8b', function(notDup8a, notDup8b){}]) // OK - .run(['notDup9a', 'notDup9b', function(notDup9c, notDup9d){}]) // OK - .run(['dup10a', 'dup10a', 'dup10a', function(dup10a, dup10a, dup10a){}]) // OK (flagged by js/duplicate-parameter-name) - .run(['dup11a', 'dup11a', function(dup11a, dup11b){ // NOT OK (alert formatting for multi-line function) + .run(function(dup6, dup6){})// OK - flagged by js/duplicate-parameter-name + .run(function(notDup7a, notDup7b){}) + .run(['notDup8a', 'notDup8b', function(notDup8a, notDup8b){}]) + .run(['notDup9a', 'notDup9b', function(notDup9c, notDup9d){}]) + .run(['dup10a', 'dup10a', 'dup10a', function(dup10a, dup10a, dup10a){}]) // OK - flagged by js/duplicate-parameter-name + .run(['dup11a', 'dup11a', function(dup11a, dup11b){ // $ Alert - alert formatting for multi-line function }]) ; })(); From fb79ab1c8c37d68b68db46729d702527998b2a69 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 11 Feb 2025 12:56:49 +0100 Subject: [PATCH 2/8] JS: Update line numbers --- .../AngularJS/DuplicateDependency/DuplicateDependency.expected | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected index 2bf4bc4b4fb7..4745bfbe12cc 100644 --- a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected +++ b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected @@ -2,4 +2,4 @@ | duplicates.js:6:33:6:56 | functio ... up2b){} | This function has a duplicate dependency $@. | duplicates.js:6:24:6:30 | 'dup2a' | dup2a | | duplicates.js:7:33:7:56 | functio ... up3b){} | This function has a duplicate dependency $@. | duplicates.js:7:24:7:30 | 'dup3b' | dup3b | | duplicates.js:8:43:8:78 | functio ... up4C){} | This function has a duplicate dependency $@. | duplicates.js:8:35:8:40 | 'dup4' | dup4 | -| duplicates.js:15:35:15:112 | functio ... } | This function has a duplicate dependency $@. | duplicates.js:15:25:15:32 | 'dup11a' | dup11a | +| duplicates.js:15:35:15:113 | functio ... } | This function has a duplicate dependency $@. | duplicates.js:15:25:15:32 | 'dup11a' | dup11a | From 84c02d0863f6a0dd6022bba23c34b62c25f13f09 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 11 Feb 2025 12:57:48 +0100 Subject: [PATCH 3/8] JS: Enable test post-processing --- .../DuplicateDependency/DuplicateDependency.expected | 5 +++++ .../AngularJS/DuplicateDependency/DuplicateDependency.qlref | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected index 4745bfbe12cc..4d2b9cf22a08 100644 --- a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected +++ b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected @@ -1,5 +1,10 @@ +#select | duplicates.js:2:5:2:18 | function f(){} | This function has a duplicate dependency $@. | duplicates.js:3:26:3:31 | 'dup5' | dup5 | | duplicates.js:6:33:6:56 | functio ... up2b){} | This function has a duplicate dependency $@. | duplicates.js:6:24:6:30 | 'dup2a' | dup2a | | duplicates.js:7:33:7:56 | functio ... up3b){} | This function has a duplicate dependency $@. | duplicates.js:7:24:7:30 | 'dup3b' | dup3b | | duplicates.js:8:43:8:78 | functio ... up4C){} | This function has a duplicate dependency $@. | duplicates.js:8:35:8:40 | 'dup4' | dup4 | | duplicates.js:15:35:15:113 | functio ... } | This function has a duplicate dependency $@. | duplicates.js:15:25:15:32 | 'dup11a' | dup11a | +testFailures +| duplicates.js:2:5:2:18 | This function has a duplicate dependency $@. | Unexpected result: Alert | +| duplicates.js:3:35:3:44 | // $ Alert | Missing result: Alert | +| duplicates.js:15:61:15:113 | // $ Al ... unction | Missing result: Alert | diff --git a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.qlref b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.qlref index bb9a8f9849f7..9c299135425c 100644 --- a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.qlref +++ b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.qlref @@ -1 +1,2 @@ -AngularJS/DuplicateDependency.ql \ No newline at end of file +query: AngularJS/DuplicateDependency.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql From 5b0eb0f6cc1f78793e031091d56c1145e4cf647f Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 11 Feb 2025 12:58:13 +0100 Subject: [PATCH 4/8] JS: Move an Alert annotation to its correct line --- .../DuplicateDependency/DuplicateDependency.expected | 2 -- .../query-tests/AngularJS/DuplicateDependency/duplicates.js | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected index 4d2b9cf22a08..7d2088773492 100644 --- a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected +++ b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected @@ -5,6 +5,4 @@ | duplicates.js:8:43:8:78 | functio ... up4C){} | This function has a duplicate dependency $@. | duplicates.js:8:35:8:40 | 'dup4' | dup4 | | duplicates.js:15:35:15:113 | functio ... } | This function has a duplicate dependency $@. | duplicates.js:15:25:15:32 | 'dup11a' | dup11a | testFailures -| duplicates.js:2:5:2:18 | This function has a duplicate dependency $@. | Unexpected result: Alert | -| duplicates.js:3:35:3:44 | // $ Alert | Missing result: Alert | | duplicates.js:15:61:15:113 | // $ Al ... unction | Missing result: Alert | diff --git a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/duplicates.js b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/duplicates.js index 528178ada019..966f70ad5c11 100644 --- a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/duplicates.js +++ b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/duplicates.js @@ -1,6 +1,6 @@ (function(){ - function f(){} - f.$inject = ['dup5', 'dup5']; // $ Alert + function f(){} // $ Alert + f.$inject = ['dup5', 'dup5']; angular.module('myModule', []) .run(['dup1a', 'dup1a', function(dup1a, dup1a){}]) // OK - flagged by js/duplicate-parameter-name .run(['dup2a', 'dup2a', function(dup2a, dup2b){}]) // $ Alert From 967c0860f920e36ca7ec7cb8388d1fbb7367f316 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 11 Feb 2025 12:52:05 +0100 Subject: [PATCH 5/8] Test: support queries that don't select a Location --- .../util/test/InlineExpectationsTest.qll | 121 ++++++++++++++++-- 1 file changed, 109 insertions(+), 12 deletions(-) diff --git a/shared/util/codeql/util/test/InlineExpectationsTest.qll b/shared/util/codeql/util/test/InlineExpectationsTest.qll index a3143c4848e4..3e26983c45f0 100644 --- a/shared/util/codeql/util/test/InlineExpectationsTest.qll +++ b/shared/util/codeql/util/test/InlineExpectationsTest.qll @@ -643,12 +643,109 @@ module TestPostProcessing { module Make Input2> { private import InlineExpectationsTest as InlineExpectationsTest - private import InlineExpectationsTest::Make + + 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 + result = Input2::getRelativeUrl(loc).splitAt(":", 0) + ) + } + + private newtype TTestLocation = + MkInputLocation(Input::Location loc) or + 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 + not Input2::getRelativeUrl(_) = data // avoid duplicate locations + ) + } + + /** + * A location that is either an `Input::Location` or a location from an alert. + * + * We use this location type to support queries that select a location that does not correspond + * to an instance of `Input::Location`. + */ + abstract private class TestLocationImpl extends TTestLocation { + string getAbsoluteFile() { this.hasLocationInfo(result, _, _, _, _) } + + int getStartLine() { this.hasLocationInfo(_, result, _, _, _) } + + int getStartColumn() { this.hasLocationInfo(_, _, result, _, _) } + + int getEndLine() { this.hasLocationInfo(_, _, _, result, _) } + + int getEndColumn() { this.hasLocationInfo(_, _, _, _, result) } + + abstract string getRelativeUrl(); + + abstract string toString(); + + abstract predicate hasLocationInfo(string file, int sl, int sc, int el, int ec); + } + + private class LocationFromResult extends TestLocationImpl, MkResultLocation { + override string getRelativeUrl() { + exists(string file, int sl, int sc, int el, int ec | + this = MkResultLocation(file, sl, sc, el, ec) and + result = file + ":" + sl + ":" + sc + ":" + el + ":" + ec + ) + } + + override string toString() { result = this.getRelativeUrl() } + + override predicate hasLocationInfo(string file, int sl, int sc, int el, int ec) { + this = MkResultLocation(getRelativePathTo(file), sl, sc, el, ec) + } + } + + private class LocationFromInput extends TestLocationImpl, MkInputLocation { + private Input::Location loc; + + LocationFromInput() { this = MkInputLocation(loc) } + + override string getRelativeUrl() { result = Input2::getRelativeUrl(loc) } + + override string toString() { result = this.getRelativeUrl() } + + override predicate hasLocationInfo(string file, int sl, int sc, int el, int ec) { + loc.hasLocationInfo(file, sl, sc, el, ec) + } + } + + final class TestLocation = TestLocationImpl; + + module TestImpl2 implements InlineExpectationsTestSig { + final class Location = TestLocation; + + class ExpectationComment instanceof Input::ExpectationComment { + string getContents() { result = super.getContents() } + + Location getLocation() { result = MkInputLocation(super.getLocation()) } + + string toString() { result = super.toString() } + } + } + + private import InlineExpectationsTest::Make /** Holds if the given locations refer to the same lines, but possibly with different column numbers. */ bindingset[loc1, loc2] pragma[inline_late] - private predicate sameLineInfo(Input::Location loc1, Input::Location loc2) { + private predicate sameLineInfo(TestLocation loc1, TestLocation loc2) { exists(string file, int line1, int line2 | loc1.hasLocationInfo(file, line1, _, line2, _) and loc2.hasLocationInfo(file, line1, _, line2, _) @@ -656,8 +753,8 @@ module TestPostProcessing { } pragma[nomagic] - private predicate mainQueryResult(int row, int column, Input::Location loc) { - queryResults(mainResultSet(), row, column, Input2::getRelativeUrl(loc)) + private predicate mainQueryResult(int row, int column, TestLocation loc) { + queryResults(mainResultSet(), row, column, loc.getRelativeUrl()) } /** @@ -668,7 +765,7 @@ module TestPostProcessing { */ private string getSourceTag(int row) { getQueryKind() = "path-problem" and - exists(Input::Location sourceLoc, Input::Location selectLoc | + exists(TestLocation sourceLoc, TestLocation selectLoc | mainQueryResult(row, 0, selectLoc) and mainQueryResult(row, 2, sourceLoc) and if sameLineInfo(selectLoc, sourceLoc) then result = "Alert" else result = "Source" @@ -733,7 +830,7 @@ module TestPostProcessing { } additional predicate hasPathProblemSource( - int row, Input::Location location, string element, string tag, string value + int row, TestLocation location, string element, string tag, string value ) { getQueryKind() = "path-problem" and mainQueryResult(row, 2, location) and @@ -742,7 +839,7 @@ module TestPostProcessing { value = "" } - predicate hasActualResult(Input::Location location, string element, string tag, string value) { + predicate hasActualResult(TestLocation location, string element, string tag, string value) { hasPathProblemSource(_, location, element, tag, value) } } @@ -770,7 +867,7 @@ module TestPostProcessing { private predicate hasPathProblemSource = PathProblemSourceTestInput::hasPathProblemSource/5; private predicate hasPathProblemSink( - int row, Input::Location location, string element, string tag + int row, TestLocation location, string element, string tag ) { getQueryKind() = "path-problem" and mainQueryResult(row, 4, location) and @@ -778,7 +875,7 @@ module TestPostProcessing { tag = getSinkTag(row) } - private predicate hasAlert(int row, Input::Location location, string element, string tag) { + private predicate hasAlert(int row, TestLocation location, string element, string tag) { getQueryKind() = ["problem", "path-problem"] and mainQueryResult(row, 0, location) and queryResults(mainResultSet(), row, 2, element) and @@ -793,7 +890,7 @@ module TestPostProcessing { * present). */ private string getValue(int row) { - exists(Input::Location location, string element, string tag, string val | + exists(TestLocation location, string element, string tag, string val | hasPathProblemSource(row, location, element, tag, val) and result = PathProblemSourceTest::getAMatchingExpectation(location, element, tag, val, false) @@ -801,7 +898,7 @@ module TestPostProcessing { ) } - predicate hasActualResult(Input::Location location, string element, string tag, string value) { + predicate hasActualResult(TestLocation location, string element, string tag, string value) { exists(int row | hasPathProblemSource(row, location, element, tag, _) or @@ -840,7 +937,7 @@ module TestPostProcessing { rankedTestFailures(row, f) and f = MkTestFailure(fl, message) | - column = 0 and data = Input2::getRelativeUrl(fl.getLocation()) + column = 0 and data = fl.getLocation().getRelativeUrl() or column = 1 and data = fl.toString() or From 56ff9351f20dc59b249018262a67038658cece9b Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 11 Feb 2025 12:59:11 +0100 Subject: [PATCH 6/8] JS: Update test output again --- .../AngularJS/DuplicateDependency/DuplicateDependency.expected | 3 --- 1 file changed, 3 deletions(-) diff --git a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected index 7d2088773492..4745bfbe12cc 100644 --- a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected +++ b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected @@ -1,8 +1,5 @@ -#select | duplicates.js:2:5:2:18 | function f(){} | This function has a duplicate dependency $@. | duplicates.js:3:26:3:31 | 'dup5' | dup5 | | duplicates.js:6:33:6:56 | functio ... up2b){} | This function has a duplicate dependency $@. | duplicates.js:6:24:6:30 | 'dup2a' | dup2a | | duplicates.js:7:33:7:56 | functio ... up3b){} | This function has a duplicate dependency $@. | duplicates.js:7:24:7:30 | 'dup3b' | dup3b | | duplicates.js:8:43:8:78 | functio ... up4C){} | This function has a duplicate dependency $@. | duplicates.js:8:35:8:40 | 'dup4' | dup4 | | duplicates.js:15:35:15:113 | functio ... } | This function has a duplicate dependency $@. | duplicates.js:15:25:15:32 | 'dup11a' | dup11a | -testFailures -| duplicates.js:15:61:15:113 | // $ Al ... unction | Missing result: Alert | From 80e79b11f70abc263ea59e775775092eb242c06f Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 11 Feb 2025 16:53:13 +0100 Subject: [PATCH 7/8] Apply suggestions from code review Co-authored-by: Tom Hvitved --- .../util/codeql/util/test/InlineExpectationsTest.qll | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/shared/util/codeql/util/test/InlineExpectationsTest.qll b/shared/util/codeql/util/test/InlineExpectationsTest.qll index 3e26983c45f0..2e52e8010967 100644 --- a/shared/util/codeql/util/test/InlineExpectationsTest.qll +++ b/shared/util/codeql/util/test/InlineExpectationsTest.qll @@ -659,7 +659,7 @@ module TestPostProcessing { private string getRelativePathTo(string absolutePath) { exists(Input::Location loc | loc.hasLocationInfo(absolutePath, _, _, _, _) and - result = Input2::getRelativeUrl(loc).splitAt(":", 0) + parseLocationString(Input2::getRelativeUrl(loc), result, _, _, _, _) ) } @@ -692,7 +692,7 @@ module TestPostProcessing { abstract string getRelativeUrl(); - abstract string toString(); + final string toString() { result = this.getRelativeUrl() } abstract predicate hasLocationInfo(string file, int sl, int sc, int el, int ec); } @@ -731,12 +731,10 @@ module TestPostProcessing { module TestImpl2 implements InlineExpectationsTestSig { final class Location = TestLocation; - class ExpectationComment instanceof Input::ExpectationComment { - string getContents() { result = super.getContents() } + final private class ExpectationCommentFinal = Input::ExpectationComment; + class ExpectationComment extends ExpectationCommentFinal { Location getLocation() { result = MkInputLocation(super.getLocation()) } - - string toString() { result = super.toString() } } } From c306f44589906169dee7e011a491c48c84409f3f Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 11 Feb 2025 17:07:09 +0100 Subject: [PATCH 8/8] Remove override of final predicate --- shared/util/codeql/util/test/InlineExpectationsTest.qll | 4 ---- 1 file changed, 4 deletions(-) diff --git a/shared/util/codeql/util/test/InlineExpectationsTest.qll b/shared/util/codeql/util/test/InlineExpectationsTest.qll index 2e52e8010967..17ac7f1cd257 100644 --- a/shared/util/codeql/util/test/InlineExpectationsTest.qll +++ b/shared/util/codeql/util/test/InlineExpectationsTest.qll @@ -705,8 +705,6 @@ module TestPostProcessing { ) } - override string toString() { result = this.getRelativeUrl() } - override predicate hasLocationInfo(string file, int sl, int sc, int el, int ec) { this = MkResultLocation(getRelativePathTo(file), sl, sc, el, ec) } @@ -719,8 +717,6 @@ module TestPostProcessing { override string getRelativeUrl() { result = Input2::getRelativeUrl(loc) } - override string toString() { result = this.getRelativeUrl() } - override predicate hasLocationInfo(string file, int sl, int sc, int el, int ec) { loc.hasLocationInfo(file, sl, sc, el, ec) }