Skip to content

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Dec 15, 2025

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 Ord methods. This PR adds those models, as well as the test case.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Dec 15, 2025
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Dec 15, 2025
@hvitved hvitved marked this pull request as ready for review December 15, 2025 12:15
@hvitved hvitved requested a review from a team as a code owner December 15, 2025 12:15
Copilot AI review requested due to automatic review settings December 15, 2025 12:15
Copy link
Contributor

Copilot AI left a 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.

@hvitved hvitved requested a review from geoffw0 December 15, 2025 13:13
Copy link
Contributor

@geoffw0 geoffw0 left a 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"]
Copy link
Contributor

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).

@hvitved hvitved merged commit 74ed18a into github:main Dec 15, 2025
26 checks passed
@hvitved hvitved deleted the rust/ord-models branch December 15, 2025 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants