Skip to content

Rust: Add telemetry for comparing against rust-analyzer #19025

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
Mar 19, 2025

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Mar 14, 2025

Adds telemetry data, for use in DCA, for comparing our QL-based implementations of path resolution and static call graph construction against what we get from rust-analyzer.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Mar 14, 2025

predicate same(Source s, Target t) {
t = RustAnalyzer::resolve(s) and
t = Ql::resolve(s)

Check warning

Code scanning / CodeQL

Redundant assignment. Warning

The variable t
has previously been assigned
the same value
.
@hvitved hvitved force-pushed the rust/rust-analyzer-comparison branch from 28ea69c to 30da4dd Compare March 14, 2025 09:18
@hvitved hvitved marked this pull request as ready for review March 14, 2025 13:33
@hvitved hvitved force-pushed the rust/rust-analyzer-comparison branch from 30da4dd to 89f6245 Compare March 14, 2025 13:48
@hvitved hvitved requested a review from paldepind March 19, 2025 07:59
Copy link
Contributor

@paldepind paldepind left a comment

Choose a reason for hiding this comment

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

A few questions, but LGTM.

private import codeql.rust.elements.internal.CallExprBaseImpl::Impl as CallExprBaseImpl

predicate isResolvable(CallExprBase c) {
CallExprBaseImpl::getCallResolvable(c).hasResolvedPath()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this different from exists(resolve(c))? If it isn't, isResolvable wouldn't have to be on the signature as it would be exists(resolve(c)) in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, there is a subtle difference: Since we don't yet have entities for all dependencies in the DB (such as built-in types), we may reference that can resolve to a canonical path, but where there is no corresponding Item with the same canonical path.

t1 != t2
}

predicate diffCount(int c) { c = count(Source s | not same(s, _) and diff(s, _, _)) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we exclude those that are in same(s, _) here? If there's overlap I guess it's because one of the resolvers produce several results for a Source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to report cases where our QL implementation resolves to multiple items, and one of them is correct. We have a consistency check for multiple resolutions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, that makes sense :)

predicate pathResolutionCompare(string key, int value) {
exists(string suffix |
PathResolutionCompare::summary(suffix, value) and
key = "Rust-analyzer path resolution comparison: " + suffix
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I would expect the value to come after a :. Maybe , or some other symbol would work better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would rather not change it, since we already have logic in DCA that depends on the exact wording of the string.

@paldepind
Copy link
Contributor

Thanks for the answers. Let's merge 🎉

@hvitved hvitved merged commit 179bae8 into github:main Mar 19, 2025
15 checks passed
@hvitved hvitved deleted the rust/rust-analyzer-comparison branch March 19, 2025 10:06
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