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

require-atomic-updates false positive #11899

Open
medikoo opened this issue Jun 25, 2019 · 55 comments
Open

require-atomic-updates false positive #11899

medikoo opened this issue Jun 25, 2019 · 55 comments

Comments

@medikoo
Copy link

@medikoo medikoo commented Jun 25, 2019

Tell us about your environment

  • ESLint Version: 6.0.1
  • Node Version: 12.4.0
  • npm Version: 6.9.0

What parser (default, Babel-ESLint, etc.) are you using?

Default

Please show your full configuration:

https://github.com/medikoo/eslint-config-medikoo/blob/master/index.js

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

const fn = async someObj => {
  let { foo } = someObj;
  await Promise.resolve();
  // Possible race condition: `someObj.bar` might be reassigned based on an outdated value of `someObj.bar`
  someObj.bar = "whatever";
};

What did you expect to happen?

No error reported

What actually happened? Please include the actual, raw output from ESLint.

Error reported:

Possible race condition: `someObj.bar` might be reassigned based on an outdated value of 

Are you willing to submit a pull request to fix this bug?

Not at this point

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Jun 25, 2019

Thank you for your report.

I think this is intentional behavior because that someObj object was read before await expression and written after await expression.

@medikoo
Copy link
Author

@medikoo medikoo commented Jun 25, 2019

I think this is intentional behavior because that someObj object was read before await expression and written after await expression.

Ok, but that's pretty aggressive assumption, as in many cases that can be an intentional well-thought construct

In place where it reported for me, it's totally intentional and there's no way to write it differently (aside of dropping async/await), due to that I needed to turn off this rule globally.

Still I'd love it to report for me cases as count += await getCount() which are error-prone obviously (and even if not, easy to write in a way they do not report)

@stinovlas
Copy link

@stinovlas stinovlas commented Jun 25, 2019

I would say that if nothing else, error message is pretty misleading.

someObj.bar might be reassigned based on an outdated value of someObj.bar

That is simply not true, because someObj.bar = "whatever"; assigns string "whatever" which is not depending on someObj.bar in any way.

Also, it's not following the rule definition which clearly states that following must be true:

A variable or property is reassigned to a new value which is based on its old value.
@jwalton
Copy link

@jwalton jwalton commented Jun 27, 2019

Here's a pretty vanilla Express middleware that triggers this problem, as you might see in any app that does virtual hosting based on the URL, if you're looking for a concrete example:

export function populateAccount() {
    return function(req, res, next) {
        Promise.resolve()
            .then(async () => {
                const host = req.headers['host'];
                req.account = await getAccount(host);
            })
            .then(() => next(), next);
    };
}
/*eslint require-atomic-updates: error */

Demo link.

rocka added a commit to SB-IM/SDWC that referenced this issue Jun 30, 2019
see: eslint/eslint#11899
didn't figure out what happened
@j0h
Copy link

@j0h j0h commented Jul 4, 2019

@mysticatea could the following POC also be related to the issue?

function dataGetter() {
  return {};
}

function anotherDataGetter() {
  return {};
}

async function main() {
  let get_data = await dataGetter();

  const get_another_data = await anotherDataGetter(get_data.id);

  (() => {
    get_data[get_another_data.foo] = 1;
  });

  // Error Here: Possible race condition: `get_data` might be reassigned based on an outdated value of `get_data`.
  get_data = {
    ...get_data,
    ...get_another_data,
  };
}

main();

Demo Link

medikoo added a commit to serverless/eslint-config that referenced this issue Jul 5, 2019
More info: eslint/eslint#11899
@ololoepepe
Copy link

@ololoepepe ololoepepe commented Jul 8, 2019

There is a workaround:

// Given:
const obj = {foo: 'bar'};

// Instead of:
obj.prop = 'whatever';

// Do:
Object.assign(obj, {prop: 'whatever'});

This way the rule is not triggered (as expected).
But here comes another question: given current (incorrect) behavior, shouldn't the rule be triggered on Object.assign as well?

@Lonniebiz
Copy link

@Lonniebiz Lonniebiz commented Jul 9, 2019

It is my understanding that await prevents any further execution from happening, within that code block until the promise is resolved. someObj is an argument passed into that function by reference and someObj is accessible everywhere it is used within that code block originally posted.

Even if you awaited a promise that did 100 things to the same object that someObj references, once that promise-work is done there is nothing to stop this code block from also setting someObj.bar = "whatever";. Sure, the fulfilled promise could have made AnotherReferenceTosomeObj.bar = "something specific";, but that should get over-written by someObj.bar = "whatever";

To assume that the promise does this on a "might", and invoke an eslint error, seems like an attempt to oversimplify async programming to me.

If eslint isn't sophisticated enough to point out BOTH lines of code, that are "racing" to overwrite something (race condition), then it probably needs to keep its might-suspicions to itself (until it gains that sophistication). Don't block my build on a "might". It takes two to race. If I've got a race condition, show me both lines of code that are racing each other.

Here's my example:
#11967

