Shared: Make some generalizations in type inference library #20358
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces generalizations in the type inference library to make it more flexible and abstract. It separates the constraint type from being limited to TypeMentions and introduces a new state-aware matching module.
- Abstract the
IsInstantiationOfInputsignature to work with different constraint types instead of onlyTypeMentions - Introduce
MatchingWithStatewhich extends the existingMatchingmodule to track state during type matching - Preserve backward compatibility by implementing the existing
Matchingmodule in terms ofMatchingWithState
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| shared/typeinference/codeql/typeinference/internal/TypeInference.qll | Core refactoring to generalize constraint types and add state-aware matching functionality |
| rust/ql/lib/codeql/rust/internal/TypeInference.qll | Updates to use new generalized signature parameters in Rust-specific type inference implementation |
| final private class FinalTypeMention = TypeMention; | ||
|
|
||
| /** An adapter for type mentions to implement `HasTypeTreeSig`. */ | ||
| final class TypeMentionTypeTree extends FinalTypeMention { | ||
| Type getTypeAt(TypePath path) { result = this.resolveTypeAt(path) } | ||
| } |
There was a problem hiding this comment.
[nitpick] This adapter class is defined early but only used later in the code. Consider moving this definition closer to where it's first used (around line 731) to improve code organization and readability.
Copilot uses AI. Check for mistakes.
| private module Inp implements MatchingWithStateInputSig { | ||
| private import codeql.util.Unit | ||
| import Input | ||
|
|
||
| predicate adjustAccessType = Input::adjustAccessType/6; | ||
|
|
||
| class State = Unit; | ||
|
|
||
| final private class AccessFinal = Input::Access; | ||
|
|
||
| class Access extends AccessFinal { | ||
| Type getInferredType(State state, AccessPosition apos, TypePath path) { | ||
| exists(state) and | ||
| result = super.getInferredType(apos, path) | ||
| } | ||
|
|
||
| Declaration getTarget(State state) { | ||
| exists(state) and | ||
| result = super.getTarget() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The exists(state) conditions in lines 1660 and 1665 are redundant since state is bound by the method signature. These conditions don't provide any meaningful constraint and should be removed for clarity.
Copilot uses AI. Check for mistakes.
|
I'm trying to understand why a client of the |
Indeed, I tried that in my use case, but that resulted in non-monotonic recursion. |
|
I don't understand why putting it in Another suggestion, what about renaming |
I tried it again, and the non-monotonicity happens via the three calls we have to |
|
Ok, without understanding all the details I can see how the negation there could cause non-monotonic recursion. What do you think about keeping the In that case I would just bikeshed about the name. I think "state" doesn't say much, and to me "state" implies something that's mutated or otherwise updated, whereas in #20282 it seems that |
I would prefer to keep it here, to reduce the size of the other PR (which is already pretty big).
You are right, the "state" is not mutated; how about |
👍
That's fine by me. I think the |
hvitved
force-pushed
the
shared/type-inference-generalize
branch
from
f955378 to
e3e1bcd
Compare
Extracted from #20282:
IsInstantiationOfInput, instead of limiting it toTypeMentions.MatchingWithState, which is likeMatching, but allows for state to be tracked. This will eventually allow for us to get rid ofadjustAccessType.DCA is uneventful.