feat: add allowProperties option to require-atomic-updates (#15238)

* feat: add `allowProperties` option to require-atomic-updates Fixes #11899 * fix description of the option * Update docs/rules/require-atomic-updates.md Co-authored-by: Nicholas C. Zakas <nicholas@nczconsulting.com> * Update docs/rules/require-atomic-updates.md Co-authored-by: Nicholas C. Zakas <nicholas@nczconsulting.com> * Update docs/rules/require-atomic-updates.md Co-authored-by: Nicholas C. Zakas <nicholas@nczconsulting.com> * Update docs/rules/require-atomic-updates.md Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com> * Update docs * Update docs Co-authored-by: Nicholas C. Zakas <nicholas@nczconsulting.com> Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
eslint · Nov 21, 2021 · 60b0a29 · 60b0a29
1 parent 79278a1
commit 60b0a29
Show file tree

Hide file tree

Showing 3 changed files with 237 additions and 25 deletions.
diff --git a/docs/rules/require-atomic-updates.md b/docs/rules/require-atomic-updates.md
@@ -38,32 +38,50 @@ 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 from step 1.
+
+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:
+
+* If the variable is read again between steps 2 and 3.
+* 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:
 
 ```js
 /* 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 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.
 
-  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`, the 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
@@ -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
@@ -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]
         }
     ]
 });