Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strange behavior of require-atomic-updates #14695

Closed
nullromo opened this issue Jun 10, 2021 · 8 comments
Closed

Strange behavior of require-atomic-updates #14695

nullromo opened this issue Jun 10, 2021 · 8 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion repro:yes Issues with a reproducible example rule Relates to ESLint's core rules Stale

Comments

@nullromo
Copy link

Environment

  • ESLint Version: v7.21.0
  • Node Version: v14.16.1
  • npm Version: v6.14.11

Parser
react plugin and @typescript-eslint plugin

Full configuration:

Configuration
{
    "env": {
        "browser": true,
        "es2021": true,
        "node": true
    },
    "extends": [
        "eslint:recommended",
        "plugin:react/recommended",
        "plugin:@typescript-eslint/recommended"
    ],
    "parser": "@typescript-eslint/parser",
    "parserOptions": {
        "ecmaFeatures": {
            "jsx": true
        },
        "ecmaVersion": 12,
        "sourceType": "module",
        "project": "./tsconfig.json"
    },
    "plugins": [
        "react",
        "@typescript-eslint"
    ],
    "settings": {
        "react": {
            "version": "detect"
        }
    },
    "rules": {
        "no-await-in-loop": "error",
        "no-extra-semi": "off",
        "@typescript-eslint/no-extra-semi": ["error"],
        "no-loss-of-precision": "off",
        "@typescript-eslint/no-loss-of-precision": ["error"],
        "no-promise-executor-return": "error",
        "no-template-curly-in-string": "error",
        "no-unreachable-loop": "error",
        "no-unsafe-optional-chaining": ["error", {"disallowArithmeticOperators": true}],
        "no-useless-backreference": "error",
        "require-atomic-updates": "error",
        "use-isnan": ["error", {"enforceForIndexOf": true}],
        "array-callback-return": ["error", {"checkForEach": true}],
        "block-scoped-var": "error",
        "class-methods-use-this": "error",
        "consistent-return": ["error", {"treatUndefinedAsUnspecified": true}],
        "default-case": "error",
        "default-case-last": "error",
        "default-param-last": "off",
        "@typescript-eslint/default-param-last": ["error"],
        "dot-location": ["error", "property"],
        "dot-notation": "off",
        "@typescript-eslint/dot-notation": ["error"],
        "grouped-accessor-pairs": "error",
        "guard-for-in": "error",
        "no-alert": "error",
        "no-caller": "error",
        "no-constructor-return": "error",
        "no-div-regex": "error",
        "no-else-return": ["error", {"allowElseIf": false}],
        "no-empty-function": "off",
        "@typescript-eslint/no-empty-function": ["error"],
        "no-eq-null": "error",
        "no-eval": "error",
        "no-extend-native": "error",
        "no-extra-bind": "error",
        "no-extra-label": "error",
        "no-floating-decimal": "error",
        "no-implicit-coercion": "error",
        "no-implicit-globals": "error",
        "no-implied-eval": "off",
        "@typescript-eslint/no-implied-eval": ["error"],
        "no-invalid-this": "off",
        "@typescript-eslint/no-invalid-this": ["error"],
        "no-iterator": "error",
        "no-lone-blocks": "error",
        "no-loop-func": "off",
        "@typescript-eslint/no-loop-func": ["error"],
        "no-new": "error",
        "no-new-func": "error",
        "no-new-wrappers": "error",
        "no-nonoctal-decimal-escape": "error",
        "no-octal-escape": "error",
        "no-param-reassign": "error",
        "no-proto": "error",
        "no-redeclare": "off",
        "@typescript-eslint/no-redeclare": ["error"],
        "no-return-assign": ["error", "always"],
        "no-script-url": "error",
        "no-self-compare": "error",
        "no-sequences": "error",
        "no-throw-literal": "off",
        "@typescript-eslint/no-throw-literal": ["error"],
        "no-unmodified-loop-condition": "error",
        "no-unused-expressions": "off",
        "@typescript-eslint/no-unused-expressions": ["error"],
        "no-useless-call": "error",
        "no-useless-concat": "error",
        "no-useless-return": "error",
        "no-void": "error",
        "no-warning-comments": "off",
        "prefer-named-capture-group": "error",
        "prefer-promise-reject-errors": "error",
        "prefer-regex-literals": ["error", {"disallowRedundantWrapping": true}],
        "radix": "error",
        "require-await": "off",
        "@typescript-eslint/require-await": "error",
        "wrap-iife": "error",
        "yoda": "error",
        "no-label-var": "error",
        "no-undef-init": "error",
        "no-unused-vars": "off",
        "@typescript-eslint/no-unused-vars": ["warn", {"argsIgnorePattern": "^_$"}],
        "no-use-before-define": "off",
        "@typescript-eslint/no-use-before-define": ["error"],
        "jsx-quotes": ["error", "prefer-single"],
        "lines-between-class-members": "off",
        "@typescript-eslint/lines-between-class-members": ["error", "always", {"exceptAfterOverload": true}],
        "new-parens": "error",
        "no-array-constructor": "off",
        "@typescript-eslint/no-array-constructor": ["error"],
        "no-lonely-if": "error",
        "no-multi-assign": "error",
        "no-new-object": "error",
        "no-unneeded-ternary": ["error", {"defaultAssignment": false}],
        "one-var": ["error", {"initialized": "never"}],
        "padded-blocks": ["error", "never"],
        "sort-keys": ["warn", "asc", {"natural": true}],
        "sort-vars": "warn",
        "arrow-body-style": ["error", "always"],
        "arrow-parens": "error",
        "no-dupe-class-members": "off",
        "@typescript-eslint/no-dupe-class-members": ["error"],
        "no-duplicate-imports": "off",
        "@typescript-eslint/no-duplicate-imports": ["error", {"includeExports": true}],
        "no-useless-computed-key": ["error", {"enforceForClassMembers": true}],
        "no-useless-constructor": "off",
        "@typescript-eslint/no-useless-constructor": ["error"],
        "no-useless-rename": "error",
        "no-var": "error",
        "object-shorthand": ["error", "always"],
        "prefer-arrow-callback": "error",
        "prefer-const": "error",
        "prefer-numeric-literals": "error",
        "prefer-rest-params": "error",
        "prefer-spread": "error",
        "prefer-template": "error",
        "symbol-description": "error",
        "@typescript-eslint/array-type": ["error", {"default": "array-simple"}],
        "@typescript-eslint/ban-tslint-comment": "error",
        "@typescript-eslint/class-literal-property-style": ["error", "fields"],
        "@typescript-eslint/consistent-indexed-object-style": ["error", "record"],
        "@typescript-eslint/consistent-type-assertions": ["error", {"assertionStyle": "as", "objectLiteralTypeAssertions": "never"}],
        "@typescript-eslint/explicit-member-accessibility": ["error", {"accessibility": "explicit"}],
        "@typescript-eslint/explicit-module-boundary-types": ["off"],
        "@typescript-eslint/member-delimiter-style": ["error"],
        "@typescript-eslint/method-signature-style": "error",
        "@typescript-eslint/no-base-to-string": "error",
        "@typescript-eslint/no-confusing-non-null-assertion": "error",
        "@typescript-eslint/no-confusing-void-expression": "error",
        "@typescript-eslint/no-dynamic-delete": "error",
        "@typescript-eslint/no-explicit-any": "error",
        "@typescript-eslint/no-implicit-any-catch": "error",
        "@typescript-eslint/no-invalid-void-type": "error",
        "@typescript-eslint/no-require-imports": "error",
        "@typescript-eslint/no-unnecessary-boolean-literal-compare": "error",
        "@typescript-eslint/no-unnecessary-condition": "error",
        "@typescript-eslint/no-unnecessary-qualifier": "error",
        "@typescript-eslint/no-unnecessary-type-arguments": "error",
        "@typescript-eslint/no-unnecessary-type-constraint": "error",
        "@typescript-eslint/prefer-for-of": "error",
        "@typescript-eslint/prefer-function-type": "error",
        "@typescript-eslint/prefer-includes": "error",
        "@typescript-eslint/prefer-literal-enum-member": "error",
        "@typescript-eslint/prefer-nullish-coalescing": "error",
        "@typescript-eslint/prefer-optional-chain": "error",
        "@typescript-eslint/prefer-readonly": "error",
        "@typescript-eslint/prefer-reduce-type-parameter": "error",
        "@typescript-eslint/prefer-string-starts-ends-with": "error",
        "@typescript-eslint/prefer-ts-expect-error": "error",
        "@typescript-eslint/promise-function-async": "error",
        "@typescript-eslint/require-array-sort-compare": ["error", {"ignoreStringArrays": true}],
        "@typescript-eslint/restrict-plus-operands": ["error", {"checkCompoundAssignments": true}],
        "@typescript-eslint/sort-type-union-intersection-members": "error",
        "@typescript-eslint/switch-exhaustiveness-check": "error",
        "@typescript-eslint/unified-signatures": "error",
        "react/button-has-type": "error",
        "react/default-props-match-prop-types": "error",
        "react/destructuring-assignment": ["error", "never"],
        "react/forbid-foreign-prop-types": "error",
            "react/function-component-definition": ["error", {"namedComponents": "arrow-function", "unnamedComponents": "arrow-function"}],
        "react/no-access-state-in-setstate": "error",
        "react/no-array-index-key": "error",
        "react/no-danger": "error",
        "react/no-redundant-should-component-update": "error",
        "react/no-this-in-sfc": "error",
        "react/no-typos": "warn",
        "react/no-unsafe": "error",
        "react/no-unused-prop-types": "error",
        "react/no-unused-state": "warn",
        "react/prefer-es6-class": "error",
        "react/require-default-props": ["error", {"forbidDefaultForRequired": true}],
        "react/self-closing-comp": ["warn", {"component": true, "html": true}],
        "react/sort-comp": "warn",
        "react/sort-prop-types": ["warn", {"callbacksLast": true, "requiredFirst": true, "sortShapeProp": true}],
        "react/state-in-constructor": "error",
        "react/static-property-placement": "error",
        "react/style-prop-object": "error",
        "react/void-dom-elements-no-children": "error",
        "react/jsx-boolean-value": ["error", "always"],
        "react/jsx-child-element-spacing": "warn",
        "react/jsx-filename-extension": ["error", {"allow": "as-needed", "extensions": [".tsx", ".jsx"]}],
        "react/jsx-fragments": "error",
        "react/jsx-key": ["error", {"checkFragmentShorthand": true, "checkKeyMustBeforeSpread": true}],
        "react/jsx-no-constructed-context-values": "error",
        "react/jsx-no-script-url": "error",
            "react/jsx-no-useless-fragment": "warn",
        "react/jsx-pascal-case": "error",
        "react/jsx-sort-default-props": "warn",
        "react/jsx-sort-props": ["warn", {"callbacksLast": true, "shorthandFirst": true, "reservedFirst": true}]
    }
}

What did you do?

 1  export const test1 = async (thing: { name: string }) => {
 2     const name = thing.name;
 3     await 0;
 4     thing.name = name; // <= error here
 5 };
 6 
 7 export const test2 = async (thing: { name: string }) => {
 8     const name = thing.name;
 9     await 0;
10     const _ = thing;
11     thing.name = name; // <= no error here
12 };
npx eslint server/test.ts

What did you expect to happen?

Look at the test1 function. I put a useless await inside, then I got an error on the line thing.name = name;. I don't see how the line in question is a race condition. Yes, there is an await before it, but that does nothing. If this is just marked improperly as a "potential" race condition, that would be just fine with me and no need to report a bug.

However, take a look at the test2 function. Now I add another useless line, const _ = thing;. This does nothing, yet it causes the require-atomic-updates error on the following line to go away. I'm don't think this should be the expected behavior here.

What actually happened?

/mnt/c/Users/<PATH>/server/test.ts
   4:5   error    Possible race condition: `thing.name` might be reassigned based on an outdated value of `thing.name`  require-atomic-updates
  10:11  warning  '_' is assigned a value but never used                                                                @typescript-eslint/no-unused-vars

✖ 2 problems (1 error, 1 warning)
@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Jun 10, 2021
@nullromo
Copy link
Author

