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

Fix: track variables, not names in require-atomic-updates (fixes #14208) #14282

Merged
merged 3 commits into from May 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
43 changes: 23 additions & 20 deletions lib/rules/require-atomic-updates.js
Expand Up @@ -13,6 +13,10 @@
*/
function createReferenceMap(scope, outReferenceMap = new Map()) {
for (const reference of scope.references) {
if (reference.resolved === null) {
continue;
}

outReferenceMap.set(reference.identifier, reference);
}
for (const childScope of scope.childScopes) {
Expand Down Expand Up @@ -86,42 +90,42 @@ class SegmentInfo {
* @returns {void}
*/
initialize(segment) {
const outdatedReadVariableNames = new Set();
const freshReadVariableNames = new Set();
const outdatedReadVariables = new Set();
const freshReadVariables = new Set();

for (const prevSegment of segment.prevSegments) {
const info = this.info.get(prevSegment);

if (info) {
info.outdatedReadVariableNames.forEach(Set.prototype.add, outdatedReadVariableNames);
info.freshReadVariableNames.forEach(Set.prototype.add, freshReadVariableNames);
info.outdatedReadVariables.forEach(Set.prototype.add, outdatedReadVariables);
info.freshReadVariables.forEach(Set.prototype.add, freshReadVariables);
}
}

this.info.set(segment, { outdatedReadVariableNames, freshReadVariableNames });
this.info.set(segment, { outdatedReadVariables, freshReadVariables });
}

/**
* Mark a given variable as read on given segments.
* @param {PathSegment[]} segments The segments that it read the variable on.
* @param {string} variableName The variable name to be read.
* @param {Variable} variable The variable to be read.
* @returns {void}
*/
markAsRead(segments, variableName) {
markAsRead(segments, variable) {
for (const segment of segments) {
const info = this.info.get(segment);

if (info) {
info.freshReadVariableNames.add(variableName);
info.freshReadVariables.add(variable);

// If a variable is freshly read again, then it's no more out-dated.
info.outdatedReadVariableNames.delete(variableName);
info.outdatedReadVariables.delete(variable);
}
}
}

/**
* Move `freshReadVariableNames` to `outdatedReadVariableNames`.
* Move `freshReadVariables` to `outdatedReadVariables`.
* @param {PathSegment[]} segments The segments to process.
* @returns {void}
*/
Expand All @@ -130,23 +134,23 @@ class SegmentInfo {
const info = this.info.get(segment);

if (info) {
info.freshReadVariableNames.forEach(Set.prototype.add, info.outdatedReadVariableNames);
info.freshReadVariableNames.clear();
info.freshReadVariables.forEach(Set.prototype.add, info.outdatedReadVariables);
info.freshReadVariables.clear();
}
}
}

/**
* Check if a given variable is outdated on the current segments.
* @param {PathSegment[]} segments The current segments.
* @param {string} variableName The variable name to check.
* @param {Variable} variable The variable to check.
* @returns {boolean} `true` if the variable is outdated on the segments.
*/
isOutdated(segments, variableName) {
isOutdated(segments, variable) {
for (const segment of segments) {
const info = this.info.get(segment);

if (info && info.outdatedReadVariableNames.has(variableName)) {
if (info && info.outdatedReadVariables.has(variable)) {
return true;
}
}
Expand Down Expand Up @@ -214,14 +218,13 @@ module.exports = {
if (!reference) {
return;
}
const name = reference.identifier.name;
const variable = reference.resolved;
const writeExpr = getWriteExpr(reference);
const isMemberAccess = reference.identifier.parent.type === "MemberExpression";

// Add a fresh read variable.
if (reference.isRead() && !(writeExpr && writeExpr.parent.operator === "=")) {
segmentInfo.markAsRead(codePath.currentSegments, name);
segmentInfo.markAsRead(codePath.currentSegments, variable);
}

/*
Expand All @@ -245,7 +248,7 @@ module.exports = {

/*
* Verify assignments.
* If the reference exists in `outdatedReadVariableNames` list, report it.
* If the reference exists in `outdatedReadVariables` list, report it.
*/
":expression:exit"(node) {
const { codePath, referenceMap } = stack;
Expand All @@ -267,9 +270,9 @@ module.exports = {
assignmentReferences.delete(node);

for (const reference of references) {
const name = reference.identifier.name;
const variable = reference.resolved;

if (segmentInfo.isOutdated(codePath.currentSegments, name)) {
if (segmentInfo.isOutdated(codePath.currentSegments, variable)) {
context.report({
node: node.parent,
messageId: "nonAtomicUpdate",
Expand Down
49 changes: 48 additions & 1 deletion tests/lib/rules/require-atomic-updates.js
Expand Up @@ -53,6 +53,7 @@ ruleTester.run("require-atomic-updates", rule, {
"let foo; async function x() { foo = condition ? foo : await bar; }",
"async function x() { let foo; bar(() => { let foo; blah(foo); }); foo += await result; }",
"let foo; async function x() { foo = foo + 1; await bar; }",
"async function x() { foo += await bar; }",


/*
Expand Down Expand Up @@ -162,6 +163,52 @@ ruleTester.run("require-atomic-updates", rule, {
count -= 1
return
}
`,

// https://github.com/eslint/eslint/issues/14208
`
async function foo(e) {
}

async function run() {
const input = [];
const props = [];

for(const entry of input) {
const prop = props.find(a => a.id === entry.id) || null;
await foo(entry);
}

for(const entry of input) {
const prop = props.find(a => a.id === entry.id) || null;
}

for(const entry2 of input) {
const prop = props.find(a => a.id === entry2.id) || null;
}
}
`,

`
async function run() {
{
let entry;
await entry;
}
{
let entry;
() => entry;

entry = 1;
}
}
`,

`
async function run() {
await a;
b = 1;
}
`
],

Expand Down Expand Up @@ -251,7 +298,7 @@ ruleTester.run("require-atomic-updates", rule, {
errors: [COMPUTED_PROPERTY_ERROR, STATIC_PROPERTY_ERROR]
},
{
code: "async function x() { foo += await bar; }",
code: "let foo = ''; async function x() { foo += await bar; }",
errors: [VARIABLE_ERROR]
},
{
Expand Down