Rust: Path resolution improvements #20453
Conversation
hvitved
force-pushed
the
rust/path-resolution-use-reexport
branch
3 times, most recently
from
053ac0e to
507bbf5
Compare
hvitved
changed the title
pub use from use in path resolution
hvitved
force-pushed
the
rust/path-resolution-use-reexport
branch
from
0216c76 to
f6bdfba
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR improves Rust path resolution by properly handling visibility modifiers and fixing import-with-rename behavior. The key changes distinguish between pub and non-pub items in path resolution logic, while ensuring non-pub items remain visible within the same crate. Additionally, the PR fixes a bug where importing an already renamed item would import under the original name instead of the correct renamed identifier.
- Better visibility handling for path resolution based on
pubmodifiers - Fixed import-with-rename bug to use correct names
- Improved crate-local visibility for non-public items
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/internal/PathResolution.qll | Core logic changes for visibility handling and import name resolution |
| rust/ql/test/library-tests/path-resolution/main.rs | Test case updates to verify fixed import behavior |
| rust/ql/test/library-tests/path-resolution/my2/mod.rs | Test case for renamed imports and visibility |
| rust/ql/test/library-tests/path-resolution/my2/my3/mod.rs | Additional test for nested import resolution |
| rust/ql/test/library-tests/dataflow/sources/test.rs | Comment updates reflecting improved resolution |
| ModuleLikeNode getSuper() { | ||
| result = any(ModuleItemNode mod | fileImport(mod, this)).getASuccessor("super") | ||
| } | ||
| ModuleLikeNode getSuper() { fileImport(result.getAnItemInScope(), this) } |
There was a problem hiding this comment.
The logic change from using any(ModuleItemNode mod | fileImport(mod, this)).getASuccessor(\"super\") to fileImport(result.getAnItemInScope(), this) appears to invert the relationship. The original code finds a module that imports this file and gets its super, while the new code finds what this file imports. This could break super module resolution.
| ModuleLikeNode getSuper() { fileImport(result.getAnItemInScope(), this) } | |
| ModuleLikeNode getSuper() { any(ModuleItemNode mod | fileImport(mod, this)).getASuccessor("super") } |
Copilot uses AI. Check for mistakes.
| exists(string pathName | | ||
| pathName = tree.getPath().getText() and | ||
| if pathName = "self" then name = item.getName() else name = pathName | ||
| ) |
There was a problem hiding this comment.
This logic assumes that when pathName is not "self", the path text should be used as the import name. However, for qualified paths like foo::bar::baz, this would use "baz" as the name, but the original logic used item.getName() which might be different if the item was already renamed in an intermediate import.
| exists(string pathName | | |
| pathName = tree.getPath().getText() and | |
| if pathName = "self" then name = item.getName() else name = pathName | |
| ) | |
| name = item.getName() |
Copilot uses AI. Check for mistakes.
| let mut f1 = async_std::fs::OpenOptions::new().open("f1.txt").await?; // $ Alert[rust/summary/taint-sources] | ||
| let mut buffer = [0u8; 1024]; | ||
| let _bytes = f1.read(&mut buffer).await?; | ||
| sink(&buffer); // $ MISSING: hasTaintFlow="f1.txt" |
| // that information in `getASuccessor`. So, for simplicity, we allow for non-public | ||
| // items when the path and the item are in the same crate. | ||
| getCrate(path) = getCrate(result) and | ||
| not result instanceof TypeParam |
There was a problem hiding this comment.
This line is just because the case we're handling here never overlaps with type parameters?
There was a problem hiding this comment.
It is to avoid the case that you originally fixed in #20096.
Before this PR,
pub usewas distinguished fromuse, but e.g.pub fnwas not distinguished fromfnin path resolution. Now, we only considerpubitems to be externally accessible. However, even non-pubitems may be visible when accessing a declaration from an ancestor module; we currently approximate this by requiring that the access and the item belong to the same crate.This PR also fixes a bug where importing an already renamed item was not handled correctly (it would import under the original name instead of the rename).
DCA looks fine; a small increase in resolvable calls.