-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Rust: Improve handling of Self type parameter
#20707
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
Conversation
6002b2d to
7225a86
Compare
Self type parameter
7225a86 to
9022f99
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 adds support for handling trait type parameters with defaults that contain the implicit Self type parameter. The main enhancement allows the CodeQL type inference system to correctly resolve type defaults like trait TraitWithSelfTp<A = Option<Self>> by substituting Self with the concrete implementing type.
Key changes:
- Added logic to detect and substitute
Selftype parameters in trait type parameter defaults - Added support for resolving
Selftype parameters from trait bounds, impl blocks, and type parameter bounds - Added comprehensive test cases covering various scenarios where
Selfappears in type parameter defaults
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/internal/TypeMention.qll | Implements core logic for detecting and resolving Self type parameters in trait defaults across different contexts |
| rust/ql/test/library-tests/type-inference/main.rs | Adds test module trait_default_self_type_parameter with various test cases and corresponding call in main function |
| rust/ql/test/library-tests/type-inference/CONSISTENCY/PathResolutionConsistency.expected | Updates expected line numbers in consistency test results (shifted by test additions) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private TypeMention getSelfTypeParameter() { | ||
| exists(ImplItemNode impl | this = impl.getTraitPath() and result = impl.(Impl).getSelfTy()) | ||
| or | ||
| exists(Trait subTrait | | ||
| this = subTrait.getATypeBound().getTypeRepr().(PathTypeRepr).getPath() and | ||
| result.(SelfTypeParameterMention).getTrait() = subTrait | ||
| ) | ||
| or | ||
| exists(TypeParamItemNode tp | this = tp.getABoundPath() and result = tp) | ||
| } |
Copilot
AI
Oct 29, 2025
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 getSelfTypeParameter method lacks documentation explaining its purpose and the three distinct contexts where Self type parameters can be resolved. Consider adding a comment block explaining: (1) impl blocks, (2) trait supertraits, and (3) type parameter bounds.
| if not ty = TSelfTypeParameter(resolved) | ||
| then result = ty and path = prefix | ||
| else | ||
| // When a default contains an implicit `Self` type parameter, it should | ||
| // be substituted for the type that implements the trait. | ||
| exists(TypePath suffix | | ||
| path = prefix.append(suffix) and | ||
| result = this.getSelfTypeParameter().resolveTypeAt(suffix) | ||
| ) |
Copilot
AI
Oct 29, 2025
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.
[nitpick] The comment on lines 186-187 could be more specific. It would be clearer to state that when a default type parameter is the Self type parameter (e.g., A = Option<Self>), it must be substituted with the concrete type from the trait implementation context.
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.
Very nice work; some trivial comments.
|
|
||
| mod trait_default_self_type_parameter { | ||
| // A trait with a type parameter that defaults to `Self`. | ||
| // trait TraitWithSelfTp<A = Self> { |
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.
Remove
| * Gets the type mention that instantiates the implicit `Self` type parameter | ||
| * for this path, if it occurs in the position of a trait bound. | ||
| */ | ||
| private TypeMention getSelfTypeParameter() { |
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.
getSelfTraitBoundArg?
| result = this.getDefaultPositionalTypeArgument(i, path) | ||
| } | ||
|
|
||
| private Type getDefaultPositionalTypeArgument(int i, TypePath path) { |
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.
I would move this up before SelfTypeParameterMention.
|
Thanks for the review. I've addressed the comments. The DCA report shows a small slowdown on average. When I evaluate locally I'm not seeing anything that sticks out (bad joins, etc.), so it might be noise or unavoidable. |
This PR fixes two issues in how we handle
Selftype parameters. They're separate but related issues, so I've bunched them into one PR. Together these fixes should address https://github.com/github/codeql/pull/20682/files#r2464739113.1/
Selfin a default for a trait type parameterIf we have a default like this:
Then, when
TraitWithSelfTpis used as a trait bound, the meaning of theSelfdepends where that trait bound occurs. For instance,T : TraitWithSelfTpis the same asT : TraitWithSelfTp<T>and notT : TraitWithSelfTp<Self_TraitWithSelfTp>The type mention
Selfin the default refers to the theSelftype parameter forTraitWithSelfTp, but this needs to be translated at trait bounds.2/
Selfin supertraitsWhen
Selfoccurs in the trait hierarchythen those
Selfoccurrences need to be instantiated such that the sharedconditionSatisfiesConstraintTypeAtmachinery calculates the correct trait hierarchy. Above, we should have thatSubSubimplementsSuper<Self_SubSub>and not some otherSelfparameter. To this end, the type mention for supertraits must instantiate the supertrait'sSelftype parameter.