Skip to content

Rust: Enable CFG consistency checks #17558

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 6 commits into from
Sep 25, 2024

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Sep 24, 2024

Commit-by-commit review suggested.

@github-actions github-actions bot added Ruby Rust Pull requests that update Rust code labels Sep 24, 2024
@@ -0,0 +1,33 @@
private import codeql.rust.elements.Expr

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
codeql.rust.elements.BinaryExpr
.
Redundant import, the module is already imported inside
codeql.rust.elements.PrefixExpr
.
@hvitved hvitved force-pushed the rust/cfg-consistency-queries branch from 16c74aa to 0b0b87e Compare September 24, 2024 12:14
@hvitved hvitved force-pushed the rust/cfg-consistency-queries branch from 0b0b87e to cbc2389 Compare September 25, 2024 09:00
@github-actions github-actions bot added the C# label Sep 25, 2024
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Sep 25, 2024
@hvitved hvitved marked this pull request as ready for review September 25, 2024 09:49
@hvitved hvitved requested review from a team as code owners September 25, 2024 09:49
@@ -22,5 +21,7 @@ module Impl {
* x += y;
* ```
*/
class BinaryExpr extends Generated::BinaryExpr { }
class BinaryExpr extends Generated::BinaryExpr {
override string toString() { result = "... " + this.getOperatorName() + " ..." }
Copy link
Contributor

Choose a reason for hiding this comment

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

We should perhaps allow specifying a toString implementation in the schema so they can be generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is that easier than doing it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of editing many files, you can do it all in one file. We may even be able to generate pretty sensible default toString implementations from the rust.ungram file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sensible default toStrings would be nice. However, I think this would be the right place to override them; this is also where one would add additional predicates.

aibaars
aibaars previously approved these changes Sep 25, 2024
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. Some minor suggestions and questions.

If you want you could also run the consistency queries against rustlang/rustat some point. Their test suite is rather extensive covering all of the language.

@@ -73,7 +73,7 @@ class BecomeExprTree extends StandardPostOrderTree instanceof BecomeExpr {
}

class BinaryOpExprTree extends StandardPostOrderTree instanceof BinaryExpr {
BinaryOpExprTree() { super.getOperatorName() != "&&" and super.getOperatorName() != "||" }
BinaryOpExprTree() { not this instanceof LogicalOrExpr and not this instanceof LogicalAndExpr }
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be slightly shorter, not sure if it is better though.

Suggested change
BinaryOpExprTree() { not this instanceof LogicalOrExpr and not this instanceof LogicalAndExpr }
BinaryOpExprTree() { not this instanceof LogicalOperation }

query predicate nonPostOrderExpr(Expr e, string cls) {
cls = e.getPrimaryQlClasses() and
not e instanceof LetExpr and
not e instanceof LogicalAndExpr and // todo
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 LogicalNotExpr ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet modeled in the CFG.

@@ -22,5 +21,7 @@ module Impl {
* x += y;
* ```
*/
class BinaryExpr extends Generated::BinaryExpr { }
class BinaryExpr extends Generated::BinaryExpr {
override string toString() { result = "... " + this.getOperatorName() + " ..." }
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of editing many files, you can do it all in one file. We may even be able to generate pretty sensible default toString implementations from the rust.ungram file.

@hvitved hvitved merged commit 90869ec into github:main Sep 25, 2024
51 of 52 checks passed
@hvitved hvitved deleted the rust/cfg-consistency-queries branch September 25, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# no-change-note-required This PR does not need a change note Ruby Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants