Skip to content
This repository was archived by the owner on Jan 19, 2019. It is now read-only.

New: Add some parserServices #415

Closed
wants to merge 6 commits into from
Closed

New: Add some parserServices #415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b92dfa7
New: Add some parserServices
j-f1 Dec 16, 2017
437f5c1
Commas exist for a reason
j-f1 Dec 16, 2017
e779847
This is not the result
j-f1 Dec 16, 2017
3298aec
Make the original node non-enumerable so it doesn’t get logged
j-f1 Dec 16, 2017
300f1ca
Use WeakMaps to map between nodes
j-f1 Jan 5, 2018
6b2777f
typo
j-f1 Jan 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ module.exports = function convert(config) {
loc: nodeUtils.getLoc(node, ast)
};

nodeUtils.map.set(result, node);
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

map and reverseMap are not clear names IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

nodeUtils.reverseMap.set(node, result);

/**
* Copies the result object into an ESTree node with just a type property.
* This is used only for leaf nodes that have no other properties.
Expand Down
11 changes: 11 additions & 0 deletions lib/node-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,12 @@ TOKEN_TO_TEXT[SyntaxKind.CaretEqualsToken] = "^=";
TOKEN_TO_TEXT[SyntaxKind.AtToken] = "@";
TOKEN_TO_TEXT[SyntaxKind.InKeyword] = "in";

// ASTNode => TSNode
Copy link
Member

Choose a reason for hiding this comment

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

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

const map = new WeakMap();

// TSNode => ASTNode
const reverseMap = new WeakMap();

/**
* Find the first matching child based on the given sourceFile and predicate function.
* @param {TSNode} node The current TSNode
Expand Down Expand Up @@ -155,6 +161,8 @@ module.exports = {
* Expose the enum of possible TSNode `kind`s.
*/
SyntaxKind,
map,
reverseMap,
isAssignmentOperator,
isLogicalOperator,
getTextForTokenKind,
Expand Down Expand Up @@ -553,6 +561,9 @@ function fixExports(node, result, ast) {
loc: getLocFor(exportKeyword.getStart(), result.range[1], ast)
};

map.set(newResult, node);
Copy link
Member

Choose a reason for hiding this comment

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

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

reverseMap.set(node, newResult);

if (!declarationIsDefault) {
newResult.specifiers = [];
newResult.source = null;
Expand Down
39 changes: 33 additions & 6 deletions parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
const astNodeTypes = require("./lib/ast-node-types"),
ts = require("typescript"),
convert = require("./lib/ast-converter"),
semver = require("semver");
semver = require("semver"),
nodeUtils = require("./lib/node-utils");

const SUPPORTED_TYPESCRIPT_VERSIONS = require("./package.json").devDependencies.typescript;
const ACTIVE_TYPESCRIPT_VERSION = ts.version;
Expand Down Expand Up @@ -44,12 +45,14 @@ function resetExtra() {
// Parser
//------------------------------------------------------------------------------

/** @typedef {{ ast: Program, program: ts.Program }} Result */

/**
* Parses the given source code to produce a valid AST
* @param {mixed} code TypeScript code
* @param {Object} options configuration object for the parser
* @param {Object} additionalParsingContext additional internal configuration
* @returns {Object} the AST
* @returns {Result} the result
*/
function generateAST(code, options, additionalParsingContext) {
additionalParsingContext = additionalParsingContext || {};
Expand Down Expand Up @@ -176,7 +179,28 @@ function generateAST(code, options, additionalParsingContext) {
const ast = program.getSourceFile(FILENAME);

extra.code = code;
return convert(ast, extra);
return {
ast: convert(ast, extra),
program
};
}

/**
* Generate the `parserServices` object for a parse result
* @param {Result} result The return value of `generateAST`
* @returns {Object} The `parserServices` object
*/
function getServices(result) {
const typeChecker = result.program.getTypeChecker();
return {
program: result.program,
getTSNode: node => nodeUtils.map.get(node),
getESNode: tsNode => nodeUtils.reverseMap.get(tsNode),
Copy link
Member

Choose a reason for hiding this comment

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

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

typeChecker,
Copy link
Member

Choose a reason for hiding this comment

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

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?

getType(node) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return typeChecker.getTypeAtLocation(nodeUtils.map.get(node));
}
};
}

//------------------------------------------------------------------------------
Expand All @@ -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 });
Copy link
Member

Choose a reason for hiding this comment

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

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

return generateAST(code, options, { isParseForESLint: false }).ast;
};

exports.parseForESLint = function parseForESLint(code, options) {
const ast = generateAST(code, options, { isParseForESLint: true });
return { ast };
const result = generateAST(code, options, { isParseForESLint: true });
return {
ast: result.ast,
services: getServices(result)
};
};

// Deep copy.
Expand Down