Rust: Improve handling of where clauses in type inference and path resolution #20140
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
paldepind
force-pushed
the
rust/type-inference-where
branch
from
955ad03 to
ff4cb51
Compare
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Looks promising, I think I've spotted a couple of bugs (I could be wrong), and we need to review DCA when it's finished.
paldepind
force-pushed
the
rust/type-inference-where
branch
from
ac4e9f9 to
6e34415
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR improves handling of where clauses in Rust type inference and path resolution by ensuring type bounds from where clauses are properly considered alongside direct trait bounds. The changes add member predicates to traits and type parameters that aggregate bounds from all sources (direct bounds and where clauses) and update path resolution and type inference to use these comprehensive bound collections.
Key Changes:
- Added
getATypeBound()andgetTypeBound(int)methods to traits and type parameters that include both direct bounds andwhereclause bounds - Updated type inference and path resolution logic to use the new comprehensive bound predicates
- Added test cases covering
whereclause scenarios for trait bounds and supertrait relationships
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/elements/internal/TypeParamImpl.qll | Adds methods to collect type bounds from both direct bounds and where clauses for type parameters |
| rust/ql/lib/codeql/rust/elements/internal/TraitImpl.qll | Adds methods to collect type bounds from both direct bounds and where clauses for traits |
| rust/ql/lib/codeql/rust/internal/TypeInference.qll | Updates type inference to use new comprehensive bound collection methods |
| rust/ql/lib/codeql/rust/internal/PathResolution.qll | Updates path resolution to use new comprehensive bound collection methods and simplifies bound checking logic |
| rust/ql/test/library-tests/type-inference/main.rs | Adds test cases for where clause scenarios in type inference |
| rust/ql/test/library-tests/path-resolution/main.rs | Adds test cases for where clause scenarios in path resolution |
| wp = any(WhereClause wc).getPredicate(i) | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
[nitpick] The getTypeBoundAt method uses magic numbers and has unclear indexing logic. Consider adding documentation to explain the indexing scheme where i=0 represents direct bounds and i>0 represents where clause predicates, or use more descriptive parameter names like source and boundIndex.
| or | ||
| exists(WherePred wp | | ||
| wp = this.getWhereClause().getAPredicate() and | ||
| wp.getTypeRepr().(PathTypeRepr).getPath().getText() = "Self" and |
There was a problem hiding this comment.
[nitpick] The hardcoded string comparison getText() = \"Self\" is fragile and may not handle all valid representations of Self. Consider using a more robust method to identify Self type references, such as checking the resolved type or using semantic analysis rather than text comparison.
| wp.getTypeRepr().(PathTypeRepr).getPath().getText() = "Self" and | |
| wp.getTypeRepr() instanceof SelfTypeRepr and |
paldepind
force-pushed
the
rust/type-inference-where
branch
from
6e34415 to
69ca7ff
Compare
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
I think we can merge this. There are a couple of things that might want addressing afterwards (namely the situation with two restrictions on Self in the where clause; and analysis performance).
|
@hvitved I'd like to merge this, and avoid letting it getting stale while @paldepind is away. However there's an analysis performance regression, particularly on a couple of projects in the DCA run. It seems possible that could improve with #20155 . What are your thoughts? |
I have opened #20177, which is a rebased version of this PR. DCA looks good, so let's merge that one. |
3 participants
It turns out that we don't handle type bounds added by
whereclauses in all cases in path resolution and type inference.This PR improves the situration by adding member predicates to traits and type parameters that take all sources of bounds into account. These are then used in path resolution and type inference.