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
Conversation
There was a problem hiding this 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
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. |
There was a problem hiding this comment.
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:
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe “resolved” then?
There was a problem hiding this comment.
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.
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>
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 therequire-atomic-updates
rule. When this option is set totrue
, the rule does not report assignments to properties. Default isfalse
.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?