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

Merged
merged 8 commits into from
Feb 19, 2025
Merged

Rust ref pattern #18754

merged 8 commits into from
Feb 19, 2025

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
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
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Overall LGTM, some minor things. As we discussed offline, there are a few CFG issues to look into follow-up. Also, remember DCA :-)

@@ -658,7 +662,11 @@ module PatternTrees {
}

override predicate succ(AstNode pred, AstNode succ, Completion c) {
// Edge from succesful subpattern to name
super.succ(pred, succ, c) and c.(MatchCompletion).succeeded()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I prefer for this to be more explicit, i.e.

last(this.getPat(), pred, c) and
first(this.getName(), succ) and
c.(MatchCompletion).succeeded()

then we should also change StandardPostOrderTree to just PostOrderTree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, that is clearer.

@paldepind paldepind requested a review from hvitved February 17, 2025 14:06
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

DCA doesn't look too happy; probably a bad join somewhere.

@paldepind
Copy link
Contributor Author

I've fixed a very bad cartesian product. DCA went from 😭 to 😐. A few things went up and down by tiny amounts, which I don't think matters. We should be good to merge.

@paldepind paldepind requested a review from hvitved February 19, 2025 13:02
@paldepind paldepind merged commit ae7e15d into github:main Feb 19, 2025
17 checks passed
@paldepind paldepind deleted the rust-ref-pattern branch February 19, 2025 13:24
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