Shared, Rust: Use HasTypeTreeSig for TypeMention
#21215
Merged
+139
−164
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paldepind
changed the title
paldepind
changed the title
HasTypeTreeSig for TypeMention
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the shared type inference library to eliminate code duplication by making TypeMention a parameter to the Make2 module that uses the HasTypeTreeSig signature. The Rust implementation is adapted by renaming resolveTypeAt to getTypeAt and resolveType to getType to directly implement the HasTypeTreeSig interface, removing the need for the TypeMentionTypeTree adapter class.
Changes:
- Parameterized
InputSig2in the shared library withHasTypeTreeSig TypeMention, removing duplication - Renamed
resolveTypeAttogetTypeAtandresolveTypetogetTypethroughout Rust's type inference implementation - Removed the
TypeMentionTypeTreeadapter class from both shared and Rust libraries
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| shared/typeinference/codeql/typeinference/internal/TypeInference.qll | Parameterized InputSig2 with HasTypeTreeSig TypeMention, removed TypeMentionTypeTree adapter, renamed internal helpers from resolveTypeMentionRoot to getTypeMentionRoot, updated documentation |
| rust/ql/lib/codeql/rust/internal/typeinference/TypeMention.qll | Renamed resolveTypeAt to getTypeAt and resolveType to getType in TypeMention class and all subclasses |
| rust/ql/lib/codeql/rust/internal/typeinference/TypeInferenceConsistency.qll | Updated call from resolveType() to getType() |
| rust/ql/lib/codeql/rust/internal/typeinference/TypeInference.qll | Updated module instantiation to use Make2<TypeMention, Input2>, updated all call sites to use getTypeAt |
| rust/ql/lib/codeql/rust/internal/typeinference/FunctionType.qll | Updated calls from resolveTypeAt to getTypeAt |
| rust/ql/lib/codeql/rust/internal/typeinference/FunctionOverloading.qll | Updated calls from resolveTypeAt to getTypeAt and resolveType to getType |
| rust/ql/lib/codeql/rust/internal/typeinference/DerefChain.qll | Updated call from resolveTypeAt to getTypeAt |
| rust/ql/lib/codeql/rust/internal/typeinference/BlanketImplementation.qll | Updated calls from resolveType to getType and resolveTypeAt to getTypeAt |
| rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll | Updated calls from resolveType to getType |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
DCA shows nothing as expected. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the shared type inference library the
TypeMentionclass really just duplicates the content of theHasTypeTreeSig.This PR turns
TypeMentioninto a parameter to the module. The parameter usesHasTypeTreeSigwhich removes the duplication. This also gets rid ofTypeMentionTypeTreein the shared library.Adapting Rust is done in two commits. The first is the minimum necessary changes. Here
TypeMentionTypeTreestill exists on the Rust side. In a second commit Rust'sTypeMentionis made to directly implementHasTypeTreeSigby renamingresolveTypeAttogetTypeAt. As far as I recall that rename was discussed previously with some disagreement. So consider the last commit a proposal that we can potentially drop from the PR. Personally I like it as it simplifies things and I don't see much difference between using "get" or "resolve".