Fix: Revert setting node.parent early (fixes #9331) (#9336)

This reverts commit 1488b51. Some rules
from plugins were relying on the behavior that a a `node.parent`
property is only sometines present, and other rules were relying on the
behavior that the AST contains no cycles when rules are created.

docs/developer-guide/working-with-plugins.md

@@ -204,7 +204,7 @@ All nodes must have `range` property.
 * `range` (`number[]`) is an array of two numbers. Both numbers are a 0-based index which is the position in the array of source code characters. The first is the start position of the node, the second is the end position of the node. `code.slice(node.range[0], node.range[1])` must be the text of the node. This range does not include spaces/parentheses which are around the node.
 * `loc` (`SourceLocation`) must not be `null`. [The `loc` property is defined as nullable by ESTree](https://github.com/estree/estree/blob/25834f7247d44d3156030f8e8a2d07644d771fdb/es5.md#node-objects), but ESLint requires this property. On the other hand, `SourceLocation#source` property can be `undefined`. ESLint does not use the `SourceLocation#source` property.
 
-The `parent` property of all nodes must be rewriteable. ESLint sets each node's `parent` property to its parent node while traversing, before any rules have access to the AST.
+The `parent` property of all nodes must be rewriteable. ESLint sets each node's parent properties to its parent node while traversing.
 
 #### The `Program` node:
 

lib/linter.js

@@ -660,22 +660,6 @@ function markVariableAsUsed(scopeManager, currentNode, parserOptions, name) {
     return false;
 }
 
-/**
- * Gets all the ancestors of a given node
- * @param {ASTNode} node The node
- * @returns {ASTNode[]} All the ancestor nodes in the AST, not including the provided node, starting
- * from the root node and going inwards to the parent node.
- */
-function getAncestors(node) {
-    if (node.parent) {
-        const parentAncestors = getAncestors(node.parent);
-
-        parentAncestors.push(node.parent);
-        return parentAncestors;
-    }
-    return [];
-}
-
 // methods that exist on SourceCode object
 const DEPRECATED_SOURCECODE_PASSTHROUGHS = {
     getSource: "getText",
@@ -835,6 +819,7 @@ module.exports = class Linter {
         }
 
         const emitter = new EventEmitter().setMaxListeners(Infinity);
+        const traverser = new Traverser();
         const ecmaFeatures = config.parserOptions.ecmaFeatures || {};
         const ecmaVersion = config.parserOptions.ecmaVersion || 5;
         const scopeManager = eslintScope.analyze(sourceCode.ast, {
@@ -846,19 +831,6 @@ module.exports = class Linter {
             fallback: Traverser.getKeys
         });
 
-        let currentNode = sourceCode.ast;
-        const nodeQueue = [];
-
-        new Traverser().traverse(sourceCode.ast, {
-            enter(node, parent) {
-                node.parent = parent;
-                nodeQueue.push({ isEntering: true, node });
-            },
-            leave(node) {
-                nodeQueue.push({ isEntering: false, node });
-            }
-        });
-
         /*
          * Create a frozen object with the ruleContext properties and methods that are shared by all rules.
          * All rule contexts will inherit from this object. This avoids the performance penalty of copying all the
@@ -868,12 +840,12 @@ module.exports = class Linter {
             Object.assign(
                 Object.create(BASE_TRAVERSAL_CONTEXT),
                 {
-                    getAncestors: () => getAncestors(currentNode),
+                    getAncestors: () => traverser.parents(),
                     getDeclaredVariables: scopeManager.getDeclaredVariables.bind(scopeManager),
                     getFilename: () => filename,
-                    getScope: () => getScope(scopeManager, currentNode, config.parserOptions.ecmaVersion),
+                    getScope: () => getScope(scopeManager, traverser.current(), config.parserOptions.ecmaVersion),
                     getSourceCode: () => sourceCode,
-                    markVariableAsUsed: name => markVariableAsUsed(scopeManager, currentNode, config.parserOptions, name),
+                    markVariableAsUsed: name => markVariableAsUsed(scopeManager, traverser.current(), config.parserOptions, name),
                     parserOptions: config.parserOptions,
                     parserPath: config.parser,
                     parserServices,
@@ -978,13 +950,19 @@ module.exports = class Linter {
 
         const eventGenerator = new CodePathAnalyzer(new NodeEventGenerator(emitter));
 
-        nodeQueue.forEach(traversalInfo => {
-            currentNode = traversalInfo.node;
-
-            if (traversalInfo.isEntering) {
-                eventGenerator.enterNode(currentNode);
-            } else {
-                eventGenerator.leaveNode(currentNode);
+        /*
+         * Each node has a type property. Whenever a particular type of
+         * node is found, an event is fired. This allows any listeners to
+         * automatically be informed that this type of node has been found
+         * and react accordingly.
+         */
+        traverser.traverse(sourceCode.ast, {
+            enter(node, parent) {
+                node.parent = parent;
+                eventGenerator.enterNode(node);
+            },
+            leave(node) {
+                eventGenerator.leaveNode(node);
             }
         });
 

lib/util/source-code.js

@@ -349,13 +349,15 @@ class SourceCode extends TokenStore {
      * @returns {ASTNode} The node if found or null if not found.
      */
     getNodeByRangeIndex(index) {
-        let result = null;
+        let result = null,
+            resultParent = null;
         const traverser = new Traverser();
 
         traverser.traverse(this.ast, {
-            enter(node) {
+            enter(node, parent) {
                 if (node.range[0] <= index && index < node.range[1]) {
                     result = node;
+                    resultParent = parent;
                 } else {
                     this.skip();
                 }
@@ -367,7 +369,7 @@ class SourceCode extends TokenStore {
             }
         });
 
-        return result;
+        return result ? Object.assign({ parent: resultParent }, result) : null;
     }
 
     /**

tests/lib/linter.js

@@ -100,21 +100,6 @@ describe("Linter", () => {
                 linter.verify(code, config, filename, true);
             }, "Intentional error.");
         });
-
-        it("has all the `parent` properties on nodes when the rule listeners are created", () => {
-            linter.defineRule("checker", context => {
-                const ast = context.getSourceCode().ast;
-
-                assert.strictEqual(ast.body[0].parent, ast);
-                assert.strictEqual(ast.body[0].expression.parent, ast.body[0]);
-                assert.strictEqual(ast.body[0].expression.left.parent, ast.body[0].expression);
-                assert.strictEqual(ast.body[0].expression.right.parent, ast.body[0].expression);
-
-                return {};
-            });
-
-            linter.verify("foo + bar", { rules: { checker: "error" } });
-        });
     });
 
     describe("context.getSourceLines()", () => {
@@ -447,6 +432,43 @@ describe("Linter", () => {
             linter.verify(code, config);
             assert(spy.calledOnce);
         });
+
+        it("should attach the node's parent", () => {
+            const config = { rules: { checker: "error" } };
+            const spy = sandbox.spy(context => {
+                const node = context.getNodeByRangeIndex(14);
+
+                assert.property(node, "parent");
+                assert.equal(node.parent.type, "VariableDeclarator");
+                return {};
+            });
+
+            linter.defineRule("checker", spy);
+            linter.verify(code, config);
+            assert(spy.calledOnce);
+        });
+
+        it("should not modify the node when attaching the parent", () => {
+            const config = { rules: { checker: "error" } };
+            const spy = sandbox.spy(context => {
+                const node1 = context.getNodeByRangeIndex(10);
+
+                assert.equal(node1.type, "VariableDeclarator");
+
+                const node2 = context.getNodeByRangeIndex(4);
+
+                assert.equal(node2.type, "Identifier");
+                assert.property(node2, "parent");
+                assert.equal(node2.parent.type, "VariableDeclarator");
+                assert.notProperty(node2.parent, "parent");
+                return {};
+            });
+
+            linter.defineRule("checker", spy);
+            linter.verify(code, config);
+            assert(spy.calledOnce);
+        });
+
     });
 
 

tests/lib/util/source-code.js

@@ -1775,6 +1775,25 @@ describe("SourceCode", () => {
             node = sourceCode.getNodeByRangeIndex(-99);
             assert.isNull(node);
         });
+
+        it("should attach the node's parent", () => {
+            const node = sourceCode.getNodeByRangeIndex(14);
+
+            assert.property(node, "parent");
+            assert.equal(node.parent.type, "VariableDeclarator");
+        });
+
+        it("should not modify the node when attaching the parent", () => {
+            let node = sourceCode.getNodeByRangeIndex(10);
+
+            assert.equal(node.type, "VariableDeclarator");
+            node = sourceCode.getNodeByRangeIndex(4);
+            assert.equal(node.type, "Identifier");
+            assert.property(node, "parent");
+            assert.equal(node.parent.type, "VariableDeclarator");
+            assert.notProperty(node.parent, "parent");
+        });
+
     });
 
     describe("isSpaceBetweenTokens()", () => {