Rust: Rework type inference for method calls #20282
Conversation
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
e4cfb86 to
4a8c37c
Compare
4a8c37c to
e75d79e
Compare
|
|
||
| /** | ||
| * Holds if `constraint` might occur as the third argument of | ||
| * `potentialInstantiationOf`. Defaults to simply projecting the third | ||
| * argument of `potentialInstantiationOf`. | ||
| */ | ||
| default predicate relevantTypeMention(TypeMention tm) { potentialInstantiationOf(_, _, tm) } | ||
| default predicate relevantTypeMention(Constraint tm) { potentialInstantiationOf(_, _, tm) } |
Check warning
Code scanning / CodeQL
Missing QLDoc for parameter Warning
61866bf to
2d1ed65
Compare
2d1ed65 to
3d19a06
Compare
3d19a06 to
153c10b
Compare
153c10b to
e161d4c
Compare
dd45f7b to
a20c440
Compare
f45d2d5 to
f9f8782
Compare
| pragma[nomagic] | ||
| private Type inferMethodCallExprType(AstNode n, TypePath path) { | ||
| exists( | ||
| MethodCallMatchingInput::Access a, MethodCallMatchingInput::AccessPosition apos, string state, |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
| | | ||
| functionResolutionDependsOnArgument(impl, _, result, pos, _, _) | ||
| or | ||
| exists(TypeParameter tp | traitTypeParameterOccurrence(trait, resolved, _, pos, _, tp) | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
4bfd7aa to
8cd0e06
Compare
| * `self5` | `impl T2 for X` | `X` | ||
| */ | ||
| private class FunctionPositionType extends TFunctionPositionType { | ||
| private predicate asFunctionPositionType(Function f, FunctionPosition pos, ImplOrTraitItemNode i) { |
Check warning
Code scanning / CodeQL
Predicates starting with "get" or "as" should return a value Warning
|
|
||
| pragma[nomagic] | ||
| abstract Type getParameterType(DeclarationPosition dpos, TypePath path); | ||
| private predicate asInheritedFunctionPositionType( |
Check warning
Code scanning / CodeQL
Predicates starting with "get" or "as" should return a value Warning
eb173a1 to
39b1a4a
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR reworks type inference for method calls in the Rust codeQL extractor, addressing a major limitation where method calls with multiple potential targets couldn't be properly resolved. The changes implement a more sophisticated approach that distinguishes between stateful and stateless contexts to improve method resolution accuracy.
Key changes:
- Introduced stateful type inference to handle method calls with multiple candidates
- Updated type inference infrastructure to support state-based resolution
- Enhanced method call target resolution for complex scenarios
Reviewed Changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
shared/typeinference/codeql/typeinference/internal/TypeInference.qll |
Major refactoring to support stateful type inference, introducing MatchingWithState module and state-based method resolution |
rust/ql/lib/codeql/rust/internal/TypeMention.qll |
Added new type mention classes including SelfParameterImplicitMention for handling implicit self parameters |
rust/ql/lib/codeql/rust/internal/Type.qll |
Added support for union types, never type, and pointer types, enhancing the type system coverage |
Multiple .expected files |
Updated test expectations reflecting improved method call resolution accuracy |
| filepath.matches("%/main.rs") and | ||
| startline = 52 | ||
| startline = 167 |
There was a problem hiding this comment.
This hardcoded line number (167) in the debug module appears to be a leftover from development/testing. Consider removing this specific line constraint or making it configurable to avoid issues when the test file structure changes.
See below for a potential fix:
filepath.matches("%/main.rs")
Copilot uses AI. Check for mistakes.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
39b1a4a to
b4bd65b
Compare
| } | ||
|
|
||
| /** Holds if this relevant access should satisfy `constraint`. */ | ||
| Type getConstraint() { relevantAccessConstraint(a, target, apos, path, result) } | ||
| Type getConstraint(Declaration target) { |
Check warning
Code scanning / CodeQL
Missing QLDoc for parameter Warning
b4bd65b to
cb8a603
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
2748229 to
fb5f768
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fb5f768 to
6f6437f
Compare
| pragma[nomagic] | ||
| private Type getACandidateReceiverTypeAtNoBorrowNoMatch(TypePath path, string derefChain) { | ||
| result = this.getACandidateReceiverTypeAtNoBorrow(path, derefChain) and | ||
| exists(Type type, string name, int arity, string derefChainBorrow | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
| pragma[nomagic] | ||
| private Type getACandidateReceiverTypeAtNoBorrowNoMatch(TypePath path, string derefChain) { | ||
| result = this.getACandidateReceiverTypeAtNoBorrow(path, derefChain) and | ||
| exists(Type type, string name, int arity, string derefChainBorrow | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
| pragma[nomagic] | ||
| private Type getACandidateReceiverTypeAtNoMatch(TypePath path, string derefChain) { | ||
| result = this.getACandidateReceiverTypeAtNoBorrowNoMatch(path, derefChain) and | ||
| exists(Type type, string name, int arity, string derefChainBorrow | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
| pragma[nomagic] | ||
| private Type getACandidateReceiverTypeAtNoMatch(TypePath path, string derefChain) { | ||
| result = this.getACandidateReceiverTypeAtNoBorrowNoMatch(path, derefChain) and | ||
| exists(Type type, string name, int arity, string derefChainBorrow | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
| predicate potentialInstantiationOf( | ||
| MethodCallCand mcc, TypeAbstraction abs, FunctionPositionType constraint | ||
| ) { | ||
| exists(MethodCall mc, Type type, string name, int arity | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
| predicate potentialInstantiationOf( | ||
| MethodCallCand mcc, TypeAbstraction abs, FunctionPositionType constraint | ||
| ) { | ||
| exists(MethodCall mc, Type type, string name, int arity | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
6f6437f to
adfced0
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements a reworked type inference for method calls in Rust, improving type resolution accuracy. The changes modify the TypeInference.qll module to add better support for type state tracking during type inference, allowing for more precise method call resolution that can distinguish between different contexts (such as & vs &mut).
Key changes include:
- Added state tracking to type matching logic for method calls
- Extended the
MatchingWithStatemodule signature to handle state-aware type inference - Refactored type path resolution to improve performance by reducing redundant computations
Reviewed Changes
Copilot reviewed 32 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/typeinference/codeql/typeinference/internal/TypeInference.qll | Major rework of type inference logic to support state-aware method call resolution |
| rust/ql/test/**/*.expected | Test expectation updates reflecting improved type inference accuracy |
| rust/ql/lib/codeql/rust/internal/TypeMention.qll | Enhanced type mention classes with better performance optimizations |
| rust/ql/lib/codeql/rust/internal/Type.qll | Added support for union types, never types, and pointer types |
| rust/ql/lib/codeql/rust/internal/PathResolution.qll | Improved trait visibility computation using transitive closure |
| rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll | Added standard library trait definitions for Deref and Index |
| // `app` and `tm` differ on a concrete type | ||
| exists(Type t, TypePath path | | ||
| t = resolveNthTypeAt(app, abs, tm, _, path) and | ||
| not t = abs.getATypeParameter() and | ||
| not path.isEmpty() and | ||
| app.getTypeAt(path) != t | ||
| ) | ||
| or | ||
| // `app` uses inconsistent type parameter instantiations | ||
| exists(TypeParameter tp | | ||
| potentialInstantiationOf(app, abs, tm) and | ||
| app.getTypeAt(getNthTypeParameterPath(tm, tp, _)) != | ||
| app.getTypeAt(getNthTypeParameterPath(tm, tp, _)) | ||
| ) | ||
| } |
There was a problem hiding this comment.
The removal of the not path.isEmpty() condition and the simplification of isNotInstantiationOf may cause incorrect behavior. The original condition prevented matching at the root path when there were concrete type differences, which could be important for type safety. Consider whether this simplification maintains the correct semantics.
See below for a potential fix:
// `app` and `tm` differ on a concrete type at a non-root path
exists(Type t, TypePath path |
t = resolveNthTypeAt(app, abs, tm, _, path) and
not t = abs.getATypeParameter() and
not path.isEmpty() and
app.getTypeAt(path) != t
)
Copilot uses AI. Check for mistakes.
| | | ||
| path | ||
| ) | ||
| rank[i + 1](TypePath path | exists(tm.getTypeAt(path)) and relevantTypeMention(tm) | path) |
There was a problem hiding this comment.
The change from tm.resolveTypeAt(path) to tm.getTypeAt(path) should be consistent throughout the module. Verify that all call sites have been updated to use the new interface to avoid performance issues or incorrect results.
Copilot uses AI. Check for mistakes.
| private predicate traitIsVisibleTC(TNode trait, TNode element) = | ||
| doublyBoundedFastTC(step/2, isTrait/1, isElement/1)(trait, element) |
There was a problem hiding this comment.
The replacement of the recursive traitLookup predicate with transitive closure using doublyBoundedFastTC is a good optimization. However, ensure that the performance characteristics are tested with large codebases, as transitive closure can be expensive for deeply nested scopes.
| private predicate traitIsVisibleTC(TNode trait, TNode element) = | |
| doublyBoundedFastTC(step/2, isTrait/1, isElement/1)(trait, element) | |
| /** | |
| * Holds if there is a path from `trait` to `element` via `step`, bounded by `isTrait` and `isElement`. | |
| * To avoid performance issues in large or deeply nested codebases, we limit the maximum path length. | |
| * If you encounter performance problems, consider tightening the bounds or using the original recursive predicate. | |
| */ | |
| private predicate traitIsVisibleTC(TNode trait, TNode element) { | |
| exists(int depth | | |
| depth < 20 and | |
| doublyBoundedFastTC(step/2, isTrait/1, isElement/1, depth)(trait, element) | |
| ) | |
| } |
Copilot uses AI. Check for mistakes.
No description provided.