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: Improve CFG for let expressions #18007

Merged
merged 4 commits into from
Nov 19, 2024
Merged

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Nov 18, 2024

Example:

    fn test_and_if_let(a: bool, b: Option<bool>, c: bool) -> bool {
        if a && let Some(d) = b {
            d
        } else {
            false
        }
    }
CFG before:
flowchart TD
1["enter test_and_if_let"]
10["BlockExpr"]
11["IfExpr"]
12["a"]
13["... && ..."]
14["[boolean(false)] ... && ..."]
15["LetExpr"]
16["TupleStructPat"]
17["d"]
18["b"]
19["BlockExpr"]
2["exit test_and_if_let"]
20["d"]
21["BlockExpr"]
22["false"]
3["exit test_and_if_let (normal)"]
4["a"]
5["Param"]
6["b"]
7["Param"]
8["c"]
9["Param"]

1 --> 4
3 --> 2
4 -- match --> 5
5 --> 6
6 -- match --> 7
7 --> 8
8 -- match --> 9
9 --> 12
10 --> 3
11 --> 10
12 -- false --> 14
12 -- true --> 15
13 -- false --> 22
13 -- true --> 20
14 -- false --> 22
15 --> 18
16 -- match --> 17
16 -- no-match --> 13
17 -- match --> 13
18 --> 16
19 --> 11
20 --> 19
21 --> 11
22 --> 21
Loading
CFG after:
flowchart TD
1["enter test_and_if_let"]
10["BlockExpr"]
11["IfExpr"]
12["a"]
13["[boolean(false)] ... && ..."]
14["[boolean(true)] ... && ..."]
15["LetExpr"]
16["TupleStructPat"]
17["d"]
18["b"]
19["BlockExpr"]
2["exit test_and_if_let"]
20["d"]
21["BlockExpr"]
22["false"]
3["exit test_and_if_let (normal)"]
4["a"]
5["Param"]
6["b"]
7["Param"]
8["c"]
9["Param"]

1 --> 4
3 --> 2
4 -- match --> 5
5 --> 6
6 -- match --> 7
7 --> 8
8 -- match --> 9
9 --> 12
10 --> 3
11 --> 10
12 -- false --> 13
12 -- true --> 15
13 -- false --> 22
14 -- true --> 20
15 --> 18
16 -- match --> 17
16 -- no-match --> 13
17 -- match --> 14
18 --> 16
19 --> 11
20 --> 19
21 --> 11
22 --> 21
Loading

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Nov 18, 2024
@@ -141,6 +141,22 @@
0
}

fn test_and_if_let(a: bool, b: Option<bool>, c: bool) -> bool {

Check notice

Code scanning / CodeQL

Unused variable Note test

Variable 'c' is not used.
@paldepind
Copy link
Contributor

Do we want to support this? Using let as expressions is experimental and not stable syntax.

When I was looking at CFG inconsistencies in the projects we have on DCA the Rust compiler was the only project using this syntax.

@hvitved
Copy link
Contributor Author

hvitved commented Nov 18, 2024

Do we want to support this? Using let as expressions is experimental and not stable syntax.

When I was looking at CFG inconsistencies in the projects we have on DCA the Rust compiler was the only project using this syntax.

Oh, I had no idea that this was experimental. Given the simplicity of supporting it, I think we should do it.

@paldepind
Copy link
Contributor

Oh, I had no idea that this was experimental. Given the simplicity of supporting it, I think we should do it.

The Rust compiler complains about it by default. There's an issue tracking the feature here. But yes, if it is simple then it won't hurt to support. And it should fix quite a few CFG inconsistencies on the Rust compiler.

false
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's another example that I think results in a dead-end. As far as I recall, when I looked at the Rust code base, this pattern of starting with let and then having some conditions was by far the most common.

Suggested change
fn test_and_if_let3(a: bool, b: Option<i64>, c: bool) -> bool {
if let Some(d) = b && c && c {
d > 0
} else {
false
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's perhaps do that in a follow-up PR?

Copy link
Contributor

@paldepind paldepind Nov 18, 2024

Choose a reason for hiding this comment

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

I don't think there's much value in adding partial support for let expressions without handling that (most common) case as well.

Is that change orthogonal to the one in this PR? If yes, then doing it in a follow up PR seems fine, otherwise I think I'd be easier to review the changes together.

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, it's orthogonal to this PR; this PR merely improves CFG splitting a bit (see example in the PR description).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, great :)

@hvitved hvitved marked this pull request as ready for review November 18, 2024 19:08
}

fn test_and_if_let2(a: bool, b: i64, c: bool) -> bool {
if a && let d = b && c{
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
if a && let d = b && c{
if a && let d = b && c {

@@ -141,6 +141,24 @@
0
}

fn test_and_if_let(a: bool, b: Option<bool>, c: bool) -> bool {
if a && let Some(d) = b {

Check notice

Code scanning / CodeQL

Unused variable Note test

Variable 'd' is not used.
}

fn test_and_if_let2(a: bool, b: i64, c: bool) -> bool {
if a && let d = b

Check notice

Code scanning / CodeQL

Unused variable Note test

Variable 'd' is not used.
@hvitved hvitved merged commit ef9f383 into github:main Nov 19, 2024
15 checks passed
@hvitved hvitved deleted the rust/cfg/and-let branch November 19, 2024 09:23
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