pzavolinsky / ts-unused-exports

ts-unused-exports finds unused exported symbols in your Typescript project
MIT License
727 stars 46 forks source link

TSLint or ESLint rule #19

Open felixfbecker opened 6 years ago

felixfbecker commented 6 years ago

I was confused that this is not implemented as a tslint rule. It would be great to have the editor integration, line/column data for errors, shareable configuration and autofixing of tslint for this.

vsviridov commented 5 years ago

AFAIK, tslint operates on one file at a time.

mrseanryan commented 4 years ago

Yes tslint rules only work on 1 file at a time.

So I don't see how this could be implemented.

Closing.

felixfbecker commented 4 years ago

Don’t tslint/typescript-eslint rules have access to the type checker which has the whole project loaded? Can that bot be used to find references?

pzavolinsky commented 4 years ago

We should definitely explore the linter possibility. Specially now that we have TS support in ESLint.

In raw TS, with createProgram we can access the TypeChecker. I'm not sure if that's enough to catch all usage instances but we could look into it.

mrseanryan commented 4 years ago

(re-opened)

With tslint my understanding is, it works on 1 file at a time, and will have access to whatever types that file uses. I don't think it works by first opening the whole project. So, I don't see how a tslint rule can determine if an export is unused, without false positives ...

However - eslint (eslint-typescript) is replacing tslint.

I don't know that much about how eslint works.

There is a Visitor pattern, and a rule can implement one or more of the visiting methods.

For example, this is a simple rule, that accepts visits from Node:

https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/src/rules/camelcase.ts

This is the same general pattern used by tslint rules.


brainstorming:

Is is possible that eslint-typescript can load a whole project, before evaluating each file?

OR if it's possible to have static state across 'rule runs', then it could be possible to check exports by sharing a 'used exports' state across runs. But this would require 2-passes: a collection pass, and then an evaluation pass.

A concern could be performance ...

mrseanryan commented 4 years ago

There is a discussion here at typescript-eslint

monfera commented 4 years ago

Yes tslint rules only work on 1 file at a time.

File boundaries seem convenient for linter implementation but otherwise a fairly arbitrary compartmentalization of linting, because file bounds have relatively little coincidence with data flow, calls etc.

pzavolinsky commented 4 years ago

I was reading a little bit about ESLint rules/plugins yesterday and I don't think we can access multiple files inside a rule. More specifically I found this: image

That being said, if we completely ignore that :point_up: then we might get away with something. If we (pre-)generate the analysis for the whole project and we could incrementally update/replace everything related to the current file, we might get away with a workable prototype.

Given that we start the POC by blatantly ignoring everything that is considered a good practice from the ESLint POV this might not be an ideal solution but it just might work.

A brutal first approach would be to run a a full ts-unused-exports analysis for each file. Performance will be laughable (specially in big projects) but we could quickly check if this as a viable solution. If it is, we could start thinking on incremental analysis (something that will be required to get this done).

felixfbecker commented 4 years ago

I'd recommend looking to integrate with typescript-eslint. typescript-eslint has many rules that use type information, which means it does have access to the whole project, because otherwise it wouldn't be able to make use of full type information (which needs resolving types from other files). This also means there is a way for rules to get a reference to the type checker/project, and hopefully that would be enough to implement this rule.

maneetgoyal commented 4 years ago

Hi all, will the new rule offer something better than the approach described here (https://github.com/typescript-eslint/typescript-eslint/issues/371#issuecomment-500927802). I understand that approach requires some configuration but apart from that, does this new rule seem to offer any other benefits?


Edit: Upon testing, that approach was catching unused-exports of functions and variables but not the interfaces.

mrseanryan commented 4 years ago

@maneetgoyal the approach in that comment seems more suited to eslint architecture. (Parser and Resolver).

But either way sounds like a rewrite

cscleison commented 3 years ago

You might try this https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-unused-modules.md

It worked like a charm for me. The extends config with "plugin:import/typescript" was essential for me, otherwise the lint would take forever and return nothing. If you do any absolute webpack config, check their docs for the resolvers.

Zamiell commented 3 years ago

Thanks @cscleison, that rule is working perfectly for me!

RE: "plugin:import/typescript" For people who are using the Airbnb TypeScript Config, then you don't have to do anything extra, it just works.