-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: Path resolution associated type fix #20096
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
base: main
Are you sure you want to change the base?
Rust: Path resolution associated type fix #20096
Conversation
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 fixes spurious results in path resolution for Rust code analysis that were causing exponential blowup in type inference. The fix addresses a fundamental issue where type parameters and associated types in trait/impl blocks were getting mixed up during path resolution, leading to incorrect recursive type interpretations.
- Introduces predicates to distinguish between local and external availability of items in trait/impl scopes
- Prevents associated items from being directly accessible inside trait/impl blocks without qualified paths
- Prevents type parameters from being accessible outside their declaring trait/impl blocks
Reviewed Changes
Copilot reviewed 5 out of 6 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 fix implementing exclusion predicates for proper path resolution scope handling |
rust/ql/test/library-tests/type-inference/main.rs | Test cases demonstrating the fixed behavior and minor formatting changes |
rust/ql/test/library-tests/path-resolution/main.rs | Additional test cases for associated types path resolution |
rust/ql/test/library-tests/type-inference/CONSISTENCY/PathResolutionConsistency.expected | Updated expected results reflecting line number changes |
rust/ql/test/library-tests/path-resolution/path-resolution.expected | Updated expected results with new test cases and line number adjustments |
@@ -112,13 +112,18 @@ abstract class ItemNode extends Locatable { | |||
result = this.(SourceFileItemNode).getSuper() | |||
} | |||
|
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 new getAChildSuccessor
predicate lacks documentation explaining its purpose and relationship to the existing successor methods.
/** | |
* Gets a child item of this item that has the specified name. | |
* | |
* This predicate is a helper for resolving paths and is used in conjunction | |
* with other successor methods like `getADescendant` and `getImmediateParent`. | |
* It identifies a child item by matching its name and its immediate parent. | |
*/ |
Copilot uses AI. Check for mistakes.
* for unqualified paths. | ||
* | ||
* This has the effect that a path of the form `name` inside `this` will not | ||
* resolve to `item`. |
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 excludedLocally
predicate would benefit from more detailed documentation explaining the specific scenarios where associated items require Self::
qualification.
* resolve to `item`. | |
* resolve to `item`. | |
* | |
* ## Specific Scenarios | |
* In Rust, associated items (such as methods, associated types, or constants) | |
* defined within an `impl` or `trait` block are not directly accessible | |
* without qualification. To access these items, a `Self::` prefix is required. | |
* | |
* For example: | |
* ``` | |
* impl MyTrait for MyType { | |
* fn my_method() {} | |
* } | |
* | |
* // Inside the `impl` block, `my_method` must be accessed as `Self::my_method`. | |
* ``` | |
* | |
* This predicate identifies such cases where unqualified access is not allowed. | |
* | |
* ## References | |
* For more details, see the Rust reference on [associated items](https://doc.rust-lang.org/reference/items/associated-items.html). |
Copilot uses AI. Check for mistakes.
* externally for qualified paths that resolve to this item. | ||
* | ||
* This has the effect that a path of the form `Qualifier::name`, where | ||
* `Qualifier` resolves to this item, will not resolve to `item`. |
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 excludedExternally
predicate should include documentation with examples showing when type parameters are not accessible outside their declaring scope.
* `Qualifier` resolves to this item, will not resolve to `item`. | |
* `Qualifier` resolves to this item, will not resolve to `item`. | |
* | |
* ## Examples | |
* | |
* Consider the following Rust code: | |
* | |
* ``` | |
* impl<T> MyStruct<T> { | |
* fn new() -> Self { | |
* // ... | |
* } | |
* } | |
* ``` | |
* | |
* Here, the type parameter `T` is not accessible outside the `impl` block. | |
* For example, the following code would result in a compilation error: | |
* | |
* ``` | |
* let x: MyStruct::T; // Error: `T` is not accessible here | |
* ``` | |
* | |
* The `excludedExternally` predicate ensures that such type parameters are | |
* excluded from external path resolution. |
Copilot uses AI. Check for mistakes.
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.
Thanks for investigating and fixing this, and for providing a great description of both the problem and solution (the recursive case surprised me!). The fix looks very plausible to me, the DCA run looks fantastic, though it would indeed be good to hear from @hvitved in case he has any suggestions before we merge this.
This PR fixes spurious results in path resolution that could cause blowup in type inference.
The path resolution problem
For a trait or an impl block its type parameters and associated types are both successors of the block's item node. This causes them to get mixed up as seen in this example:
The
Bar
in the return type offoo
should resolve to the type parameter and theSelf::Bar
in the return type ofbar
should resolve to the associated type. But path resolution doesn't distinguish them causing them to get mixed up.The effect on type inference
Here is an example of how this causes problems for type inference:
Here the
Output
type is interpreted correctly, but also incorrectly as if the type is recursive and theOutput
on the rhs. is a self reference. This causes its type to be all of the below:Result<Output, Output>
Result<Result<Output, Output>, Result<Output, Output>>
Result<Result<Result<Output, Output>, Result<Output, Output>>, Result<Result<Output, Output>, Result<Output, Output>>>
This puts type inference in an exponential "infinite" loop that gets cut off when we reach the maximum path length.
I believe the performance problems we saw with tuple types is because the
databend
project contains a case like the above but where the rhs. is a tuple and a macro is generating instances for all tuples from arity 1 up to arity 2. This leads to (in addition other things) a type mention with 12^10 paths under it (10 is the max path limit). Hence this PR should also unblock tuple type inference.DCA
We get a little above a
0.2%
point increase in resolved calls and analysis time drops by 1.4%. Path resolution inconsistencies wobble a bit but overall decreases.Some thoughts on the fix
(These are just some thoughts, maybe for the future, that can be ignored for initial review)
I struggled a bit to fix this in path resolution and the first two approached I tried didn't pan out.
The way I think of it now, the successor relation in the item graph kinda conflates two different things:
1/ the items that are available locally in the scope of an item
2/ the items that are available externally for qualified paths that resolve to the item.
For instance for the trait example
Bar1
is something that's available locally in the trait block (aBar1
path inside it can refer to it, but it's not available outside through aFoo::Bar1
path). On the other handBar2
is available externally (aFoo::Bar2
path can refer to it but aBar2
path inside it as is invalid). Most of the time this conflation evidently works perfectly fine, but in this specific case name overlap caused problems.I wonder if a design that separated the two things would be cleaner? Perhaps to different member predicates on
ItemNode
or different classes altogether? I'm not really sure though and @hvitved may have some thoughts on whether that would or wouldn't work. Instead this PR just introduces two predicates that can be used to exclude certaingetAChildSuccessor
edges for the local or external case.