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.

Sign up for GitHub

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

Jump to bottom

feat!: move AST traversal into SourceCode #18167

Merged
merged 11 commits into from
Mar 21, 2024
Merged

feat!: move AST traversal into SourceCode #18167

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6365d91
feat: Implement SourceCode#traverse()
nzakas Aug 3, 2023
4d09c62
Add more debugging
nzakas Aug 9, 2023
5c6c7d4
Clean up sourcecode
nzakas Aug 10, 2023
12d7ebc
Try to fix code paths
nzakas Feb 21, 2024
3795c60
Fix rules and update migration guide
nzakas Mar 4, 2024
ce17554
Cache traversal steps
nzakas Mar 4, 2024
6c9ca84
Fix lint error
nzakas Mar 4, 2024
f70559f
Fix fuzz test
nzakas Mar 4, 2024
549c080
Simplify TraversalStep constructor
nzakas Mar 4, 2024
6495471
Remove unused parent arg
nzakas Mar 7, 2024
63ee6bd
Fix constructor-super
nzakas Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions docs/src/use/migrate-to-9.0.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ The lists below are ordered roughly by the number of users each change is expect
* [Removed multiple `context` methods](#removed-context-methods)
* [Removed `sourceCode.getComments()`](#removed-sourcecode-getcomments)
* [Removed `CodePath#currentSegments`](#removed-codepath-currentsegments)
* [Code paths are now precalculated](#codepath-precalc)
* [Function-style rules are no longer supported](#drop-function-style-rules)
* [`meta.schema` is required for rules with options](#meta-schema-required)
* [`FlatRuleTester` is now `RuleTester`](#flat-rule-tester)
Expand Down Expand Up @@ -465,6 +466,19 @@ ESLint v9.0.0 removes the deprecated `CodePath#currentSegments` property.

**Related Issues(s):** [#17457](https://github.com/eslint/eslint/issues/17457)

## <a name="codepath-precalc"></a> Code paths are now precalculated

Prior to ESLint v9.0.0, code paths were calculated during the same AST traversal used by rules, meaning that the information passed to methods like `onCodePathStart` and `onCodePathSegmentStart` was incomplete. Specifically, array properties like `CodePath#childCodePaths` and `CodePathSegment#nextSegments` began empty and then were filled with the appropriate information as the traversal completed, meaning that those arrays could have different elements depending on when you checked their values.

ESLint v9.0.0 now precalculates code path information before the traversal used by rules. As a result, the code path information is now complete regardless of where it is accessed inside of a rule.

**To address:** If you are accessing any array properties on `CodePath` or `CodePathSegment`, you'll need to update your code. Specifically:

* If you are checking the `length` of any array properties, ensure you are using relative comparison operators like `<`, `>`, `<=`, and `>=` instead of equals.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too general to understand the advice, I think we should leave just the second bullet (if you're accessing nextSegments and others, verify that your code will still work as expected).

Copy link
Member Author

@nzakas nzakas Mar 7, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the bugs I ran into when implementing this, where we were doing === 1 in one spot. I think it's worth calling this out because it took me a while to realize why that was a problem.

* If you are accessing the `nextSegments`, `prevSegments`, `allNextSegments`, or `allPrevSegments` properties on a `CodePathSegment`, or `CodePath#childCodePaths`, verify that your code will still work as expected. To be backwards compatible, consider moving the logic that checks these properties into `onCodePathEnd`.

**Related Issues(s):** [#16999](https://github.com/eslint/eslint/issues/16999)

## <a name="drop-function-style-rules"></a> Function-style rules are no longer supported

ESLint v9.0.0 drops support for function-style rules. Function-style rules are rules created by exporting a function rather than an object with a `create()` method. This rule format was deprecated in 2016.
Expand Down
1 change: 0 additions & 1 deletion lib/linter/code-path-analysis/code-path-analyzer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,6 @@ function forwardCurrentToHead(analyzer, node) {
: "onUnreachableCodePathSegmentStart";

debug.dump(`${eventName} ${headSegment.id}`);

CodePathSegment.markUsed(headSegment);
analyzer.emitter.emit(
eventName,
Expand Down
59 changes: 30 additions & 29 deletions lib/linter/linter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ const
} = require("@eslint/eslintrc/universal"),
Traverser = require("../shared/traverser"),
{ SourceCode } = require("../source-code"),
CodePathAnalyzer = require("./code-path-analysis/code-path-analyzer"),
applyDisableDirectives = require("./apply-disable-directives"),
ConfigCommentParser = require("./config-comment-parser"),
NodeEventGenerator = require("./node-event-generator"),
Expand All @@ -53,6 +52,8 @@ const commentParser = new ConfigCommentParser();
const DEFAULT_ERROR_LOC = { start: { line: 1, column: 0 }, end: { line: 1, column: 1 } };
const parserSymbol = Symbol.for("eslint.RuleTester.parser");
const { LATEST_ECMA_VERSION } = require("../../conf/ecma-version");
const STEP_KIND_VISIT = 1;
const STEP_KIND_CALL = 2;

//------------------------------------------------------------------------------
// Typedefs
Expand Down Expand Up @@ -937,22 +938,13 @@ function createRuleListeners(rule, ruleContext) {
* @param {string} physicalFilename The full path of the file on disk without any code block information
* @param {Function} ruleFilter A predicate function to filter which rules should be executed.
* @returns {LintMessage[]} An array of reported problems
* @throws {Error} If traversal into a node fails.
*/
function runRules(sourceCode, configuredRules, ruleMapper, parserName, languageOptions, settings, filename, disableFixes, cwd, physicalFilename, ruleFilter) {
const emitter = createEmitter();
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
});

// must happen first to assign all node.parent properties
const eventQueue = sourceCode.traverse();

/*
* Create a frozen object with the ruleContext properties and methods that are shared by all rules.
Expand Down Expand Up @@ -1082,25 +1074,34 @@ function runRules(sourceCode, configuredRules, ruleMapper, parserName, languageO
});
});

// only run code path analyzer if the top level node is "Program", skip otherwise
const eventGenerator = nodeQueue[0].node.type === "Program"
? new CodePathAnalyzer(new NodeEventGenerator(emitter, { visitorKeys: sourceCode.visitorKeys, fallback: Traverser.getKeys }))
: new NodeEventGenerator(emitter, { visitorKeys: sourceCode.visitorKeys, fallback: Traverser.getKeys });
const eventGenerator = new NodeEventGenerator(emitter, { visitorKeys: sourceCode.visitorKeys, fallback: Traverser.getKeys });

nodeQueue.forEach(traversalInfo => {
currentNode = traversalInfo.node;
for (const step of eventQueue) {
switch (step.kind) {
case STEP_KIND_VISIT: {
try {
if (step.phase === 1) {
eventGenerator.enterNode(step.target);
} else {
eventGenerator.leaveNode(step.target);
}
} catch (err) {
err.currentNode = step.target;
throw err;
}
break;
}

try {
if (traversalInfo.isEntering) {
eventGenerator.enterNode(currentNode);
} else {
eventGenerator.leaveNode(currentNode);
case STEP_KIND_CALL: {
emitter.emit(step.target, ...step.args);
break;
}
} catch (err) {
err.currentNode = currentNode;
throw err;

default:
throw new Error(`Invalid traversal step found: "${step.type}".`);
}
});

}

return lintingProblems;
}
Expand Down
49 changes: 34 additions & 15 deletions lib/rules/constructor-super.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,30 @@ function isPossibleConstructor(node) {
}
}

/**
* A class to store information about a code path segment.
*/
class SegmentInfo {

/**
* Indicates if super() is called in all code paths.
* @type {boolean}
*/
calledInEveryPaths = false;

/**
* Indicates if super() is called in any code paths.
* @type {boolean}
*/
calledInSomePaths = false;

/**
* The nodes which have been validated and don't need to be reconsidered.
* @type {ASTNode[]}
*/
validNodes = [];
}

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -159,12 +183,8 @@ module.exports = {
*/
let funcInfo = null;

/*
* {Map<string, {calledInSomePaths: boolean, calledInEveryPaths: boolean}>}
* Information for each code path segment.
* - calledInSomePaths: A flag of be called `super()` in some code paths.
* - calledInEveryPaths: A flag of be called `super()` in all code paths.
* - validNodes:
/**
* @type {Record<string, SegmentInfo>}
*/
let segInfoMap = Object.create(null);

Expand All @@ -174,7 +194,7 @@ module.exports = {
* @returns {boolean} The flag which shows `super()` is called in some paths
*/
function isCalledInSomePath(segment) {
return segment.reachable && segInfoMap[segment.id].calledInSomePaths;
return segment.reachable && segInfoMap[segment.id]?.calledInSomePaths;
}

/**
Expand All @@ -189,12 +209,12 @@ module.exports = {
* skip the segment.
* If not skipped, this never becomes true after a loop.
*/
if (segment.nextSegments.length === 1 &&
if (segment.nextSegments.length &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this introduces new false negatives. For example:

/* eslint constructor-super: "error" */

class C extends D {

    // no errors here
    constructor() {
        do {
            something();
        } while (foo);
    }

}

I'm not sure exactly what this condition was targeting. When we figure that out, it would be good to explain it better in the comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I comment out this condition altogether, these valid tests fails:

class A extends B { constructor(a) { super(); for (const b of a) { this.a(); } } }:

class A extends Object {
    constructor() {
        super();
        for (let i = 0; i < 0; i++);
    }
}

class A extends B {
    constructor(props) {
        super(props);

        try {
            let arr = [];        
            for (let a of arr) { 
            }
        } catch (err) {
        }
    }
}

So it seems like it's designed to catch loops that occur after super().

I couldn't figure out anything else about how this rule works, though, so any ideas would be greatly appreciated.

segment.nextSegments[0].isLoopedPrevSegment(segment)
) {
return true;
}
return segment.reachable && segInfoMap[segment.id].calledInEveryPaths;
return segment.reachable && segInfoMap[segment.id]?.calledInEveryPaths;
}

return {
Expand Down Expand Up @@ -278,11 +298,7 @@ module.exports = {
}

// Initialize info.
const info = segInfoMap[segment.id] = {
calledInSomePaths: false,
calledInEveryPaths: false,
validNodes: []
};
const info = segInfoMap[segment.id] = new SegmentInfo();

// When there are previous segments, aggregates these.
const prevSegments = segment.prevSegments;
Expand Down Expand Up @@ -326,7 +342,7 @@ module.exports = {
funcInfo.codePath.traverseSegments(
{ first: toSegment, last: fromSegment },
segment => {
const info = segInfoMap[segment.id];
const info = segInfoMap[segment.id] ?? new SegmentInfo();
const prevSegments = segment.prevSegments;

// Updates flags.
Expand All @@ -348,6 +364,9 @@ module.exports = {
});
}
}

// save just in case we created a new SegmentInfo object
segInfoMap[segment.id] = info;
}
);
},
Expand Down
37 changes: 28 additions & 9 deletions lib/rules/no-this-before-super.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,29 @@ function isConstructorFunction(node) {
);
}

/*
* Information for each code path segment.
* - superCalled: The flag which shows `super()` called in all code paths.
* - invalidNodes: The array of invalid ThisExpression and Super nodes.
*/
/**
*
*/
class SegmentInfo {

/**
* Indicates whether `super()` is called in all code paths.
* @type {boolean}
*/
superCalled = false;

/**
* The array of invalid ThisExpression and Super nodes.
* @type {ASTNode[]}
*/
invalidNodes = [];
}

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -64,13 +87,7 @@ module.exports = {
*/
let funcInfo = null;

/*
* Information for each code path segment.
* Each key is the id of a code path segment.
* Each value is an object:
* - superCalled: The flag which shows `super()` called in all code paths.
* - invalidNodes: The array of invalid ThisExpression and Super nodes.
*/
/** @type {Record<string, SegmentInfo>} */
let segInfoMap = Object.create(null);

/**
Expand All @@ -79,7 +96,7 @@ module.exports = {
* @returns {boolean} `true` if `super()` is called.
*/
function isCalled(segment) {
return !segment.reachable || segInfoMap[segment.id].superCalled;
return !segment.reachable || segInfoMap[segment.id]?.superCalled;
}

/**
Expand Down Expand Up @@ -285,7 +302,7 @@ module.exports = {
funcInfo.codePath.traverseSegments(
{ first: toSegment, last: fromSegment },
(segment, controller) => {
const info = segInfoMap[segment.id];
const info = segInfoMap[segment.id] ?? new SegmentInfo();

if (info.superCalled) {
controller.skip();
Expand All @@ -295,6 +312,8 @@ module.exports = {
) {
info.superCalled = true;
}

segInfoMap[segment.id] = info;
}
);
},
Expand Down
9 changes: 7 additions & 2 deletions lib/rules/no-useless-return.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,9 @@ module.exports = {
continue;
}

uselessReturns.push(...segmentInfoMap.get(segment).uselessReturns);
if (segmentInfoMap.has(segment)) {
uselessReturns.push(...segmentInfoMap.get(segment).uselessReturns);
}
}

return uselessReturns;
Expand Down Expand Up @@ -182,6 +184,10 @@ module.exports = {

const info = segmentInfoMap.get(segment);

if (!info) {
return;
}

info.uselessReturns = info.uselessReturns.filter(node => {
if (scopeInfo.traversedTryBlockStatements && scopeInfo.traversedTryBlockStatements.length > 0) {
const returnInitialRange = node.range[0];
Expand Down Expand Up @@ -275,7 +281,6 @@ module.exports = {
* NOTE: This event is notified for only reachable segments.
*/
onCodePathSegmentStart(segment) {

scopeInfo.currentSegments.add(segment);

const info = {
Expand Down