Skip to content

Rust: Prune CFG for obviously impossible true/false edges #17602

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.

Sign up for GitHub

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 4 commits into from
Sep 30, 2024
Merged

Rust: Prune CFG for obviously impossible true/false edges #17602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
431b33a
Rust: Implement `LiteralExpr::toString()`
hvitved Sep 27, 2024
bf58bdd
Rust: Update CFG tests
hvitved Sep 27, 2024
dc4160b
Rust: Prune CFG for impossible `true/false` edges
hvitved Sep 27, 2024
90a8bef
Rust: Run codegen
hvitved Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion rust/ql/.generated.list
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion rust/ql/.gitattributes
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 18 additions & 2 deletions rust/ql/lib/codeql/rust/controlflow/internal/Completion.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,21 @@
abstract ConditionalCompletion getDual();
}

/** Holds if node `n` has the Boolean constant value `value`. */
private predicate isBooleanConstant(AstNode n, Boolean value) {
n.(LiteralExpr).getTextValue() = value.toString()
or
isBooleanConstant(n.(ParenExpr).getExpr(), value)
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a little odd. What about parentheses around other expressions? I'd have expected something like:

...
or
parent = any(BlockExpr be | e = be.getStmtList().getTailExpr())
or
parent = any(ParenExpr pe | e = pe.getExpr())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the ParenExpr case is a workaround for now; once we add ConditonalSplitting a la Ruby, we will only need the LiteralExpr case, and edges out of ParenExpr, BlockExpr, etc will then be restricted via splitting.

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, I was just surprised to see that ParenExpr was handled differently than BlockExpr. The control flow of {{{expr}}} isn't very different from (((expr))).

}

