Force DisjointSet.union to always pick a root
@josephsavona had the intuition that we were picking the wrong root in the previous infinite loop test case, so the fix for this is to force a root to always be picked. This works because `find` implements path compression (if a <- b and b <- c, then we can just point a <- c to "flatten" the tree which makes subsequent `find` operations more efficient since we don't have to follow the ancestor chain each time), so we're forcing all those unions to pick one parent. From some googling it looks like the traditional way to implement union is to call `find` so this should be the "right" way to fix it (?). I'm also adding a basic unit test for DisjointSet, I think we could revisit later and see if property testing is worth it but for now I mainly wanted to capture the regression test as a unit test.
Lauren Tan committed
Dec 22, 2022 at 16:16 UTC
8246956331b690452a3d3690c70f45b714f9683a
3 files changed
+114
-2
compiler/forget/src/Utils/DisjointSet.ts
+1
-1
@@ -24,7 +24,7 @@ export default class DisjointSet<T> {
24
// determine an arbitrary "root" for this set: if the first
25
// item already has a root then use that, otherwise the first item
26
// will be the new root.
27
- let root = this.#entries.get(first);
27
+ let root = this.find(first);
28
if (root == null) {
29
root = first;
30
this.#entries.set(first, first);
compiler/forget/src/__tests__/DisjointSet-test.ts
new
+112
@@ -0,0 +1,112 @@
1
+import DisjointSet from "../Utils/DisjointSet";
2
+
3
+type TestIdentifier = {
4
+ id: number;
5
+ name: string;
6
+};
7
+
8
+describe("DisjointSet", () => {
9
+ let identifierId = 0;
10
+ function makeIdentifier(name: string): TestIdentifier {
11
+ return {
12
+ id: identifierId++,
13
+ name,
14
+ };
15
+ }
16
+
17
+ function makeIdentifiers(...names: string[]): TestIdentifier[] {
18
+ return names.map((name) => makeIdentifier(name));
19
+ }
20
+
21
+ beforeEach(() => {
22
+ identifierId = 0;
23
+ });
24
+
25
+ it(".find - finds the correct group which the item is associated with", () => {
26
+ const identifiers = new DisjointSet<TestIdentifier>();
27
+ const [x, y, z] = makeIdentifiers("x", "y", "z");
28
+
29
+ identifiers.union([x]);
30
+ identifiers.union([y, x]);
31
+
32
+ expect(identifiers.find(x)).toBe(y);
33
+ expect(identifiers.find(y)).toBe(y);
34
+ expect(identifiers.find(z)).toBe(null);
35
+ });
36
+
37
+ it(".size - returns 0 when empty", () => {
38
+ const identifiers = new DisjointSet<TestIdentifier>();
39
+
40
+ expect(identifiers.size).toBe(0);
41
+ });
42
+
43
+ it(".size - returns the correct size when non-empty", () => {
44
+ const identifiers = new DisjointSet<TestIdentifier>();
45
+ const [x, y] = makeIdentifiers("x", "y", "z");
46
+
47
+ identifiers.union([x]);
48
+ identifiers.union([y, x]);
49
+
50
+ expect(identifiers.size).toBe(2);
51
+ });
52
+
53
+ it(".buildSets - returns non-overlapping sets", () => {
54
+ const identifiers = new DisjointSet<TestIdentifier>();
55
+ const [a, b, c, x, y, z] = makeIdentifiers("a", "b", "c", "x", "y", "z");
56
+
57
+ identifiers.union([a]);
58
+ identifiers.union([b, a]);
59
+ identifiers.union([c, b]);
60
+
61
+ identifiers.union([x]);
62
+ identifiers.union([y, x]);
63
+ identifiers.union([z, y]);
64
+ identifiers.union([x, z]);
65
+
66
+ expect(identifiers.buildSets()).toMatchInlineSnapshot(`
67
+ [
68
+ Set {
69
+ {
70
+ "id": 0,
71
+ "name": "a",
72
+ },
73
+ {
74
+ "id": 1,
75
+ "name": "b",
76
+ },
77
+ {
78
+ "id": 2,
79
+ "name": "c",
80
+ },
81
+ },
82
+ Set {
83
+ {
84
+ "id": 3,
85
+ "name": "x",
86
+ },
87
+ {
88
+ "id": 4,
89
+ "name": "y",
90
+ },
91
+ {
92
+ "id": 5,
93
+ "name": "z",
94
+ },
95
+ },
96
+ ]
97
+ `);
98
+ });
99
+
100
+ // Regression test for issue #933
101
+ it("`forEach` doesn't infinite loop when there are cycles", () => {
102
+ const identifiers = new DisjointSet<TestIdentifier>();
103
+ const [x, y, z] = makeIdentifiers("x", "y", "z");
104
+
105
+ identifiers.union([x]);
106
+ identifiers.union([y, x]);
107
+ identifiers.union([z, y]);
108
+ identifiers.union([x, z]);
109
+
110
+ identifiers.forEach((_, group) => expect(group).toBe(z));
111
+ });
112
+});
compiler/forget/src/__tests__/fixtures/hir/issue933-disjoint-set-infinite-loop.expect.md
+1
-1
@@ -2,7 +2,7 @@
2
## Input
3
4
```javascript
5
-// This causes an infinite loop in the compiler
5
+// This caused an infinite loop in the compiler
6
function MyApp(props) {
7
const y = makeObj();
8
const tmp = y.a;