Skip to content

Rust: Account for disambiguation in path resolution tests#21261

Open
hvitved wants to merge 1 commit intogithub:mainfrom
hvitved:rust/path-resolution-tests
Open

Rust: Account for disambiguation in path resolution tests#21261
hvitved wants to merge 1 commit intogithub:mainfrom
hvitved:rust/path-resolution-tests

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Feb 3, 2026

No description provided.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Feb 3, 2026
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Feb 3, 2026
@hvitved hvitved marked this pull request as ready for review February 3, 2026 13:40
@hvitved hvitved requested a review from a team as a code owner February 3, 2026 13:40
@hvitved hvitved requested review from Copilot and paldepind February 3, 2026 13:40
Copy link
Contributor

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 pull request updates the path resolution tests to properly account for disambiguation when resolving function call paths in Rust. The changes use CallExpr.getResolvedTarget() instead of resolvePath() for paths that are function expressions in call sites, which provides more accurate resolution when multiple implementations are available.

Changes:

  • Modified the path resolution query and test utility to use getResolvedTarget() for call expression function paths
  • Updated test expectations to reflect the new, more accurate path resolutions
  • Updated test input comments to document expected vs. spurious resolutions

Reviewed changes

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

File Description
rust/ql/test/library-tests/path-resolution/path-resolution.ql Added logic to distinguish call expression paths and use getResolvedTarget() for disambiguation
rust/ql/lib/utils/test/PathResolutionInlineExpectationsTest.qll Applied the same disambiguation logic to the inline expectations test
rust/ql/test/library-tests/path-resolution/path-resolution.expected Updated expected test results to reflect the new resolution behavior
rust/ql/test/library-tests/path-resolution/main.rs Updated test comments to reflect which resolutions are now found vs. missing

or
exists(CallExpr ce |
p = ce.getFunction().(PathExpr).getPath() and
i = ce.getResolvedTarget()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I think this belongs here.

I'd expect these tests to only be testing path resolution and this resolvePath predicate to reflect path resolution's resolvePath

By using getResolvedTarget we're now testing type inference + method resolution as well which seems like a conflation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll only make the change to PathResolutionInlineExpectationsTest.qll.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry that I wasn't clear enough, but I have the same thoughts about the change in PathResolutionInlineExpectationsTest.qll

I think it's worthwhile that the tests for path resolution document the state of resolvePath and that the further refinements done by type inference is documented by the tests for type inference.

As an example, some of the annotations touched by this PR are also changed by #21247. There it's useful that one can see exactly how the changes to path resolution affects resolvePath independently of type inference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, though it is a bit of a shame that some of the inline expectations indicate shortcomings fixed by type inference or shortcomings not covered by type inference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. We could add comments in the places where gaps are not covered by type inference?

And btw, #21253 is the PR I meant to link to above. I think that PR addresses the as paths by making it clear what path resolution is responsible for for such paths.

@hvitved hvitved force-pushed the rust/path-resolution-tests branch from 9c483cd to 3e362be Compare February 4, 2026 12:28
@hvitved hvitved requested a review from paldepind February 4, 2026 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants