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

Proposal: defer rule listener calls until after traversal is complete #9122

Closed
not-an-aardvark opened this Issue Aug 17, 2017 · 7 comments

Comments

Projects
2 participants
@not-an-aardvark
Member

not-an-aardvark commented Aug 17, 2017

For a long time, rules have had a slight usability issue where the parent property of a node is only present after a node is traversed and its listeners have been invoked. This can lead to confusing issues (e.g. #9101) where a function call on a node can change behavior depending on whether the node has been traversed.

Previously, there has been discussion about adding the parent property during the scope analysis traversal (eslint/eslint-scope#27). However, I realized there's a much easier solution: We can just defer all rule listener calls until after traversal is complete.

The current traversal logic looks roughly like this (pseudocode):

for each node in the AST in traversal order:
    node.parent = parentNode
    for each rule listener that matches the node:
        invoke the listener on the node

We could change it to this:

for each node in the AST in traversal order:
    node.parent = parentNode
    for each rule listener that matches the node:
        push (listener, node) into a FIFO queue

for each (listener, node) pair in the queue:
    invoke the listener on the node

The effect is that all parent properties would be present for all rule listeners.

We would need to evaluate the performance effects of doing this. I suspect that most of the time spent traversing the AST is a result of enumerating the children of a node, so after all the nodes have already been enumerated in order, I don't think invoking a function on a small fraction of the nodes will create a big performance problem. (Actually, this might allow us to move scope analysis into the same traversal too, since rules wouldn't be able to observe the scope until the traversal is complete.)

Note that rules can observe the AST before traversal starts, so they would still be able to observe nodes without a parent property. (edit: actually, this can be avoided.)

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Sep 9, 2017

TSC Summary: This issue proposes updating traversal logic to call rule listeners after all nodes have a parent property. This is intended to prevent rules from having confusing issues where only some nodes have a parent property. There is no compatibility impact, and based on an implementation locally, there is no noticeable performance impact.
TSC Question: Should we accept this proposal?

@platinumazure

This comment has been minimized.

Member

platinumazure commented Sep 9, 2017

How does memory consumption look with your local implementation?

not-an-aardvark added a commit that referenced this issue Sep 9, 2017

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Sep 9, 2017

I'm unsure of the best way to measure memory consumption precisely, but if you tell me a good way then I can try it out. Alternatively, I pushed up my implementation in 92bedb8, so feel free to test that out if you'd like.

My guess is that memory consumption is not affected very much (we create an additional array with length equal to twice the number of nodes in the AST, but that's fairly small in comparison to e.g. the token arrays that we already use).

not-an-aardvark added a commit that referenced this issue Sep 10, 2017

not-an-aardvark added a commit that referenced this issue Sep 10, 2017

not-an-aardvark added a commit that referenced this issue Sep 11, 2017

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Sep 14, 2017

This was accepted in today's TSC meeting.

@btmills btmills closed this in 1488b51 Sep 15, 2017

Aladdin-ADD added a commit to Aladdin-ADD/eslint that referenced this issue Sep 18, 2017

ilyavolodin added a commit that referenced this issue Sep 18, 2017

Revert "Chore: rewrite parseListConfig for a small perf gain." (#9325)
* Revert "4.7.0"

This reverts commit 439e8e6.

* Revert "Build: changelog update for 4.7.0"

This reverts commit 2ec62f9.

* Revert "Upgrade: Espree v3.5.1 (fixes #9153) (#9314)"

This reverts commit 787b78b.

* Revert "Update: run rules after `node.parent` is already set (fixes #9122) (#9283)"

This reverts commit 1488b51.

* Revert "Docs: fix wrong config in max-len example. (#9309)"

This reverts commit 4431d68.

* Revert "Chore: Revert "avoid handling Rules instances in config-validator" (#9295)"

This reverts commit 9d1df92.

* Revert "Docs: Fix code snippet to refer to the correct option (#9313)"

This reverts commit 7d24dde.

* Revert "�Chore: rewrite parseListConfig for a small perf gain. (#9300)"

This reverts commit 12388d4.
@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Sep 21, 2017

Reopening because this change was reverted to fix #9331.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Sep 21, 2017

TSC Summary: This issue proposes updating traversal logic to call rule listeners after all nodes have a parent property. The issue was previously accepted by the TSC, but it was reverted because there was some compatibility impact (a plugin was relying on the fact that the AST is non-circular, and another plugin was relying on the parent property not existing in some cases).

TSC Question: Should we schedule this as a breaking change in v5.0.0?

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Oct 13, 2017

In today's TSC meeting, the TSC decided to accept this as a breaking change for 5.0.

@not-an-aardvark not-an-aardvark added accepted and removed evaluating labels Oct 13, 2017

@not-an-aardvark not-an-aardvark added this to Accepted in v5.0.0 Oct 13, 2017

not-an-aardvark added a commit that referenced this issue Feb 23, 2018

@not-an-aardvark not-an-aardvark moved this from Accepted, ready to implement to Ready to merge in v5.0.0 Feb 23, 2018

not-an-aardvark added a commit that referenced this issue Feb 27, 2018

v5.0.0 automation moved this from Ready to merge to Done Mar 22, 2018

@eslint eslint bot locked and limited conversation to collaborators Sep 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.