/**
* A completion that represents evaluation of an expression
* with a Boolean value.
*/
class BooleanCompletion extends ConditionalCompletion, TBooleanCompletion {
BooleanCompletion() { this = TBooleanCompletion(value) }

override predicate isValidForSpecific(AstNode e) {
private predicate isValidForSpecific0(AstNode e) {

Check warning

Code scanning / CodeQL

Candidate predicate not marked as `nomagic` Warning

Candidate predicate to
isValidForSpecific
Loading
is not marked as nomagic.
e = any(IfExpr c).getCondition()
or
any(MatchArm arm).getGuard() = e
Expand All @@ -84,7 +91,7 @@
e = expr.getLhs()
)
or
exists(Expr parent | this.isValidForSpecific(parent) |
exists(Expr parent | this.isValidForSpecific0(parent) |
parent =
any(PrefixExpr expr |
expr.getOperatorName() = "!" and
Expand All @@ -103,6 +110,15 @@
)
}

override predicate isValidForSpecific(AstNode e) {
this.isValidForSpecific0(e) and
(
isBooleanConstant(e, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have expected some logic in the last() predicate of a LiteralExprTree that restricts the outgoing edge for a boolean literal to matching ConditionalCompletion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That happens automatically via the completionIsValidFor predicate.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not really "automatic", is it? You either handle it explicitly in isValidForSpecific or in LiteralExprTree . Either way should be fine, I just have a slight preference for handling it in LiteralExprTree. In the end it is a matter of "taste", so let's leave it the way you prefer.

or
not isBooleanConstant(e, _)
)
}

/** Gets the dual Boolean completion. */
override BooleanCompletion getDual() { result = TBooleanCompletion(value.booleanNot()) }

Expand Down
6 changes: 4 additions & 2 deletions rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
// generated by codegen, remove this comment if you wish to edit this file
/**
* This module provides a hand-modifiable wrapper around the generated class `LiteralExpr`.
*
Expand All @@ -12,6 +11,7 @@ private import codeql.rust.elements.internal.generated.LiteralExpr
* be referenced directly.
*/
module Impl {
// the following QLdoc is generated: if you need to edit it, do it in the schema file
/**
* A literal expression. For example:
* ```rust
Expand All @@ -25,5 +25,7 @@ module Impl {
* true;
* ```
*/
class LiteralExpr extends Generated::LiteralExpr { }
class LiteralExpr extends Generated::LiteralExpr {
override string toString() { result = this.getTextValue() }
}
}
10 changes: 5 additions & 5 deletions rust/ql/test/extractor-tests/generated/ArrayExpr/ArrayExpr_getExpr.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
| gen_array_expr.rs:5:5:5:13 | ArrayExpr | 0 | gen_array_expr.rs:5:6:5:6 | LiteralExpr |
| gen_array_expr.rs:5:5:5:13 | ArrayExpr | 1 | gen_array_expr.rs:5:9:5:9 | LiteralExpr |
| gen_array_expr.rs:5:5:5:13 | ArrayExpr | 2 | gen_array_expr.rs:5:12:5:12 | LiteralExpr |
| gen_array_expr.rs:6:5:6:11 | ArrayExpr | 0 | gen_array_expr.rs:6:6:6:6 | LiteralExpr |
| gen_array_expr.rs:6:5:6:11 | ArrayExpr | 1 | gen_array_expr.rs:6:9:6:10 | LiteralExpr |
| gen_array_expr.rs:5:5:5:13 | ArrayExpr | 0 | gen_array_expr.rs:5:6:5:6 | 1 |
| gen_array_expr.rs:5:5:5:13 | ArrayExpr | 1 | gen_array_expr.rs:5:9:5:9 | 2 |
| gen_array_expr.rs:5:5:5:13 | ArrayExpr | 2 | gen_array_expr.rs:5:12:5:12 | 3 |
| gen_array_expr.rs:6:5:6:11 | ArrayExpr | 0 | gen_array_expr.rs:6:6:6:6 | 1 |
| gen_array_expr.rs:6:5:6:11 | ArrayExpr | 1 | gen_array_expr.rs:6:9:6:10 | 10 |
2 changes: 1 addition & 1 deletion rust/ql/test/extractor-tests/generated/BreakExpr/BreakExpr_getExpr.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
| gen_break_expr.rs:12:13:12:27 | BreakExpr | gen_break_expr.rs:12:26:12:27 | LiteralExpr |
| gen_break_expr.rs:12:13:12:27 | BreakExpr | gen_break_expr.rs:12:26:12:27 | 42 |
4 changes: 2 additions & 2 deletions rust/ql/test/extractor-tests/generated/IndexExpr/IndexExpr_getIndex.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
| gen_index_expr.rs:5:5:5:12 | IndexExpr | gen_index_expr.rs:5:10:5:11 | LiteralExpr |
| gen_index_expr.rs:6:5:6:12 | IndexExpr | gen_index_expr.rs:6:10:6:11 | LiteralExpr |
| gen_index_expr.rs:5:5:5:12 | IndexExpr | gen_index_expr.rs:5:10:5:11 | 42 |
| gen_index_expr.rs:6:5:6:12 | IndexExpr | gen_index_expr.rs:6:10:6:11 | 42 |
4 changes: 2 additions & 2 deletions rust/ql/test/extractor-tests/generated/LetStmt/LetStmt_getInitializer.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
| gen_let_stmt.rs:5:5:5:15 | LetStmt | gen_let_stmt.rs:5:13:5:14 | LiteralExpr |
| gen_let_stmt.rs:6:5:6:20 | LetStmt | gen_let_stmt.rs:6:18:6:19 | LiteralExpr |
| gen_let_stmt.rs:5:5:5:15 | LetStmt | gen_let_stmt.rs:5:13:5:14 | 42 |
| gen_let_stmt.rs:6:5:6:20 | LetStmt | gen_let_stmt.rs:6:18:6:19 | 42 |
| gen_let_stmt.rs:9:5:9:24 | LetStmt | gen_let_stmt.rs:9:18:9:23 | TupleExpr |
| gen_let_stmt.rs:10:5:12:6 | LetStmt | gen_let_stmt.rs:10:19:10:38 | CallExpr |
16 changes: 8 additions & 8 deletions rust/ql/test/extractor-tests/generated/LiteralExpr/LiteralExpr.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
| gen_literal_expr.rs:5:5:5:6 | LiteralExpr | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:6:5:6:8 | LiteralExpr | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:7:5:7:19 | LiteralExpr | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:8:5:8:20 | LiteralExpr | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:9:5:9:7 | LiteralExpr | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:10:5:10:8 | LiteralExpr | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:11:5:11:20 | LiteralExpr | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:12:5:12:8 | LiteralExpr | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:5:5:5:6 | 42 | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:6:5:6:8 | 42.0 | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:7:5:7:19 | "Hello, world!" | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:8:5:8:20 | b"Hello, world!" | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:9:5:9:7 | 'x' | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:10:5:10:8 | b'x' | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:11:5:11:20 | r"Hello, world!" | getNumberOfAttrs: | 0 | hasTextValue: | yes |
| gen_literal_expr.rs:12:5:12:8 | true | getNumberOfAttrs: | 0 | hasTextValue: | yes |
16 changes: 8 additions & 8 deletions rust/ql/test/extractor-tests/generated/LiteralExpr/LiteralExpr_getTextValue.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
| gen_literal_expr.rs:5:5:5:6 | LiteralExpr | 42 |
| gen_literal_expr.rs:6:5:6:8 | LiteralExpr | 42.0 |
| gen_literal_expr.rs:7:5:7:19 | LiteralExpr | "Hello, world!" |
| gen_literal_expr.rs:8:5:8:20 | LiteralExpr | b"Hello, world!" |
| gen_literal_expr.rs:9:5:9:7 | LiteralExpr | 'x' |
| gen_literal_expr.rs:10:5:10:8 | LiteralExpr | b'x' |
| gen_literal_expr.rs:11:5:11:20 | LiteralExpr | r"Hello, world!" |
| gen_literal_expr.rs:12:5:12:8 | LiteralExpr | true |
| gen_literal_expr.rs:5:5:5:6 | 42 | 42 |
| gen_literal_expr.rs:6:5:6:8 | 42.0 | 42.0 |
| gen_literal_expr.rs:7:5:7:19 | "Hello, world!" | "Hello, world!" |
| gen_literal_expr.rs:8:5:8:20 | b"Hello, world!" | b"Hello, world!" |
| gen_literal_expr.rs:9:5:9:7 | 'x' | 'x' |
| gen_literal_expr.rs:10:5:10:8 | b'x' | b'x' |
| gen_literal_expr.rs:11:5:11:20 | r"Hello, world!" | r"Hello, world!" |
| gen_literal_expr.rs:12:5:12:8 | true | true |
2 changes: 1 addition & 1 deletion rust/ql/test/extractor-tests/generated/LiteralPat/LiteralPat_getLiteral.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
| gen_literal_pat.rs:6:9:6:10 | LiteralPat | gen_literal_pat.rs:6:9:6:10 | LiteralExpr |
| gen_literal_pat.rs:6:9:6:10 | LiteralPat | gen_literal_pat.rs:6:9:6:10 | 42 |
4 changes: 2 additions & 2 deletions rust/ql/test/extractor-tests/generated/MatchArm/MatchArm_getExpr.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
| gen_match_arm.rs:6:9:6:29 | MatchArm | gen_match_arm.rs:6:28:6:28 | PathExpr |
| gen_match_arm.rs:7:9:7:26 | MatchArm | gen_match_arm.rs:7:25:7:25 | LiteralExpr |
| gen_match_arm.rs:7:9:7:26 | MatchArm | gen_match_arm.rs:7:25:7:25 | 0 |
| gen_match_arm.rs:10:9:10:35 | MatchArm | gen_match_arm.rs:10:30:10:34 | ... / ... |
| gen_match_arm.rs:11:9:11:15 | MatchArm | gen_match_arm.rs:11:14:11:14 | LiteralExpr |
| gen_match_arm.rs:11:9:11:15 | MatchArm | gen_match_arm.rs:11:14:11:14 | 0 |
4 changes: 2 additions & 2 deletions rust/ql/test/extractor-tests/generated/PrefixExpr/PrefixExpr_getExpr.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
| gen_prefix_expr.rs:5:13:5:15 | - ... | gen_prefix_expr.rs:5:14:5:15 | LiteralExpr |
| gen_prefix_expr.rs:6:13:6:17 | ! ... | gen_prefix_expr.rs:6:14:6:17 | LiteralExpr |
| gen_prefix_expr.rs:5:13:5:15 | - ... | gen_prefix_expr.rs:5:14:5:15 | 42 |
| gen_prefix_expr.rs:6:13:6:17 | ! ... | gen_prefix_expr.rs:6:14:6:17 | true |
| gen_prefix_expr.rs:7:13:7:16 | * ... | gen_prefix_expr.rs:7:14:7:16 | PathExpr |
8 changes: 4 additions & 4 deletions rust/ql/test/extractor-tests/generated/RangeExpr/RangeExpr_getEnd.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
| gen_range_expr.rs:5:13:5:18 | RangeExpr | gen_range_expr.rs:5:17:5:18 | LiteralExpr |
| gen_range_expr.rs:6:13:6:17 | RangeExpr | gen_range_expr.rs:6:16:6:17 | LiteralExpr |
| gen_range_expr.rs:8:13:8:16 | RangeExpr | gen_range_expr.rs:8:15:8:16 | LiteralExpr |
| gen_range_expr.rs:9:13:9:17 | RangeExpr | gen_range_expr.rs:9:16:9:17 | LiteralExpr |
| gen_range_expr.rs:5:13:5:18 | RangeExpr | gen_range_expr.rs:5:17:5:18 | 10 |
| gen_range_expr.rs:6:13:6:17 | RangeExpr | gen_range_expr.rs:6:16:6:17 | 10 |
| gen_range_expr.rs:8:13:8:16 | RangeExpr | gen_range_expr.rs:8:15:8:16 | 10 |
| gen_range_expr.rs:9:13:9:17 | RangeExpr | gen_range_expr.rs:9:16:9:17 | 10 |
6 changes: 3 additions & 3 deletions rust/ql/test/extractor-tests/generated/RangeExpr/RangeExpr_getStart.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
| gen_range_expr.rs:5:13:5:18 | RangeExpr | gen_range_expr.rs:5:13:5:13 | LiteralExpr |
| gen_range_expr.rs:6:13:6:17 | RangeExpr | gen_range_expr.rs:6:13:6:13 | LiteralExpr |
| gen_range_expr.rs:7:13:7:16 | RangeExpr | gen_range_expr.rs:7:13:7:14 | LiteralExpr |
| gen_range_expr.rs:5:13:5:18 | RangeExpr | gen_range_expr.rs:5:13:5:13 | 1 |
| gen_range_expr.rs:6:13:6:17 | RangeExpr | gen_range_expr.rs:6:13:6:13 | 1 |
| gen_range_expr.rs:7:13:7:16 | RangeExpr | gen_range_expr.rs:7:13:7:14 | 10 |
4 changes: 2 additions & 2 deletions rust/ql/test/extractor-tests/generated/RecordExprField/RecordExprField_getExpr.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
| gen_record_expr_field.rs:5:11:5:14 | RecordExprField | gen_record_expr_field.rs:5:14:5:14 | LiteralExpr |
| gen_record_expr_field.rs:5:17:5:20 | RecordExprField | gen_record_expr_field.rs:5:20:5:20 | LiteralExpr |
| gen_record_expr_field.rs:5:11:5:14 | RecordExprField | gen_record_expr_field.rs:5:14:5:14 | 1 |
| gen_record_expr_field.rs:5:17:5:20 | RecordExprField | gen_record_expr_field.rs:5:20:5:20 | 2 |
2 changes: 1 addition & 1 deletion rust/ql/test/extractor-tests/generated/ReturnExpr/ReturnExpr_getExpr.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
| gen_return_expr.rs:5:5:5:13 | ReturnExpr | gen_return_expr.rs:5:12:5:13 | LiteralExpr |
| gen_return_expr.rs:5:5:5:13 | ReturnExpr | gen_return_expr.rs:5:12:5:13 | 42 |
8 changes: 4 additions & 4 deletions rust/ql/test/extractor-tests/generated/TupleExpr/TupleExpr_getField.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
| gen_tuple_expr.rs:5:5:5:14 | TupleExpr | 0 | gen_tuple_expr.rs:5:6:5:6 | LiteralExpr |
| gen_tuple_expr.rs:5:5:5:14 | TupleExpr | 1 | gen_tuple_expr.rs:5:9:5:13 | LiteralExpr |
| gen_tuple_expr.rs:6:5:6:14 | TupleExpr | 0 | gen_tuple_expr.rs:6:6:6:6 | LiteralExpr |
| gen_tuple_expr.rs:6:5:6:14 | TupleExpr | 1 | gen_tuple_expr.rs:6:9:6:13 | LiteralExpr |
| gen_tuple_expr.rs:5:5:5:14 | TupleExpr | 0 | gen_tuple_expr.rs:5:6:5:6 | 1 |
| gen_tuple_expr.rs:5:5:5:14 | TupleExpr | 1 | gen_tuple_expr.rs:5:9:5:13 | "one" |
| gen_tuple_expr.rs:6:5:6:14 | TupleExpr | 0 | gen_tuple_expr.rs:6:6:6:6 | 2 |
| gen_tuple_expr.rs:6:5:6:14 | TupleExpr | 1 | gen_tuple_expr.rs:6:9:6:13 | "two" |
2 changes: 1 addition & 1 deletion rust/ql/test/extractor-tests/generated/YeetExpr/YeetExpr_getExpr.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
| gen_yeet_expr.rs:6:8:6:36 | YeetExpr | gen_yeet_expr.rs:6:16:6:36 | LiteralExpr |
| gen_yeet_expr.rs:6:8:6:36 | YeetExpr | gen_yeet_expr.rs:6:16:6:36 | "index out of bounds" |
2 changes: 1 addition & 1 deletion rust/ql/test/extractor-tests/generated/YieldExpr/YieldExpr_getExpr.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
| gen_yield_expr.rs:7:13:7:19 | YieldExpr | gen_yield_expr.rs:7:19:7:19 | LiteralExpr |
| gen_yield_expr.rs:7:13:7:19 | YieldExpr | gen_yield_expr.rs:7:19:7:19 | 1 |
2 changes: 1 addition & 1 deletion rust/ql/test/extractor-tests/utf8/ast.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,4 @@
| utf8-identifiers.rs:11:5:11:24 | LetStmt |
| utf8-identifiers.rs:11:9:11:9 | IdentPat |
| utf8-identifiers.rs:11:9:11:9 | Name |
| utf8-identifiers.rs:11:14:11:23 | LiteralExpr |
| utf8-identifiers.rs:11:14:11:23 | 0.00001f64 |
Loading
Loading