I didn't see this discussed in #11899, and I think it may be a separate issue, so that's why I brought it up separately.

@btmills
Copy link
Member

btmills commented Jun 11, 2021

Strange indeed. I’m able to repro this in the ESLint demo. This feels like it might be a side effect of #13915. The code reads the variable, but the assign is still stale. It might be possible to fix this and #11899 in the same commit if we were to tweak how this rule works and start looking at the right hand side of the assign to see if it depends on anything that was read from the target of the assign before the await.

@btmills btmills added bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion repro:yes Issues with a reproducible example rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Jun 11, 2021
@mdjermanovic
Copy link
Member

 1  export const test1 = async (thing: { name: string }) => {
 2     const name = thing.name;
 3     await 0;
 4     thing.name = name; // <= error here
 5 };

I think this is the same as #11899. A property of thing is read before await and is assigned to after await. The rule warns that thing may have been changed in between, i.e., that the assignment is based on a previous state of thing.

 7 export const test2 = async (thing: { name: string }) => {
 8     const name = thing.name;
 9     await 0;
10     const _ = thing;
11     thing.name = name; // <= no error here
12 };

I think this is correct behavior for this rule - thing is read after await, so it assumes that the assignment is based on current state of thing.

@nullromo
Copy link
Author

I see what you mean about how it is read after the await. I think the rule is behaving correctly as defined, but I think the definition of the rule is a bit off. #11899 is about false positives. Here I am calling attention to the false negation of the error whether it's a false positive or not; that is, the fact that adding the line obj; (looking at the demo code from @btmills) removes the error. In #11899, people are saying "just because there is an await here doesn't mean I have an error." Here I am saying "just because the variable is read doesn't mean there is no error." Perhaps this example illustrates it more clearly:

