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

@mdjermanovic mdjermanovic commented Nov 1, 2021

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?

nzakas
nzakas approved these changes Nov 5, 2021
Copy link
Member

@nzakas nzakas left a comment

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

@nzakas nzakas Nov 5, 2021

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

@nzakas nzakas Nov 5, 2021

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

@mdjermanovic mdjermanovic Nov 5, 2021

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

@nzakas nzakas Nov 6, 2021

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

@mdjermanovic mdjermanovic Nov 19, 2021

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 Nov 5, 2021
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>
@mdjermanovic mdjermanovic requested a review from nzakas Nov 19, 2021
nzakas
nzakas approved these changes Nov 20, 2021
@btmills btmills merged commit 60b0a29 into main Nov 21, 2021
14 checks passed
@btmills btmills deleted the issue11899 branch Nov 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants