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

feat: add allowProperties option to require-atomic-updates #15238

Merged
merged 8 commits into from Nov 21, 2021
Merged

Conversation

mdjermanovic
Copy link
Member

Prerequisites checklist

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

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

Fixes #11899 by adding an option to allow assignments to properties.

What changes did you make? (Give an overview)

Added allowProperties option to the require-atomic-updates rule. When this option is set to true, the rule does not report assignments to properties. Default is false.

Also updated the documentation to clarify how this rule works.

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

Is the documentation update good?

@mdjermanovic mdjermanovic added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion labels Nov 1, 2021
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Overall looks good. I think the docs still need some clarification and I did my best to leave suggestions where I thought I might know how to clarify.

docs/rules/require-atomic-updates.md Outdated Show resolved Hide resolved
2. A `yield` or `await` pauses the function.
3. After the function is resumed, a value is assigned to the variable.

The assignment in step 3 is reported because this flow indicates that the assignment is based on the possibly outdated value of the variable from step 1, as the variable may have been reassigned elsewhere while the function was paused in step 2.
Copy link
Member

Choose a reason for hiding this comment

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

I think we can simplify this a bit:

Suggested change
The assignment in step 3 is reported because this flow indicates that the assignment is based on the possibly outdated value of the variable from step 1, as the variable may have been reassigned elsewhere while the function was paused in step 2.
The assignment in step 3 is reported because it may be incorrectly calculated because the value of the variable from step 1 may have changed between steps 2 and 3.

Copy link
Member

Choose a reason for hiding this comment

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

It may also be helpful to explicitly call out the difference between local and global variables hazards with regards to this rule.

Copy link
Member Author

Choose a reason for hiding this comment

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

because it may be incorrectly calculated

I wanted to avoid the word "calculated", as that may be interpreted as if there must be a non-trivial expression that calculates the new value, and that the old value must be directly used in that expression (for example, result = result + await something). We had complaints that assignments such as x = "foo" should never be reported because there is no calculation, and the old value does not contribute to the new value. This rule reports any read x -> await -> write x flow, even if the write is a trivial x = "foo", because the old value may have been used in a condition that determines whether or not x = "foo" should be executed.

may have changed between steps 2 and 3

This could be interpreted as:

x; // step 1

await something; // step 2

/*
    some code between step 2 and step 3, `x` is changed here
*/

x = y; // step 3

I wanted to emphasize that x may have changed outside of this execution flow, while it was awaiting in step 2.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe “resolved” then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated this paragraph now, please take a look.

docs/rules/require-atomic-updates.md Outdated Show resolved Hide resolved
docs/rules/require-atomic-updates.md Outdated Show resolved Hide resolved
docs/rules/require-atomic-updates.md Outdated Show resolved Hide resolved
mdjermanovic and others added 6 commits November 5, 2021 12:45
Co-authored-by: Nicholas C. Zakas <nicholas@nczconsulting.com>
Co-authored-by: Nicholas C. Zakas <nicholas@nczconsulting.com>
Co-authored-by: Nicholas C. Zakas <nicholas@nczconsulting.com>
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
@btmills btmills merged commit 60b0a29 into main Nov 21, 2021
@btmills btmills deleted the issue11899 branch November 21, 2021 06:02
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators May 21, 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 May 21, 2022
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 enhancement This change enhances an existing feature of ESLint 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
3 participants