Skip to content

Rust: Implement type inference for ref expression as type equality #19724

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

Merged
merged 2 commits into from
Jun 11, 2025

Conversation

paldepind
Copy link
Contributor

@paldepind paldepind commented Jun 11, 2025

I was debugging an issue where types where not inferred correctly, and realized that for a call like f(&a) the declared type of fs first parameters would not flow to a through the borrow.

This PR fixes that and also removes some logic for ref expressions that doesn't match Rust's type system. Due to implicit dereferencing &&&&a and &a can seem interchangeable, but the types are distinct. Removing this collapsing doesn't seem to affect the tests.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Jun 11, 2025
@paldepind paldepind marked this pull request as ready for review June 11, 2025 07:49
@Copilot Copilot AI review requested due to automatic review settings June 11, 2025 07:49
@paldepind paldepind requested a review from a team as a code owner June 11, 2025 07:49
@paldepind paldepind requested a review from hvitved June 11, 2025 07:50
Copy link
Contributor

@Copilot 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 enhances Rust expression type inference by treating &x as a true type equality and removes legacy reference-handling logic. Key changes include:

  • Added new test cases in main.rs to verify boolean and custom-flag type inference through multiple borrows.
  • Extended the typeEquality predicate to include RefExpr and simplified inferRefExprType in QL.
  • Removed outdated logic for reference-expression inference in TypeInference.qll.

Reviewed Changes

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

File Description
rust/ql/test/library-tests/type-inference/main.rs Added MyFlag struct tests and **&&true inference
rust/ql/lib/codeql/rust/internal/TypeInference.qll Updated typeEquality, simplified inferRefExprType and inference pipeline
Comments suppressed due to low confidence (2)

rust/ql/test/library-tests/type-inference/main.rs:1158

  • [nitpick] The field name bool shadows the built-in type name and may be confusing; consider renaming it to something more descriptive like flag or state.
    struct MyFlag {

rust/ql/lib/codeql/rust/internal/TypeInference.qll:980

  • [nitpick] Update this doc comment to mention that inferRefExprType no longer takes a path parameter and now only returns TRefType for the given RefExpr.
/** Gets the root type of the reference expression `re`. */

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

LGTM. Let's see what DCA says before merging.

@paldepind
Copy link
Contributor Author

DCA seems fine. Overall a reduction in missing call targets.

@paldepind paldepind merged commit 75caa18 into main Jun 11, 2025
17 checks passed
@paldepind paldepind deleted the rust/type-inference-borrow branch June 11, 2025 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants