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: require-atomic-updates false positive (fixes #11194, fixes #11687) #11224

Merged
merged 4 commits into from May 10, 2019

Conversation

Projects
None yet
4 participants
@mysticatea
Copy link
Member

commented Dec 30, 2018

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

[X] Bug fixes #11194, fixes #11687

What changes did you make? (Give an overview)

First of all, I'm sorry that this change is huge.

I have improved the rule's logic with using information that eslint-scope made. The new logic is the following steps (roughly):

  1. Create IdentifierReference map when ESLint entered each async/generator function.
  2. In an async/generator function,
    1. initialize each code path segment having two information: outdated read variables and fresh read variables. Segments inherit that information from the previous segments.
    2. On Identifier nodes, find the reference of the node by the map (of step 1). Then:
      • If reference.isRead() is true, add it to the fresh read variable list of the current code path segment.
      • If reference.isWrite() is true, store the reference.writeExpr (this is AssignmentExpression#right if it's assignment) to verify it later.
    3. On AwaitExpression/YieldExpression nodes (exit), move the fresh read variables to the outdated read variables, on the current code path segment.
    4. On expression nodes (exit), if it's a stored reference.writeExpr, do the verification. If the variable that the reference refers is outdated on the current code path, report it.

Therefore, this logic does the verification on the main traversal instead of extra traversal.

The important part for #11194 bug fix is the step 1. The rule ignores Identifier node if it's not a reference.

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

@eslint eslint bot added the triage label Dec 30, 2018

@not-an-aardvark not-an-aardvark self-requested a review Jan 4, 2019

@not-an-aardvark not-an-aardvark added bug rule accepted and removed triage labels Jan 4, 2019

@not-an-aardvark
Copy link
Member

left a comment

I have a minor suggestion, but otherwise this looks fine to me.


The existing solution does an extra traversal because this would be necessary in order to handle loops within assignment expressions, like this:

// invalid; `await baz()` can happen after `read(foo)`
let foo;
async function x() {
    foo = do {
        while (bar()) {
            await baz();
            read(foo);
        }
    };
}

I think the new implementation would incorrectly consider the above case valid. After we start supporting do expressions (or if we start supporting custom code path analysis from plugins), it would be necessary to change the rule to use the old version again.

In other words, I think the new implementation is theoretically incorrect, but it works fine for the syntax that is currently in the ES spec. Since we don't allow customizing code path analysis yet, I would be okay with adding it since it should improve performance for now.


As a sidenote, both implementations are incorrect in cases where AST traversal order is different from execution order. For example:

let foo;
async function x() {
    foo = class {
        static x = await bar;
        static [read(foo)] = baz;
    };
}

In the above example, read(foo) gets executed before await bar, but it's not possible to use the code path analysis API to figure this out, because await bar would appear first in AST traversal and both would be part of the same code path segment. As a result, rules that depend on code path ordering would all have minor bugs. I think we might need to make changes to the code path analysis API to allow rules to only rely on execution order rather than traversal order.

Show resolved Hide resolved lib/rules/require-atomic-updates.js Outdated
@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

@mysticatea Friendly ping

@platinumazure

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

@mysticatea Friendly ping: Are you still working on this?

@mysticatea

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

I apologize that I had lost track of this.

@mysticatea

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

I agreed that the code path analysis needs some changes...

mysticatea added some commits Apr 13, 2019

@mysticatea mysticatea changed the title Fix: require-atomic-updates false positive (fixes #11194) Fix: require-atomic-updates false positive (fixes #11194, fixes #11687) May 8, 2019

@mysticatea mysticatea merged commit e4400b5 into master May 10, 2019

5 checks passed

commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details

@mysticatea mysticatea deleted the issue11194 branch May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.