Rust: Strengthen modeling of the Clone trait
#19442
Merged
Conversation
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
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the CodeQL model of Rust’s Clone trait by inserting implicit borrows and updating test expectations to reflect that clone dereferences its receiver, and adjusts related debug/test code.
- Updated
MyOption::clonedandClone::clonesummaries to includeReferenceon theselfargument - Adjusted expected model files and inline-flow tests to remove now-unneeded generated clone edges
- Refined the
CloneCallablemodel to only matchArgument[self].Referenceand updated a debug filter path
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rust/ql/test/utils-tests/modelgenerator/option.rs | Added .Reference in summaries for cloned and clone methods |
| rust/ql/test/utils-tests/modelgenerator/CaptureSummaryModels.expected | Updated expected summaries for Clone::clone; removed cloned entry |
| rust/ql/test/library-tests/dataflow/modeled/main.rs | Marked missing flow tests for clonable types due to lack of builtins |
| rust/ql/test/library-tests/dataflow/modeled/inline-flow.expected | Removed generated clone edges from inline-flow expectations |
| rust/ql/lib/codeql/rust/internal/TypeInference.qll | Changed debug filter from plugin.rs to main.rs at line 28 |
| rust/ql/lib/codeql/rust/frameworks/stdlib/Clone.qll | Restricted CloneCallable.propagatesFlow input to Argument[self].Reference |
Comments suppressed due to low confidence (1)
rust/ql/test/utils-tests/modelgenerator/CaptureSummaryModels.expected:4
- Also add an
Expected summary missingentry for the<crate::option::MyOption>::clonedmethod to mirror the updated summaries and ensure the test suite covers bothclonedandclone.
| Expected summary missing: repo::test;<crate::option::MyOption as crate::clone::Clone>::clone;Argument[self].Reference.Field[crate::option::MyOption::MySome(0)];ReturnValue.Field[crate::option::MyOption::MySome(0)];value;dfc-generated |
paldepind
approved these changes
May 1, 2025
| sink(a); // $ hasValueFlow=12 | ||
| let b = a.clone(); | ||
| sink(b); // $ hasValueFlow=12 | ||
| sink(b); // $ MISSING: hasValueFlow=12 - lack of builtins means that we cannot resolve clone call above, and hence not insert implicit borrow |
Contributor
There was a problem hiding this comment.
If stuff like this persists being an issue, for some reason, we could reinstate the old heuristic only in the cases where no type is inferred for a method receiver.
Contributor
Author
There was a problem hiding this comment.
Makes sense, though builtins should be extracted soonish.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The clone method takes a
&selfparameter, meaning that in terms of data flow it dereferences the receiver argument. Now that we have a principled way of inserting implicit borrows, it makes sense to strengthen the modeling of theClonetrait.Related, I also updated some expected flow summaries related to
clone.