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
Update: Improve parser integrations (fixes #8392) #8755
Conversation
Note: this might break the current |
b29d205
to
fbd7552
Compare
Thanks for the pull request, @mysticatea! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
@mysticatea Thanks for working on this. The problem was a little more complicated than I originally envisioned. I like this solution. I think removing the dependency on estraverse is a good choice and maybe we could allow the traverser to be passed into eslint-scope and remove the extra dependency from there as well. Allowing a parser to pass in a scope anyalysis tool will solve the problem of monkey patching right away. Which is something that will benefit babel-eslint and typescirpt-eslint-parser and close several issues. I'm still a little concerned we are going to create a fork of eslint-scope that is used by both of those parser and have bug fixes that need to be done in all three forks, increasing the development burden. It might allow for development of a better scope analysis tool that does allow for plugins. Something I really want to work towards. Overall I think this will remove the monkey patch and will provide immediate benefit to custom parser. |
@hzoo @JamesHenry Pinging you two since you're the most knowledgeable about babel-eslint and typescript-eslint-parser, respectively. If you get a few minutes, would love to get your input! |
@mysticatea If we implement all this, is there anything else missing from us fully supporting babel-eslint and typescript-eslint-parser completely through the given API? Thanks for working on this! |
We can always get babel-eslint to provide this API before the ESLint release happens so that it won't break for anyone (this should happen in general really). And one way to know how well the API works is just to make a PR to babel-eslint targeting this version of ESLint and trying it out |
These are good news. I think I would prefer to statically export those things as they are not really part of the "result", and I do not think they should change between different compilations? So something like in the parser: export const VISITOR_KEYS = { ... }; Still the solution in this PR seems to be more flexible in case the visitor-keys in the parser are determined by options or the source. It would be super nice to also support changing the default |
Do we feel like this needs to be coupled to this issue? Or could we address this in a follow up PR? FWIW, this implementation seems fine to me. @eslint/eslint-team Any other opinions on this? I'd love to get this figured out so we can say we fully support TypeScript and can start updating babel-eslint to use the new API. |
Is this ready to be added to the TSC agenda (and hopefully approved), or are there more details that need to be worked out? |
I'd love to see this figured out! This should hopefully make the TS parser fully functional :) |
@eslint/eslint-team Can we get some more eyes/opinions on this? Seems like it's ready for TSC, but this would be hard to go through quickly during a meeting so it might make more sense for each of us to look at it asynchronously. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a great improvement! Thanks @mysticatea
lib/linter.js
Outdated
} | ||
|
||
// if espree failed to parse the file, there's no sense in setting up rules | ||
if (ast) { | ||
if (this.sourceCode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this include a check for this.sourceCode.ast
to match the previous behaviour?
We should make a PR in babel-eslint that uses this PR and implement it and see if that looks good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as @JamesHenry, but otherwise LGTM!
This proposal was accepted in today's TSC meeting. |
@not-an-aardvark @JamesHenry Could you review again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have anything further to add, great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small typos and a question, but otherwise this LGTM!
#### tainted | ||
|
||
* **Type:** `boolean` | ||
* **Description:** The `tained` flag. (I'm not sure what this means.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small typo: tained
-> tainted
#### taints | ||
|
||
* **Type:** `Map<string, boolean>` | ||
* **Description:** The map from variable names to `tained` flag. (I'm not sure what this means.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small typo: tained
-> tainted
Also, yeah, what does that mean? 😄
#### tainted | ||
|
||
* **Type:** `boolean` | ||
* **Description:** The `tained` flag. (I'm not sure what this means.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small typo: tained
-> tainted
lib/linter.js
Outdated
@@ -501,6 +504,28 @@ function getRuleOptions(ruleConfig) { | |||
|
|||
} | |||
|
|||
/** | |||
* Analyze scope of the given AST. | |||
* @param {ASTNoe} ast The `Program` node to analyze. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small typo: ASTNoe
-> ASTNode
lib/linter.js
Outdated
* Save all values that `parseForESLint()` returned. | ||
* If a `SourceCode` object is given as the first parameter instead of source code text, | ||
* linter skips the parsing process and reuses the source code object. | ||
* In that case, linter needs the all values that `parseForESLint()` returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small typo: needs the all
-> needs all the
lib/util/traverser.js
Outdated
this._enter(node, parent); | ||
|
||
if (!this._skipped && !this._broken) { | ||
const keys = this._visitorKeys[node.type] || Traverser.getKeys(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to have some kind of warning/error if keys don't exist for a node type? I can also see the argument for not doing that (so that it will still parse traverse even when it encounters an unknown node type), but failing to visit a node type silently could be undesirable.
Edit: typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it would cause a lot of noise for existing setups where people are using custom parsers without custom keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add an ESLint CLI/CLIEngine option to allow users to opt-in to warnings/errors in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That feels like a non-trivial amount of work for potentially very little benefit. I more just wanted to make sure we at least think about this and are aware in case something crops up. We can always revisit/add later too, if we feel like we're missing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can add debug("detect unknown node: %s", node.type)
message if people use with --debug
option.
tests/lib/util/traverser.js
Outdated
@@ -40,4 +40,51 @@ describe("Traverser", () => { | |||
assert.deepStrictEqual(enteredNodes, [fakeAst, fakeAst.body[0], fakeAst.body[1], fakeAst.body[1].foo]); | |||
assert.deepStrictEqual(exitedNodes, [fakeAst.body[0], fakeAst.body[1].foo, fakeAst.body[1], fakeAst]); | |||
}); | |||
|
|||
it("traverses AST as using 'keys' option if given", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be visitorKeys
instead of keys
?
Thank you for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few small comments, but these are looking great! None of these are blockers, so let me know if you decide not to make any changes and I need to re-review. Thanks!
* `ast` should contain the AST. | ||
* `services` can contain any parser-dependent services (such as type checkers for nodes). The value of the `services` property is available to rules as `context.parserServices`. Default is an empty object. | ||
* `scopeManager` can be a [ScopeManager](./scope-manager-interface.md) object. Custom parsers can use customized scope analysis for experimental/enhancement syntaxes. Default is the `ScopeManager` object which is created by [eslint-scope](https://github.com/eslint/eslint-scope). | ||
* `visitorKeys` can be an object to customize AST traversal. The keys of the object are the type of AST nodes. Each value is an array of the property names which should be traversed. Default is [KEYS of `eslint-visitor-keys`](https://github.com/eslint/eslint-scope). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link should point to https://github.com/eslint/eslint-visitor-keys).
tests/lib/linter.js
Outdated
let sourceCode = null; | ||
|
||
before(() => { | ||
scopeAnalyzeStub = sinon.spy(escope, "analyze"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to see a sinon sandbox used here, to make it easier to restore multiple stubs if we add to this set of tests; but I won't insist on it.
tests/lib/util/source-code.js
Outdated
node = sourceCode.getNodeByRangeIndex(0); | ||
assert.strictEqual(node.type, "Identifier"); | ||
|
||
// no traverse BinaryExpression#left |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this test be split in two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the former is just a check to make sure that visitorKeys
option changed the result. I replaced that with a comment.
tests/lib/util/traverser.js
Outdated
|
||
visited.splice(0, visited.length); | ||
|
||
// with keys option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this test be split in two? (Or, do we even need the first half of this test?)
…grations # Conflicts: # docs/developer-guide/working-with-plugins.md
Thank you for review! I updated this PR. |
What is the purpose of this pull request? (put an "X" next to item)
[X] Add something to the core
Fixes #8392.
What changes did you make? (Give an overview)
This is another implementation for #8392.
This would help to eliminate monkey patching from custom parsers.
This is based on my comment #8392 (comment).
In this PR,
parser.parse()
(orparseForESLint()
) method can providescope
andvisitorKeys
.scope
is provided, ESLint uses it instead of own scope analysis. This custom parser is an example which analyzes class decorators properly as using this feature.visitorKeys
is provided, ESLint traverses ASTs with it. Also ESLint analyzes scopes with it. This custom parser and this test are an example which uses this feature. FYI, ESLint was not able to handle decorators correctly with only this feature becauseeslint-scope
does not usevisitorKeys
to traverse classes.Additionally, this PR enhances
SourceCode
class to haveparserServices
,scope
, andvisitorKeys
. Because ifSourceCode
object is given tolinter.verify()
, ESLint reusesast
and does not parse code. In that case,parserServices
,scope
, andvisitorKeys
are going to lack. We have to use all ofast
,parserServices
,scope
, andvisitorKeys
as a set.Is there anything you'd like reviewers to focus on?
Let's discuss.