Rust: Avoid unnecessary constraint satisfaction#21172
Conversation
2dfa3e5 to
a116932
Compare
There was a problem hiding this comment.
Pull request overview
This PR optimizes constraint satisfaction in Rust type inference by eliminating duplicate entries for dyn Trait types. Previously, every occurrence of dyn Foo in source code would create a separate constraint satisfaction entry, even though they all had the same effect. The PR introduces a canonical representation by selecting a single dyn Trait occurrence per trait.
Changes:
- Moved
TypeAbstractionclass hierarchy fromType.qllto a new dedicatedTypeAbstraction.qllfile - Introduced canonical selection for
DynTraitTypeReprusing an equivalence relation to pick one representative per trait - Updated
DynTypeBoundListMentionto only create type mentions for canonicaldyn Traitoccurrences
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 |
|---|---|
| TypeAbstraction.qll | New file containing TypeAbstraction classes with canonical DynTypeAbstraction logic |
| TypeMention.qll | Restricts DynTypeBoundListMention to only canonical DynTypeAbstraction instances |
| TypeInference.qll | Updates import to use TypeAbstraction from new module |
| Type.qll | Removes TypeAbstraction classes (moved to TypeAbstraction.qll) |
| FunctionType.qll | Adds import for TypeAbstraction module |
| FunctionOverloading.qll | Adds import for TypeAbstraction module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| final class ImplTraitTypeReprAbstraction extends TypeAbstraction, ImplTraitTypeRepr { | ||
| override TypeParamTypeParameter getATypeParameter() { | ||
| exists(TImplTraitTypeParameter(this, result.getTypeParam())) |
There was a problem hiding this comment.
In the original code (Type.qll line 621), this was implTraitTypeParam(this, _, result.(TypeParamTypeParameter).getTypeParam()). The new version uses exists(TImplTraitTypeParameter(this, result.getTypeParam())) which assumes result is a TypeParamTypeParameter without casting. This change appears inconsistent with the move operation and could potentially cause issues if result could be other TypeParameter types. Consider adding the explicit cast: exists(TImplTraitTypeParameter(this, result.(TypeParamTypeParameter).getTypeParam()))
| exists(TImplTraitTypeParameter(this, result.getTypeParam())) | |
| exists(TImplTraitTypeParameter(this, result.(TypeParamTypeParameter).getTypeParam())) |
There was a problem hiding this comment.
Suggested change makes zero difference. This was mostly to avoid using implTraitTypeParam which is private inside Type.qll.
hvitved
left a comment
There was a problem hiding this comment.
One comment, otherwise LGTM.
In
conditionSatisfiesConstraintatwe ended up creating a way for a
dyn Footype to implementFoofor every occurrence ofdyn Fooin the source. This PR gets rid of that duplication by arbitrarily picking a single occurrence ofdyn Fooand using only that.DCA shows a smaaal speedup.