Skip to content
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

Rust: Model Result.ok and Result.err. #18777

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Feb 13, 2025

Model Result.ok and Result.err, which convert a Result to an Option corresponding to one of its variants.

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code labels Feb 13, 2025
@Copilot Copilot bot review requested due to automatic review settings February 13, 2025 17:45

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR adds new dataflow tests for modeling the behavior of Rust's Result.ok and Result.err methods and updates the CodeQL model to support these conversions.

  • Added a new test function option_ok() in main.rs to verify that Result.ok returns Some for Ok values and Result.err returns Some for Err values.
  • Updated the CodeQL standard library model (lang-core.model.yml) to map the ok and err methods to their corresponding Option variants.

Changes

File Description
rust/ql/test/library-tests/dataflow/local/main.rs Added option_ok() to test the behavior of Result.ok and Result.err conversions
rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml Included new entries for modeling Result.ok and Result.err conversions to Option

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

rust/ql/test/library-tests/dataflow/local/main.rs:270

  • [nitpick] Consider removing the extra space before the colon for type annotations to maintain consistency with the rest of the file (e.g., 'let r1: Result<i64, i64> = Ok(source(21));').
let r1 : Result<i64, i64> = Ok(source(21));

rust/ql/test/library-tests/dataflow/local/main.rs:274

  • [nitpick] Consider adding an inline comment indicating that unwrapping o1b is expected to fail (or yield no value flow) since r1.err() returns None in the Ok case.
sink(o1b.unwrap());

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@paldepind
Copy link
Contributor

Note, these models are included in #18787. I think it would make sense to add the tests in this PR but use the generated models.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Feb 17, 2025

Agreed, and the models are identical, which is reassuring.

I'll update this PR...

@geoffw0
Copy link
Contributor Author

geoffw0 commented Feb 17, 2025

Updated.

Though the tests will fail until your PR is merged.

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