feat!: move AST traversal into SourceCode

[nzakas]

Prerequisites checklist

• I have read the contributing guidelines.

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

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

• Moved AST traversal out of Linter and into SourceCode#traverse().
• Updated no-useless-returns, constructor-super, and no-this-before-super to they still work. These rules were also tested against v8.57.0 to ensure that such changes would be backwards compatible.
• Updated the v9 migration guide

Refs #16999

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

I deviated from the RFC in how I represented the traversal steps. I realized while implementing that relying on type and phase as string comparisons would be a big performance hit, so I added the kind property as a number (keeping type for easier debugging, though not sure if that's needed) and switched phase to a number. That saves us a lot of string comparisons during traversal.

[netlify]

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	63ee6bd
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/65f8b2f67f0959000850f5ef

[nzakas]

TSC Summary: As part of this work, we are moving the AST traversal from Linter into SourceCode. For normal AST traversal, this works exactly the same way; for code path analysis, there is actually a difference. With the traversal in Linter, the code paths are incomplete during the traversal and are only completed upon complete traversal of the AST. That means CodePathSegment fields like nextSegments and prevSegments would be empty when onCodePathSegment is called with Linter traversal, whereas they would be filled with SourceCode traversal. Rules relying on the Linter-based traversal of incomplete code path data might not work correctly when all data is present.

In our codebase we have three rules that broke: no-useless-returns, no-this-before-super, and super-constructor.

TSC Question: This would be another breaking change for v9.0.0. Are we okay with that?

[mdjermanovic]

Questions:

1. Do these enterNode and leaveNode ever get called with the parent argument?
2. Are we using args property of type: "visit" steps anywhere?

[nzakas]

Good questions. I need to look into it.

[nzakas]

The answers are:

1. No, they don't, so I'll update that.
2. We aren't using args right now because everything is going through NodeEventGenerator, but I'd still like to keep these here as the foundation for the rewrite.

[mdjermanovic]

Are we sure that the current implementation of code path analysis doesn't expect parent property to be pre-set on all nodes (including not yet traversed ones)?

[nzakas]

I can only go by all of the tests that are passing. I'm assuming if there was a timing issue, that something would break.

[mdjermanovic]

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).

[nzakas]

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.

[mdjermanovic]

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.

[nzakas]

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.

[sam3k]

Per 2024-03-07 TSC meeting, this will be included in v9.

[mdjermanovic]

LGTM, thanks!