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 ref pattern #18754

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Rust ref pattern #18754

wants to merge 5 commits into from

Conversation

paldepind
Copy link
Contributor

This PR aims to add support for ref in patterns.

For a let statement these two lines are equivalent:

let a = &foo;
let ref a = foo;

So we'd like their effect to be the same. For a pattern like Some(ref a) the introduced variable a is a reference and should have the content of the matched value stored in the reference content type.

The cleanest way to do this is to add a new store edge from ref a (identity pattern) -> a (name) in the data flow graph. But, currently the name nodes in the AST are not really considered everywhere from the CFG to the data flow library.

Hence this PR:

  • Include names in patterns in the CFG
  • Let SSA writes target the name inside identifier patterns instead of the pattern itself
  • Include relevant names in the data flow graph
  • Add a store step from a identifier patterns with ref into the contained name. So we have an edge ref a -> a that stores in the reference content type. For identifier patterns without a ref we add a normal value preserving step, that maintains current behavior.

Here are some diagrams depicting the change in the CFG.

Example 1

Here there is a new map node before the mut map node.

fn method_call() {
    let mut map = HashMap::new();
    map.insert(37, "a");
}

Before

flowchart TD
1["enter fn method_call"]
10["map.insert(...)"]
11["ExprStmt"]
12["37"]
13[""a""]
2["exit fn method_call"]
3["exit fn method_call (normal)"]
4["{ ... }"]
5["let ... = ..."]
6["mut map"]
7["...::new"]
8["...::new(...)"]
9["map"]

1 --> 5
3 --> 2
4 --> 3
5 --> 7
6 -- match --> 11
7 --> 8
8 --> 6
9 --> 12
10 --> 4
11 --> 9
12 --> 13
13 --> 10
Loading

After

flowchart TD
1["enter fn method_call"]
10["map"]
11["map.insert(...)"]
12["ExprStmt"]
13["37"]
14[""a""]
2["exit fn method_call"]
3["exit fn method_call (normal)"]
4["{ ... }"]
5["let ... = ..."]
6["mut map"]
7["map"]
8["...::new"]
9["...::new(...)"]

1 --> 5
3 --> 2
4 --> 3
5 --> 8
6 -- match --> 12
7 --> 6
8 --> 9
9 --> 7
10 --> 13
11 --> 4
12 --> 10
13 --> 14
14 --> 11
Loading
Example 2

This shows that the PR also makes a change regarding splitting, in that it is no longer done for identifier patterns.

fn identifier_pattern_with_ref() -> i64 {
    let mut a = 10;
    match a {
        ref mut n @ 1..10 => *n += 10,
        ref mut n => *n = 0,
    };
    a
}

Before

flowchart TD
1["enter fn identifier_pattern_with_ref"]
10["a"]
11["[match(true)] ref mut n @ ..."]
12["1"]
12["1"]
14["RangePat"]
15["10"]
15["10"]
17["* ..."]
18["... += ..."]
19["n"]
2["exit fn identifier_pattern_with_ref"]
20["10"]
21["ref mut n"]
22["* ..."]
23["... = ..."]
24["n"]
25["0"]
26["a"]
3["exit fn identifier_pattern_with_ref (normal)"]
4["{ ... }"]
5["let ... = 10"]
6["mut a"]
7["10"]
8["match a { ... }"]
9["ExprStmt"]

1 --> 5
3 --> 2
4 --> 3
5 --> 7
6 -- match --> 9
7 --> 6
8 --> 26
9 --> 10
10 --> 14
11 -- match --> 19
12 -- match --> 15
12 -- no-match --> 21
12 --> 12
14 -- match --> 12
14 -- no-match --> 21
15 -- match --> 11
15 -- no-match --> 21
15 --> 15
17 --> 20
18 --> 8
19 --> 17
20 --> 18
21 -- match --> 24
22 --> 25
23 --> 8
24 --> 22
25 --> 23
26 --> 4
Loading

After

flowchart TD
1["enter fn identifier_pattern_with_ref"]
10["ExprStmt"]
11["a"]
12["ref mut n @ ..."]
13["n"]
14["1"]
14["1"]
16["RangePat"]
17["10"]
17["10"]
19["* ..."]
2["exit fn identifier_pattern_with_ref"]
20["... += ..."]
21["n"]
22["10"]
23["ref mut n"]
24["n"]
25["* ..."]
26["... = ..."]
27["n"]
28["0"]
29["a"]
3["exit fn identifier_pattern_with_ref (normal)"]
4["{ ... }"]
5["let ... = 10"]
6["mut a"]
7["a"]
8["10"]
9["match a { ... }"]

1 --> 5
3 --> 2
4 --> 3
5 --> 8
6 -- match --> 10
7 --> 6
8 --> 7
9 --> 29
10 --> 11
11 --> 16
12 -- match --> 21
12 -- no-match --> 24
13 --> 12
14 -- match --> 17
14 -- no-match --> 24
14 --> 14
16 -- match --> 14
16 -- no-match --> 24
17 -- match --> 13
17 -- no-match --> 24
17 --> 17
19 --> 22
20 --> 9
21 --> 19
22 --> 20
23 -- match --> 27
24 --> 23
25 --> 28
26 --> 9
27 --> 25
28 --> 26
29 --> 4
Loading

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Feb 12, 2025
@paldepind paldepind force-pushed the rust-ref-pattern branch 3 times, most recently from 341c62f to be6ac13 Compare February 12, 2025 12:07
any(SelfParam sp |
definingNode = sp.getName() and
name = sp.getName().getText() and
private predicate variableDecl(AstNode definingNode, Name name, string text) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an orthogonal refactor, but since I was making changes around Name it occurred to me that it might be nice to have Name here, since both SelfParam and IdentPat have that in common.

To do this we:
* Let SSA writes target the name inside identifier patterns instead of
  the pattern itself
* Include relevant names in the data flow graph
* Add a store step from a identifier patterns with `ref` into the
  contained name. So we have an edge `ref a` -> `a` that stores in the
  reference content type.
@paldepind paldepind marked this pull request as ready for review February 12, 2025 12:53
@paldepind paldepind requested a review from hvitved February 12, 2025 12:53
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.

1 participant