Rust: Remove external locations in tests using post-processing #19669
Conversation
hvitved
force-pushed
the
rust/external-location-post-processing
branch
from
73d132d to
d384e78
Compare
hvitved
force-pushed
the
rust/external-location-post-processing
branch
from
d384e78 to
aa0fc05
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds a shared post‐processing step to normalize external file locations in CodeQL tests and updates Rust tests to use it instead of inline location handling.
- Introduce
ExternalLocationPostProcessing.qllwith logic to map external locations to{EXTERNAL LOCATION} - Make existing test predicates in
InlineExpectationsTest.qllprivate - Remove per-test “hasLocationInfo” classes in Rust library tests and wire in the new postprocessor via updated
.qlreffiles
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/util/codeql/util/test/InlineExpectationsTest.qll | Changed external predicates to external private |
| shared/util/codeql/util/test/ExternalLocationPostProcessing.qll | New module that replaces external locations with a marker |
| rust/ql/test/library-tests//.qlref | Added postprocess: utils/test/ExternalLocationPostProcessing.ql |
| rust/ql/test/library-tests//.ql | Removed inline location helper classes and updated predicate sigs |
| rust/ql/lib/utils/test/ExternalLocationPostProcessing.ql | QL pack wrapper importing shared postprocessor and prefix logic |
| not s = "file://" + any(string suffix) or | ||
| s = "file://:0:0:0:0" or | ||
| s = getSourceLocationPrefix() + any(string suffix) |
There was a problem hiding this comment.
Operator precedence is ambiguous here: the not only negates the first equality. Wrap the entire set of prefix and exact-match checks in parentheses so you get if not (s = "file://" + … or s = "file://:0:0:0:0" or s = getSourceLocationPrefix() + …) then as intended.
| not s = "file://" + any(string suffix) or | |
| s = "file://:0:0:0:0" or | |
| s = getSourceLocationPrefix() + any(string suffix) | |
| not (s = "file://" + any(string suffix) or | |
| s = "file://:0:0:0:0" or | |
| s = getSourceLocationPrefix() + any(string suffix)) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I believe the way it is written is the way it's intended, and this change would break correctness.
| query predicate results(string relation, int row, int column, string data) { | ||
| exists(string s | queryResults(relation, row, column, s) | | ||
| if | ||
| not s = "file://" + any(string suffix) or |
There was a problem hiding this comment.
[nitpick] Consider using the startsWith predicate (e.g., s.startsWith("file://")) instead of s = "file://" + any(string suffix) for clearer intent and potentially better performance.
| not s = "file://" + any(string suffix) or | |
| not s.startsWith("file://") or |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I don't think we have a built-in startsWith predicate. You could do .matches("file://%") and that might perform better (or worse).
There was a problem hiding this comment.
I think .matches("file://%") actually performs worse; the current solution compiles down to inverse string append, which I think uses startsWith or similar under-the-hood.
This PR introduces (shared)
.qlrefpost-processing logic for converting external locations to special{EXTERNAL LOCATION}markers instead.By changing these locations via post-processing instead of in the individual
.qltest files, we avoid replicating logic and also keeps the ability to run the.qlfiles in VS Code using the real external locations.For Rust this means we get rid of all the annoying
Location is outside of test directorywarnings.