Rust: Adjustments to type inference #19081
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses adjustments to type inference by updating generic parameter names for improved consistency.
- Renamed the generic parameter in struct MyThing from A to T.
- Renamed the generic parameters in traits MyTrait1, MyTrait2, and MyTrait3 to Tr1, Tr2, and Tr3 respectively.
Files not reviewed (8)
- rust/ql/lib/codeql/rust/elements/internal/FieldExprImpl.qll: Language not supported
- rust/ql/lib/codeql/rust/elements/internal/StructImpl.qll: Language not supported
- rust/ql/lib/codeql/rust/elements/internal/VariantImpl.qll: Language not supported
- rust/ql/lib/codeql/rust/internal/Type.qll: Language not supported
- rust/ql/lib/codeql/rust/internal/TypeInference.qll: Language not supported
- rust/ql/test/library-tests/type-inference/type-inference.expected: Language not supported
- rust/ql/test/library-tests/type-inference/type-inference.ql: Language not supported
- shared/typeinference/codeql/typeinference/internal/TypeInference.qll: Language not supported
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
|
|
||
| trait MyTrait1<A> { | ||
| fn m1(self) -> A; | ||
| trait MyTrait1<Tr1> { |
There was a problem hiding this comment.
The expected file contained some duplicated lines, making it a bit hard to se what was what. So this renames some generics s.t. fewer names are identical.
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
Thanks for making these changes. I have some mostly nitpicking comments.
| * the type parameter `tp` in `target`. | ||
| * the type parameter `tp` in `target`, if any. | ||
| * | ||
| * Note, that this predicate crucially does not depend on type inference, |
| * where the method call is an access and `new Sub<int>()` is an access | ||
| * position , which is the receiver of a method call, we have: |
There was a problem hiding this comment.
new Sub<int>() in not an access position, but it is at an access position.
| * | ||
| * new Sub<int>().ToString(); | ||
| * new Sub<int>().ToString(); | ||
| * // ^^^^^^^^^^^^^^ `apos` |
There was a problem hiding this comment.
Not quite right, see comment below.
|
|
||
| /** | ||
| * Holds if the type of `a` at `apos` has the base type `base`, and when | ||
| * viewed as an element of that type has at `path` the type `t`. |
There was a problem hiding this comment.
has at path the type t -> has the type t at path.
There was a problem hiding this comment.
Yeah, let's change it. I was trying to phrase this to match the order of the parameters, but it is clumpsy.
| Access a, Declaration target, TypePath path, Type t, TypeParameter tp | ||
| ) { | ||
| not exists(getTypeArgument(a, target, tp, _)) and | ||
| target = a.getTarget() and |
There was a problem hiding this comment.
This may or may not introduce a bad join, let's see what DCA says.
shared/typeinference/codeql/typeinference/internal/TypeInference.qll
Outdated
Show resolved
Hide resolved
aschackmull
left a comment
aschackmull
left a comment
There was a problem hiding this comment.
One minor typepath mistake, otherwise LGTM
Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>
3 participants
This PR attempts to address most of @aschackmull's comments over in #18632, plus a few additional tweaks to the type inference implementation.