Rust: Type inference for .await expressions
#19584
Conversation
84ae3af to
92e4659
Compare
92e4659 to
25b928d
Compare
There was a problem hiding this comment.
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_traittests exercising.awaitandimpl Traitinference. - Update expected outputs for type‐inference tests to include await and
impl Traitcases. - Extend QL libraries (
TypeMention.qll,TypeInference.qll,Type.qll,Stdlib.qll) to resolve associated types,impl Traitparameters, 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 Traitbound. For multi-boundimpl Traittypes, 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
typeEqualitypredicate, matchingn2 = any(BlockExpr be | ...)could match unrelated block expressions. It should constrainbeto 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.
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.
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
}| * We represent `impl Trait` types as generic types with as many type parameters | ||
| * as there are bounds. | ||
| * | ||
| * [1] https://doc.rust-lang.org/book/ch10-02-traits.html#traits-as-parameters |
There was a problem hiding this comment.
I think it's better to link to the reference than to the book.
| * [1] https://doc.rust-lang.org/book/ch10-02-traits.html#traits-as-parameters | |
| * [1] https://doc.rust-lang.org/reference/types/impl-trait.html |
| TImplTraitType(int bounds) { | ||
| bounds = any(ImplTraitTypeRepr impl).getTypeBoundList().getNumberOfBounds() | ||
| } or |
There was a problem hiding this comment.
If we have two functions like
fn f() -> impl MyTrait { .. }
fn g() -> impl MyTrait { .. }then they return two different types (in the general case) even though they are syntactically identical.
Given that wouldn't it be more accurate to represent impl types like:
| TImplTraitType(int bounds) { | |
| bounds = any(ImplTraitTypeRepr impl).getTypeBoundList().getNumberOfBounds() | |
| } or | |
| TImplTraitType(ImplTraitTypeRepr impl) or |
I.e., every impl is it own types, similar to how every struct or enum is its own type. I think this would also simplify things a bit as the impl types wouldn't need to have type parameters.
| not be.isAsync() and | ||
| n1 = be.getStmtList().getTailExpr() and | ||
| path1 = path2 | ||
| ) |
There was a problem hiding this comment.
Using exists here would be more consistent with the surrounding code.
| pub async fn f() { | ||
| f1().await.f(); // $ method=S1f | ||
| f2().await.f(); // $ method=S1f | ||
| f3().await.f(); // $ method=S1f |
There was a problem hiding this comment.
Would be nice to test this case as well:
| f3().await.f(); // $ method=S1f | |
| f3().await.f(); // $ method=S1f | |
| S2.await.f(); // $ MISSING: method=S1f |
| not exprPath | ||
| .isCons(TImplTraitTypeParameter(_, _), | ||
| any(TypePath path0 | path0.isCons(getFutureOutputTypeParameter(), _))) | ||
| ) |
There was a problem hiding this comment.
Is it correct that this covers most of the special cases that occurs in practice but doesn't handle the general case where the awaited expression is some arbitrary type that implements Future?
It might be nice to add some comments about which cases are covered and about what is not covered.
For full generality I guess that we might have to expose conditionSatisfiesConstraintTypeAt in some form or implement await using the Matching module? (EDIT: Just to be clear I'm not saying we should do that.)
Adds type inference support for
.awaitexpressions. Since functions can be either implicitly asynchronous, using theasynckeyword, or explicitly using animpl Future<Output = ...>return type, this PR also add support for associated type argument (Output = ...) andimpl Traittypes.