Skip to content

Rust: Also include prelude path resolution in Core #19580

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

Merged
merged 1 commit into from
May 28, 2025

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented May 26, 2025

Needed in order to resolve paths inside the Core library itself, such as resolving Option at https://github.com/rust-lang/rust/blob/95a2212587f1b8500af1ac630322b0cb2e030c61/library/core/src/iter/traits/iterator.rs#L373.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label May 26, 2025
@hvitved hvitved marked this pull request as ready for review May 26, 2025 13:13
@Copilot Copilot AI review requested due to automatic review settings May 26, 2025 13:13
@hvitved hvitved requested a review from a team as a code owner May 26, 2025 13:13
Copy link
Contributor

@Copilot Copilot AI left a 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 extends path resolution to include the Rust core crate itself when resolving prelude items, and updates a debug helper to point at a new test file and line.

  • Include the core crate in preludeEdge, not just its dependencies
  • Update Debug.getRelevantLocatable to match test.rs at line 74 instead of term.rs at line 71
Comments suppressed due to low confidence (2)

rust/ql/lib/codeql/rust/internal/PathResolution.qll:1441

  • The filename pattern was changed from %/term.rs to %/test.rs; verify that the target file is actually named test.rs. If the intention was to match term.rs, update the pattern accordingly.
filepath.matches("%/test.rs") and

rust/ql/lib/codeql/rust/internal/PathResolution.qll:1442

  • The startline comparison changed from a list literal [71] to a single integer 74. Ensure this aligns with the predicate’s expected type: if startline can have multiple values, wrap 74 in a list.
startline = 74

@@ -1413,7 +1413,7 @@ private predicate useImportEdge(Use use, string name, ItemNode item) {
*/
private predicate preludeEdge(SourceFile f, string name, ItemNode i) {
exists(Crate core, ModuleLikeNode mod, ModuleItemNode prelude, ModuleItemNode rust |
f = any(Crate c0 | core = c0.getDependency(_)).getASourceFile() and
f = any(Crate c0 | core = c0.getDependency(_) or core = c0).getASourceFile() and
Copy link
Preview

Copilot AI May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The combined dependency check core = c0.getDependency(_) or core = c0 can be hard to read; consider adding parentheses around the OR expression or splitting it into two predicates for clarity.

Suggested change
f = any(Crate c0 | core = c0.getDependency(_) or core = c0).getASourceFile() and
f = any(Crate c0 | (core = c0.getDependency(_) or core = c0)).getASourceFile() and

Copilot uses AI. Check for mistakes.

@hvitved hvitved requested a review from paldepind May 26, 2025 17:17
@hvitved hvitved merged commit 78b9d8f into github:main May 28, 2025
16 checks passed
@hvitved hvitved deleted the rust/path-resolution-prelude-in-core branch May 28, 2025 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants