-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Rust: Add models for core::cmp::Ord::{min,max,clamp}
#21035
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
351c295 to
fc49360
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 adds data flow models for Rust's core::cmp::Ord trait methods (min, max, and clamp), enabling CodeQL to track data flow through these comparison operations. The PR also includes comprehensive test cases to verify the models work correctly, particularly with custom dereferencing scenarios.
Key Changes:
- Added flow models for
Ord::{min,max,clamp}to track that values flow from both/all arguments to the return value - Added new test case
test_ord()demonstrating flow through these methods - Expanded some if-else blocks in existing code for better readability (formatting change only)
- Updated test expectations to reflect the new models
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
rust/ql/lib/codeql/rust/frameworks/stdlib/core.model.yml |
Adds three new flow models for min, max, and clamp methods |
rust/ql/test/library-tests/dataflow/models/models.ext.yml |
Removes test model for max (now covered by main models) |
rust/ql/test/library-tests/dataflow/models/models.expected |
Updates expected test output to show Argument[self,0] instead of Argument[self] |
rust/ql/test/library-tests/dataflow/modeled/main.rs |
Adds test_ord() function with test cases for all three methods |
rust/ql/test/library-tests/dataflow/modeled/inline-flow.expected |
Updates expected test results with new flow paths |
rust/ql/test/library-tests/dataflow/global/main.rs |
Expands if-else formatting and adds a.min(1042) test case |
rust/ql/test/library-tests/dataflow/global/inline-flow.expected |
Updates expected results reflecting line number changes |
rust/ql/test/library-tests/dataflow/global/viableCallable.expected |
Updates expected results reflecting line number changes |
rust/ql/test/library-tests/dataflow/global/CONSISTENCY/PathResolutionConsistency.expected |
Updates line numbers for consistency checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
geoffw0
left a comment
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.
LGTM.
| # Ord | ||
| - ["<_ as core::cmp::Ord>::min", "Argument[self,0]", "ReturnValue", "value", "manual"] | ||
| - ["<_ as core::cmp::Ord>::max", "Argument[self,0]", "ReturnValue", "value", "manual"] | ||
| - ["<_ as core::cmp::Ord>::clamp", "Argument[self,0,1]", "ReturnValue", "value", "manual"] |
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.
Oh its nice to see Rust has a clamp method (and we're modelling it).
As part of #20987, I wanted to write a data flow test using implicit custom dereferencing, and found out that we are lacking flow models for
Ordmethods. This PR adds those models, as well as the test case.