-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Rust: Unify type inference for tuple indexing expressions #20182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rust: Unify type inference for tuple indexing expressions #20182
Conversation
0d7a2f6
to
454ab4d
Compare
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 unifies type inference for tuple indexing expressions by treating all field expressions (x.i
) uniformly, regardless of whether the type of x
is a tuple or a tuple struct/enum. This aligns with the comment in the GitHub issue to standardize the handling of tuple field access.
- Introduces a new
TupleTypeParameterDecl
declaration type within the field expression matching framework - Removes separate handling functions for tuple indexing expressions in favor of the unified field expression approach
- Adds test case to validate tuple field access through references
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.qll |
Main implementation: adds TupleTypeParameterDecl to the field expression matching system and removes old tuple-specific inference functions |
rust/ql/lib/codeql/rust/internal/Type.qll |
Minor fix: removes redundant TTuple constraint from UnitType class |
rust/ql/test/library-tests/type-inference/main.rs |
Adds test case for tuple field access through references |
rust/ql/test/library-tests/type-inference/type-inference.expected |
Updates expected test output to reflect the unified type inference approach |
abstract string toString(); | ||
|
||
abstract Location getLocation(); | ||
} |
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.
The abstract Declaration
class has a concrete implementation of getTypeParameter
that returns none()
for all subclasses. This suggests that either this method should be abstract (forcing subclasses to implement it) or it should have a more meaningful default implementation. Consider making this method abstract if different declaration types should handle type parameters differently.
Copilot uses AI. Check for mistakes.
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.
Changes and DCA LGTM.
@aibaars FYI.
This PR adresses the comment https://github.com/github/codeql/pull/20041/files#r2207752592, i.e. type inference for all field expressions,
x.i
, are treated uniformly, regardless of whether the type ofx
is a tuple or a tuple struct/enum.