Rust: Disambiguate some method calls based on argument types #19749
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
paldepind
force-pushed
the
rust/impl-parameter-resolution
branch
3 times, most recently
from
eb1b0fb to
3a6f5ca
Compare
|
DCA shows that we loose call targets, which is not too surprising given that the PR is about pruning call targets. One guess as to why we might loose correct targets, is if we (for some other reason) can't infer the type of an arguments for a call that now requires a type at an argument. |
hvitved
reviewed
Jun 13, 2025
| * // ^ `path` = "T" | ||
| * } | ||
| * impl MyAdd<i64> for i64 { | ||
| * fn method(&self, value: i64) -> Self { ... } |
There was a problem hiding this comment.
Either it should be Foo<i64>, or remove Foo above.
| Impl impl, TraitItemNode trait, Type rootType, TypeMention selfTy | ||
| ) { | ||
| trait = impl.(ImplItemNode).resolveTraitTy() and | ||
| not exists(impl.getAttributeMacroExpansion()) and |
There was a problem hiding this comment.
Perhaps a comment why this restriction is needed?
| /** Gets a method from an `impl` block that matches the method call `mc`. */ | ||
| private Function getMethodFromImpl(MethodCall mc) { | ||
| exists(Impl impl | | ||
| IsInstantiationOf<MethodCall, IsInstantiationOfInput>::isInstantiationOf(mc, impl, _) and | ||
| result = getMethodSuccessor(impl, mc.getMethodName()) | ||
| | |
There was a problem hiding this comment.
I would suggest the following diff, just to be sure that the method being restricted by methodResolutionDependsOnArgument is the same as the method being returned:
diff --git a/rust/ql/lib/codeql/rust/internal/TypeInference.qll b/rust/ql/lib/codeql/rust/internal/TypeInference.qll
index 6ea2df6550c..bc80a1a14f3 100644
--- a/rust/ql/lib/codeql/rust/internal/TypeInference.qll
+++ b/rust/ql/lib/codeql/rust/internal/TypeInference.qll
@@ -1275,12 +1275,13 @@ private predicate methodTypeAtPath(Function f, int pos, TypePath path, Type type
}
/**
- * Holds if resolving the method in `impl` with the name `methodName` requires
+ * Holds if resolving the method `f` in `impl` with the name `methodName` requires
* inspecting the types of applied _arguments_ in order to determine whether it
* is the correct resolution.
*/
+pragma[nomagic]
private predicate methodResolutionDependsOnArgument(
- Impl impl, string methodName, int pos, TypePath path, Type type
+ Impl impl, Function f, string methodName, int pos, TypePath path, Type type
) {
/*
* As seen in the example below, when an implementation has a sibling for a
@@ -1310,20 +1311,21 @@ private predicate methodResolutionDependsOnArgument(
exists(TraitItemNode trait |
implHasSibling(impl, trait) and
traitTypeParameterOccurrence(trait, methodName, pos, path) and
- methodTypeAtPath(getMethodSuccessor(impl, methodName), pos, path, type)
+ methodTypeAtPath(f, pos, path, type) and
+ f = getMethodSuccessor(impl, methodName)
)
}
/** Gets a method from an `impl` block that matches the method call `mc`. */
private Function getMethodFromImpl(MethodCall mc) {
exists(Impl impl |
- IsInstantiationOf<MethodCall, IsInstantiationOfInput>::isInstantiationOf(mc, impl, _) and
- result = getMethodSuccessor(impl, mc.getMethodName())
+ IsInstantiationOf<MethodCall, IsInstantiationOfInput>::isInstantiationOf(mc, impl, _)
|
- not methodResolutionDependsOnArgument(impl, _, _, _, _)
+ not methodResolutionDependsOnArgument(impl, _, _, _, _, _) and
+ result = getMethodSuccessor(impl, mc.getMethodName())
or
exists(int pos, TypePath path, Type type |
- methodResolutionDependsOnArgument(impl, mc.getMethodName(), pos, path, type) and
+ methodResolutionDependsOnArgument(impl, result, mc.getMethodName(), pos, path, type) and
inferType(mc.getArgument(pos), path) = type
)
)
paldepind
force-pushed
the
rust/impl-parameter-resolution
branch
2 times, most recently
from
0e2ff25 to
1d55771
Compare
paldepind
force-pushed
the
rust/impl-parameter-resolution
branch
from
1d55771 to
ef15df3
Compare
|
I've applied the suggestions and rebased. |
hvitved
approved these changes
Jun 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR handles some cases where resolving the correct method requires inspecting the types of arguments. In such cases we currently resolve multiple targets, and this PR should reduce that.
I've tried to do the simplest possible thing in this PR, but I think this should be quite effective for common cases where this occurs.
Quick eval'ing
getMethodFromImplonrustshows a reduction of results by around 50%. DCA will show us if we also loose desirable things.