-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: Extend jump-to-def query with method calls #19809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
aae01b9
to
5cd7295
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the jump-to-definition query in the Rust CodeQL library to handle method calls and to restrict path-based definitions to individual segments (e.g., only M2
in M1::M2
).
- Introduces a new
MethodUse
class to resolve method call identifiers - Refines
PathUse
to work onPathSegment
nodes and include call disambiguation - Updates test suite to verify method and segmented-path resolutions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
rust/ql/test/library-tests/definitions/main.rs | Adds a nested module with S::method and corresponding use in main for testing method-resolution |
rust/ql/test/library-tests/definitions/Definitions.expected | Updates expected definitions output to include new path-segment and method entries |
rust/ql/lib/codeql/rust/internal/Definitions.qll | Extends PathUse to PathSegment , adds call-aware logic, and introduces MethodUse for method call resolution |
Comments suppressed due to low confidence (2)
rust/ql/lib/codeql/rust/internal/Definitions.qll:143
- [nitpick] The method name getCall() is somewhat vague; consider renaming it to getAssociatedCallExpr() or similar to clarify its purpose.
private CallExpr getCall() { result.getFunction().(PathExpr).getPath() = path }
rust/ql/lib/codeql/rust/internal/Definitions.qll:138
- [nitpick] Add a brief comment above this class explaining that it handles individual path segments (including calls) for jump-to-definition resolutions, to aid future readers.
private class PathUse extends Use instanceof PathSegment {
// Our call resolution logic may disambiguate some calls, so only use those | ||
result.asItemNode() = this.getCall().getStaticTarget() | ||
or | ||
not exists(this.getCall()) and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider caching the result of getCall() in a local variable or predicate to avoid traversing the AST twice in getDefinition, improving performance.
// Our call resolution logic may disambiguate some calls, so only use those | |
result.asItemNode() = this.getCall().getStaticTarget() | |
or | |
not exists(this.getCall()) and | |
// Cache the result of getCall() to avoid traversing the AST twice | |
CallExpr cachedCall = this.getCall(); | |
// Our call resolution logic may disambiguate some calls, so only use those | |
result.asItemNode() = cachedCall.getStaticTarget() | |
or | |
not exists(cachedCall) and |
Copilot uses AI. Check for mistakes.
Also fixes jump-to-def for paths so only the path segment is used, for example in
M1::M2
only theM2
segment jumps to theM2
module, not the entireM1::M2
path).