Skip to content

Rust: Path resolution for extern crates #19614

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 2 commits into from
Jun 10, 2025

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented May 28, 2025

I noticed that we were not able to resolve the String struct. This happens because it is reexported using an extern crate definition. This PR adds path resolution support for extern crates, and as the test output witnesses we are now able to resolve String.

DCA shows a whopping 4 percentage point increase in resolved calls (up 54 % from 50 %).

@github-actions github-actions bot added the Rust Pull requests that update Rust code label May 28, 2025
@hvitved hvitved force-pushed the rust/path-resolution-extern-crate branch 5 times, most recently from 65edd1c to 521b69d Compare June 3, 2025 07:03
@hvitved hvitved marked this pull request as ready for review June 3, 2025 07:03
@Copilot Copilot AI review requested due to automatic review settings June 3, 2025 07:03
@hvitved hvitved requested a review from a team as a code owner June 3, 2025 07:03
@hvitved hvitved requested a review from paldepind June 3, 2025 07:03
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 adds support for resolving items reexported via extern crate, updates core resolution logic, and adjusts test expectations accordingly

  • Introduces externCrateEdge and ExternCrateItemNode in PathResolution.qll to handle extern crate imports
  • Extends library path-resolution tests (main.rs, my.rs) to validate resolving String and other items through extern crate aliases
  • Updates numerous expected outputs and diagnostics (including SummaryStatsReduced.expected) to reflect new path resolutions

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.

File Description
rust/ql/lib/codeql/rust/internal/PathResolution.qll Added externCrateEdge, ExternCrateItemNode, and stub overrides
rust/ql/test/library-tests/path-resolution/main.rs Added extern crate self as zelf and updated use to test aliasing
rust/ql/test/library-tests/path-resolution/my.rs Extended test to assert String resolution via extern crate
rust/ql/test/diagnostics/SummaryStatsReduced.expected Incremented path resolution inconsistency count
Comments suppressed due to low confidence (3)

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

  • [nitpick] The ExternCrateItemNode.getNamespace override returns none, preventing extern crate aliases from providing a namespace; consider returning an appropriate namespace or documenting why none() is correct.
override Namespace getNamespace() { none() }

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

  • [nitpick] The ExternCrateItemNode.hasCanonicalPath override returns none, which may unintentionally disable canonical path mappings for extern crates; ensure this behavior is intended or implement proper canonical path logic.
override predicate hasCanonicalPath(Crate c) { none() }

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

  • [nitpick] The ExternCrateItemNode.getCanonicalPath override returns none, which may prevent resolution of canonical paths for extern crate items; consider providing a canonical path or clarifying why none() is appropriate.
override string getCanonicalPath(Crate c) { none() }

@hvitved hvitved force-pushed the rust/path-resolution-extern-crate branch from 521b69d to 721ffb1 Compare June 4, 2025 19:14
@hvitved hvitved merged commit 79a8942 into github:main Jun 10, 2025
17 checks passed
@hvitved hvitved deleted the rust/path-resolution-extern-crate branch June 10, 2025 10:37
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