-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: Account for borrows in operators in type inference #19789
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
base: main
Are you sure you want to change the base?
Conversation
5213ba3
to
f18acdf
Compare
There was a problem hiding this 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 type inference to correctly handle implicit borrows in operator desugaring and generalizes the Call
abstraction to track borrows on any argument.
- Introduces an
ArgumentPosition
type and updates theCall
interface to represent and query borrowed arguments. - Updates
TypeInference
to useArgumentPosition
for matching declaration and access positions, undoing the previous workaround. - Extends the operator overload predicate to include a
borrows
parameter, 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
isOverloaded
should explicitly document the newborrows
parameter (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
mce
to something likecall
to accurately reflect its type (Call
) and improve readability in the debug function.
Function debugResolveMethodCallTarget(Call mce) {
@@ -5,29 +5,58 @@ private import codeql.rust.elements.internal.ExprImpl::Impl as ExprImpl | |||
private import codeql.rust.elements.Operation | |||
|
|||
module Impl { | |||
newtype TArgumentPosition = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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
.
Copilot uses AI. Check for mistakes.
predicate isSelf() { this = TSelfDeclarationPosition() } | ||
predicate isSelf() { this.asArgumentPosition().isSelf() } | ||
|
||
int asPosition() { result = this.asArgumentPosition().asPosition() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The asPosition
method assumes the DeclarationPosition
wraps an argument; consider guarding against or documenting its invalid use on return positions.
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(); | |
} |
Copilot uses AI. Check for mistakes.
Some operators contain borrows in their desugaring. For instance
a == b
desugars toPartialEq::eq(&a, &b)
. This PR adds handling for that in type inference. As part of thatCall
is generalized to allows for borrows that are not on theself
position.This undoes the temporary workaround in #19755.
Ceveats:
ArgumentPosition
type which was useful inCall
. Right now it's located in theCallImpl.qll
but maybe we want to have it somewhere else? In its own separate file?self
currently 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.