Bug: require-atomic-updates misleading error message

[m90]

Environment

Node version: v14.15.5
npm version: 7.23.0
Local ESLint version: 7.32.0
Global ESLint version: -
Operating System: Ubuntu 20.04

What parser are you using?

Default (Espree)

What did you do?

Configuration

    // ...
    'require-atomic-updates': 2,
    // ...

'use strict';

const opts = {}; // stubbed for repro
const run = () => Promise.resolve({ exit_code: 0 }); // stubbed for repro

(async () => {
  if (opts._.length) {
    if (opts._[0] === '-') {
      opts.spec = process.stdin;
    } else {
      opts.spec = opts._;
    }
  }
  delete opts._;
  try {
    const { exit_code } = await run(opts);
    process.exitCode = exit_code;
  } catch (e) {
    console.error(e.stack);
    process.exitCode = 1;
  }
})();

Playground version

What did you expect to happen?

I would expect the above code not to raise require-atomic-updates.

What actually happened?

I'm seeing

  17:5  error  Possible race condition: `process.exitCode` might be reassigned based on an outdated value of `process.exitCode`  require-atomic-updates
  20:5  error  Possible race condition: `process.exitCode` might be reassigned based on an outdated value of `process.exitCode`  require-atomic-updates

Participation

• I am willing to submit a pull request for this issue.

Additional comments

This started happening after adding a simple condition where opts.spec might be assigned to process.stdin.

Before the change code read like this and passed the rule:

'use strict';

const opts = {}; // stubbed for repro
const run = () => Promise.resolve({ exit_code: 0 }); // stubbed for repro

(async () => {
  if (opts._.length) {
    opts.spec = opts._;
  }
  delete opts._;
  try {
    const { exit_code } = await run(opts);
    process.exitCode = exit_code;
  } catch (e) {
    console.error(e.stack);
    process.exitCode = 1;
  }
})();

I have a hard time understanding how the change introduces a possible race condition (maybe I'm wrong though), so I would assume this is a bug. In case it is a bug I would be happy to submit a PR in case someone can point me to a place where I can start.

---

Edit: unwrapping the async function and using a Promise chain instead does not raise any warnings when including the conditional assignment:

'use strict';

const opts = {}; // stubbed for repro
const run = () => Promise.resolve({ exit_code: 0 }); // stubbed for repro

if (opts._.length) {
  if (opts._[0] === '-') {
    opts.spec = process.stdin;
  } else {
    opts.spec = opts._;
  }
}
delete opts._;

run(opts)
  .catch((err) => {
    console.error(err.stack);
    return { exit_code: 1 };
  })
  .then(({ exit_code }) => {
    process.exitCode = exit_code;
  });

[nzakas]

I’m not very familiar with this rule, by based on the rather nonsensical error message it’s producing, I’d guess this is a bug.

@eslint/eslint-tsc thoughts?

[btmills]

This invalid test case added in #11774 looks to be related:

https://github.com/eslint/eslint/blob/07175b8e9532d79e55c499aa27f79f023abda3c3/tests/lib/rules/require-atomic-updates.js#L322L331

It looks like even though different properties on the object are being read and assigned, the rule considers the whole object stale. I can’t tell based on the issue and PR discussion whether that behavior is intentional, however.

[mdjermanovic]

I think this is the same as #11899.

Based on the discussion in #11899, I believe this is intended behavior, but the error message is wrong (or at least misleading).

[m90]

I agree this sounds pretty much like a duplicate of #11899 - feel free to close this if it makes sense for you and thanks for looking into this.

[mdjermanovic]

Thanks for the confirmation! I'll mark this as accepted to fix the error message (it doesn't seem it was mentioned in #11899), and I'm working on that.