Skip to content

Rust: Fix type inference for explicit dereference with * to the Deref trait #19820

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 7 commits into from
Jun 20, 2025

Conversation

paldepind
Copy link
Contributor

@paldepind paldepind commented Jun 19, 2025

This PR should fix type inference for explicit dereferencing.

  • Support methods on & types. Previously types in the receiver position where always dereferenced before method lookup. Now for a &T we lookup methods both on &T itself and on T.
  • Implement the * in the desugaring of *. *e desugars to *Deref::deref(&e) and two handle the * after the call we need a special case in the return position of *.
  • Account for "certain" borrows in the Call abstraction. For instance, since a == b desugars to Equal::eq(&a, &b) so we can be certain that a and b are borrowed. Making these certain sidesteps some issues with the way we infer implicit borrows. For instance for an argument type of the form &T we will attempt to infer a dereference and never infer an implicit borrow. But the deref method on &T is a actually a borrow from &T to &&T.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Jun 19, 2025
@paldepind paldepind marked this pull request as ready for review June 19, 2025 14:37
@Copilot Copilot AI review requested due to automatic review settings June 19, 2025 14:37
@paldepind paldepind requested a review from a team as a code owner June 19, 2025 14:37
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 the Rust type-inference engine to correctly handle explicit * dereferences via the Deref trait and to distinguish “certain” borrows, and it updates tests and internal QL logic accordingly.

  • Add new dereference tests and update existing expected-results files to exercise explicit/implicit Deref calls.
  • Extend CallExprBaseMatchingInput to carry a certain flag on borrows and adjust TypeInference.qll to honor explicit * in both argument and return positions.
  • Update internal QL modules (OperationImpl.qll, CallImpl.qll) to propagate the new borrow flags and update test annotations for write‐formatted calls and dataflow.

Reviewed Changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
rust/ql/test/query-tests/security/.../PathResolutionConsistency.expected Add expected write_fmt paths for explicit deref consistency tests.
rust/ql/test/library-tests/type-inference/main.rs Introduce MyInt, ATrait, remove spurious annotations, register new dereference module.
rust/ql/test/library-tests/type-inference/dereference.rs New file with explicit/implicit deref tests through Deref trait.
rust/ql/test/library-tests/type-inference/CONSISTENCY/... Add consistency expectations for e1.deref().
rust/ql/test/library-tests/dataflow/.../DataFlowStep.expected Insert unwrap receiver entries in local dataflow steps.
rust/ql/test/library-tests/dataflow/global/viableCallable.expected Add explicit * deref entries to viable callables.
rust/ql/test/library-tests/dataflow/global/main.rs Update sink comment to include hasTaintFlow.
rust/ql/lib/codeql/rust/internal/TypeInference.qll Track “certain” borrows in AccessPosition, adjust primitive handling of explicit *.
rust/ql/lib/codeql/rust/elements/internal/OperationImpl.qll Change deref operator’s borrows count from 0 to 1.
rust/ql/lib/codeql/rust/elements/internal/CallImpl.qll Expand implicitBorrowAt predicate to accept a certain flag.

@paldepind paldepind changed the title Rust: Correct type inference for explicit dereference with * to the Deref trait Rust: Fix type inference for explicit dereference with * to the Deref trait Jun 19, 2025
@paldepind paldepind force-pushed the rust/explicit-dereference branch from c9943a2 to 6b2c125 Compare June 19, 2025 19:02
@paldepind
Copy link
Contributor Author

DCA shows a decent increase in resolved method calls. Unfortunately rust takes 4.6x more time.

@paldepind
Copy link
Contributor Author

The blowup on rust is caused by 7d536a3.

Two guesses as to why that might be: 1/ There is ambiguity in method calls where both T and &T have the same method. In those cases Rust will prefer one, but we take both. 2/ Many trait implementations have the form impl<I: SomeTrait> Iterator for &I and we don't consider type parameter bounds like I: SomeTrait. This might cause us to accept to many methods on references.

In interest of getting this PR in a mergeable state before I go on vacation I've partially reverted the aforementioned commit such that we only resolve deref methods on reference types. This workaround is obviously not ideal, but it still makes * work on references and avoids exploding on rust.

@paldepind paldepind force-pushed the rust/explicit-dereference branch from 6ec844f to bd2812c Compare June 20, 2025 10:49
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.

Great!

@paldepind paldepind merged commit 6773903 into github:main Jun 20, 2025
17 checks passed
@paldepind paldepind deleted the rust/explicit-dereference branch June 20, 2025 12:39
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