Repository navigation
Rust: Data flow through trait methods - #19881
hvitved merged 5 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
Enhance data-flow tracking by dispatching trait method calls to all implementations and by applying data-flow models to implementations without source code.
- Extend models to include Ord::max and PartialOrd::lt trait methods
- Update tests to cover trait dispatch scenarios and adjust expectations
- Introduce QL predicates for runtime trait dispatch (getARuntimeTarget, implements) and adjust DataFlowImpl to use them
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file| File | Description |
|---|---|
| rust/ql/test/library-tests/dataflow/models/models.ext.yml | Add models for <_ as Ord>::max and <_ as PartialOrd>::lt |
| rust/ql/test/library-tests/dataflow/models/models.expected | Update expected summaries to include new trait models |
| rust/ql/test/library-tests/dataflow/models/main.rs | Add test_trait_model and trait implementations for Ord |
| rust/ql/test/library-tests/dataflow/global/main.rs | Introduce MyTrait, MyTrait2, and trait-dispatch tests |
| rust/ql/lib/codeql/rust/internal/PathResolution.qll | Add getAssocItem(name) helper |
| rust/ql/lib/codeql/rust/elements/internal/CallImpl.qll | Define getARuntimeTarget for dispatching to impls |
| rust/ql/lib/codeql/rust/elements/internal/AssocItemImpl.qll | Implement implements relation for associated items |
| rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll | Override hasProvenance to pick up summary models |
| rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll | Enhance viableCallable to consider runtime trait dispatch |
| rust/ql/.gitattributes | Remove generated marker for AssocItemImpl.qll |
| rust/ql/.generated.list | Remove AssocItemImpl.qll from generated list |
rust/ql/test/library-tests/dataflow/global/main.rs:280
- [nitpick] Using a numeric suffix in the trait name (MyTrait2) can be confusing; consider a more descriptive name or merge it into the existing trait to avoid ambiguity.
trait MyTrait2 {
rust/ql/lib/codeql/rust/elements/internal/CallImpl.qll:71
- [nitpick] Method name getARuntimeTarget is a bit verbose and uses mixed-case acronym; consider renaming to getRuntimeTarget for consistency with your naming conventions.
Function getARuntimeTarget() {
Sorry, something went wrong.
| @@ -404,10 +404,20 @@ module RustDataFlow implements InputSig<Location> { | |||
|
|
|||
| /** Gets a viable implementation of the target of the given `Call`. */ | |||
| DataFlowCallable viableCallable(DataFlowCall call) { | |||
Copilot
AI
Jun 26, 2025
There was a problem hiding this comment.
[nitpick] The viableCallable definition contains deeply nested exists and or expressions; extracting the trait-dispatch logic into a helper predicate could improve readability and maintenance.
Sorry, something went wrong.
geoffw0
left a comment
There was a problem hiding this comment.
LGTM.
I've started in the DCA run on a discussion of a couple of points (I think you've seen it), neither of them block merging this work.
Sorry, something went wrong.
| * `Field[core::option::Option::Some(0)]`. | ||
| * - `Field[i]`: the `i`th element of a tuple. | ||
| * - `Reference`: the referenced value. | ||
| * - `Future`: the value being computed asynchronously. |
There was a problem hiding this comment.
Good spot. 👍
Sorry, something went wrong.
This PR makes two improvements to data flow:
The first improvement is likely to result in some false positive flow, which we can address to some extent later by tracking types in data flow.