-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Rust: Rework type inference for method calls #20282
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?
Rust: Rework type inference for method calls #20282
Conversation
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.
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
in this argument
f9f8782
to
4bfd7aa
Compare
| | ||
functionResolutionDependsOnArgument(impl, _, result, pos, _, _) | ||
or | ||
exists(TypeParameter tp | traitTypeParameterOccurrence(trait, resolved, _, pos, _, tp) | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
in this argument
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
*/ | ||
// pragma[nomagic] | ||
pragma[inline] | ||
private predicate methodCandidateTrait( |
Check warning
Code scanning / CodeQL
Missing QLDoc for parameter Warning
eb173a1
to
39b1a4a
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
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 reworks type inference for method calls in Rust by introducing a state-based type matching system. The main goal is to improve method resolution accuracy by tracking type parameter instantiations across different candidate methods during resolution.
Key changes:
- Introduces a new
MatchingWithState
module that tracks state during type matching - Refactors the
IsInstantiationOf
module to support generic constraints alongside applications - Adds support for additional type constructs like unions, never types, and pointer types
- Improves method call resolution by considering multiple potential targets with different states
Reviewed Changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
shared/typeinference/codeql/typeinference/internal/TypeInference.qll | Major refactoring of type inference logic to support state-based matching and improved instantiation checking |
rust/ql/test/utils-tests/modelgenerator/CaptureSummaryModels.expected | Updated test expectations for generated model summaries |
rust/ql/test/library-tests/type-inference/main.rs | Test file updates to reflect improved method resolution capabilities |
rust/ql/test/library-tests/type-inference/dereference.rs | Added comprehensive tests for dereferencing scenarios and method resolution |
rust/ql/lib/codeql/rust/internal/TypeMention.qll | Enhanced type mention resolution with support for self parameters and additional type constructs |
rust/ql/lib/codeql/rust/internal/Type.qll | Added support for union types, never types, and pointer types |
Various test expected files | Updated to reflect improved type inference and method resolution accuracy |
app.getTypeAt(path) != t | ||
) |
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.
The removed code that checked for inconsistent type parameter instantiations appears to be important validation logic. Removing this without clear justification could lead to incorrect type inference results. Consider adding a comment explaining why this validation is no longer needed or restore it if the removal was unintentional.
app.getTypeAt(path) != t | |
) | |
) | |
or | |
// There is an inconsistent instantiation of a type parameter | |
exists(int i | | |
exists(TypeParameter tp | tp = getNthTypeParameter(abs, i)) and | |
not typeParametersHaveEqualInstantiationFromIndex(app, abs, tm, i) | |
) |
Copilot uses AI. Check for mistakes.
@@ -1740,7 +1742,7 @@ private module Debug { | |||
exists(string filepath, int startline, int startcolumn, int endline, int endcolumn | | |||
result.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) and | |||
filepath.matches("%/main.rs") and | |||
startline = 52 | |||
startline = 167 |
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] This hardcoded line number in the debug module should be removed or made configurable. Having hardcoded debug coordinates in production code can cause confusion and makes the code less maintainable.
See below for a potential fix:
/**
* Returns a relevant locatable for debugging, configurable by line number.
*/
private Locatable getRelevantLocatable(int startline) {
exists(string filepath, int sline, int startcolumn, int endline, int endcolumn |
result.getLocation().hasLocationInfo(filepath, sline, startcolumn, endline, endcolumn) and
filepath.matches("%/main.rs") and
sline = startline
)
}
/** Convenience: default line number for debugging. */
private int defaultDebugStartLine() { result = 167 }
/** Overload for backward compatibility: uses default line number. */
private Locatable getRelevantLocatable() { result = getRelevantLocatable(defaultDebugStartLine()) }
predicate debugUnqualifiedPathLookup(
RelevantPath p, string name, Namespace ns, ItemNode encl, string path
) {
p = getRelevantLocatable(defaultDebugStartLine()) and
unqualifiedPathLookup(encl, name, ns, p) and
path = p.toStringDebug()
}
ItemNode debugResolvePath(RelevantPath path) {
path = getRelevantLocatable(defaultDebugStartLine()) and
result = resolvePath(path)
}
predicate debugUseImportEdge(Use use, string name, ItemNode item, SuccessorKind kind) {
use = getRelevantLocatable(defaultDebugStartLine()) and
useImportEdge(use, name, item, kind)
}
ItemNode debugGetASuccessor(ItemNode i, string name, SuccessorKind kind) {
i = getRelevantLocatable(defaultDebugStartLine()) and
result = i.getASuccessor(name, kind)
}
predicate debugFileImportEdge(Module mod, string name, ItemNode item, SuccessorKind kind) {
mod = getRelevantLocatable(defaultDebugStartLine()) and
fileImportEdge(mod, name, item, kind)
}
predicate debugFileImport(Module m, SourceFile f) {
m = getRelevantLocatable(defaultDebugStartLine()) and
fileImport(m, f)
}
predicate debugPreludeEdge(SourceFile f, string name, ItemNode i) {
preludeEdge(f, name, i) and
f = getRelevantLocatable(defaultDebugStartLine())
}
string debugGetCanonicalPath(ItemNode i, Crate c) {
result = i.getCanonicalPath(c) and
i = getRelevantLocatable(defaultDebugStartLine())
Copilot uses AI. Check for mistakes.
ImplItemNode i, FunctionPositionType self, string derefChainBorrow, MethodCall mc, Trait trait | ||
) { | ||
exists(Type rootType, string name, int arity | | ||
mc.(MethodCall).isMethodCall0(rootType, name, arity, derefChainBorrow) and |
Check warning
Code scanning / CodeQL
Redundant cast Warning
No description provided.