Skip to content

Rust: Remove source/library deduplication in path resolution - #20192

Merged
hvitved merged 1 commit into
github:mainfrom
hvitved:rust/path-resolution-remove-source-lib-dedup
Aug 11, 2025
Merged

Rust: Remove source/library deduplication in path resolution#20192
hvitved merged 1 commit into
github:mainfrom
hvitved:rust/path-resolution-remove-source-lib-dedup

Conversation

@hvitved

@hvitvedhvitved commented Aug 8, 2025

Copy link
Copy Markdown
Contributor

Removes logic that was left over from when library code was not extracted as source code.

@github-actionsgithub-actionsBot added the Rust Pull requests that update Rust code label Aug 8, 2025
@hvitvedhvitved added the no-change-note-required This PR does not need a change note label Aug 9, 2025
@hvitved
hvitved marked this pull request as ready for review August 9, 2025 10:17
@hvitved
hvitved requested a review from a team as a code ownerAugust 9, 2025 10:17
@hvitved
hvitved requested review from Copilot and geoffw0August 9, 2025 10:17

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 removes legacy deduplication logic for source and library code in Rust path resolution. Previously, when both source and library versions of functions existed, the code preferred source versions, but this is no longer needed since library code is no longer extracted as source code.

  • Removes getASuccessorFull method and related deduplication logic
  • Renames getASuccessorFull to getASuccessor throughout the codebase
  • Updates all references to use the simplified successor method

Reviewed Changes

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

FileDescription
rust/ql/lib/codeql/rust/internal/PathResolution.qllRemoves deduplication logic and renames method calls from getASuccessorFull to getASuccessor
rust/ql/lib/codeql/rust/internal/CachedStages.qllUpdates method reference in cached stages from getASuccessorFull to getASuccessor

@geoffw0geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I gather:

  • we used to extract library code less completely than code in the source.
  • the successors relation of interest here is on the item graph, it finds things that extend a path by a particular name.
  • we used to prefer taking the source successor, presumably because that would be more complete? than a successor defined in a library?
  • that's no longer necessary / useful because both versions would contain equally valid information now???

The last two bullets are guesswork. Please help fill the gaps in my understanding.

@hvitved

Copy link
Copy Markdown
ContributorAuthor

I gather:

  • we used to extract library code less completely than code in the source.
  • the successors relation of interest here is on the item graph, it finds things that extend a path by a particular name.
  • we used to prefer taking the source successor, presumably because that would be more complete? than a successor defined in a library?
  • that's no longer necessary / useful because both versions would contain equally valid information now???

The last two bullets are guesswork. Please help fill the gaps in my understanding.

Prior to #19506, library code was extracted in a way where a crate could be extracted both as source code and as library code, and we would have to do deduplication in QL. After that PR, everything gets extracted as source code, and the extractor does the deduplication.

@geoffw0geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. DCA run is clean. 👍

@hvitved
hvitved merged commit 9905cd6 into github:mainAug 11, 2025
21 checks passed
@hvitved
hvitved deleted the rust/path-resolution-remove-source-lib-dedup branch August 11, 2025 11:40
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-requiredThis PR does not need a change noteRustPull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@hvitved@geoffw0