Rust: Restrict type propagation into receivers#21333
Rust: Restrict type propagation into receivers#21333hvitved wants to merge 3 commits intogithub:mainfrom
Conversation
652c8db to
e587541
Compare
| strictcount(Expr e | bodyReturns(parent, e)) > 1 and | ||
| prefix.isEmpty() | ||
| or | ||
| exists(Struct s | |
There was a problem hiding this comment.
This change is not what solves the timeout, but I saw cases where type information would incorrectly flow between limits in range expressions, so I decided to treat them as LUB conversions.
There was a problem hiding this comment.
Pull request overview
This PR restricts type propagation into method receivers to fix a combinatorial explosion issue in Rust type inference, addressing a timeout on the stalwartlabs/stalwart repository. The change prevents type information from being propagated back into receiver positions during type inference, since the receiver type must already be known for method resolution to occur.
Changes:
- Modified type inference logic to restrict type propagation into receivers by introducing a new predicate
assocFunctionMentionsTypeParameterAtNonRetPosand updating the context typing logic to never propagate types directly into receivers when the prefix is empty - Refactored the type inference signature from
boolean isReturntoFunctionPosition posfor more precise position tracking - Moved Range type parameter constraints from
typeEqualitytolubCoercionto better reflect their coercion semantics - Added a regression test demonstrating the combinatorial explosion scenario with recursive enum types and method chaining
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/internal/typeinference/TypeInference.qll | Core type inference logic changes: refactored position tracking, added receiver restriction, moved Range coercion logic, and simplified several helper predicates |
| rust/ql/test/library-tests/type-inference/regressions.rs | New regression test file demonstrating the combinatorial explosion case with recursive enum types |
| rust/ql/test/library-tests/type-inference/main.rs | Added module declaration for the new regressions test file |
| rust/ql/test/library-tests/type-inference/type-inference.expected | Updated expected output reflecting the restricted type propagation (one line removed at 9514, new entries added for regression test) |
rust/ql/lib/codeql/rust/internal/typeinference/TypeInference.qll
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| pos.isPosition() | ||
| or | ||
| // Never propagate type information directly into the receiver, since its type | ||
| // must already have been known in order to resolve the call |
There was a problem hiding this comment.
Is this always true? Can we not have cases where a non-self argument is used to find the receiver?
| // Never propagate type information directly into the receiver, since its type | ||
| // must already have been known in order to resolve the call | ||
| pos.isSelf() and | ||
| not prefix.isEmpty() |
There was a problem hiding this comment.
We already know that pos is not a return position, so we can simplify this a bit.
| not prefix.isEmpty() | |
| // Never propagate type information directly into the root of the receiver, since its type | |
| // must already have been known in order to resolve the call | |
| pos.isSelf() implies not prefix.isEmpty() |
| } | ||
|
|
||
| private module ForIterableSatisfiesConstraint = | ||
| SatisfiesConstraint<ForIterableExpr, ForIterableSatisfiesConstraintInput>; |
There was a problem hiding this comment.
These aliases will be picked up by the compiler to provide better names for the module instantiation predicates in the RA 😄 (which you know obviously).
| let _ = if let Some(last) = vec_e.pop() // $ target=pop | ||
| { | ||
| opt_e = last.into(); // $ target=into | ||
| }; |
There was a problem hiding this comment.
Is there a purpose with the block here?
Fixes a source of type inference combinatorial explosion (see test), which fixes a timeout on
stalwartlabs/stalwart.DCA is great: only a modest decrease in
Percentage of calls with call target, but on the other hand a large decrease inNodes With Type At Length Limit.