microsoft / vscode-eslint

VSCode extension to integrate eslint into VSCode
MIT License
1.74k stars 336 forks source link

ESLint does not re-compute cross-file information on file changes #1774

Open JoshuaKGoldberg opened 9 months ago

JoshuaKGoldberg commented 9 months ago

There's a known issue with typescript-eslint that modifying one file doesn't impact the view of that file's types seen by other files:

Copying that issue's explanation here: @typescript-eslint/parser sets up a TypeScript program behind-the-scenes when it parses files. The program for a file is later made on ESLintUtils.getParserServicescontext).program. This is documented in Custom Rules > Typed Rules. That program is what's used for type information in typed lint rules.

One known issue with the parser-generating-type-information strategy is that programs are only recreated when ESLint re-parses. I'm not sure how best to solve this on the editor side, so asking for help here 🙂. Would it be reasonable for vscode-eslint to trigger an ESLint re-parse whenever a file is modified on disk?

Edit: summarizing discussion below, this is not limited to just typescript-eslint's type-checked rules. Any lint rule that uses cross-file information is impacted. That includes plugins like eslint-plugin-import / eslint-plugin-import-x.

dbaeumer commented 9 months ago

@JoshuaKGoldberg thanks for reaching out to me.

Would it be reasonable for vscode-eslint to trigger an ESLint re-parse whenever a file is modified on disk

How does that re-parse API look like. What I would like to avoid is that the ESLint server needs to know about dependencies in TS files.

dbaeumer commented 9 months ago

And would there be API for me to find out (on the ESLint level) if the parser has TS type information loaded to avoid this in cases where it is not necessary.

JoshuaKGoldberg commented 9 months ago

How does that re-parse API look like

Looking at interface ESLintClass and its await eslintClass.lintText usage, I'm thinking calling the same validate(...) should be sufficient? Is that what you mean?

avoid this in cases where it is not necessary

Yeah the difficulty there is in theory any file can cause this issue. A global.d.ts that augments a global type, for example, could impact any other file in the project. In order to know about TypeScript augmentations you'd have to talk to the TypeScript language service (guessing: not doable?)... but even that wouldn't be enough for everyone:

I can think of a few directions...

dbaeumer commented 9 months ago

A reasonable approach would be like this:

A "correct" approach would be like this:

This would be the most effective implementation since it would only revalidate files if the TS program has changed.

Let me know if you think the second approach would be doable for the plugin.

JoshuaKGoldberg commented 9 months ago

API for me to find out (on the ESLint level) if the parser has TS type information find out whether the plugin has TS symbol information

Theoretically yes: the resolved ESLint parserOptions for the file can be checked to determine if project or EXPERIMENTAL_useProjectService are truthy. That would determine whether typical TypeScript files should be re-linted.

But that wouldn't fix things for the users of other multi-file-linting parsers mentioned in https://github.com/microsoft/vscode-eslint/issues/1774#issuecomment-1906234646. That list is non-exhaustive: I also remembered n/no-missing-import and n/no-missing-require suffer from this issue.

have an action that enforces re-validation of all files

Would this be noticeably better than the existing Restart ESLint Server action? That one is pretty fast in my experience. And most of the slowness folks experience from expensive linting occurs from out-of-file analysis that would happen in both commands.

the TypeScript ESLint plugin runs the TypeScript language service in watch mode

I think this watch mode is theoretically doable. Two difficulties:

the TypeScript ESLint plugin provides a way to notify about such a change and then the server could re-validate the files

I suppose this is possible, with more of the same two difficulties noted in the previous session.


cc @bradzacher - who has often had a superset of my knowledge about these workings.

ljharb commented 9 months ago

This is relevant to eslint-plugin-import, since editing any file could cause any other file to become invalid. Tightly integrating the concept of “vscode” or “a language server” into individual eslint plugins (that shouldn’t ever need to be aware of such concepts) doesn’t seem like a viable or maintainable approach to me, altho ofc I’m willing to make changes if that would solve the problem.

bradzacher commented 9 months ago

