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.

Sign up for GitHub

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

Jump to bottom

Fix: avoid exponential require-atomic-updates traversal (fixes #10893) #10894

Merged
merged 2 commits into from Sep 28, 2018
Merged

Fix: avoid exponential require-atomic-updates traversal (fixes #10893) #10894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6fc397d
Fix: avoid exponential-time no-atomic-updates traversal (fixes #10893)
not-an-aardvark Sep 26, 2018
de6d8f2
Remove unnecessary test change
not-an-aardvark Sep 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
117 changes: 87 additions & 30 deletions lib/rules/require-atomic-updates.js
View file
Open in desktop
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
Open in desktop
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]
}
]
});