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

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

Merged
merged 3 commits into from May 8, 2021

Conversation

patriscus
Copy link
Contributor

@patriscus patriscus commented Mar 31, 2021

Prerequisites checklist

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.

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Mar 31, 2021
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Apr 5, 2021
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

lib/rules/require-atomic-updates.js Outdated Show resolved Hide resolved
@patriscus
Copy link
Contributor Author

@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!

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

@patriscus
Copy link
Contributor Author

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

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mdjermanovic mdjermanovic changed the title Fix: require-atomic-updates false positives in loops (fixes #14208) Fix: track variables, not names in require-atomic-updates (fixes #14208) May 3, 2021
@patriscus
Copy link
Contributor Author

Thanks for guiding me through this PR! Much appreciated

btmills
btmills approved these changes May 8, 2021
Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@btmills btmills merged commit ae6dbd1 into eslint:master May 8, 2021
14 checks passed
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Nov 5, 2021
@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 Nov 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[require-atomic-updates]: False positive on reused loop variable name
3 participants