Uh oh!
There was an error while loading. Please reload this page.
Rust: Follow-up work to make path resolution and type inference tests pass again - #19519
Merged
hvitved merged 1 commit intoMay 19, 2025
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR updates the Rust QL tests and library modules to filter out macro-generated AST nodes, enforce source-only elements, and reflect real standard-library paths in test expectations. Key changes include:
- Adding
fromSource()andisFromMacroExpansion()filters in type inference and path resolution tests - Updating expected outputs to point to the real
core/resultdefinitions in the Rust stdlib - Implementing new predicates (
fromSource,startsBefore,startsStrictlyBefore, etc.) and adapting path-resolution and type-inference logic in the QL libraries
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/ql/test/library-tests/type-inference/type-inference.ql | Added source/macro filters to inferType and related predicates |
| rust/ql/test/library-tests/type-inference/type-inference.expected | Updated expected test outputs to real stdlib Result and Arguments paths |
| rust/ql/test/library-tests/path-resolution/path-resolution.ql | Applied source/macro filters to resolvePath queries |
| rust/ql/test/library-tests/path-resolution/path-resolution.expected | Updated expected module paths to real stdlib locations |
| rust/ql/test/TestUtils.qll | Expanded toBeTested to only include source AST nodes |
| rust/ql/lib/utils/test/PathResolutionInlineExpectationsTest.qll | Added source/macro filters to inline-expectations test helpers |
| rust/ql/lib/codeql/rust/internal/TypeInference.qll | Adjusted hard-coded debug line number |
| rust/ql/lib/codeql/rust/internal/PathResolution.qll | Replaced getModuleNode with getSourceFile, removed outdated isRoot |
| rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll | Refactored OptionEnum/ResultEnum to use getASuccessor |
| rust/ql/lib/codeql/rust/elements/internal/MacroCallImpl.qll | Introduced isInMacroExpansion and getATokenTreeNode |
| rust/ql/lib/codeql/rust/elements/internal/LocationImpl.qll | Renamed/extended location predicates (startsBefore, etc.) |
| rust/ql/lib/codeql/rust/elements/internal/LocatableImpl.qll | Changed fromSource to delegate to File.fromSource() |
| rust/ql/lib/codeql/rust/elements/internal/AstNodeImpl.qll | Hooked isInMacroExpansion into new MacroCallImpl predicates |
| rust/ql/lib/codeql/files/FileSystem.qll | Strengthened File.fromSource() by requiring a relative path |
Comments suppressed due to low confidence (1)
rust/ql/test/TestUtils.qll:17
- The
CrateElementpredicate no longer includesthis instanceof Crate, which may prevent actualCrateelements from being recognized. Consider restoringthis instanceof Cratein the predicate.
this instanceof NamedCrate
| result.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) and | ||
| filepath.matches("%/main.rs") and | ||
| startline = 28 | ||
| startline = 948 |
There was a problem hiding this comment.
[nitpick] Avoid hard-coding line numbers in production predicates. Consider computing this value dynamically or defining a named constant to explain its origin.
Comment on lines
+16
to
21
| f.getAPrecedingComment().getCommentText() = value and | ||
| f.fromSource() | ||
| or | ||
| not exists(f.getAPrecedingComment()) and | ||
| not any(f.getAPrecedingComment()).fromSource() and | ||
| // TODO: Default to canonical path once that is available | ||
| value = f.getName().getText() |
There was a problem hiding this comment.
[nitpick] To make the OR conditions clearer, wrap each branch in parentheses. For example: (f.getAPrecedingComment().getCommentText() = value and f.fromSource()) or (not any(f.getAPrecedingComment()).fromSource() and value = f.getName().getText()).
aibaars
reviewed
May 19, 2025
| | main.rs:658:19:658:22 | self | Fst | main.rs:656:10:656:12 | Fst | | ||
| | main.rs:658:19:658:22 | self | Snd | main.rs:656:15:656:17 | Snd | | ||
| | main.rs:659:43:659:82 | MacroExpr | | main.rs:656:15:656:17 | Snd | | ||
| | main.rs:659:50:659:81 | MacroExpr | | file:///Users/hvitved/.rustup/toolchains/1.85-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:549:1:584:1 | Arguments | |
Contributor
There was a problem hiding this comment.
Here the home directory is leaking into expected test output.
ContributorAuthor
There was a problem hiding this comment.
I believe those should be fixed when you merge latest main into your branch, which contains
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#19506 follow-up.