New: Add some parserServices #415

j-f1 · 2017-12-16T19:26:33Z

This lets you get the ts.Program instance for a file, as well as the TypeScript AST node corresponding to a particular node, and the type of any node.

JamesHenry · 2017-12-18T13:43:45Z

I have a branch from a year ago(!) which was looking to address this: https://github.com/eslint/typescript-eslint-parser/compare/parser-services

I think we should use that Map based implementation rather than adding things to every AST node, and I don't think it is necessary for the parser to concern itself with the type checker yet. Exposing the ts.Program gives plugin authors everything they need right? I think we should look to add the bare minimum that is useful (as this effectively a new public API we are supporting) and then keep an eye on the actual usage. WDYT?

j-f1 · 2017-12-18T17:59:05Z

Exposing the ts.Program gives plugin authors everything they need right?

As seen above in my PR to desktop/desktop, the current usage doesn’t properly handle types imported from other files, or from globals.d.ts files. I’m working on a better system, but it needs a patch in ESLint core to send the path of the file being parsed to the parser.

I think we should use that Map based implementation rather than adding things to every AST node.

👍 Will take a look at using Map/WeakMap.

j-f1 · 2018-01-05T13:37:15Z

WeakMap added. Sorry for the delay.

JamesHenry · 2018-01-12T14:27:32Z

parser.js

