-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
MethodUseclass to resolve method call identifiers - Refines
PathUseto work onPathSegmentnodes 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 |
Copilot
AI
Jun 18, 2025
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 |
Also fixes jump-to-def for paths so only the path segment is used, for example in
M1::M2only theM2segment jumps to theM2module, not the entireM1::M2path).