Rust: Distinguish internal/external items in path resolution #20191
Conversation
hvitved
force-pushed
the
rust/path-resolution-successor-kind
branch
7 times, most recently
from
31aeb97 to
b059956
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
hvitved
force-pushed
the
rust/path-resolution-successor-kind
branch
4 times, most recently
from
b42b737 to
79c1c5b
Compare
hvitved
requested review from
Copilot and
geoffw0
and removed request for
a team
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a distinction between internal and external items in Rust path resolution to reduce ambiguities and improve type inference performance. The changes modify the item graph's successor relation to separate locally available items from externally available items for qualified paths.
Key changes:
- Introduces
SuccessorKindenumeration to distinguish internal, external, and both visibility types - Updates path resolution logic to consider successor kinds when resolving paths
- Refactors various edge predicates to include successor kind information
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/internal/PathResolution.qll | Core implementation adding SuccessorKind and updating path resolution logic |
| rust/ql/lib/codeql/rust/internal/TypeInference.qll | Updates type inference consistency checks to work with new path resolution |
| rust/ql/lib/codeql/rust/internal/TypeInferenceConsistency.qll | Adds query predicate for non-unique certain types |
| rust/ql/lib/codeql/rust/internal/CachedStages.qll | Updates cached stages to reference new successor method signature |
| Multiple .expected files | Test expectation updates reflecting improved path resolution consistency |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| ModuleItemNode rust | ||
| | | ||
| stdOrCore.getName() = stdOrCoreName and | ||
| stdOrCoreName = ["std", "core"] and |
There was a problem hiding this comment.
[nitpick] The variable stdOrCoreName is redundant as it's immediately assigned from stdOrCore.getName(). Consider removing this intermediate variable and using stdOrCore.getName() directly in the condition.
| stdOrCoreName = ["std", "core"] and | |
| Crate stdOrCore, ModuleLikeNode mod, ModuleItemNode prelude, | |
| ModuleItemNode rust | |
| | | |
| stdOrCore.getName() = ["std", "core"] and |
Copilot uses AI. Check for mistakes.
| @@ -580,7 +617,7 @@ class ImplItemNode extends ImplOrTraitItemNode instanceof Impl { | |||
|
|
|||
| Path getTraitPath() { result = super.getTrait().(PathTypeRepr).getPath() } | |||
|
|
|||
| ItemNode resolveSelfTy() { result = resolvePath(this.getSelfPath()) } | |||
| TypeItemNode resolveSelfTy() { result = resolvePath(this.getSelfPath()) } | |||
There was a problem hiding this comment.
The return type has been changed from ItemNode to TypeItemNode, which is a breaking change. Ensure that all callers of this method can handle the more restrictive return type.
Copilot uses AI. Check for mistakes.
| @@ -669,7 +706,7 @@ class ImplItemNode extends ImplOrTraitItemNode instanceof Impl { | |||
| } | |||
| } | |||
|
|
|||
| private class ImplTraitTypeReprItemNode extends ItemNode instanceof ImplTraitTypeRepr { | |||
| private class ImplTraitTypeReprItemNode extends TypeItemNode instanceof ImplTraitTypeRepr { | |||
There was a problem hiding this comment.
The parent class has been changed from ItemNode to TypeItemNode, which is a breaking change. Verify that this change is compatible with all existing usage patterns.
| private class ImplTraitTypeReprItemNode extends TypeItemNode instanceof ImplTraitTypeRepr { | |
| private class ImplTraitTypeReprItemNode extends ItemNode instanceof ImplTraitTypeRepr { |
Copilot uses AI. Check for mistakes.
hvitved
force-pushed
the
rust/path-resolution-successor-kind
branch
from
79c1c5b to
a07e357
Compare
This PR addresses @paldepind's comment
from #20096. The goal is to reduce path resolution ambiguities, which in turn helps avoiding type inference blowup.
DCA looks fine: We significantly reduce the number of path resolution and type inference inconsistencies, get a marginal speedup, but at the cost of a small decrease in percentage of resolvable calls (down 61.28 % from 61.61 %).