Skip to content

Commit

Permalink
Fix: avoid exponential require-atomic-updates traversal (fixes #10893) (
Browse files Browse the repository at this point in the history 
#10894)

Previously, the `require-atomic-updates` rule would traverse over every path in the control flow graph, resulting in a runtime that was exponential in the number of edges in the graph. This commit updates the rule to use a lattice-based [dataflow analysis](https://en.wikipedia.org/wiki/Data-flow_analysis) algorithm, similar to the algorithms used by optimizing compilers.
  • Loading branch information
@not-an-aardvark
not-an-aardvark committed Sep 28, 2018
1 parent 9432b10 commit 9b26bdb
Show file tree
Hide file tree
Showing 2 changed files with 157 additions and 31 deletions.
117 changes: 87 additions & 30 deletions lib/rules/require-atomic-updates.js
View file
Expand Up @@ -119,6 +119,66 @@ module.exports = {
});
}

const alreadyReportedAssignments = new WeakSet();

class AssignmentTrackerState {
constructor({ openAssignmentsWithoutReads = new Set(), openAssignmentsWithReads = new Set() } = {}) {
this.openAssignmentsWithoutReads = openAssignmentsWithoutReads;
this.openAssignmentsWithReads = openAssignmentsWithReads;
}

copy() {
return new AssignmentTrackerState({
openAssignmentsWithoutReads: new Set(this.openAssignmentsWithoutReads),
openAssignmentsWithReads: new Set(this.openAssignmentsWithReads)
});
}

merge(other) {
const initialAssignmentsWithoutReadsCount = this.openAssignmentsWithoutReads.size;
const initialAssignmentsWithReadsCount = this.openAssignmentsWithReads.size;

other.openAssignmentsWithoutReads.forEach(assignment => this.openAssignmentsWithoutReads.add(assignment));
other.openAssignmentsWithReads.forEach(assignment => this.openAssignmentsWithReads.add(assignment));

return this.openAssignmentsWithoutReads.size > initialAssignmentsWithoutReadsCount ||
this.openAssignmentsWithReads.size > initialAssignmentsWithReadsCount;
}

enterAssignment(assignmentExpression) {
(assignmentExpression.operator === "=" ? this.openAssignmentsWithoutReads : this.openAssignmentsWithReads).add(assignmentExpression);
}

exitAssignment(assignmentExpression) {
this.openAssignmentsWithoutReads.delete(assignmentExpression);
this.openAssignmentsWithReads.delete(assignmentExpression);
}

exitAwaitOrYield(node, surroundingFunction) {
return [...this.openAssignmentsWithReads]
.filter(assignment => !isLocalVariableWithoutEscape(assignment.left, surroundingFunction))
.forEach(assignment => {
if (!alreadyReportedAssignments.has(assignment)) {
reportAssignment(assignment);
alreadyReportedAssignments.add(assignment);
}
});
}

exitIdentifierOrMemberExpression(node) {
[...this.openAssignmentsWithoutReads]
.filter(assignment => (
assignment.left !== node &&
assignment.left.type === node.type &&
astUtils.equalTokens(assignment.left, node, sourceCode)
))
.forEach(assignment => {
this.openAssignmentsWithoutReads.delete(assignment);
this.openAssignmentsWithReads.add(assignment);
});
}
}

