Rust: Handle path attributes in path resolution #19216
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
hvitved
force-pushed
the
rust/path-resolution-path-attr
branch
3 times, most recently
from
4dd7239 to
cbf6f94
Compare
hvitved
force-pushed
the
rust/path-resolution-path-attr
branch
from
cbf6f94 to
15ac9d3
Compare
hvitved
force-pushed
the
rust/path-resolution-path-attr
branch
from
15ac9d3 to
13f4a6a
Compare
paldepind
requested changes
Apr 9, 2025
Contributor
paldepind
left a comment
paldepind
left a comment
There was a problem hiding this comment.
Looks great 👍. I've left a few minor comments.
Comment on lines
261
to
264
| exists(int components | | ||
| components = (-1).maximum(max(int comp | exists(getComponent(relativePath, comp)) | comp)) and | ||
| result = appendStep(f, relativePath, components) | ||
| ) |
Contributor
There was a problem hiding this comment.
Two thoughts:
- Maybe factor out a
getNrOfComponentspredicate from this? Then this predicate would become justresult = appendStep(f, relativePath, getNrOfComponents(relativePath) - The
comphere is not a component but an index. Maybe rename it to justi?
| ) | ||
| } | ||
|
|
||
| private predicate append(Folder f, string relativePath) { pathAttrImport(f, _, relativePath) } |
Contributor
There was a problem hiding this comment.
This had me a bit confused at first as the predicate is called append but doesn't seem to append :)
If I understand correctly this is about limiting the things that are appended by append inside Append in order for it not to append everything in the world?
What about calling it shouldAppend/limitAppend/appendOnly/shouldBeAppended (or something along those lines) and then using the same name for the parameter to the Append module (what is now called app)?
| ) | ||
| } | ||
|
|
||
| private Meta getPathAttrMeta(Module m) { |
Contributor
There was a problem hiding this comment.
Suggested change
| private Meta getPathAttrMeta(Module m) { | |
| /** | |
| * Gets the `Meta` of the module `m`'s [path attribute][1]. | |
| * | |
| * [1]: https://doc.rust-lang.org/reference/items/modules.html#r-items.mod.outlined.path | |
| */ | |
| private Meta getPathAttrMeta(Module m) { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds basic support for taking path attributes into account in path resolution.
DCA shows that, while we maintain performance, we gain an additional 10% true positive call edges (up 522,938 from 475,383) and an additional 6% true positive resolved paths (up 388,991 from 368,369), all numbers computed across the entire DCA suite.