Rust: Remove member predicates on Type#20557
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Rust type system by removing member predicates getStructField and getTupleField from the base Type class and replacing asItemNode() with more specific methods asStruct() and asUnion(). The changes improve code clarity by making type-specific operations explicit rather than abstract.
- Removes abstract methods
getStructFieldandgetTupleFieldfrom the baseTypeclass - Replaces
asItemNode()calls with more specificasStruct()andasUnion()methods - Updates field resolution logic to use type-specific accessors directly
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/internal/Type.qll | Removes abstract predicates from Type class and adds asStruct()/asUnion() methods |
| rust/ql/lib/codeql/rust/internal/TypeInference.qll | Updates field resolution and type checking to use new accessor methods |
| rust/ql/lib/codeql/rust/frameworks/rustcrypto/RustCrypto.qll | Updates algorithm name extraction to use asStruct() instead of asItemNode() |
|
|
||
| override TupleField getTupleField(int i) { result = struct.getTupleField(i) } | ||
| /** Get the struct that this struct type represents. */ | ||
| Struct asStruct() { result = struct } |
There was a problem hiding this comment.
nit: I think getStruct is better; as is normally used when there is a possibility that the predicate does not hold.
There was a problem hiding this comment.
Alternatively, move the predicate all the way up into Type; then you can replace all of the (StructType).asStruct() occurrences with .asStruct().
There was a problem hiding this comment.
Thanks, I didn't know that. I thought as* was for when the predicate returned "everything" in this and thus was a kind of type cast. I prefer to keep it on StructType though, otherwise we need all the none() predicates again.
| override StructField getStructField(string name) { result = struct.getStructField(name) } | ||
|
|
||
| override TupleField getTupleField(int i) { result = struct.getTupleField(i) } | ||
| /** Get the struct that this struct type represents. */ |
|
|
||
| override TupleField getTupleField(int i) { none() } | ||
| /** Get the union that this union type represents. */ | ||
| Union asUnion() { result = union } |
|
Thanks for the review. Comments should be addressed now. |
2 participants
I don't think the
getStructFieldandgetTupleFieldpredicates carries their weight enough to be on theTypeclass itself. I thus wanted to define them only on the relevant subclasses, but realized that removing them altogether also makes sense.With the new
asStructandasUnionwe don't needasItemNodeeither as usingasStructis clearer. For instance inwe're checking if the left hand side is a special subclass of
Struct, so using aStructdirectly makes more sense than using anItemNode.