Rust: Rework call resolution and type inference for calls #20282
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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.
hvitved
force-pushed
the
rust/type-inference-method-call-resolution-rework
branch
2 times, most recently
from
e4cfb86 to
4a8c37c
Compare
hvitved
force-pushed
the
rust/type-inference-method-call-resolution-rework
branch
from
4a8c37c to
e75d79e
Compare
hvitved
force-pushed
the
rust/type-inference-method-call-resolution-rework
branch
8 times, most recently
from
61866bf to
2d1ed65
Compare
hvitved
force-pushed
the
rust/type-inference-method-call-resolution-rework
branch
from
2d1ed65 to
3d19a06
Compare
hvitved
force-pushed
the
rust/type-inference-method-call-resolution-rework
branch
from
3d19a06 to
153c10b
Compare
hvitved
force-pushed
the
rust/type-inference-method-call-resolution-rework
branch
from
153c10b to
e161d4c
Compare
hvitved
force-pushed
the
rust/type-inference-method-call-resolution-rework
branch
from
dd45f7b to
a20c440
Compare
hvitved
force-pushed
the
rust/type-inference-method-call-resolution-rework
branch
5 times, most recently
from
f45d2d5 to
f9f8782
Compare
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
This sounds great, and I agree the DCA and QA results look fantastic overall. As does the reduction of inconsistencies in tests. Thank you for the explanation, thorough testing and explaining the impact here so there won't be any surprises after this is merged - and keeping track of some of the regressions that are part of this net improvement. 👍
I haven't looked at the code changes yet. @paldepind is probably the better reviewer for this, but I'll have a look as well.
hvitved
force-pushed
the
rust/type-inference-method-call-resolution-rework
branch
from
88c9f0f to
2ad8e2b
Compare
|
Rebased to resolve merge conflicts in |
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
I looked through the code, I don't have much to say there. It's a complicated area of code and thus important it stays well documented.
I also tried running a data flow query on one of the projects that slowed down the most on DCA - it does seem like something may be going wrong in the type inference recursion in some cases. Given that there's a net performance improvement on DCA (and QA) I don't think this needs to block merging the PR, but it might be worth looking into afterwards.
Already on it :-) |
hvitved
force-pushed
the
rust/type-inference-method-call-resolution-rework
branch
from
6e54fa0 to
8a25e32
Compare
hvitved
force-pushed
the
rust/type-inference-method-call-resolution-rework
branch
2 times, most recently
from
89da13c to
b6bbffd
Compare
paldepind
left a comment
paldepind
left a comment
There was a problem hiding this comment.
Submitting the comments that I have thus far, in case you want to look at some of this before I'm completely done.
| * [1]: https://doc.rust-lang.org/stable/reference/items/associated-items.html#r-items.associated.fn.method.self-ty | ||
| */ | ||
| pragma[nomagic] | ||
| predicate complexSelfRoot(Type root, TypeParameter tp) { |
There was a problem hiding this comment.
This predicate does two orthogonal things. 1/ Picking the types that can appear for self and 2/ getting the first positional type parameter of a type. I think it would be worthwhile to have to former as a separate predicate called something like validSelfType?
| s instanceof BoxStruct | ||
| or | ||
| s instanceof RcStruct | ||
| or | ||
| s instanceof ArcStruct | ||
| or | ||
| s instanceof PinStruct |
There was a problem hiding this comment.
| s instanceof BoxStruct | |
| or | |
| s instanceof RcStruct | |
| or | |
| s instanceof ArcStruct | |
| or | |
| s instanceof PinStruct | |
| s instanceof BoxStruct or | |
| s instanceof RcStruct or | |
| s instanceof ArcStruct or | |
| s instanceof PinStruct |
| class FunctionTypePosition extends TFunctionTypePosition { | ||
| predicate isSelf() { this.asArgumentPosition().isSelf() } | ||
|
|
||
| int asPositional() { result = this.asArgumentPosition().asPosition() } |
There was a problem hiding this comment.
Wouldn't it be nicer to reuse the same name? It also seems more natural to say that the predicate returns a "position" rather than a "positional".
| int asPositional() { result = this.asArgumentPosition().asPosition() } | |
| int asPosition() { result = this.asArgumentPosition().asPosition() } |
| private predicate hasTypeParameterAt(TypePath path, TypeParameter tp) { | ||
| this.getDeclaredTypeAt(path) = tp |
There was a problem hiding this comment.
This would be natural as a predicate with result:
| private predicate hasTypeParameterAt(TypePath path, TypeParameter tp) { | |
| this.getDeclaredTypeAt(path) = tp | |
| private TypeParameter getTypeParameterAt(TypePath path) { | |
| result = this.getDeclaredTypeAt(path) |
rust/ql/lib/codeql/rust/internal/typeinference/FunctionType.qll
Outdated
Show resolved
Hide resolved
| private predicate assocFunctionInfo( | ||
| Function f, string name, int arity, ImplOrTraitItemNode i, FunctionTypePosition pos, | ||
| AssocFunctionType t | ||
| ) { | ||
| f = i.getASuccessor(name) and | ||
| arity = f.getParamList().getNumberOfParams() and | ||
| t.appliesTo(f, pos, i) | ||
| } |
There was a problem hiding this comment.
Given that (f, i, pos) uniquely determines and is uniquely determined by t, we're including the same data twice. Could we remove one of them like this?
| private predicate assocFunctionInfo( | |
| Function f, string name, int arity, ImplOrTraitItemNode i, FunctionTypePosition pos, | |
| AssocFunctionType t | |
| ) { | |
| f = i.getASuccessor(name) and | |
| arity = f.getParamList().getNumberOfParams() and | |
| t.appliesTo(f, pos, i) | |
| } | |
| private predicate assocFunctionInfo(AssocFunctionType t, string name, int arity) { | |
| t.getFunction() = t.getImpl().getASuccessor(name) and | |
| arity = t.getFunction().getParamList().getNumberOfParams() | |
| } |
Same question for several predicates below, like methodInfo, methodCallNonBlanketCandidate, etc.
There was a problem hiding this comment.
It is true that some columns are uniquely determined by others, but it is quite convenient to have predicates like these, where one can -- in a single predicate call -- derive some columns from others.
There was a problem hiding this comment.
Ok, though I'm not entirely convinced that we need to carry around all the values in all the predicates.
| abstract class MethodCall extends Expr { | ||
| abstract predicate hasNameAndArity(string name, int arity); | ||
|
|
||
| override TypeParameter getTypeParameter(TypeParameterPosition ppos) { | ||
| typeParamMatchPosition(this.getGenericParamList().getATypeParam(), result, ppos) | ||
| } | ||
| abstract Expr getArgument(ArgumentPosition pos); |
There was a problem hiding this comment.
This class duplicates a bunch of stuff already present in Call. For instance, the getArgument overrides below are identical to the getArgument implementations in Call.
I think we should be able to extend Call, remove hasNameAndArity (which duplicates getMethodName, and remove getNumberOfArguments) and getArgument (which duplicates getArgument).
| abstract class MethodCall extends Expr { | |
| abstract predicate hasNameAndArity(string name, int arity); | |
| override TypeParameter getTypeParameter(TypeParameterPosition ppos) { | |
| typeParamMatchPosition(this.getGenericParamList().getATypeParam(), result, ppos) | |
| } | |
| abstract Expr getArgument(ArgumentPosition pos); | |
| abstract class MethodCall extends Call { | |
| MethodCall() { exists(super.getMethodName()) } |
The suggestion doesn't work as-is, because there is a difference in which CallExprs are considered method calls. For instance, this call is considered a method call by MethodCall but not by Call. I suppose we should just change Call to behave like MethodCall is currently doing?
There was a problem hiding this comment.
I would like to ultimately get rid of the Call class, which is why I duplicated some of the logic here.
There was a problem hiding this comment.
Why? It's useful both in type inference, data flow, and for queries (for instance if you want a query that considers all calls to Index::index).
There was a problem hiding this comment.
Sorry, what I meant was I want to get rid of some of the functionality in the user-visible Call class (such as implicitBorrowAt and getTrait), and move it in here instead, but I would prefer to do that follow-up.
There was a problem hiding this comment.
Ok. I completely agree that is makes sense to remove implicitBorrowAt if we're no longer using it in type inference as it was only added for type inference 👍
But wouldn't getArgument, getMethodName, and getNumberOfArguments be things we would keep and thus could reuse here?
There was a problem hiding this comment.
We definitely want to keep getArgument and getNumberOfArguments, not sure about getMethodName, because determining whether a call targets a method is non-trivial. We can't replace hasNameAndArity with getMethodName and getNumberOfArguments because then joining will no longer happen on the two values simultaneously.
There was a problem hiding this comment.
If we extend Call we can just define hasNameAndArity once using getMethodName and getNumberOfArguments. Wouldn't that be fine in terms of joins as it'd only happen once. Otherwise, we could add that predicate on Call.
| exists(FunctionTypePosition pos | | ||
| assocFunctionInfo(m, name, arity, i, pos, selfType) and | ||
| strippedType = selfType.getTypeAt(strippedTypePath) and | ||
| isComplexRootStripped(strippedTypePath, strippedType) and |
There was a problem hiding this comment.
Does this work if we are in fact adding a method to one of the types that can appear for self, like Box or Pin?
There was a problem hiding this comment.
In that case strippedType will be a type parameter, and it is handled by methodInfoBlanket, which cover all blanket (like) implementations.
Thanks a lot. I have addressed your comments. |
paldepind
left a comment
paldepind
left a comment
There was a problem hiding this comment.
Thanks for addressing my comments.
Here's a few more. And, again, feel free to ignore them until I'm completely done if that's easiest for you. I just prefer not to have too many "pending" comments.
rust/ql/lib/codeql/rust/internal/typeinference/FunctionOverloading.qll
Outdated
Show resolved
Hide resolved
hvitved
force-pushed
the
rust/type-inference-method-call-resolution-rework
branch
from
819a537 to
41602d3
Compare
paldepind
left a comment
paldepind
left a comment
There was a problem hiding this comment.
Just a few more comments. After that, let's get this merged 💪
| exists(pos.getTypeMention(f)) | ||
| } or | ||
| MkInheritedAssocFunctionType( | ||
| Function f, FunctionPosition pos, TypeMention parentMention, ImplOrTraitItemNode parent, |
There was a problem hiding this comment.
The parent can only be an impl block.
| Function f, FunctionPosition pos, TypeMention parentMention, ImplOrTraitItemNode parent, | |
| Function f, FunctionPosition pos, TypeMention parentMention, TraitItemNode parent, |
| } | ||
|
|
||
| private predicate isInheritedFunctionType( | ||
| Function f, FunctionPosition pos, TypeMention parentMention, ImplOrTraitItemNode parent, |
There was a problem hiding this comment.
| Function f, FunctionPosition pos, TypeMention parentMention, ImplOrTraitItemNode parent, | |
| Function f, FunctionPosition pos, TypeMention parentMention, TraitItemNode parent, |
| ) | ||
| or | ||
| exists( | ||
| Function f, FunctionPosition pos, TypeMention parentMention, ImplOrTraitItemNode parent, |
There was a problem hiding this comment.
| Function f, FunctionPosition pos, TypeMention parentMention, ImplOrTraitItemNode parent, | |
| Function f, FunctionPosition pos, TypeMention parentMention, TraitItemNode parent, |
| /** | ||
| * Holds if the candidate receiver type represented by `derefChain` does not | ||
| * have a matching method target. | ||
| */ | ||
| pragma[nomagic] | ||
| predicate hasNoCompatibleTargetNoBorrow(string derefChain) { | ||
| ( | ||
| this.supportsAutoDerefAndBorrow() | ||
| or | ||
| result = TTypeParamTypeParameter(enum.getGenericParamList().getATypeParam()) and | ||
| path = TypePath::singleton(result) | ||
| // needed for the `hasNoCompatibleTarget` check in | ||
| // `SatisfiesBlanketConstraintInput::hasBlanketCandidate` | ||
| derefChain = "" | ||
| ) and | ||
| exists(TypePath strippedTypePath, Type strippedType | | ||
| not derefChain.matches("%.ref") and // no need to try a borrow if the last thing we did was a deref | ||
| strippedType = this.getComplexStrippedType(derefChain, false, strippedTypePath) and | ||
| this.hasNoCompatibleTargetCheck(derefChain, false, strippedTypePath, strippedType) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| additional class FunctionDecl extends Declaration, Function { | ||
| override TypeParameter getTypeParameter(TypeParameterPosition ppos) { | ||
| typeParamMatchPosition(this.getGenericParamList().getATypeParam(), result, ppos) | ||
| /** | ||
| * Holds if the candidate receiver type represented by `derefChain`, followed | ||
| * by a borrow, does not have a matching method target. | ||
| */ | ||
| pragma[nomagic] | ||
| predicate hasNoCompatibleTargetBorrow(string derefChain) { | ||
| exists(TypePath strippedTypePath, Type strippedType | | ||
| this.hasNoCompatibleTargetNoBorrow(derefChain) and | ||
| strippedType = this.getComplexStrippedType(derefChain, true, strippedTypePath) and | ||
| this.hasNoCompatibleTargetCheck(derefChain, true, strippedTypePath, strippedType) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Maybe now that borrow is just a boolean, it would make sense to combine these two predicates into one. That would also be a bit simpler at the usage down in hasNoCompatibleTargetEq.
| /** | |
| * Holds if the candidate receiver type represented by `derefChain` does not | |
| * have a matching method target. | |
| */ | |
| pragma[nomagic] | |
| predicate hasNoCompatibleTargetNoBorrow(string derefChain) { | |
| ( | |
| this.supportsAutoDerefAndBorrow() | |
| or | |
| result = TTypeParamTypeParameter(enum.getGenericParamList().getATypeParam()) and | |
| path = TypePath::singleton(result) | |
| // needed for the `hasNoCompatibleTarget` check in | |
| // `SatisfiesBlanketConstraintInput::hasBlanketCandidate` | |
| derefChain = "" | |
| ) and | |
| exists(TypePath strippedTypePath, Type strippedType | | |
| not derefChain.matches("%.ref") and // no need to try a borrow if the last thing we did was a deref | |
| strippedType = this.getComplexStrippedType(derefChain, false, strippedTypePath) and | |
| this.hasNoCompatibleTargetCheck(derefChain, false, strippedTypePath, strippedType) | |
| ) | |
| } | |
| } | |
| additional class FunctionDecl extends Declaration, Function { | |
| override TypeParameter getTypeParameter(TypeParameterPosition ppos) { | |
| typeParamMatchPosition(this.getGenericParamList().getATypeParam(), result, ppos) | |
| /** | |
| * Holds if the candidate receiver type represented by `derefChain`, followed | |
| * by a borrow, does not have a matching method target. | |
| */ | |
| pragma[nomagic] | |
| predicate hasNoCompatibleTargetBorrow(string derefChain) { | |
| exists(TypePath strippedTypePath, Type strippedType | | |
| this.hasNoCompatibleTargetNoBorrow(derefChain) and | |
| strippedType = this.getComplexStrippedType(derefChain, true, strippedTypePath) and | |
| this.hasNoCompatibleTargetCheck(derefChain, true, strippedTypePath, strippedType) | |
| ) | |
| } | |
| /** | |
| * Holds if the candidate receiver type represented by `derefChain` and | |
| * `borrow` does not have a matching method target. | |
| */ | |
| pragma[nomagic] | |
| predicate hasNoCompatibleTarget2(string derefChain, boolean borrow) { | |
| exists(TypePath strippedTypePath, Type strippedType | | |
| strippedType = this.getComplexStrippedType(derefChain, borrow, strippedTypePath) and | |
| this.hasNoCompatibleTargetCheck(derefChain, borrow, strippedTypePath, strippedType) | |
| | | |
| borrow = true and this.hasNoCompatibleTarget2(derefChain, false) | |
| or | |
| borrow = false and | |
| not derefChain.matches("%.ref") and // no need to try a borrow if the last thing we did was a deref | |
| ( | |
| this.supportsAutoDerefAndBorrow() | |
| or | |
| // needed for the `hasNoCompatibleTarget` check in | |
| // `SatisfiesBlanketConstraintInput::hasBlanketCandidate` | |
| derefChain = "" | |
| ) | |
| ) | |
| } |
There was a problem hiding this comment.
In terms of performance, having the predicates separate is better, because they appear in a non-linear recursion context, so I prefer to keep them like this.
|
|
||
| private module CallExprBaseMatching = Matching<CallExprBaseMatchingInput>; | ||
| /** A method call tagged with a candidate receiver type. */ |
There was a problem hiding this comment.
| /** A method call tagged with a candidate receiver type. */ | |
| /** A method call with a dereference chain and a potential borrow. */ |
| private predicate assocFunctionInfo( | ||
| Function f, string name, int arity, ImplOrTraitItemNode i, FunctionTypePosition pos, | ||
| AssocFunctionType t | ||
| ) { | ||
| f = i.getASuccessor(name) and | ||
| arity = f.getParamList().getNumberOfParams() and | ||
| t.appliesTo(f, pos, i) | ||
| } |
There was a problem hiding this comment.
Ok, though I'm not entirely convinced that we need to carry around all the values in all the predicates.
3 participants
Overview
This PR rewrites how we do call resolution and type inference for calls, to make it more faithful to what actually happens in the compiler.
Impact
The changes to expected test output shows that this PR resolves many shortcomings, as well as removes a lot of inconsistencies.
DCA is excellent: On some projects we achieve a whopping ~90 % reduction in analysis time, which follows the decrease in
Nodes With Type At Length Limitfor those projects. Oncoreutils,rendiation,peace,ruff, andgluon, however, we see increases in both analysis time andNodes With Type At Length Limit.I also did a QA run, which confirms the overall reduction in analysis time:
Top 50 largest absolute deltas
The QA run also showed that we have resolved analysis timeouts/failures for 29 projects:
Projects timeout/failure before
williamlion218/rust-sgx-sdk
zkMIPS/zkMIPS
zama-ai/tfhe-rs
veloren/veloren
kentakom1213/kyopro
TimTheBig/geo-3d
Univa/rumcake
10XGenomics/cellranger
ricosjp/truck
okaponta/atcoder-rust
jblindsay/whitebox-tools
futureversecom/trn-seed
mycroft/challenges
galacticcouncil/hydration-node
ChristopherBiscardi/advent-of-code
gasp-xyz/gasp-monorepo
dimforge/nalgebra
Apollo-Lab-Yale/apollo-rust
awsdocs/aws-doc-sdk-examples
rickyota/genoboost
SparkyPotato/radiance
strawlab/strand-braid
10XGenomics/spaceranger
sarah-quinones/faer-rs
hackmad/pbrt-v3-rs
attack68/rateslib
wingrew/thcore
Gleb-Zaslavsky/RustedSciThe
feos-org/feos
However, timeouts/failures have been introduced for 7 new projects
Projects timeout/failure after
golemfactory/yagna
carthage-software/mago
MaterializeInc/materialize
stencila/stencila
typedb/typedb
Feodor2/Mypal68
mattwparas/steel
In summary, both DCA and QA indicate overall performans wins, which is not necessarily expected (and certainly not the case for many earlier iterations of this PR), as this PR extends on the kinds of calls we are able to resolve.
For the reviewer
Note for review: As usual, commit-by-commit review is encouraged. As for the changes to
TypeInference.qll, I very much recommend using split diff view.Method call resolution
According to the spec, when resolving a method call
x.m():Before this PR, we handled the above in a very ad hoc way, where we did attempt to model implicit dereferencing and borrowing, but we did not model the construction of candidate receiver types and prioritized lookup order. In particular, if
xhad type&Foo, we would only lookup the method inFoo.With this PR, we model prioritized method lookup in the list of candidate receiver types in the module
MethodResolution, but instead of constructing the full list of candidate receiver types, we recursively compute a set of candidates, only adding a new candidate receiver type to the set when we can rule out that the method cannot be found for the current candidate:Care must be taken to ensure that the
not current_candidate matches methodcheck is monotonic, which we achieve using the monotonicisNotInstantiationOfpredicate from the shared type inference library.Method lookup
For a given candidate receiver type
C, we need to match that type against the type of theselfparameters of all potential call targets, taking into account thatselfparameters can have both explicit types and use shorthand syntax. Further care must be taken for methods that are inherited (either a trait method with a default implementation inherited by animplblock or a trait method inherited by a sub trait), so it only makes sense to talk about the type of aselfparameter in the context of a givenimplblock or trait where that method is available (either directly or inherited). We model this using the classAssocFunctionTypein the newly introducedFunctionType.qlllibrary.As before this PR, we use the
IsInstantiationOflibrary for matchingCagainst a givenAssocFunctionTypetypeS, now distinguishing between the following three cases:Cthat matches the blanket type parameter also satisfies the blanket constraint. This means that blanket implementations are now also taken into account in the context of auto-dereferencing/borrowing.Srepresents the type of aselfparameter for a method in a trait: In caseCis e.g.dyn Trait, then we wantCto matchS, but only if the traits match up as well. We achieve this by substituting in the trait in bothSandCbefore performing theIsInstantiationOfcheck.IsInstantiationOfcheck.Method call type inference
When we have identified a valid call target for
x.m()with a given candidate receiver typeC, we need to use that type as well when doing type inference. Before this PR, we used theMatchingmodule from the shared type inference library, but now we instead useMatchingWithEnvironment, where we recordCin the environment via the sequence of auto-dereferences and borrows that happened to obtainC. This means we replace the ad hoc handling mentioned earlier, because we now have explicit knowledge about dereferencing/borrowing. The implementation is in the newMethodCallMatchingInputmodule.Non-method call resolution
Resolution of non-method calls is much easier, since there is no such thing as auto-dereferencing and borrowing, even if the target is a method (
Foo::m(&x)vsx.m()). However, as for method call resolution, we still need to take three cases into account:IsInstantiationOfcheck.IsInstantiationOfcheck, using an argument or the call context, when it provides information about the return type.The implementation is in the module
NonMethodResolutionfor calls that target non-methods, and in theMethodResolutionmodule for calls that target methods.Non-method call type inference
When the call is an operator call, we need to take into account that implicit borrowing may happen. For example,
x == yis syntactic sugar forPartialEq::eq(&x, &y), so in order for the types to properly match up, we adjust the types of the operator, by stripping away the&s. This is done in the newOperationMatchingInputmodule.When the call is not an operator call, we can match types directly, which happens in the
NonMethodCallMatchingInputmodule for calls that target non-methods, and in theMethodCallMatchingInputmodule for calls that target methods.Future work
Dereftrait when performing auto-dereferencing and unsized coercions. With this PR, however, it should be much easier to support that.C_i, we will still lookup inC_(i+1)as well. Supporting this is not straightforward, since we need a monotonic way of checking blanket constraint non-satisfaction.