Update: Improve parser integrations (fixes #8392) #8755

mysticatea · 2017-06-17T14:05:58Z

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

[X] Add something to the core

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() (or parseForESLint()) method can provide scope and visitorKeys.

If 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.
If 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 because eslint-scope does not use visitorKeys to traverse classes.

Additionally, this PR enhances SourceCode class to have parserServices, scope, and visitorKeys. Because if SourceCode object is given to linter.verify(), ESLint reuses ast and does not parse code. In that case, parserServices, scope, and visitorKeys are going to lack. We have to use all of ast, parserServices, scope, and visitorKeys as a set.

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

Let's discuss.

mysticatea · 2017-06-17T14:06:48Z

Note: this might break the current babel-eslint behavior.

eslintbot · 2017-06-17T16:06:11Z

Thanks for the pull request, @mysticatea! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

soda0289 · 2017-06-17T18:23:54Z

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

kaicataldo · 2017-06-17T21:44:20Z

@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!

kaicataldo · 2017-06-20T04:28:45Z

@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!

hzoo · 2017-06-20T14:46:08Z

cc @danez @zertosh

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

danez · 2017-06-20T14:57:31Z

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 parserOptions as mentioned in #8744, which most probably would not work this way, as the options are needed earlier, before the actual parsing.

kaicataldo · 2017-06-29T21:16:33Z

It would be super nice to also support changing the default parserOptions as mentioned in #8744, which most probably would not work this way, as the options are needed earlier, before the actual parsing.

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.

not-an-aardvark · 2017-07-17T04:57:56Z

Is this ready to be added to the TSC agenda (and hopefully approved), or are there more details that need to be worked out?

kaicataldo · 2017-08-05T16:13:54Z

I'd love to see this figured out! This should hopefully make the TS parser fully functional :)

kaicataldo · 2017-08-07T15:12:25Z

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

JamesHenry

This seems like a great improvement! Thanks @mysticatea

JamesHenry · 2017-08-07T16:46:00Z

lib/linter.js

        }

        // if espree failed to parse the file, there's no sense in setting up rules
