Skip to content

Commit

Permalink
Fix: require-atomic-updates reports parameters (fixes #11723) (#11774)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mysticatea
mysticatea authored Jun 17, 2019
1 parent aef8ea1 commit 81c3823
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 84 deletions.
147 changes: 63 additions & 84 deletions lib/rules/require-atomic-updates.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,51 @@ function createReferenceMap(scope, outReferenceMap = new Map()) {
return outReferenceMap;
}

/**
* Get `reference.writeExpr` of a given reference.
* If it's the read reference of MemberExpression in LHS, returns RHS in order to address `a.b = await a`
* @param {escope.Reference} reference The reference to get.
* @returns {Expression|null} The `reference.writeExpr`.
*/
function getWriteExpr(reference) {
if (reference.writeExpr) {
return reference.writeExpr;
}
let node = reference.identifier;

while (node) {
const t = node.parent.type;

if (t === "AssignmentExpression" && node.parent.left === node) {
return node.parent.right;
}
if (t === "MemberExpression" && node.parent.object === node) {
node = node.parent;
continue;
}

break;
}

return null;
}

/**
* Checks if an expression is a variable that can only be observed within the given function.
* @param {escope.Variable} variable The variable to check
* @param {Variable|null} variable The variable to check
* @param {boolean} isMemberAccess If `true` then this is a member access.
* @returns {boolean} `true` if the variable is local to the given function, and is never referenced in a closure.
*/
function isLocalVariableWithoutEscape(variable) {
function isLocalVariableWithoutEscape(variable, isMemberAccess) {
if (!variable) {
return false; // A global variable which was not defined.
}

// If the reference is a property access and the variable is a parameter, it handles the variable is not local.
if (isMemberAccess && variable.defs.some(d => d.type === "Parameter")) {
return false;
}

const functionScope = variable.scope.variableScope;

return variable.references.every(reference =>
Expand All @@ -47,67 +86,64 @@ class SegmentInfo {
* @returns {void}
*/
initialize(segment) {
const outdatedReadVariables = new Set();
const freshReadVariables = new Set();
const outdatedReadVariableNames = new Set();
const freshReadVariableNames = new Set();

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

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

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

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

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

/**
* Move `freshReadVariables` to `outdatedReadVariables`.
* Move `freshReadVariableNames` to `outdatedReadVariableNames`.
* @param {PathSegment[]} segments The segments to process.
* @returns {void}
*/
makeOutdated(segments) {
const vars = new Set();

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

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

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

if (info && info.outdatedReadVariables.has(variable)) {
if (info && info.outdatedReadVariableNames.has(variableName)) {
return true;
}
}
Expand Down Expand Up @@ -140,69 +176,10 @@ module.exports = {

create(context) {
const sourceCode = context.getSourceCode();
const globalScope = context.getScope();
const dummyVariables = new Map();
const assignmentReferences = new Map();
const segmentInfo = new SegmentInfo();
let stack = null;

/**
* Get the variable of a given reference.
* If it's not defined, returns a dummy object.
* @param {escope.Reference} reference The reference to get.
* @returns {escope.Variable} The variable of the reference.
*/
function getVariable(reference) {
if (reference.resolved) {
return reference.resolved;
}

// Get or create a dummy.
const name = reference.identifier.name;
let variable = dummyVariables.get(name);

if (!variable) {
variable = {
name,
scope: globalScope,
references: []
};
dummyVariables.set(name, variable);
}
variable.references.push(reference);

return variable;
}

/**
* Get `reference.writeExpr` of a given reference.
* If it's the read reference of MemberExpression in LHS, returns RHS in order to address `a.b = await a`
* @param {escope.Reference} reference The reference to get.
* @returns {Expression|null} The `reference.writeExpr`.
*/
function getWriteExpr(reference) {
if (reference.writeExpr) {
return reference.writeExpr;
}
let node = reference.identifier;

while (node) {
const t = node.parent.type;

if (t === "AssignmentExpression" && node.parent.left === node) {
return node.parent.right;
}
if (t === "MemberExpression" && node.parent.object === node) {
node = node.parent;
continue;
}

break;
}

return null;
}

return {
onCodePathStart(codePath) {
const scope = context.getScope();
Expand Down Expand Up @@ -234,12 +211,14 @@ module.exports = {
if (!reference) {
return;
}
const variable = getVariable(reference);
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, variable);
segmentInfo.markAsRead(codePath.currentSegments, name);
}

/*
Expand All @@ -248,7 +227,7 @@ module.exports = {
*/
if (writeExpr &&
writeExpr.parent.right === writeExpr && // ← exclude variable declarations.
!isLocalVariableWithoutEscape(variable)
!isLocalVariableWithoutEscape(variable, isMemberAccess)
) {
let refs = assignmentReferences.get(writeExpr);

Expand All @@ -263,7 +242,7 @@ module.exports = {

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

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

if (segmentInfo.isOutdated(codePath.currentSegments, variable)) {
if (segmentInfo.isOutdated(codePath.currentSegments, name)) {
context.report({
node: node.parent,
messageId: "nonAtomicUpdate",
Expand Down
32 changes: 32 additions & 0 deletions tests/lib/rules/require-atomic-updates.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,27 @@ ruleTester.run("require-atomic-updates", rule, {
await doElse();
}
}
`,

// https://github.com/eslint/eslint/issues/11723
`
async function f(foo) {
let bar = await get(foo.id);
bar.prop = foo.prop;
}
`,
`
async function f(foo) {
let bar = await get(foo.id);
foo = bar.prop;
}
`,
`
async function f() {
let foo = {}
let bar = await get(foo.id);
foo.prop = bar.prop;
}
`
],

Expand Down Expand Up @@ -228,6 +249,17 @@ ruleTester.run("require-atomic-updates", rule, {
{
code: "let foo = 0; async function x() { foo = (a ? b ? c ? d ? foo : e : f : g : h) + await bar; if (baz); }",
errors: [VARIABLE_ERROR]
},

// https://github.com/eslint/eslint/issues/11723
{
code: `
async function f(foo) {
let buz = await get(foo.id);
foo.bar = buz.bar;
}
`,
errors: [STATIC_PROPERTY_ERROR]
}
]
});

0 comments on commit 81c3823

Please sign in to comment.