- Notifications
You must be signed in to change notification settings - Fork1.9k
Comments
Rust: Restrict type propagation into receivers#21333
Rust: Restrict type propagation into receivers#21333hvitved wants to merge 3 commits intogithub:mainfrom
Conversation
652c8db toe587541Compare| strictcount(Expr e | bodyReturns(parent, e)) > 1 and | ||
| prefix.isEmpty() | ||
| or | ||
| exists(Struct s | |
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.
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.
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 restricts type propagation into method receivers to fix a combinatorial explosion issue in Rust type inference, addressing a timeout on thestalwartlabs/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) |
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We already know thatpos is not a return position, so we can simplify this a bit.
| notprefix.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()impliesnotprefix.isEmpty() |
| } | ||
| private module ForIterableSatisfiesConstraint = | ||
| SatisfiesConstraint<ForIterableExpr, ForIterableSatisfiesConstraintInput>; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a purpose with the block here?
Uh oh!
There was an error while loading.Please reload this page.
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.