@ololoepepe
Copy link

@ololoepepe ololoepepe commented Jul 9, 2019

@Lonniebiz what if the other line of code is located somewhere in a 3rd-party lib? Looking through entire node_modules to find that line does no sound as a good idea.
Still, I agree with you. That suspicion should not be considered an error. A warning would be quite enough.

@ronkorving
Copy link

@ronkorving ronkorving commented Jul 9, 2019

This is a real issue (I'm hitting all the examples pasted above), and one that was not mentioned in the migration guide for ESLint 6.0.0.

I can appreciate theories on how these cases are at least on paper unsafe, but that kind of behavior should then at least exist behind a flag. Right now, we're losing a valuable feature because the only way to deal with this situation is to turn the rule off.

@j0h
Copy link

@j0h j0h commented Jul 9, 2019

@ololoepepe

what if the other line of code is located somewhere in a 3rd-party lib? Looking through entire node_modules to find that line does no sound as a good idea.

I think that might be a non-issue, I don't think you should be setting up your eslint to run through entire node_modules. This would be outside your context/source code and it would be up to the original maintainer of that said library to fix those issues (or yourself without an appropriate PR).

@viktor-ku
Copy link

@viktor-ku viktor-ku commented Jul 9, 2019

We have to disable this rule in our company due to many false positives. In paper rule is great and should prevent many bugs, but feels like it's not quite ready yet.

@Lonniebiz
Copy link

@Lonniebiz Lonniebiz commented Jul 9, 2019

Still, I agree with you. That suspicion should not be considered an error. A warning would be quite enough.

I'm not even sure you should even warn when side-effects are obvious and likely intentional.

@ronkorving
Copy link

@ronkorving ronkorving commented Jul 18, 2019

@mysticatea May I ask what the plan is? Will the behavior of this rule be reverted, or otherwise? Current behavior is keeping people from upgrading to ESLint 6. It would be good to know what the official stance is. If it's "won't fix", then we'll just all turn off this rule and move on with our lives. If it's "will fix in 6.0.2", then we know we can wait for a bit, and it will resolve itself.

@ralfstx
Copy link

@ralfstx ralfstx commented Jul 25, 2019

Here is a perfectly valid koa snippet that is faulted by this rule:

router.post('/webhook', async (ctx) => {
  await processRequest(ctx.request.body);
  ctx.body = 'OK';
});
jessety added a commit to jessety/eslint-config that referenced this issue Mar 22, 2020
This rule is too aggressive. Even writing Koa middleware in the exact recommended way triggers it.

eslint/eslint#11899
bcoe added a commit to google/gts that referenced this issue Mar 26, 2020
require-atomic-updates is a bit too strict, and is currently being tracked as a bug
here: eslint/eslint#11899
mt-ronkorving added a commit to mt-ronkorving/eslint-config that referenced this issue Jun 8, 2020
mt-ronkorving added a commit to mt-ronkorving/eslint-config that referenced this issue Jun 15, 2020
mt-ronkorving added a commit to moneytree/eslint-config that referenced this issue Jun 15, 2020
@denis-sokolov
Copy link
Contributor

@denis-sokolov denis-sokolov commented Sep 3, 2020

I would like to speak up in support of the rule. Mutation and async programming is a source of many errors in the codebases I had worked on and this rule has been very beneficial to bring them to light. I would also like to speak up in favor of keeping it in recommended ruleset, to help expose these difficult issues for less experienced contributors. The false positives are, of course, a major issue, as it leads to the rule being disabled or ignored.

Perhaps we can introduce more ways to explicitly mark up objects that we are allowed to mutate? This can already be done somewhat using renaming a variable in one’s scope, but it looks like a hack, since at the end of the day the object maintains identity:

app.get('/', async (ctx) => {
  const myCtx = ctx;
  await process(myCtx.request.body);
  myCtx.body = 'OK';
});

An explicit comment could indicate the author’s intent and judgement. At this stage it is almost identical to eslint-disable-next-line require-atomic-updates, but with a better communication of the reasons (and it works for the entire block):

app.get('/', async (ctx) => {
  // eslint-we-own: ctx
  await process(ctx.request.body);
  ctx.body = 'OK';
});

For common patterns like the Koa context, it would be convenient to whitelist the variables we are allowed to mutate by name in the configuration:

{
  "require-atomic-updates": ["error", {
    "skip-variables": ["ctx"]
  }]
}
@ololoepepe
Copy link

@ololoepepe ololoepepe commented Sep 4, 2020

@denis-sokolov There are too many situations already, when people are treated as idiots ("for their own safety", of course). Do not create another one, please.

jacksonrayhamilton added a commit to jacksonrayhamilton/eslint-config-will-robinson that referenced this issue Sep 11, 2020
This rule seems prone to false positives, and thus it may be more misleading
than useful (for now).  See: eslint/eslint#11899
olsonpm pushed a commit to olsonpm/eslint-config-personal that referenced this issue Mar 15, 2021
domenic added a commit to domenic/eslint-config that referenced this issue Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet