Rust: Path resolution improvements#19051
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| /** Provides predicates for debugging the path resolution implementation. */ | ||
| private module Debug { |
Check warning
Code scanning / CodeQL
Dead code Warning
| /** Provides predicates for debugging the path resolution implementation. */ | ||
| private module Debug { | ||
| private Locatable getRelevantLocatable() { | ||
| exists(string filepath, int startline, int startcolumn, int endline, int endcolumn | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
| /** Provides predicates for debugging the path resolution implementation. */ | ||
| private module Debug { | ||
| private Locatable getRelevantLocatable() { | ||
| exists(string filepath, int startline, int startcolumn, int endline, int endcolumn | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
| /** Provides predicates for debugging the path resolution implementation. */ | ||
| private module Debug { | ||
| private Locatable getRelevantLocatable() { | ||
| exists(string filepath, int startline, int startcolumn, int endline, int endcolumn | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
|
|
||
| predicate same(Source s, Target t) { | ||
| t = RustAnalyzer::resolve(s) and | ||
| t = Ql::resolve(s) |
Check warning
Code scanning / CodeQL
Redundant assignment. Warning
hvitved
force-pushed
the
rust/path-resolution-cross-crate
branch
4 times, most recently
from
87770c5 to
f12cce9
Compare
hvitved
force-pushed
the
rust/path-resolution-cross-crate
branch
2 times, most recently
from
dc4a355 to
9ede9a9
Compare
| predicate qlUniqueCount(int c) { c = count(Source s | qlUnique(s)) } | ||
|
|
||
| // debug predicates to find missing targets in QL implementation | ||
| private module Debug { |
Check warning
Code scanning / CodeQL
Dead code Warning
hvitved
force-pushed
the
rust/path-resolution-cross-crate
branch
2 times, most recently
from
f9255d3 to
f1ae6a3
Compare
hvitved
force-pushed
the
rust/path-resolution-cross-crate
branch
from
f1ae6a3 to
3142dbb
Compare
| * According to | ||
| * | ||
| * https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/book/second-edition/ch07-02-controlling-visibility-with-pub.html#privacy-rules | ||
| * | ||
| * this is either `itemParent` itself or any (transitive) child of `itemParent`. |
There was a problem hiding this comment.
I think I found the same thing in the official Rust reference. We could also use a reference-style Markdown link.
| * According to | |
| * | |
| * https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/book/second-edition/ch07-02-controlling-visibility-with-pub.html#privacy-rules | |
| * | |
| * this is either `itemParent` itself or any (transitive) child of `itemParent`. | |
| * According to [The Rust Reference][1] this is either `itemParent` itself or any (transitive) child of `itemParent`. | |
| * | |
| * [1]: https://doc.rust-lang.org/reference/visibility-and-privacy.html#r-vis.access |
| exists(ModuleItemNode mod | | ||
| fileImport(mod, this) and | ||
| result = mod.getASuccessor("super") | ||
| ) |
There was a problem hiding this comment.
| exists(ModuleItemNode mod | | |
| fileImport(mod, this) and | |
| result = mod.getASuccessor("super") | |
| ) | |
| result = any(ModuleItemNode mod | fileImport(mod, this)).getASuccessor("super") |
| } | ||
|
|
||
| /** | ||
| * Gets a source file that belongs to this crate, if any. |
There was a problem hiding this comment.
Not sure if it's a strong convention, but I've noticed that on our AST classes the getAFoo use a phrasing that starts with "Gets any of the foos ...".
| * Gets a source file that belongs to this crate, if any. | |
| * Gets any of the source files that belongs to this crate. |
There was a problem hiding this comment.
I actually don't like the Gets any ... style, and it also seems to conflict with https://github.com/github/codeql/blob/main/docs/qldoc-style-guide.md#predicates-with-result.
There was a problem hiding this comment.
What about this part of the rule:
Use "if any" if the item is usually unique but might be missing.
That seems to conflict with how "if any" is used here?
There was a problem hiding this comment.
Right, the "if any" bit can be removed.
| // filepath.matches("%/compile.rs") and | ||
| // startline = 1986 | ||
| // filepath.matches("%/build_steps/mod.rs") and | ||
| // startline = 17 |
There was a problem hiding this comment.
Should these be deleted?
|
I believe the |
paldepind
left a comment
paldepind
left a comment
There was a problem hiding this comment.
LGTM! Not sure what's up with the CI though.
3 participants
This PR implements various improvements to our path resolution logic, most notably the ability to resolve paths across crates. Commit-by-commit review is strongly encouraged.
DCA shows that, while we maintain performance, we gain an additional 132% true positive call edges (up 448,902 from 193,392) and an additional 94% true positive resolved paths (up 345,075 from 177,539), all numbers computed across the entire DCA suite.