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: AST support for variables #17606

Merged
merged 5 commits into from
Oct 1, 2024
Merged

Rust: AST support for variables #17606

merged 5 commits into from
Oct 1, 2024

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Sep 27, 2024

This PR adds (initial) AST support for variables. For now, two classes Variable and VariableAccess are exposed, and a VariableAccess is linked to the Variable that it accesses.

Commit-by-commit review is suggested.

Care must be taken when taking shadowing into account: A variable can be shadowed both in a nested scope

let x = 0;
use(x);         // x = 0
{
    let x = "";
    use(x);     // x = ""
}
use(x);         // x = 0

as well as in the same scope

let x = 0;
use(x);     // x = 0
let x = "";
use(x);     // x = ""

In order to identify which variable is being accessed, we employ a purely location-based analysis, as we want to avoid an analysis that depends on the CFG.

In almost all cases, a Variable is identified by the IdentPat node that introduces it, however in case of or patterns like

match either {
    Either::Left(x) | Either::Right(x) => println!(x),
}

x can be introduced by either of the two branches, so we instead identify x with the whole Either::Left(x) | Either::Right(x) pattern.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Sep 27, 2024
@hvitved hvitved force-pushed the rust/variables branch 2 times, most recently from da9b7dd to 6914fb6 Compare September 30, 2024 07:27
@hvitved hvitved marked this pull request as ready for review September 30, 2024 07:37
@hvitved hvitved force-pushed the rust/variables branch 3 times, most recently from 8c2d56d to 1029ab7 Compare September 30, 2024 10:48
@hvitved hvitved force-pushed the rust/variables branch 2 times, most recently from 96c6b17 to bf3f8dd Compare October 1, 2024 07:01
@hvitved hvitved marked this pull request as draft October 1, 2024 07:41
@hvitved hvitved marked this pull request as ready for review October 1, 2024 08:11
Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

Looks good to me.

* Gets the text of this comment, excluding the comment marker.
*/
string getCommentText() {
exists(string s | s = this.getText() | result = s.regexpCapture("///?\\s*(.*)", 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Rust also has block comments starting with /* or /** .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I skipped that for now, as it is not needed for inline test expectations.


/** Gets the outermost enclosing `|` pattern parent of `p`, if any. */
private OrPat getOutermostEnclosingOrPat(IdentPat p) {
result = getEnclosingOrPat+(p) and
Copy link
Contributor

Choose a reason for hiding this comment

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

What about ParenPat nodes? Things like a | (b | c) | d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those should be handled as well; getAPatAncestor uses the generic getImmediateParent predicate which also works for ParenPat.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, could you add a test case, just to be sure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

rust/ql/test/utils/internal/InlineExpectationsTestImpl.qll Outdated Show resolved Hide resolved
)
}

private newtype TVariableOrAccessCand =
Copy link
Contributor

Choose a reason for hiding this comment

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

This type looked a little strange to me at first sight. Perhaps you could add a comment explaining the idea of algorithm. If I understand correctly you rank all variable definitions and uses, and bind each use to the highest ranking definition that has a lower rank than the variable use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair; I have tried to document the class with the algorithm that is used.

@hvitved hvitved requested a review from aibaars October 1, 2024 12:21
@hvitved hvitved merged commit 5fb61b0 into github:main Oct 1, 2024
14 checks passed
@hvitved hvitved deleted the rust/variables branch October 1, 2024 16:13
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