/* eslint require-atomic-updates: "error" */

let obj = { value: 1 };

const changeValue = async () => {
    obj.value += 2;
    return 7;
}

// Correctly reported buggy version. Value should be
// 1 + 2 + 7 = 10, but instead it is 1 + 7 = 8
async () => {
    const value = obj.value; // value = 1
    const result = await changeValue(); // result = 7, but now obj.value = 3
    obj.value = value + result; // obj.value assigned to 8, not 10
};

// False negative version. Yes, obj is read before the final assignment,
// but the bug stil exists and in unreported now.
async () => {
    const value = obj.value;
    const result = await changeValue();
    obj;
    obj.value = value + result;
};

Same code in Demo.

@mdjermanovic
Copy link
Member

// False negative version. Yes, obj is read before the final assignment,
// but the bug stil exists and in unreported now.
async () => {
    const value = obj.value;
    const result = await changeValue();
    obj;
    obj.value = value + result;
};

I think this behavior makes sense given how the rule treats assignments to properties, because the logic isn't based on property names. The rule only checks if the object was used before await, and then modified after the await.

/* eslint require-atomic-updates: error */

let obj = { value: 1 };

async () => {
    obj;
    const result = await something;    
    obj.value = result; // error, `obj.value` might be assigned based on an outdated state of `obj`
};

If any access to the object before await is considered as using the old state of the object, then it's consistent to consider any access to the object after await as using the new state of the object from that point on.

@nullromo
Copy link
Author

nullromo commented Nov 1, 2021

If that's the proper behavior, that's fine. This issue points out that the way one might expect the rule to work is different from the actual implementation of the rule.

@github-actions
Copy link

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Dec 31, 2021
@nullromo
Copy link
Author

nullromo commented Jan 3, 2022

@github-actions The bug and evaluating labels should be removed and the issue should be closed.

@nullromo nullromo closed this as completed Jan 3, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jul 3, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 3, 2022
@nzakas nzakas moved this to Complete in Triage Jan 3, 2023
@nzakas nzakas added this to Triage Jan 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion repro:yes Issues with a reproducible example rule Relates to ESLint's core rules Stale
Projects
Archived in project
Development

No branches or pull requests

3 participants