-        if (ast) {
+        if (this.sourceCode) {


Should this include a check for this.sourceCode.ast to match the previous behaviour?

hzoo · 2017-08-07T18:21:23Z

We should make a PR in babel-eslint that uses this PR and implement it and see if that looks good

kaicataldo

Same question as @JamesHenry, but otherwise LGTM!

not-an-aardvark · 2017-12-08T03:05:37Z

This proposal was accepted in today's TSC meeting.

mysticatea · 2017-12-09T07:00:23Z

@not-an-aardvark @JamesHenry Could you review again?

JamesHenry

I don't have anything further to add, great work!

kaicataldo

A few small typos and a question, but otherwise this LGTM!

kaicataldo · 2017-12-12T19:27:48Z

docs/developer-guide/scope-manager-interface.md

+#### tainted
+
+* **Type:** `boolean`
+* **Description:** The `tained` flag. (I'm not sure what this means.)


Small typo: tained -> tainted

kaicataldo · 2017-12-12T19:30:50Z

docs/developer-guide/scope-manager-interface.md

+#### taints
+
+* **Type:** `Map<string, boolean>`
+* **Description:** The map from variable names to `tained` flag. (I'm not sure what this means.)


Small typo: tained -> tainted

Also, yeah, what does that mean? 😄

kaicataldo · 2017-12-12T19:32:00Z

docs/developer-guide/scope-manager-interface.md

+#### tainted
+
+* **Type:** `boolean`
+* **Description:** The `tained` flag. (I'm not sure what this means.)


Small typo: tained -> tainted

kaicataldo · 2017-12-12T19:33:36Z

lib/linter.js

@@ -501,6 +504,28 @@ function getRuleOptions(ruleConfig) {

 }

+/**
+ * Analyze scope of the given AST.
+ * @param {ASTNoe} ast The `Program` node to analyze.


Small typo: ASTNoe -> ASTNode

kaicataldo · 2017-12-12T19:39:46Z

lib/linter.js

+             * 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.


Small typo: needs the all -> needs all the

kaicataldo · 2017-12-12T19:53:20Z

lib/util/traverser.js

+        this._enter(node, parent);
+
+        if (!this._skipped && !this._broken) {
+            const keys = this._visitorKeys[node.type] || Traverser.getKeys(node);


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

This seems like it would cause a lot of noise for existing setups where people are using custom parsers without custom keys.

Could we add an ESLint CLI/CLIEngine option to allow users to opt-in to warnings/errors in this case?

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.

I think that we can add debug("detect unknown node: %s", node.type) message if people use with --debug option.

kaicataldo · 2017-12-12T19:59:42Z

tests/lib/util/traverser.js

@@ -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", () => {


Should this be visitorKeys instead of keys?

mysticatea · 2017-12-14T09:58:05Z

Thank you for the review!
I updated this PR.

platinumazure

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!

platinumazure · 2017-12-15T13:24:44Z

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

+* `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).


This link should point to https://github.com/eslint/eslint-visitor-keys).

platinumazure · 2017-12-15T14:35:42Z

tests/lib/linter.js

+                let sourceCode = null;
+
+                before(() => {
+                    scopeAnalyzeStub = sinon.spy(escope, "analyze");


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.

platinumazure · 2017-12-15T14:46:46Z

tests/lib/util/source-code.js

+            node = sourceCode.getNodeByRangeIndex(0);
+            assert.strictEqual(node.type, "Identifier");
+
+            // no traverse BinaryExpression#left


Could this test be split in two?

Actually, the former is just a check to make sure that visitorKeys option changed the result. I replaced that with a comment.

platinumazure · 2017-12-15T14:49:53Z

tests/lib/util/traverser.js

+
+        visited.splice(0, visited.length);
+
+        // with keys option.


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

mysticatea · 2017-12-18T10:52:37Z

Thank you for review!

I updated this PR.
And I resolved conflicts.

All comments were fixed

mysticatea added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 17, 2017

mysticatea mentioned this pull request Jun 17, 2017

New: Allow parsers to supply Visitor Keys/Fallback (fixes #8392) #8582

Closed

mysticatea added 3 commits June 18, 2017 00:57

rewrite traverser

a8fa8dd

add supports scope and visitorKeys of custorm parsers

6859007

add tests

fbd7552

mysticatea force-pushed the improve-parser-integrations branch from b29d205 to fbd7552 Compare June 17, 2017 16:06

eslint deleted a comment from eslintbot Jun 17, 2017

kaicataldo requested review from hzoo and JamesHenry June 17, 2017 21:43

kaicataldo added tsc agenda This issue will be discussed by ESLint's TSC at the next meeting and removed tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels Jun 21, 2017

not-an-aardvark mentioned this pull request Jul 20, 2017

ESLint v5 Wish List #8972

Closed

JamesHenry reviewed Aug 7, 2017

View reviewed changes

kaicataldo mentioned this pull request Aug 7, 2017

Typescript Support babel/babel-eslint#505

Closed

kaicataldo approved these changes Aug 7, 2017

View reviewed changes

not-an-aardvark self-requested a review August 7, 2017 19:45

mysticatea removed the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Dec 8, 2017

JamesHenry mentioned this pull request Dec 11, 2017

New: parseForESLint method and parser services eslint/typescript-eslint-parser#94

Closed

JamesHenry approved these changes Dec 11, 2017

View reviewed changes

not-an-aardvark approved these changes Dec 12, 2017

View reviewed changes

kaicataldo approved these changes Dec 12, 2017

View reviewed changes

mysticatea added 2 commits December 14, 2017 18:19

update for review

530b082

fix some links to escope docs

6bed81e

platinumazure previously requested changes Dec 15, 2017

View reviewed changes

mysticatea added 2 commits December 18, 2017 19:42

update for review

a2e4a83

Merge remote-tracking branch 'origin/master' into improve-parser-inte…

c5ceadb

…grations # Conflicts: # docs/developer-guide/working-with-plugins.md

upgrade eslint-visitor-keys to 1.0.0

de2c9b0

ilyavolodin approved these changes Dec 19, 2017

View reviewed changes

ilyavolodin merged commit 1c2aafd into master Dec 20, 2017

not-an-aardvark deleted the improve-parser-integrations branch December 20, 2017 03:20

renovate bot mentioned this pull request Feb 14, 2018

Update dependency eslint to v4 Urigo/tortilla#62

Merged

eslint-deprecated bot locked and limited conversation to collaborators Jun 19, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 19, 2018

Update: Improve parser integrations (fixes #8392) #8755

Update: Improve parser integrations (fixes #8392) #8755

Conversation

mysticatea commented Jun 17, 2017 • edited

mysticatea commented Jun 17, 2017

eslintbot commented Jun 17, 2017

soda0289 commented Jun 17, 2017

kaicataldo commented Jun 17, 2017

kaicataldo commented Jun 20, 2017

hzoo commented Jun 20, 2017 • edited

danez commented Jun 20, 2017 • edited

kaicataldo commented Jun 29, 2017 • edited

not-an-aardvark commented Jul 17, 2017

kaicataldo commented Aug 5, 2017

kaicataldo commented Aug 7, 2017

JamesHenry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzoo commented Aug 7, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

not-an-aardvark commented Dec 8, 2017

mysticatea commented Dec 9, 2017

JamesHenry left a comment

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo Dec 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mysticatea commented Dec 14, 2017

platinumazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mysticatea commented Dec 18, 2017

mysticatea commented Jun 17, 2017 •

edited

hzoo commented Jun 20, 2017 •

edited

danez commented Jun 20, 2017 •

edited

kaicataldo commented Jun 29, 2017 •

edited

kaicataldo Dec 12, 2017 •

edited