Skip to content
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

Rust: Implement path resolution in QL #18579

Merged
merged 5 commits into from
Jan 31, 2025
Merged

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Jan 23, 2025

This PR adds an initial implementation of path resolution in QL, that is, resolving paths to items.

The implementation is based on the idea of an item graph, which is a labeled directed graph, where an edge item1 --name--> item2 means that item2 is available inside the scope of item1 under the name name. For example, if we have

mod m1 {
    mod m2 { }
}

then there is an edge m1 --m2--> m1::m2.

The implementation has a lot of known limitations for now, for example

  • Cross-crate imports are not modeled, since we do not have access (yet) to .toml files in QL.
  • Modeling of the special Self and crate is partial.
  • Generic paths Foo<Bar> are not modeled.
  • Qualified paths like <Foo as Bar> are not modeled.
  • Shadowing glob imports are not handled correctly, and difficult to do without running into non-monotonic recursion. Probably not a big deal in practice.
  • Path attributes like #[path = "foo.rs"] mod bar; are not handled.
  • Preludes are not taken into account.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Jan 23, 2025
@hvitved hvitved force-pushed the rust/path-resolution branch 10 times, most recently from 707d17c to 9bfd713 Compare January 28, 2025 09:22
@hvitved hvitved marked this pull request as ready for review January 28, 2025 12:17
@Copilot Copilot bot review requested due to automatic review settings January 28, 2025 12:17

Choose a reason for hiding this comment

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

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

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

@paldepind paldepind self-assigned this Jan 29, 2025
pragma[inline_late]
predicate isPublic() { exists(this.getVisibility()) }

/** Gets an element that has this item as immediately enlcosing item. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** Gets an element that has this item as immediately enlcosing item. */
/** Gets an element that has this item as immediately enclosing item. */

Comment on lines 26 to 34
commmentAt(value, filepath, line) and
inMacro = false
or
(
not commmentAt(_, filepath, line)
or
inMacro = true
) and
value = i.getName()
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes it easier to see that the first disjunct is (roughly) being negated in the second disjunct.

Suggested change
commmentAt(value, filepath, line) and
inMacro = false
or
(
not commmentAt(_, filepath, line)
or
inMacro = true
) and
value = i.getName()
commmentAt(value, filepath, line) and inMacro = false
or
not (commmentAt(_, filepath, line) and inMacro = false) and

)
}

private class RelevantPath extends Path {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private class RelevantPath extends Path {
/** A path that does not access a local variable. */
private class RelevantPath extends Path {

override Visibility getVisibility() { none() }
}

private predicate sourceFileEdge(SourceFile f, string name, ItemNode item) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private predicate sourceFileEdge(SourceFile f, string name, ItemNode item) {
/** Holds if `item` has the name `name` and is a top-level item inside `f`. */
private predicate sourceFileEdge(SourceFile f, string name, ItemNode item) {

/** Holds if this item is declared as `pub`. */
bindingset[this]
pragma[inline_late]
predicate isPublic() { exists(this.getVisibility()) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this predicate handle pub(...) things eventually? If not hasVisibility might be a better name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think eventually pub(...) should be handled.

RelevantPath() { not this = any(VariableAccess va).(PathExpr).getPath() }

pragma[nomagic]
predicate isRoot(string name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This name gives me the intuition that the path is related to the crate root, but I don't think that's the case. Would it make sense to call it something along the lines of isUnqualifiedPath?

* may be looked up inside enclosing item `encl`.
*/
pragma[nomagic]
private predicate rootPathLookup(RelevantPath root, string name, ItemNode encl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of the other predicates here have ItemNode as a result, could this one be the same?

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 think I prefer it as-is, because encl is not functionally determined.

@@ -0,0 +1,464 @@
/** Provides functionality for resolving paths. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** Provides functionality for resolving paths. */
/** Provides the `resolvePath` predicate for resolving paths. */

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 prefer to keep it as-is, since we are also exposing other related concepts, such as ItemNode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. What about adding another sentence that mentions resolvePath then? It seems to be the primary export, so I think it's helpful to mention explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* any item (named `name`) inside the imported source file `f`. Using this
* edge, `m2::foo` can resolve to `f::foo`, which results in the edge
* `m1::use m2 --foo--> f::foo`. Lastly, all edges out of `use` nodes are
* lifted to predecessors in the graph, so we get an edge `m1 --foo--> f::foo`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This all helps a lot with understanding 👍

@@ -0,0 +1,201 @@
mod my; // I1
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the directory name modules -> path-resolution would make it more clear what is being tested?

@hvitved hvitved force-pushed the rust/path-resolution branch from 4a57786 to 0d5e408 Compare January 30, 2025 12:58
@hvitved hvitved force-pushed the rust/path-resolution branch from 4361307 to 1cb524f Compare January 31, 2025 09:11
@hvitved hvitved requested a review from paldepind January 31, 2025 09:11
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.

Looks great :)

@hvitved hvitved merged commit 180782d into github:main Jan 31, 2025
17 checks passed
@hvitved hvitved deleted the rust/path-resolution branch January 31, 2025 12:22
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