Skip to content

Commit

Permalink
Breaking: set parent of AST nodes before rules run (fixes #9122) (#…
Browse files Browse the repository at this point in the history 
…10014)
  • Loading branch information
@not-an-aardvark
not-an-aardvark committed Mar 22, 2018
1 parent 91ece32 commit 4eaebe5
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 101 deletions.
2 changes: 1 addition & 1 deletion docs/developer-guide/working-with-plugins.md
View file
Expand Up @@ -213,7 +213,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 properties to its parent node while traversing.
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 `Program` node:

Expand Down
59 changes: 40 additions & 19 deletions lib/linter.js
View file
Expand Up @@ -680,6 +680,22 @@ function createRuleListeners(rule, ruleContext) {
}
}

/**
* 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) {
const ancestorsStartingAtParent = [];

for (let ancestor = node.parent; ancestor; ancestor = ancestor.parent) {
ancestorsStartingAtParent.push(ancestor);
}

return ancestorsStartingAtParent.reverse();
}

// methods that exist on SourceCode object
const DEPRECATED_SOURCECODE_PASSTHROUGHS = {
getSource: "getText",
Expand Down Expand Up @@ -731,7 +747,19 @@ const BASE_TRAVERSAL_CONTEXT = Object.freeze(
*/
function runRules(sourceCode, configuredRules, ruleMapper, parserOptions, parserName, settings, filename) {
const emitter = createEmitter();
const traverser = new Traverser();
const nodeQueue = [];
let currentNode = sourceCode.ast;

Traverser.traverse(sourceCode.ast, {
enter(node, parent) {
node.parent = parent;
nodeQueue.push({ isEntering: true, node });
},
leave(node) {
nodeQueue.push({ isEntering: false, node });
},
visitorKeys: sourceCode.visitorKeys
});

/*
* Create a frozen object with the ruleContext properties and methods that are shared by all rules.
Expand All @@ -742,12 +770,12 @@ function runRules(sourceCode, configuredRules, ruleMapper, parserOptions, parser
Object.assign(
Object.create(BASE_TRAVERSAL_CONTEXT),
{
getAncestors: () => traverser.parents(),
getAncestors: () => getAncestors(currentNode),
getDeclaredVariables: sourceCode.scopeManager.getDeclaredVariables.bind(sourceCode.scopeManager),
getFilename: () => filename,
getScope: () => getScope(sourceCode.scopeManager, traverser.current(), parserOptions.ecmaVersion),
getScope: () => getScope(sourceCode.scopeManager, currentNode, parserOptions.ecmaVersion),
getSourceCode: () => sourceCode,
markVariableAsUsed: name => markVariableAsUsed(sourceCode.scopeManager, traverser.current(), parserOptions, name),
markVariableAsUsed: name => markVariableAsUsed(sourceCode.scopeManager, currentNode, parserOptions, name),
parserOptions,
parserPath: parserName,
parserServices: sourceCode.parserServices,
Expand Down Expand Up @@ -847,21 +875,14 @@ function runRules(sourceCode, configuredRules, ruleMapper, parserOptions, parser

const eventGenerator = new CodePathAnalyzer(new NodeEventGenerator(emitter));

/*
* 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);
},
visitorKeys: sourceCode.visitorKeys
nodeQueue.forEach(traversalInfo => {
currentNode = traversalInfo.node;

if (traversalInfo.isEntering) {
eventGenerator.enterNode(currentNode);
} else {
eventGenerator.leaveNode(currentNode);
}
});

return lintingProblems;
Expand Down
8 changes: 3 additions & 5 deletions lib/util/source-code.js
View file
Expand Up @@ -387,15 +387,13 @@ class SourceCode extends TokenStore {
* @public
*/
getNodeByRangeIndex(index) {
let result = null,
resultParent = null;
let result = null;

Traverser.traverse(this.ast, {
visitorKeys: this.visitorKeys,
enter(node, parent) {
enter(node) {
if (node.range[0] <= index && index < node.range[1]) {
result = node;
resultParent = parent;
} else {
this.skip();
}
Expand All @@ -407,7 +405,7 @@ class SourceCode extends TokenStore {
}
});

return result ? Object.assign({ parent: resultParent }, result) : null;
return result;
}

/**
Expand Down
55 changes: 18 additions & 37 deletions tests/lib/linter.js
View file
Expand Up @@ -118,6 +118,24 @@ describe("Linter", () => {
linter.verify("foo", { rules: { checker: "error", "no-undef": "error" } });
assert(spy.notCalled);
});

it("has all the `parent` properties on nodes when the rule listeners are created", () => {
const spy = sandbox.spy(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.defineRule("checker", spy);

linter.verify("foo + bar", { rules: { checker: "error" } });
assert(spy.calledOnce);
});
});

describe("context.getSourceLines()", () => {
Expand Down Expand Up @@ -450,43 +468,6 @@ 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.strictEqual(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.strictEqual(node1.type, "VariableDeclarator");

const node2 = context.getNodeByRangeIndex(4);

assert.strictEqual(node2.type, "Identifier");
assert.property(node2, "parent");
assert.strictEqual(node2.parent.type, "VariableDeclarator");
assert.notProperty(node2.parent, "parent");
return {};
});

linter.defineRule("checker", spy);
linter.verify(code, config);
assert(spy.calledOnce);
});

});


Expand Down
39 changes: 0 additions & 39 deletions tests/lib/util/source-code.js
View file
Expand Up @@ -16,7 +16,6 @@ const fs = require("fs"),
leche = require("leche"),
Linter = require("../../../lib/linter"),
SourceCode = require("../../../lib/util/source-code"),
Traverser = require("../../../lib/util/traverser"),
astUtils = require("../../../lib/ast-utils");

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -1791,44 +1790,6 @@ 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.strictEqual(node.parent.type, "VariableDeclarator");
});

it("should not modify the node when attaching the parent", () => {
let node = sourceCode.getNodeByRangeIndex(10);

assert.strictEqual(node.type, "VariableDeclarator");
node = sourceCode.getNodeByRangeIndex(4);
assert.strictEqual(node.type, "Identifier");
assert.property(node, "parent");
assert.strictEqual(node.parent.type, "VariableDeclarator");
assert.notProperty(node.parent, "parent");
});

it("should use visitorKeys", () => {
const text = "a + b";
const ast = espree.parse(text, DEFAULT_CONFIG);

// no traverse BinaryExpression#left
sourceCode = new SourceCode({
text,
ast,
parserServices: null,
scopeManager: null,
visitorKeys: Object.assign({}, Traverser.DEFAULT_VISITOR_KEYS, {
BinaryExpression: ["right"]
})
});
const node = sourceCode.getNodeByRangeIndex(0);

assert.strictEqual(node.type, "BinaryExpression"); // This is Identifier if 'BinaryExpression#left' was traversed.
});

});

describe("isSpaceBetweenTokens()", () => {
Expand Down

0 comments on commit 4eaebe5

Please sign in to comment.