Fix: track variables, not names in require-atomic-updates (fixes #14208)

[patriscus]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

fixes #14208

Fixes false positives like:

async function foo(e) {
}

async function run() {
  const input = [];
  const props = [];

  for(const entry of input) {
    const prop = props.find(a => a.id === entry.id) || null;
    await foo(entry);
  }

  // false positive:
  // Possible race condition: `const entry` might be reassigned based on an outdated value of `const entry`
  for(const entry of input) {
    const prop = props.find(a => a.id === entry.id) || null;
  }

  for(const entry2 of input) {
    const prop = props.find(a => a.id === entry2.id) || null;
  }
}

What changes did you make? (Give an overview)

I adapted the if condition surrounding the code block that is pushing/setting a reference to the assignmentReferences map. This map is subsequently used to get references and check for outdated identifier names.

I moved the isLocalVariableWithoutEscape in said if condition out to a separate variable and extended it with:

const isVariableDeclaration = isLocalVariableWithoutEscape(variable, isMemberAccess) ||
    node.parent.type === "VariableDeclarator";

Since the condition had a comment with "exclude variable declarations", I removed it and expressed it with the isVariableDeclaration variable name.

Is there anything you'd like reviewers to focus on?

This is the first time contributing to ESLint (or open-source that is). Please focus on edge-cases or other implications since I do not have the complete context. I'm happy to hear about your concerns and improvement points.

[mdjermanovic]

@patriscus thanks for the PR! I think that this solution does fix #14208, but not all similar cases, and maybe it wouldn't be too difficult to fix the actual root cause.

[patriscus]

@mdjermanovic I had some spare time and changed the code such that segmentInfo stores references to variables instead of storing variable names. Feel free to review & leave suggestions again. Thanks!

[mdjermanovic]

The change looks good, we should just skip unresolved references in function createReferenceMap.

reference.resolved is null if it's unresolved, so this would be a false positive:

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

async function run() {
    await a;
    b = 1; // false positive
}

(because we're now tracking undefined variables a and b as the same null variable object in SegmentInfo).

That would miss reporting some cases with the same variable name, but it's fine, most of the core rules are ignoring undefined variables.

[patriscus]

I'll get to this PR hopefully this weekend, latest next weekend

[patriscus]

@mdjermanovic I have now updated the code to skip unresolved references. Please let me know if you have further suggestions.

[mdjermanovic]

LGTM, thanks!

[patriscus]

Thanks for guiding me through this PR! Much appreciated

[btmills]

LGTM. Congrats on your first open source PR, @patriscus! This will go out in tonight's release.