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
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.
@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.
@mdjermanovic I had some spare time and changed the code such that |
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.
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.
I'll get to this PR hopefully this weekend, latest next weekend |
@mdjermanovic I have now updated the code to skip unresolved references. Please let me know if you have further suggestions. |
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.
LGTM, thanks!
Thanks for guiding me through this PR! Much appreciated |
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.
LGTM. Congrats on your first open source PR, @patriscus! This will go out in tonight's release.
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:
What changes did you make? (Give an overview)
I adapted the
if
condition surrounding the code block that is pushing/setting a reference to theassignmentReferences
map. This map is subsequently used to getreferences
and check for outdated identifier names.I moved the
isLocalVariableWithoutEscape
in saidif
condition out to a separate variable and extended it with: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.