From 4a821de647a52f702fe66de726fceb76b19f4fa0 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Mon, 1 Nov 2021 13:12:41 +0100 Subject: [PATCH 1/8] feat: add `allowProperties` option to require-atomic-updates Fixes #11899 --- docs/rules/require-atomic-updates.md | 129 ++++++++++++++++++---- lib/rules/require-atomic-updates.js | 16 ++- tests/lib/rules/require-atomic-updates.js | 117 +++++++++++++++++++- 3 files changed, 237 insertions(+), 25 deletions(-) diff --git a/docs/rules/require-atomic-updates.md b/docs/rules/require-atomic-updates.md index 8dd313815c8..238476e88d0 100644 --- a/docs/rules/require-atomic-updates.md +++ b/docs/rules/require-atomic-updates.md @@ -38,11 +38,22 @@ Promise.all([getPageLength(1), getPageLength(2)]).then(pageLengths => { ## Rule Details -This rule aims to report assignments to variables or properties where all of the following are true: +This rule aims to report assignments to variables or properties in cases where the assignments may be based on outdated values. -* A variable or property is reassigned to a new value which is based on its old value. -* A `yield` or `await` expression interrupts the assignment after the old value is read, and before the new value is set. -* The rule cannot easily verify that the assignment is safe (e.g. if an assigned variable is local and would not be readable from anywhere else while the function is paused). +### Variables + +This rule reports an assignment to a variable when it detects the following execution flow in a generator or async function: + +1. The variable is read. +2. A `yield` or `await` pauses the function. +3. After the function is resumed, a value is assigned to the variable. + +The assignment in step 3 is reported because this flow indicates that the assignment is based on the possibly outdated value of the variable from step 1, as the variable may have been reassigned elsewhere while the function was paused in step 2. + +Note that the rule does not report the aforementioned assignment in any of the following cases: + +* If the variable is read again between steps 2 and 3. +* If the variable cannot be accessed while the function is paused (e.g., if it's a local variable). Examples of **incorrect** code for this rule: @@ -50,20 +61,27 @@ Examples of **incorrect** code for this rule: /* eslint require-atomic-updates: error */ let result; -async function foo() { - result += await somethingElse; - result = result + await somethingElse; +async function foo() { + result += await something; +} - result = result + doSomething(await somethingElse); +async function bar() { + result = result + await something; } -function* bar() { - result += yield; +async function baz() { + result = result + doSomething(await somethingElse); +} - result = result + (yield somethingElse); +async function qux() { + if (!result) { + result = await initialize(); + } +} - result = result + doSomething(yield somethingElse); +function* generator() { + result += yield; } ``` @@ -73,22 +91,89 @@ Examples of **correct** code for this rule: /* eslint require-atomic-updates: error */ let result; -async function foo() { - result = await somethingElse + result; - let tmp = await somethingElse; - result += tmp; +async function foobar() { + result = await something + result; +} + +async function baz() { + const tmp = doSomething(await somethingElse); + result += tmp; +} + +async function qux() { + if (!result) { + const tmp = await initialize(); + if (!result) { + result = tmp; + } + } +} + +async function quux() { + let localVariable = 0; + localVariable += await something; +} + +function* generator() { + result = (yield) + result; +} +``` + +### Properties + +This rule reports an assignment to a property through a variable when it detects the following execution flow in a generator or async function: + +1. The variable is read. This can be reading a property through the variable. +2. A `yield` or `await` pauses the function. +3. After the function is resumed, a value is assigned to a property through the variable. - let localVariable = 0; - localVariable += await somethingElse; +This logic is similar to the logic for variables, but stricter because the property in step 3 doesn't have to be the same as the property in step 1. It is assumed that the flow depends on the state of the object as a whole. + +Example of **incorrect** code for this rule: + +```js +/* eslint require-atomic-updates: error */ + +async function foo(obj) { + if (!obj.done) { + obj.something = await getSomething(); + } } +``` -function* bar() { - result = (yield) + result; +Example of **correct** code for this rule: - result = (yield somethingElse) + result; +```js +/* eslint require-atomic-updates: error */ + +async function foo(obj) { + if (!obj.done) { + const tmp = await getSomething(); + if (!obj.done) { + obj.something = tmp; + } + } +} +``` + +## Options + +This rule has an object option: + +* `"allowProperties"` when set to `true`, this rule does not report assignments to properties. Default is `false`. + +### allowProperties + +Example of **correct** code for this rule with the `{ "allowProperties": true }` option: + +```js +/* eslint require-atomic-updates: ["error", { "allowProperties": true }] */ - result = doSomething(yield somethingElse, result); +async function foo(obj) { + if (!obj.done) { + obj.something = await getSomething(); + } } ``` diff --git a/lib/rules/require-atomic-updates.js b/lib/rules/require-atomic-updates.js index 9eee4ca38bc..248b0eb11d0 100644 --- a/lib/rules/require-atomic-updates.js +++ b/lib/rules/require-atomic-updates.js @@ -176,7 +176,17 @@ module.exports = { }, fixable: null, - schema: [], + + schema: [{ + type: "object", + properties: { + allowProperties: { + type: "boolean", + default: false + } + }, + additionalProperties: false + }], messages: { nonAtomicUpdate: "Possible race condition: `{{value}}` might be reassigned based on an outdated value of `{{value}}`.", @@ -185,6 +195,8 @@ module.exports = { }, create(context) { + const allowProperties = !!context.options[0] && context.options[0].allowProperties; + const sourceCode = context.getSourceCode(); const assignmentReferences = new Map(); const segmentInfo = new SegmentInfo(); @@ -284,7 +296,7 @@ module.exports = { value: variable.name } }); - } else { + } else if (!allowProperties) { context.report({ node: node.parent, messageId: "nonAtomicObjectUpdate", diff --git a/tests/lib/rules/require-atomic-updates.js b/tests/lib/rules/require-atomic-updates.js index be931b7e1d0..53d85b75289 100644 --- a/tests/lib/rules/require-atomic-updates.js +++ b/tests/lib/rules/require-atomic-updates.js @@ -214,7 +214,29 @@ ruleTester.run("require-atomic-updates", rule, { await a; b = 1; } - ` + `, + + // allowProperties + { + code: ` + async function a(foo) { + if (foo.bar) { + foo.bar = await something; + } + } + `, + options: [{ allowProperties: true }] + }, + { + code: ` + function* g(foo) { + baz = foo.bar; + yield something; + foo.bar = 1; + } + `, + options: [{ allowProperties: true }] + } ], invalid: [ @@ -358,6 +380,99 @@ ruleTester.run("require-atomic-updates", rule, { line: 8 } ] + }, + + // allowProperties + { + code: ` + async function a(foo) { + if (foo.bar) { + foo.bar = await something; + } + } + `, + errors: [STATIC_PROPERTY_ERROR] + }, + { + code: ` + function* g(foo) { + baz = foo.bar; + yield something; + foo.bar = 1; + } + `, + errors: [STATIC_PROPERTY_ERROR] + }, + { + code: ` + async function a(foo) { + if (foo.bar) { + foo.bar = await something; + } + } + `, + options: [{}], + errors: [STATIC_PROPERTY_ERROR] + + }, + { + code: ` + function* g(foo) { + baz = foo.bar; + yield something; + foo.bar = 1; + } + `, + options: [{}], + errors: [STATIC_PROPERTY_ERROR] + }, + { + code: ` + async function a(foo) { + if (foo.bar) { + foo.bar = await something; + } + } + `, + options: [{ allowProperties: false }], + errors: [STATIC_PROPERTY_ERROR] + + }, + { + code: ` + function* g(foo) { + baz = foo.bar; + yield something; + foo.bar = 1; + } + `, + options: [{ allowProperties: false }], + errors: [STATIC_PROPERTY_ERROR] + }, + { + code: ` + let foo; + async function a() { + if (foo) { + foo = await something; + } + } + `, + options: [{ allowProperties: true }], + errors: [VARIABLE_ERROR] + + }, + { + code: ` + let foo; + function* g() { + baz = foo; + yield something; + foo = 1; + } + `, + options: [{ allowProperties: true }], + errors: [VARIABLE_ERROR] } ] }); From d861b293d4285b3df3f1994e56998af7eb9c5a31 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Mon, 1 Nov 2021 13:43:03 +0100 Subject: [PATCH 2/8] fix description of the option --- docs/rules/require-atomic-updates.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/require-atomic-updates.md b/docs/rules/require-atomic-updates.md index 238476e88d0..6766168c333 100644 --- a/docs/rules/require-atomic-updates.md +++ b/docs/rules/require-atomic-updates.md @@ -161,7 +161,7 @@ async function foo(obj) { This rule has an object option: -* `"allowProperties"` when set to `true`, this rule does not report assignments to properties. Default is `false`. +* `"allowProperties"`: When set to `true`, the rule does not report assignments to properties. Default is `false`. ### allowProperties From eface645019555fe534d07a4f047903e3c32afc7 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Fri, 5 Nov 2021 12:45:18 +0100 Subject: [PATCH 3/8] Update docs/rules/require-atomic-updates.md Co-authored-by: Nicholas C. Zakas --- docs/rules/require-atomic-updates.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/require-atomic-updates.md b/docs/rules/require-atomic-updates.md index 6766168c333..845008b574f 100644 --- a/docs/rules/require-atomic-updates.md +++ b/docs/rules/require-atomic-updates.md @@ -46,7 +46,7 @@ This rule reports an assignment to a variable when it detects the following exec 1. The variable is read. 2. A `yield` or `await` pauses the function. -3. After the function is resumed, a value is assigned to the variable. +3. After the function is resumed, a value is assigned to the variable from step 1. The assignment in step 3 is reported because this flow indicates that the assignment is based on the possibly outdated value of the variable from step 1, as the variable may have been reassigned elsewhere while the function was paused in step 2. From a6dd3101ad78bcd9ca4b054796fa77de37d8626e Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Fri, 5 Nov 2021 12:47:04 +0100 Subject: [PATCH 4/8] Update docs/rules/require-atomic-updates.md Co-authored-by: Nicholas C. Zakas --- docs/rules/require-atomic-updates.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/require-atomic-updates.md b/docs/rules/require-atomic-updates.md index 845008b574f..2e0663ef17d 100644 --- a/docs/rules/require-atomic-updates.md +++ b/docs/rules/require-atomic-updates.md @@ -50,7 +50,7 @@ This rule reports an assignment to a variable when it detects the following exec The assignment in step 3 is reported because this flow indicates that the assignment is based on the possibly outdated value of the variable from step 1, as the variable may have been reassigned elsewhere while the function was paused in step 2. -Note that the rule does not report the aforementioned assignment in any of the following cases: +Note that the rule does not report the assignment in step 3 in any of the following cases: * If the variable is read again between steps 2 and 3. * If the variable cannot be accessed while the function is paused (e.g., if it's a local variable). From cecf22d6ff2029402acb44b13d9389ed9204a398 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Fri, 5 Nov 2021 12:47:19 +0100 Subject: [PATCH 5/8] Update docs/rules/require-atomic-updates.md Co-authored-by: Nicholas C. Zakas --- docs/rules/require-atomic-updates.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/require-atomic-updates.md b/docs/rules/require-atomic-updates.md index 2e0663ef17d..5638b44c67d 100644 --- a/docs/rules/require-atomic-updates.md +++ b/docs/rules/require-atomic-updates.md @@ -53,7 +53,7 @@ The assignment in step 3 is reported because this flow indicates that the assign Note that the rule does not report the assignment in step 3 in any of the following cases: * If the variable is read again between steps 2 and 3. -* If the variable cannot be accessed while the function is paused (e.g., if it's a local variable). +* If the variable cannot be accessed while the function is paused (for example, if it's a local variable). Examples of **incorrect** code for this rule: From 4aeec274cd9b5c5206130a93889865b5ed1e9ca9 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Fri, 19 Nov 2021 14:43:04 +0100 Subject: [PATCH 6/8] Update docs/rules/require-atomic-updates.md Co-authored-by: Nicholas C. Zakas --- docs/rules/require-atomic-updates.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/require-atomic-updates.md b/docs/rules/require-atomic-updates.md index 5638b44c67d..9aa5c16a0c4 100644 --- a/docs/rules/require-atomic-updates.md +++ b/docs/rules/require-atomic-updates.md @@ -124,7 +124,7 @@ function* generator() { This rule reports an assignment to a property through a variable when it detects the following execution flow in a generator or async function: -1. The variable is read. This can be reading a property through the variable. +1. The variable or object property is read. 2. A `yield` or `await` pauses the function. 3. After the function is resumed, a value is assigned to a property through the variable. From f301694122c745f22940765736e89a3d1f6c84d0 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Fri, 19 Nov 2021 15:27:16 +0100 Subject: [PATCH 7/8] Update docs --- docs/rules/require-atomic-updates.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/require-atomic-updates.md b/docs/rules/require-atomic-updates.md index 9aa5c16a0c4..16bcb4e2b9b 100644 --- a/docs/rules/require-atomic-updates.md +++ b/docs/rules/require-atomic-updates.md @@ -48,7 +48,7 @@ This rule reports an assignment to a variable when it detects the following exec 2. A `yield` or `await` pauses the function. 3. After the function is resumed, a value is assigned to the variable from step 1. -The assignment in step 3 is reported because this flow indicates that the assignment is based on the possibly outdated value of the variable from step 1, as the variable may have been reassigned elsewhere while the function was paused in step 2. +The assignment in step 3 is reported because it may be incorrectly resolved because the value of the variable from step 1 may have changed between steps 2 and 3. In particular, if the variable can be accessed from other execution contexts (for example, if it is not a local variable and therefore other functions can change it), the value of the variable may have changed elsewhere while the function was paused in step 2. Note that the rule does not report the assignment in step 3 in any of the following cases: From 0d718a85bc1293fc878d54bb700cc19274cf364e Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Fri, 19 Nov 2021 15:29:30 +0100 Subject: [PATCH 8/8] Update docs --- docs/rules/require-atomic-updates.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/require-atomic-updates.md b/docs/rules/require-atomic-updates.md index 16bcb4e2b9b..64fae21d619 100644 --- a/docs/rules/require-atomic-updates.md +++ b/docs/rules/require-atomic-updates.md @@ -126,7 +126,7 @@ This rule reports an assignment to a property through a variable when it detects 1. The variable or object property is read. 2. A `yield` or `await` pauses the function. -3. After the function is resumed, a value is assigned to a property through the variable. +3. After the function is resumed, a value is assigned to a property. This logic is similar to the logic for variables, but stricter because the property in step 3 doesn't have to be the same as the property in step 1. It is assumed that the flow depends on the state of the object as a whole.