/**
* If the control flow graph of a function enters an assignment expression, then does the
* both of the following steps in order (possibly with other steps in between) before exiting the
Expand All @@ -135,54 +195,51 @@ module.exports = {
codePathSegment,
surroundingFunction,
{
seenSegments = new Set(),
openAssignmentsWithoutReads = new Set(),
openAssignmentsWithReads = new Set()
stateBySegmentStart = new WeakMap(),
stateBySegmentEnd = new WeakMap()
} = {}
) {
if (seenSegments.has(codePathSegment)) {

// An AssignmentExpression can't contain loops, so it's not necessary to reenter them with new state.
return;
if (!stateBySegmentStart.has(codePathSegment)) {
stateBySegmentStart.set(codePathSegment, new AssignmentTrackerState());
}

const currentState = stateBySegmentStart.get(codePathSegment).copy();

expressionsByCodePathSegment.get(codePathSegment).forEach(({ entering, node }) => {
if (node.type === "AssignmentExpression") {
if (entering) {
(node.operator === "=" ? openAssignmentsWithoutReads : openAssignmentsWithReads).add(node);
currentState.enterAssignment(node);
} else {
openAssignmentsWithoutReads.delete(node);
openAssignmentsWithReads.delete(node);
currentState.exitAssignment(node);
}
} else if (!entering && (node.type === "AwaitExpression" || node.type === "YieldExpression")) {
[...openAssignmentsWithReads]
.filter(assignment => !isLocalVariableWithoutEscape(assignment.left, surroundingFunction))
.forEach(reportAssignment);

openAssignmentsWithReads.clear();
currentState.exitAwaitOrYield(node, surroundingFunction);
} else if (!entering && (node.type === "Identifier" || node.type === "MemberExpression")) {
[...openAssignmentsWithoutReads]
.filter(assignment => (
assignment.left !== node &&
assignment.left.type === node.type &&
astUtils.equalTokens(assignment.left, node, sourceCode)
))
.forEach(assignment => {
openAssignmentsWithoutReads.delete(assignment);
openAssignmentsWithReads.add(assignment);
});
currentState.exitIdentifierOrMemberExpression(node);
}
});

stateBySegmentEnd.set(codePathSegment, currentState);

codePathSegment.nextSegments.forEach(nextSegment => {
if (stateBySegmentStart.has(nextSegment)) {
if (!stateBySegmentStart.get(nextSegment).merge(currentState)) {

/*
* This segment has already been processed with the given set of inputs;
* no need to do it again. After no new state is available to process
* for any control flow segment in the graph, the analysis reaches a fixpoint and
* traversal stops.
*/
return;
}
} else {
stateBySegmentStart.set(nextSegment, currentState.copy());
}
findOutdatedReads(
nextSegment,
surroundingFunction,
{
seenSegments: new Set(seenSegments).add(codePathSegment),
openAssignmentsWithoutReads: new Set(openAssignmentsWithoutReads),
openAssignmentsWithReads: new Set(openAssignmentsWithReads)
}
{ stateBySegmentStart, stateBySegmentEnd }
);
});
}
Expand Down
71 changes: 70 additions & 1 deletion tests/lib/rules/require-atomic-updates.js
View file
Expand Up @@ -51,14 +51,75 @@ ruleTester.run("require-atomic-updates", rule, {
"let foo; function* x() { foo = bar + foo; }",
"async function x() { let foo; bar(() => baz += 1); foo += await amount; }",
"let foo; async function x() { foo = condition ? foo : await bar; }",
"async function x() { let foo; bar(() => { let foo; blah(foo); }); foo += await result; }"
"async function x() { let foo; bar(() => { let foo; blah(foo); }); foo += await result; }",
"let foo; async function x() { foo = foo + 1; await bar; }",


/*
* Ensure rule doesn't take exponential time in the number of branches
* (see https://github.com/eslint/eslint/issues/10893)
*/
`
async function foo() {
if (1);
if (2);
if (3);
if (4);
if (5);
if (6);
if (7);
if (8);
if (9);
if (10);
if (11);
if (12);
if (13);
if (14);
if (15);
if (16);
if (17);
if (18);
if (19);
if (20);
}
`,
`
async function foo() {
return [
1 ? a : b,
2 ? a : b,
3 ? a : b,
4 ? a : b,
5 ? a : b,
6 ? a : b,
7 ? a : b,
8 ? a : b,
9 ? a : b,
10 ? a : b,
11 ? a : b,
12 ? a : b,
13 ? a : b,
14 ? a : b,
15 ? a : b,
16 ? a : b,
17 ? a : b,
18 ? a : b,
19 ? a : b,
20 ? a : b
];
}
`
],

invalid: [
{
code: "let foo; async function x() { foo += await amount; }",
errors: [{ messageId: "nonAtomicUpdate", data: { value: "foo" } }]
},
{
code: "if (1); let foo; async function x() { foo += await amount; }",
errors: [{ messageId: "nonAtomicUpdate", data: { value: "foo" } }]
},
{
code: "let foo; async function x() { while (condition) { foo += await amount; } }",
errors: [VARIABLE_ERROR]
Expand Down Expand Up @@ -138,6 +199,14 @@ ruleTester.run("require-atomic-updates", rule, {
{
code: "async function x() { foo += await bar; }",
errors: [VARIABLE_ERROR]
},
{
code: "let foo = 0; async function x() { foo = (a ? b : foo) + await bar; if (baz); }",
errors: [VARIABLE_ERROR]
},
{
code: "let foo = 0; async function x() { foo = (a ? b ? c ? d ? foo : e : f : g : h) + await bar; if (baz); }",
errors: [VARIABLE_ERROR]
}
]
});

0 comments on commit 9b26bdb

Please sign in to comment.