Skip to content

Offer semantic services alongside AST #1

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

Merged
merged 25 commits into from
Oct 29, 2018

Conversation

uniqueiniquity
Copy link
Owner

@uniqueiniquity uniqueiniquity commented Oct 12, 2018

General organization of changes:

  • Added option to parser called project, which takes either a single project file path or an array of project file paths (top-level parser.js)
  • If given, program(s) are created from their corresponding project files (tsconfig-parser.js)
    • Changes to programs over the lifetime of the ESLint process (i.e. when changes are made to the file being linted with --fix or when ESLint is used as a server, as it is in VS) are tracked by the TS watch API.
  • During the existing TS AST traversal to build the ESTree AST, we build up two maps for a two-way mapping between TS AST nodes and ESTree AST nodes. (maps are built in convert.js, passed up through ast-converter.js to top-level).
    • References to the whole AST are provided in many places in convert.js to avoid having to ensure parent pointers are set in the TS AST (that is to say, for efficiency).
  • We return the program corresponding to the file being linted and the maps in addition to the AST (parser.js)

Note that the features added by this PR are entirely enabled or disabled (from the consumer's perspective) by the project option; the fallback behavior is the existing behavior.

I also added test coverage to ensure the two-way mapping is consistent (i.e. ts2es(es2ts(node)) = node) and that the exposed checker is usable as a rule might expect.

Map approach is based on eslint/typescript-eslint-parser#415

parser.js Outdated
if (shouldProvideParserServices) {
const FILENAME = options.filePath;
const programs = calculateProjectParserOptions(code, options, extra);
for (const program of programs) {

Choose a reason for hiding this comment

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

Is it possible for multiple programs to own the same file? Do you have to instead filter the project list down and then operate on all projects at once? Disclaimer: I don't know what the right workflow is. 😄

Copy link
Owner Author

Choose a reason for hiding this comment

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

Consider the following project structure:

  • tsconfig.json representing proj1, which includes export1.ts and import.ts
  • tsconfig.json representing proj2, which includes export2.ts and the same import.ts

where both export files define a namespace Blah with members foo that differ in type.
If foo is referenced in import.ts, then while find-all-ref points to all three sites, go-to-def and quick info just sorta... pick one.

I feel like this is the most analogous situation to what we have here, where multiple programs own the same file and would give different semantic information for a node in a file being linted. Therefore, I think it's appropriate for us here to just pick one program in their listed order in the eslint config file (and of course doc it as such).

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

It seems like we're calling ts.createProgram once per source file? That would be really bad for performance as createProgram will follow all imports and parse those too.

/**
* TODO: Remove dependency on private TypeScript method
*/
return ts.findNextToken(previousToken, parent);
Copy link

Choose a reason for hiding this comment

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

We should just make this public.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Note that the implementation I wrote differs very slightly (but importantly) from our implementation, so we should fix that too. But that's why I had to change it.

* @param {string} code The code being linted
* @param {Object} options Options provided by ESLint core
* @param {string} options.cwd The current working directory for the eslint process
* @param {string} options.filePath The path of the file being parsed
Copy link

Choose a reason for hiding this comment

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

So do we need to redo this for each file?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, currently each file iterates through the tsconfigs provided, grabs the corresponding programs from their cached watches, and then finds the right one to pass to the rules.

@uniqueiniquity
Copy link
Owner Author

@Andy-MS Unless I'm missing something, we shouldn't be doing ts.createProgram on every file linted, but rather once one is done, then we're just doing ts.WatchOfConfigFile.getProgram().getProgram() which I understood to be faster.

@ghost
Copy link

ghost commented Oct 22, 2018

@sheetalkamat would know more about performance.

@uniqueiniquity uniqueiniquity merged commit 5ada37d into semanticServices Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants