Skip to content

Rust: restrict canonical path calculations #18165

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 12 commits into from
Dec 5, 2024

Conversation

redsun82
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Nov 29, 2024
@@ -4,8 +4,8 @@
*/

private import internal.PathImpl
import codeql.rust.elements.AstNode

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
codeql.rust.elements.PathSegment
.
@redsun82 redsun82 marked this pull request as draft November 29, 2024 15:00
import codeql.rust.elements.ParenType
import codeql.rust.elements.Pat
import codeql.rust.elements.Path
import codeql.rust.elements.PathAstNode

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
codeql.rust.elements.PathExpr
.
Redundant import, the module is already imported inside
codeql.rust.elements.PathPat
.
Redundant import, the module is already imported inside
codeql.rust.elements.StructExpr
.
Redundant import, the module is already imported inside
codeql.rust.elements.StructPat
.
Redundant import, the module is already imported inside
codeql.rust.elements.TupleStructPat
.
@redsun82
Copy link
Contributor Author

redsun82 commented Dec 2, 2024

@hvitved could you lend me a hand here with the dataflow changes? I tried to preserve the implementation while shifting around the path resolution from Path to "stuff with paths" (e.g. PathExpr or RecordExpr and others), but it seems like it's not the case considering the new results in the local data flow tests.

@redsun82
Copy link
Contributor Author

redsun82 commented Dec 2, 2024

Seems like this is helping performance, although not by a great deal: end2end time went down with a median of around -4%, with some faster outliers:
https://github.com/github/codeql-dca-main/blob/data/redsun82/pr-18165-c46f44__nightly__nightly/reports/summaries/time.theme.md#end-to-end-time-per-source

@hvitved
Copy link
Contributor

hvitved commented Dec 4, 2024

@hvitved could you lend me a hand here with the dataflow changes? I tried to preserve the implementation while shifting around the path resolution from Path to "stuff with paths" (e.g. PathExpr or RecordExpr and others), but it seems like it's not the case considering the new results in the local data flow tests.

Aren't the .expected changes merely the result of the improved TupleStructPat.toString implementation? (It would be easier to review, if that change was done as a separate commit).

@redsun82
Copy link
Contributor Author

redsun82 commented Dec 4, 2024

@hvitved could you lend me a hand here with the dataflow changes? I tried to preserve the implementation while shifting around the path resolution from Path to "stuff with paths" (e.g. PathExpr or RecordExpr and others), but it seems like it's not the case considering the new results in the local data flow tests.

Aren't the .expected changes merely the result of the improved TupleStructPat.toString implementation? (It would be easier to review, if that change was done as a separate commit).

I must say the thought crossed my mind, but then I thought that replacing a unique string representation with another should not multiply results, unless there's a toString inconsistency (which doesn't seem to be the case, as we track those with consistency queries). In any case, it makes sense to split out that improvement, I'll revert it here and open a new PR later.

@hvitved
Copy link
Contributor

hvitved commented Dec 4, 2024

I must say the thought crossed my mind, but then I thought that replacing a unique string representation with another should not multiply results

Where do you see result multiplication?

@redsun82
Copy link
Contributor Author

redsun82 commented Dec 4, 2024

I must say the thought crossed my mind, but then I thought that replacing a unique string representation with another should not multiply results

Where do you see result multiplication?

😮 I hadn't noticed they went away after the previous merge from main including some of your new changes 🎉. I was referring to diffs such as this one for a previous version of this PR, but now it seems it's all good! I'll still split out the toString improvement for an easier PR.

@redsun82 redsun82 marked this pull request as ready for review December 4, 2024 10:03
@redsun82 redsun82 requested a review from hvitved December 4, 2024 10:03
@redsun82 redsun82 merged commit c4e53b8 into main Dec 5, 2024
16 checks passed
@redsun82 redsun82 deleted the redsun82/rust-less-canonical-paths branch December 5, 2024 07:57
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