Rust: Refine Self resolution inside impl blocks - #20853
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR refines the resolution of Self inside impl blocks to consistently resolve to the type being implemented rather than the impl block itself. This enables more accurate path resolution, trait method calls, and type inference. The changes include restricting the scope of default trait implementations, preventing type parameters from escaping their scope, and improving consistency in path resolution.
- Resolves Self to the implementing type in all contexts, not to the impl block
- Prevents type parameters from escaping their declared scope during type resolution
- Restricts default trait implementations to only be inherited from the directly implemented trait, not super traits
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/internal/PathResolution.qll | Updates Self resolution logic to always resolve to the implementing type; restricts default trait implementation inheritance to direct traits only; simplifies path resolution logic |
| rust/ql/lib/codeql/rust/internal/TypeMention.qll | Adds type parameter scoping logic to prevent parameters from escaping their declaring item's scope; refactors type resolution into resolvePathTypeAt |
| rust/ql/lib/codeql/rust/internal/Type.qll | Adds getDeclaringItem() method to TypeParameter to track the scope of type parameters |
| rust/ql/lib/codeql/rust/internal/TypeInferenceConsistency.qll | Filters out known limitations for escaping type parameters in consistency checks |
| rust/ql/test/library-tests/path-resolution/main.rs | Adds test cases for improved Self resolution in impl blocks; demonstrates resolution of methods from different traits |
| rust/ql/test/library-tests/type-inference/main.rs | Removes spurious target annotation that is now correctly resolved |
| rust/ql/test/library-tests/type-inference/type-inference.expected | Updates expected results reflecting improved type inference with fewer incorrect inferences |
| rust/ql/test/library-tests/type-inference/CONSISTENCY/PathResolutionConsistency.expected | Removes spurious call target and adds multiple path resolution entries for associated types |
| rust/ql/test/library-tests/sensitivedata/CONSISTENCY/PathResolutionConsistency.expected | Removes false positive for multiple call targets |
| rust/ql/test/library-tests/path-resolution/path-resolution.expected | Updates expected path resolutions reflecting improved Self resolution to implementing types |
| rust/ql/test/library-tests/path-resolution/CONSISTENCY/PathResolutionConsistency.expected | Updates line numbers for multiple call targets after code changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
paldepind
left a comment
There was a problem hiding this comment.
Nice to get the spurious result fixed!
The new path resolution inconsistencies in the type inference tests look like a problem. The Self::Output paths resolve to all the Output associated types, including those from other impl blocks.
Since Self in Self::Output now resolves to the struct S there's not enough information after that to resolve Output to the associated type in the correct impl block.
If I try "unfolding" the Self in Self::Output only something like <S<T> as MyAdd<&'a T>>::Output (for the last case) is sufficient to make the compiler happy. And these Ty as Tr paths are basically pinning down a specific impl block. So maybe resolving Self to the impl block actually is the right thing to do?
Note also that there's a big relative increase in path resolution inconsistencies on some projects such as competitive-library and radiance. That might be related to the above, but if not, it would probably be worthwhile looking into that as well.
Sorry, something went wrong.
As discussed offline, I have put in a fix specifically for disambiguating Self::AssocType paths. |
Sorry, something went wrong.
paldepind
left a comment
There was a problem hiding this comment.
Looks great with the latest fix.
The comments for getASelfPath and getAnItemInSelfScope are somewhat outdated as Self paths no longer refer to the ImplItemNode.
Sorry, something went wrong.
paldepind
left a comment
There was a problem hiding this comment.
Looks really great! Thanks for addressing my comments.
Sorry, something went wrong.
Thanks for your comments. I just added a few more test cases. |
Sorry, something went wrong.
Before this PR, we would resolve Self inside impl blocks to either (1) the type being implemented, if Self is not a qualifier of another path, or (2) the impl block itself, otherwise.
This PR changes the logic so we always resolve Self to the type being implemented, which enables us to correctly resolve more paths.
We also restrict the scope of default trait implementations in path resolution, similar to @paldepind 's #20723, and finally prevent type parameters from escaping their scope when resolving type mentions.
DCA looks great: A significant reduction in Nodes With Type At Length Limit, and a modest increase in resolved calls.