Conversation
hvitved
force-pushed
the
rust/type-inference-sibling
branch
4 times, most recently
from
87447fb to
d6ff5b1
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Consider two implementations of the same trait to be siblings when the type being implemented by one is an instantiation of the type being implemented by the other.
hvitved
force-pushed
the
rust/type-inference-sibling
branch
from
d6ff5b1 to
e60275c
Compare
hvitved
changed the title
implHasAmbiguousSiblingAtimplSiblings
There was a problem hiding this comment.
Pull request overview
Refines Rust trait-impl sibling detection in the type inference / overload resolution logic so that impls are treated as siblings when one Self type is an instantiation of the other, reducing inconsistent resolution outcomes.
Changes:
- Extend shared type inference infrastructure with an
isPseudoTypehook to ignore pseudo types during non-instantiation checks. - Update Rust’s
implSiblingslogic to use instantiation checks rather than syntactic equality ofSelftype mentions. - Adjust Rust type-inference regression tests and expected output to match the refined sibling behavior.
Show a summary per file
| File | Description |
|---|---|
| shared/typeinference/codeql/typeinference/internal/TypeInference.qll | Adds isPseudoType to the shared input signature and uses it to ignore pseudo types in isNotInstantiationOf. |
| rust/ql/lib/codeql/rust/internal/typeinference/TypeInference.qll | Implements isPseudoType for Rust (treating UnknownType/NeverType as pseudo). |
| rust/ql/lib/codeql/rust/internal/typeinference/FunctionOverloading.qll | Refines implSiblings to consider instantiation relationships between implemented Self types; updates related logic/docs. |
| rust/ql/test/library-tests/type-inference/regressions.rs | Updates regression annotation to reflect the corrected inferred type after sibling refinement. |
| rust/ql/test/library-tests/type-inference/type-inference.expected | Updates expected test output to match new inference/resolution results. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 1
| * // ^ `path` = "T" | ||
| * } | ||
| * impl MyAdd<i64> for i64 { | ||
| * fn method(&self, value: Foo<i64>) -> Self { ... } | ||
| * // ^^^ `type` = i64 |
There was a problem hiding this comment.
In this documentation example, the trait is named MyTrait, but the subsequent impl line uses MyAdd<i64> for i64. This looks like a copy/paste typo and makes the example harder to follow; consider using the same trait name consistently throughout the snippet.
See below for a potential fix:
* impl MyTrait<i64> for i64 {
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Results LGTM. Where should I start to understand the implementation?
The changes in |
Rerun has been triggered: 4 restarted 🚀 |
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Looks convincing (though I don't rate my chances of spotting an error here very highly).
Some tests are failing.
#21206 follow-up.
Consider two implementations of the same trait to be siblings when the type being implemented by one is an instantiation of the type being implemented by the other.
For example, in
the pairs
(I1, I2),(I3, I5), and(I4, I5)are siblings, but not(I3, I4).Before this PR, only
(I1, I2)were considered siblings.DCA is fine; while we do loose some call targets, we also fix inconsistencies, most notably we reduce
Nodes With Type At Length Limit.