Rust: Account for borrows in operators in type inference #19789
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paldepind
force-pushed
the
rust/operator-borrowing
branch
from
5213ba3 to
f18acdf
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR enhances type inference to correctly handle implicit borrows in operator desugaring and generalizes the Call abstraction to track borrows on any argument.
- Introduces an
ArgumentPositiontype and updates theCallinterface to represent and query borrowed arguments. - Updates
TypeInferenceto useArgumentPositionfor matching declaration and access positions, undoing the previous workaround. - Extends the operator overload predicate to include a
borrowsparameter, indicating how many leading arguments are borrowed.
Reviewed Changes
Copilot reviewed 26 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/ql/test/library-tests/type-inference/main.rs | Added Default impl and tests to cover inference of borrowed operators |
| rust/ql/lib/codeql/rust/internal/TypeInference.qll | Switched to ArgumentPosition for declaration/access matching and borrow logic |
| rust/ql/lib/codeql/rust/elements/internal/OperationImpl.qll | Extended isOverloaded to include borrows count for operators |
| rust/ql/lib/codeql/rust/elements/internal/CallImpl.qll | Defined ArgumentPosition and updated Call getters to handle borrows |
| rust/ql/lib/codeql/rust/elements/Call.qll | Exposed the new ArgumentPosition and updated Call class alias |
| rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll | Swapped to getPositionalArgument for consistency with new API |
| rust/ql/lib/codeql/rust/controlflow/CfgNodes.qll | Updated CFG node retrieval to use getPositionalArgument |
Comments suppressed due to low confidence (2)
rust/ql/lib/codeql/rust/elements/internal/OperationImpl.qll:12
- [nitpick] The Javadoc for
isOverloadedshould explicitly document the newborrowsparameter (for example, that it indicates how many leading arguments are borrowed).
* the canonical path `path` and the method name `method`, and if it borrows its
rust/ql/lib/codeql/rust/internal/TypeInference.qll:1590
- [nitpick] Rename the parameter
mceto something likecallto accurately reflect its type (Call) and improve readability in the debug function.
Function debugResolveMethodCallTarget(Call mce) {
| private import codeql.rust.elements.Operation | ||
|
|
||
| module Impl { | ||
| newtype TArgumentPosition = |
There was a problem hiding this comment.
[nitpick] Consider extracting TArgumentPosition and the ArgumentPosition class into their own file (e.g., ArgumentPosition.qll) to improve modularity and reduce the size of CallImpl.qll.
| predicate isSelf() { this = TSelfDeclarationPosition() } | ||
| predicate isSelf() { this.asArgumentPosition().isSelf() } | ||
|
|
||
| int asPosition() { result = this.asArgumentPosition().asPosition() } |
There was a problem hiding this comment.
[nitpick] The asPosition method assumes the DeclarationPosition wraps an argument; consider guarding against or documenting its invalid use on return positions.
Suggested change
| int asPosition() { result = this.asArgumentPosition().asPosition() } | |
| int asPosition() { | |
| if (this.isReturn()) { | |
| // Handle the invalid use case for return positions. | |
| throw "asPosition cannot be called on a return position."; | |
| } | |
| result = this.asArgumentPosition().asPosition(); | |
| } |
paldepind
commented
Jun 18, 2025
| or | ||
| // Dereference | ||
| op = "*" and path = "core::ops::deref::Deref" and method = "deref" | ||
| op = "*" and path = "core::ops::deref::Deref" and method = "deref" and borrows = 0 |
Contributor
Author
There was a problem hiding this comment.
Note, * borrows it's first argument. But setting borrows = 1 causes problems, so we are not ready for that yet.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some operators contain borrows in their desugaring. For instance
a == bdesugars toPartialEq::eq(&a, &b). This PR adds handling for that in type inference. As part of thatCallis generalized to allows for borrows that are not on theselfposition.This undoes the temporary workaround in #19755.
Ceveats:
ArgumentPositiontype which was useful inCall. Right now it's located in theCallImpl.qllbut maybe we want to have it somewhere else? In its own separate file?selfcurrently is. This handling also involves implicit dereference which is orthogonal to borrowing. This is not wrong (since all argument can be implicitly dereferenced) and I did it like that since it was easier than separating implicit dereference and implicit borrowing from each other. This also means that the borrows in==are treated as borrows that might be there, even though they are in fact always there.