From b96c6eb8e35a9532e427448c0797b929eadf6a6c Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Mon, 27 May 2019 10:53:35 +0900 Subject: [PATCH] Fix: make require-atomic-updates relax (fixes #11723) --- lib/rules/require-atomic-updates.js | 187 ++++++++++++++-------- tests/lib/rules/require-atomic-updates.js | 26 +++ 2 files changed, 146 insertions(+), 67 deletions(-) diff --git a/lib/rules/require-atomic-updates.js b/lib/rules/require-atomic-updates.js index 09d7670d8198..8380041aac03 100644 --- a/lib/rules/require-atomic-updates.js +++ b/lib/rules/require-atomic-updates.js @@ -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} 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} [outReferenceMap] The map from identifier nodes to each reference object. - * @returns {Map} `referenceMap`. + * @param {Scope} scope The scope to get references. + * @param {Map} [outReferenceMap] The map from identifier nodes to each reference object. + * @returns {Map} `referenceMap`. */ function createReferenceMap(scope, outReferenceMap = new Map()) { for (const reference of scope.references) { @@ -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, freshReadVariableNames: Set }>} */ 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; } } @@ -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} */ + 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) { @@ -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); } /* @@ -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); @@ -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; @@ -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", diff --git a/tests/lib/rules/require-atomic-updates.js b/tests/lib/rules/require-atomic-updates.js index 1287c356fb22..56360ccd282b 100644 --- a/tests/lib/rules/require-atomic-updates.js +++ b/tests/lib/rules/require-atomic-updates.js @@ -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; + } ` ], @@ -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] } ] });