-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: Type inference for .await
expressions
#19584
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
Conversation
92e4659
to
25b928d
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 the Rust type inference engine to support .await
expressions, associated type arguments (Output = ...
), and impl Trait
types.
- Add new
async_
andimpl_trait
tests exercising.await
andimpl Trait
inference. - Update expected outputs for type‐inference tests to include await and
impl Trait
cases. - Extend QL libraries (
TypeMention.qll
,TypeInference.qll
,Type.qll
,Stdlib.qll
) to resolve associated types,impl Trait
parameters, and infer await expressions.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
rust/ql/test/library-tests/type-inference/main.rs | Add async_ and impl_trait modules and invoke new test functions |
rust/ql/test/library-tests/type-inference/type-inference.expected | Update expected inference output to cover .await and impl Trait cases |
rust/ql/lib/codeql/rust/internal/TypeMention.qll | Support associated type arguments and impl Trait mentions in types |
rust/ql/lib/codeql/rust/internal/TypeInference.qll | Implement inferAwaitExprType , extend method resolution for impl Trait |
rust/ql/lib/codeql/rust/internal/Type.qll | Define TImplTraitType , TImplTraitTypeParameter , and related classes |
rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll | Introduce FutureTrait with its Output associated type |
Comments suppressed due to low confidence (2)
rust/ql/lib/codeql/rust/internal/TypeInference.qll:1195
- [nitpick] This only resolves the first
impl Trait
bound. For multi-boundimpl Trait
types, consider iterating over allTImplTraitTypeParameter(_, _)
parameters so methods on any bound are discovered.
result = getTraitMethod(mc.getTypeAt(TypePath::singleton(TImplTraitTypeParameter(_, _))), mc.getMethodName())
rust/ql/lib/codeql/rust/internal/TypeInference.qll:248
- In the
typeEquality
predicate, matchingn2 = any(BlockExpr be | ...)
could match unrelated block expressions. It should constrainbe
to the actualn2
(e.g.be = n2.(BlockExpr)
) to avoid spurious matches.
n2 =
@@ -77,6 +77,16 @@ private module Input1 implements InputSig1<Location> { | |||
apos.asMethodTypeArgumentPosition() = ppos.asTypeParam().getPosition() | |||
} | |||
|
|||
private int getImplTraitTypeParameterId(ImplTraitTypeParameter tp) { |
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 getImplTraitTypeParameterId
function uses rank[result]
but never assigns result
. To return the intended integer ID, add result = id
after the rank expression or switch to rank[id]
.
Copilot uses AI. Check for mistakes.
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.
Since impl
types implement traits we should have a case for them in conditionSatisfiesConstraint
. It would also be great to have a test covering that. Something like the following should pass with impl
added to conditionSatisfiesConstraint
:
trait MyTrait<A> {
fn get_a(&self) -> A;
}
impl MyTrait<S2> for S1 {
fn get_a(&self) -> S2 {
S2
}
}
fn get_a_my_trait() -> impl MyTrait<S2> {
S1
}
fn uses_my_trait<A, B: MyTrait<A>>(t: B) -> A {
t.get_a() // $ method=MyTrait::get_a
}
fn test() {
let a = get_a_my_trait();
let b = uses_my_trait(a); // $ type=b:S2
}
25b928d
to
80d91e2
Compare
14b8af8
to
15ca62e
Compare
@paldepind : PR is ready for review again. I have rebased to resolve merge conflicts; only the last two commits need reviewing. |
15ca62e
to
2ebb619
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.
Thanks for addressing my comments.
PR looks really great and what a performance improvement 🤩
I've left a few minor review comments.
As a high-level remark it seems to me that the Matching
module is a bit heavy-handed for something like .await
. Going forward it might make sense to think about other abstractions that could handle something like .await
with less boilerplate.
54f2945
to
bf58b6c
Compare
class AssocTypeArg extends Generated::AssocTypeArg { | ||
/** Gets the type representation for the associated type argument `name`. */ | ||
pragma[nomagic] | ||
TypeRepr getTypeRepr(string name) { |
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.
Might just be me, but this signature gives me the impression that an AssocTypeArg
has several different TypeRepr
s for different names?
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.
Right; I have pushed a change that moves this into GenericArgList
instead.
bf58b6c
to
3d395dd
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.
Seems like the CI is disagreeing about some versions, but otherwise LGTM!
Adds type inference support for
.await
expressions. Since functions can be either implicitly asynchronous, using theasync
keyword, or explicitly using animpl Future<Output = ...>
return type, this PR also add support for associated type argument (Output = ...
) andimpl Trait
types.DCA looks great: We increase
Percentage of calls with call target
by 1 percentage point, but this PR also fixes a performance issue resulting in a massive 61 % analysis time speedup.