for TS it would be cool if we could find out whether the plugin has TS symbol information

calculateConfigForFile will give you back the ESLint config - then really just need to check if parserOptions.project is truthy.

We can build and maintain a util for you (eg pass a config and we return a boolean) to avoid you relying on our implementation details.

That being said - as Jordan mentioned there are more usecases for "refresh" beyond type info in the eslint ecosystem so tying it to one subset is probs more limited than we'd ultimately want.


FWIW I've asked eslint for a way to register file dependency at lint time and the response was essentially "not until we rewrite eslint". So we can either build a paradigm outside their APIs, or we can wait (probably another few years). The former is burden for us (or mostly the IDE extensions), yes, but would fix the DevX that everyone currently hates and complains about to us - a lot.

bradzacher commented 9 months ago

the TypeScript ESLint plugin runs the TypeScript language service in watch mode

This wouldn't help us. We already do this really - just using the lower-level APIs. But we don't attach file watchers because eslint provides no mechanism for us to know when the "lint run is done" - so we don't know when to detach the watchers and if we don't do that the ESLint CLI never exits.

But regardless - the server in watch mode doesn't really provide us with any wins because we don't coordinate the lint run - eslint does. Us recalculating types eagerly doesn't do anything unless a lint run is done.

Once the user saves a file it'll trigger a fresh lint run on the file which will make eslint call us and we'll recalculate the types. After that if you "re lint all open files" they should be mostly free to lint because we don't need to recalculate types.

dbaeumer commented 9 months ago

Thanks for all the comments. Reading through them shows that there isn't an easy solution to fix this since none of the participants has enough information right now to do the proper calculation whether a reparse is necessary or not. To summarize things:

I also understand that not every rule want to build a dependency tree and have file watchers to determine this itself. Since VS Code as a file change and watcher capabilities it would be OK for me if the extension does the re-linting however I would like to request some more support from ESLint as well. Ideal would a solution where the ESLint extension could find out for every rule if it has inter file dependencies or not. Does anyone know if something is possible today? I haven't found an API to get to a rule object. If something like this is not possible I think we need to talk to the ESLint maintainers to see if something like this could be added.

What I could do is the following:

Any additional thought?

bradzacher commented 9 months ago

In terms of eslint building something - we may be on our own for now, at least until eslint completes it's rewrite. Based on past discussions it sounds like they don't want to make any changes to the core until the rewrite.

Refs:

bradzacher commented 9 months ago
  • assume inter file dependencies for TypeScript
  • re-lint a file on focus again. This will at least remove an error as soon as a file gets focused and doesn't need additional user action.

I'd be okay with either of these. Though the latter wouldn't clear the problems pane - at least it would clear as soon as the user focuses the file.


Always re-linting all files is bad as well since it is IMO not necessary for 90% of the rules and will have bad performance impacts.

If we're talking re-linting open files I'd have thought most user sessions would have at most a dozen files up at any given time? (I have no idea - pulling a number out by butt based on how I use it) so a full session lint should be fast - esp considering most files don't need type info re-calculated and just need the lint rules to run. But regardless you're not wrong - linting the entire session on file change would be ridiculously slow for the user.

What about if we did it just asynchronously on save instead? Is that doable? Idk if vscode exposes the signals to do that.

dbaeumer commented 9 months ago

What about if we did it just asynchronously on save instead? Is that doable? Idk if vscode exposes the signals to do that.

VS Code has auto save and it is on for a lot of users. For them this would result in some performance problems as well.

bradzacher commented 9 months ago

Could we do a "throttle" on the event? EG trigger a "session" lint on save but no more than once every 10s, and it is run asynchronously after the current file has been linted to completion (to ensure the type info is recalculated).

If we pair that with "re lint file on focus if a change has occurred since the file was last linted" then we could have good coverage of everything without being too expensive:

The trade-off here is implementation complexity, of course.

If we could make the extension aware of the explicit dependencies of a given file then we'd be well off - though IIRC it's actually pretty hard to get this information performantly out of the ts.Program.