@@ -186,12 +210,15 @@ function generateAST(code, options, additionalParsingContext) {
 exports.version = require("./package.json").version;

 exports.parse = function parse(code, options) {
-    return generateAST(code, options, { isParseForESLint: false });


The name generateAST no longer makes sense for this function after this change

JamesHenry · 2018-01-12T14:30:19Z

lib/convert.js

@@ -58,6 +58,9 @@ module.exports = function convert(config) {
        loc: nodeUtils.getLoc(node, ast)
    };

+    nodeUtils.map.set(result, node);


I think these should come right before we return the result, that way you don't need to have two lots of them (i.e. there is no need to have them again in node-utils)

map and reverseMap are not clear names IMO

I set them when the nodes are created so that in e.g.

export class Foo {}

the ClassDeclaration and ExportNamedDeclaration have a TS AST node attached to them (ClassDeclaration).

JamesHenry · 2018-01-12T14:31:38Z

lib/node-utils.js

@@ -98,6 +98,12 @@ TOKEN_TO_TEXT[SyntaxKind.CaretEqualsToken] = "^=";
 TOKEN_TO_TEXT[SyntaxKind.AtToken] = "@";
 TOKEN_TO_TEXT[SyntaxKind.InKeyword] = "in";

+// ASTNode => TSNode


We refer to them as ESTreeNode throughout the codebase so please go for that

JamesHenry · 2018-01-12T14:32:05Z

lib/node-utils.js

@@ -553,6 +561,9 @@ function fixExports(node, result, ast) {
            loc: getLocFor(exportKeyword.getStart(), result.range[1], ast)
        };

+        map.set(newResult, node);


As per comment above I don't think these Maps need to be in node-utils at all

JamesHenry · 2018-01-12T14:34:02Z

parser.js

+    return {
+        program: result.program,
+        getTSNode: node => nodeUtils.map.get(node),
+        getESNode: tsNode => nodeUtils.reverseMap.get(tsNode),


Again, ESTreeNode is the name that has been used.

I'm a little unsure about the use-case for getESNode and reverseMap as they are currently called. I feel which should err on adding as little as possible to this API surface, particularly in this initial PR, we can always add more later

JamesHenry · 2018-01-12T14:34:39Z

parser.js

+        program: result.program,
+        getTSNode: node => nodeUtils.map.get(node),
+        getESNode: tsNode => nodeUtils.reverseMap.get(tsNode),
+        typeChecker,


As per my comment above about minimal API surface - why do we need a reference to the typeChecker, if we are already exposing the created Program?

JamesHenry · 2018-01-12T14:35:55Z

parser.js

+        getTSNode: node => nodeUtils.map.get(node),
+        getESNode: tsNode => nodeUtils.reverseMap.get(tsNode),
+        typeChecker,
+        getType(node) {


Helper functions like this could always be added later if we found that the demand is high, but again, we have already exposed everything you would need to get this yourself

I found it really useful when I was writing a rule using this — otherwise you have to do

context.parserServices.program.getTypeChecker().getTypeAtLocation(context.parserServices.getTSNode(node))

just to get the type of a node. It’s also arguably the most common use of the parser services.

JamesHenry · 2018-02-09T15:00:08Z

@j-f1 I am going to try and get a TS 2.7 release out ASAP, but do you think we could pick this up again after that. No worries, if you don't have any availability right now, just let me know.

j-f1 · 2018-02-09T15:46:00Z

I gave up on the rule I was writing this for, partially because there were some issues (I don’t remember exactly what) that prevented me from getting accurate type information.

j-f1 · 2018-02-09T15:53:08Z

I feel like the best way to implement this would be to use TS’s language service API.

kaicataldo · 2018-06-11T03:03:18Z

Is this something we still want to do?

j-f1 · 2018-06-11T10:23:28Z

I’m not working on the project I was going to use this for right now so it’s pretty far down my priority list, and I don’t think I’ll be able to work on this in the near future. Feel free to work off my branch though!

kaicataldo · 2018-06-11T14:14:31Z

Thanks for the update! Should we just close this for now and revisit this issue if it’s ever needed again?

j-f1 · 2018-06-11T14:29:22Z

Sounds good 👍

uniqueiniquity · 2018-08-02T20:36:38Z

@j-f1 @JamesHenry Do you know if anyone has been working on this recently? I'm on the TypeScript team, and we'd love to see if there's a way to get semantic information available for rules. If it's not already being tackled, I'd love to pick up this effort and see where we can get with it!

j-f1 · 2018-08-03T13:24:24Z

@uniqueiniquity Go ahead! It’d be really cool to see this implemented. Feel free to start from my branch (or create your own branch if you prefer).

New: Add some parserServices

b92dfa7

j-f1 changed the title ~~Add some parserServices~~ New: Add some parserServices Dec 16, 2017

j-f1 force-pushed the add-parser-services branch from b4b3f38 to b92dfa7 Compare December 16, 2017 19:29

j-f1 added 3 commits December 16, 2017 14:37

Commas exist for a reason

437f5c1

This is not the result

e779847

Make the original node non-enumerable so it doesn’t get logged

3298aec

j-f1 mentioned this pull request Dec 16, 2017

WIP: Add no-implicit-boolean-cast rule desktop/desktop#3550

Closed

4 tasks

Use WeakMaps to map between nodes

300f1ca

typo

6b2777f

JamesHenry suggested changes Jan 12, 2018

View reviewed changes

j-f1 closed this Jun 11, 2018

uniqueiniquity mentioned this pull request Oct 12, 2018

Offer semantic services alongside AST uniqueiniquity/typescript-estree#1

Merged

uniqueiniquity mentioned this pull request Nov 3, 2018

Offer semantic services alongside AST JamesHenry/typescript-estree#24

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Add some parserServices #415

New: Add some parserServices #415

j-f1 commented Dec 16, 2017

JamesHenry commented Dec 18, 2017

j-f1 commented Dec 18, 2017

j-f1 commented Jan 5, 2018

JamesHenry Jan 12, 2018

JamesHenry Jan 12, 2018

JamesHenry Jan 12, 2018

j-f1 Feb 9, 2018

JamesHenry Jan 12, 2018

JamesHenry Jan 12, 2018

JamesHenry Jan 12, 2018

JamesHenry Jan 12, 2018

JamesHenry Jan 12, 2018

j-f1 Feb 9, 2018

JamesHenry commented Feb 9, 2018

j-f1 commented Feb 9, 2018

j-f1 commented Feb 9, 2018

kaicataldo commented Jun 11, 2018

j-f1 commented Jun 11, 2018

kaicataldo commented Jun 11, 2018

j-f1 commented Jun 11, 2018

uniqueiniquity commented Aug 2, 2018

j-f1 commented Aug 3, 2018 •

edited

Loading

New: Add some parserServices #415

New: Add some parserServices #415

Conversation

j-f1 commented Dec 16, 2017

JamesHenry commented Dec 18, 2017

j-f1 commented Dec 18, 2017

j-f1 commented Jan 5, 2018

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

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

JamesHenry commented Feb 9, 2018

j-f1 commented Feb 9, 2018

j-f1 commented Feb 9, 2018

kaicataldo commented Jun 11, 2018

j-f1 commented Jun 11, 2018

kaicataldo commented Jun 11, 2018

j-f1 commented Jun 11, 2018

uniqueiniquity commented Aug 2, 2018

j-f1 commented Aug 3, 2018 • edited Loading

j-f1 commented Aug 3, 2018 •

edited

Loading