Skip to content

Commit

Permalink
fix reactive declaration cycle detection + clearer error on cycle
Browse files Browse the repository at this point in the history
- fixes #3459
- uses DFS traversal to inspect reactive declarations for cycles
- returns the cycle detected (e.g.; `a → b → a`) for error messaging
  • Loading branch information
colincasey committed Sep 6, 2019
1 parent cc10714 commit ee8825d
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 8 deletions.
26 changes: 20 additions & 6 deletions src/compiler/compile/Component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import unwrap_parens from './utils/unwrap_parens';
import Slot from './nodes/Slot';
import { Node as ESTreeNode } from 'estree';
import add_to_set from './utils/add_to_set';
import checkGraphForCycles from './utils/checkGraphForCycles';

interface ComponentOptions {
namespace?: string;
Expand Down Expand Up @@ -1205,14 +1206,27 @@ export default class Component {
});
});

const add_declaration = declaration => {
if (seen.has(declaration)) {
this.error(declaration.node, {
code: 'cyclical-reactive-declaration',
message: 'Cyclical dependency detected'
const cycle = checkGraphForCycles(unsorted_reactive_declarations.reduce((acc, declaration) => {
declaration.assignees.forEach(v => {
declaration.dependencies.forEach(w => {
if (!declaration.assignees.has(w)) {
acc.push([v, w]);
}
});
}
});
return acc;
}, []));

if (cycle && cycle.length) {
const declarationList = lookup.get(cycle[0]);
const declaration = declarationList[0];
this.error(declaration.node, {
code: 'cyclical-reactive-declaration',
message: `Cyclical dependency detected: ${cycle.join(' → ')}`
});
}

const add_declaration = declaration => {
if (this.reactive_declarations.indexOf(declaration) !== -1) {
return;
}
Expand Down
36 changes: 36 additions & 0 deletions src/compiler/compile/utils/checkGraphForCycles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
export default function checkGraphForCycles(edges: Array<[any, any]>): any[] {
const graph: Map<any, any[]> = edges.reduce((g, edge) => {
const [u, v] = edge;
if (!g.has(u)) g.set(u, []);
if (!g.has(v)) g.set(v, []);
g.get(u).push(v);
return g;
}, new Map());

const visited = new Set();
const onStack = new Set();
const cycles = [];

function visit (v) {
visited.add(v);
onStack.add(v);

graph.get(v).forEach(w => {
if (!visited.has(w)) {
visit(w);
} else if (onStack.has(w)) {
cycles.push([...onStack, w]);
}
});

onStack.delete(v);
}

graph.forEach((_, v) => {
if (!visited.has(v)) {
visit(v);
}
});

return cycles[0];
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export default {
html: `
<p>2+2=4</p>
`,

test({ assert, target }) {
assert.htmlEqual(target.innerHTML, `
<p>2+2=4</p>
`);
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<script>
$: c = a + b;
$: a = 2;
$: b = a;
</script>

<p>{a}+{b}={c}</p>
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[{
"message": "Cyclical dependency detected",
"message": "Cyclical dependency detected: a → b → a",
"code": "cyclical-reactive-declaration",
"start": { "line": 5, "column": 1, "character": 35 },
"end": { "line": 5, "column": 14, "character": 48 },
"pos": 35
}]
}]

0 comments on commit ee8825d

Please sign in to comment.