Skip to content

Commit

Permalink
Fix: make require-atomic-updates relax (fixes #11723)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mysticatea
mysticatea committed May 27, 2019
1 parent ec105b2 commit b96c6eb
Show file tree
Hide file tree
Showing 2 changed files with 146 additions and 67 deletions.
187 changes: 120 additions & 67 deletions lib/rules/require-atomic-updates.js
View file
Expand Up @@ -5,11 +5,28 @@
*/
"use strict";

const { getPropertyName } = require("eslint-utils");

/** @typedef {import("eslint-scope").Reference} Reference */
/** @typedef {import("eslint-scope").Scope} Scope */
/** @typedef {import("eslint-scope").Variable} Variable */
/** @typedef {import("estree").Identifier} Identifier */
/** @typedef {import("estree").Node} Node */
/** @typedef {import("../linter/code-path-analysis/code-path-segment")} CodePathSegment */
/** @typedef {import("../linter/code-path-analysis/code-path")} CodePath */

/**
* @typedef {Object} FunctionInfo
* @property {FunctionInfo|null} upper The upper function info.
* @property {CodePath} codePath The code path of the current function.
* @property {Map<Identifier, Reference>} referenceMap The map to references from identifiers.
*/

/**
* Make the map from identifiers to each reference.
* @param {escope.Scope} scope The scope to get references.
* @param {Map<Identifier, escope.Reference>} [outReferenceMap] The map from identifier nodes to each reference object.
* @returns {Map<Identifier, escope.Reference>} `referenceMap`.
* @param {Scope} scope The scope to get references.
* @param {Map<Identifier, Reference>} [outReferenceMap] The map from identifier nodes to each reference object.
* @returns {Map<Identifier, Reference>} `referenceMap`.
*/
function createReferenceMap(scope, outReferenceMap = new Map()) {
for (const reference of scope.references) {
Expand All @@ -24,90 +41,154 @@ function createReferenceMap(scope, outReferenceMap = new Map()) {
return outReferenceMap;
}

/**
* Get the name of a given reference.
* The name contains property names if the reference is to access to properties. For example:
* - `foo` → `["foo"]`
* - `foo.bar` → `["foo", "bar"]`
* - `foo.bar.baz` → `["foo", "bar", "baz"]`
* - `foo.bar[baz].qiz` → `["foo", "bar"]` // until a dynamic access.
* @param {Reference} reference The reference to get.
* @returns {string[]} The reference name.
*/
function getReferenceName(reference) {
const names = [reference.identifier.name];
let node = reference.identifier;

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

if (t === "MemberExpression" && node.parent.object === node) {
const name = getPropertyName(node.parent, reference.from);

// If the property name was dynamic, decide the reference name there.
if (!name) {
break;
}

names.push(name);
node = node.parent;
} else {
break;
}
}

return names;
}

/**
* Check if given two names are matched (left-hand match).
* @param {string[]} xs A reference name to check.
* @param {string[]} ys Another reference name to check.
* @returns {boolean} `true` if the two names are matched.
*/
function isReferenceNameMatched(xs, ys) {
const len = Math.min(xs.length, ys.length);

for (let i = len - 1; i >= 0; --i) {
if (xs[i] !== ys[i]) {
return false;
}
}

return true;
}

/**
* 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 {string[]} referenceName The reference name.
* @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, referenceName) {
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 (referenceName.length >= 2 && variable.defs.some(d => d.type === "Parameter")) {
return false;
}

const functionScope = variable.scope.variableScope;

return variable.references.every(reference =>
reference.from.variableScope === functionScope);
return variable.references
.filter(reference => isReferenceNameMatched(getReferenceName(reference), referenceName))
.every(reference => reference.from.variableScope === functionScope);
}

class SegmentInfo {
constructor() {

/** @type {WeakMap<CodePathSegment, { outdatedReadVariableNames: Set<string>, freshReadVariableNames: Set<string> }>} */
this.info = new WeakMap();
}

/**
* Initialize the segment information.
* @param {PathSegment} segment The segment to initialize.
* @param {CodePathSegment} segment The segment to initialize.
* @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.
* If the variable name is `foo.x.y`, this method marks `foo`.
* @param {CodePathSegment[]} segments The segments that it read the variable on.
* @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`.
* @param {PathSegment[]} segments The segments to process.
* Move `freshReadVariableNames` to `outdatedReadVariableNames`.
* @param {CodePathSegment[]} 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 {CodePathSegment[]} segments The current segments.
* @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,45 +221,18 @@ 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);
/** @type {Map<Node, Reference[]>} */
const assignmentReferences = new Map();

return variable;
}
let stack = null;

/**
* 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`.
* @param {Reference} reference The reference to get.
* @returns {Node|null} The `reference.writeExpr`.
*/
function getWriteExpr(reference) {
if (reference.writeExpr) {
Expand Down Expand Up @@ -234,12 +288,13 @@ module.exports = {
if (!reference) {
return;
}
const variable = getVariable(reference);
const variable = reference.resolved;
const referenceName = getReferenceName(reference);
const writeExpr = getWriteExpr(reference);

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

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

Expand All @@ -263,7 +318,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 +340,7 @@ module.exports = {
assignmentReferences.delete(node);

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

if (segmentInfo.isOutdated(codePath.currentSegments, variable)) {
if (segmentInfo.isOutdated(codePath.currentSegments, reference.identifier.name)) {
context.report({
node: node.parent,
messageId: "nonAtomicUpdate",
Expand Down
26 changes: 26 additions & 0 deletions tests/lib/rules/require-atomic-updates.js
View file
Expand Up @@ -129,6 +129,21 @@ 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() {
let foo = {}
let bar = await get(foo.id);
foo.prop = bar.prop;
}
`
],

Expand Down Expand Up @@ -228,6 +243,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 b96c6eb

Please sign in to comment.