Implement "no-unused-import" rule

[FelixSchuSi]

I have tried to implement a "no-unused-import" rule and so far it has been going great. This is still work in progress — I have included a checklist at the bottom showing all the changes that need to be done.

An import statement is unused, when none of the component definitions that were loaded by this import statement are used. The dependency traversal now collects information about the import statement that triggered loading a component definition and stores this information in the dependencyStore. Currently, only side effect only imports (e. g.: import "./my-module") are evaluated.

TODO

• [x] Implement a codefix that removes the unused import statement.
• [x] Create tests for this rule.
• [x] Manually test if this rule works with the CLI and the VSCode plugin (so far it has only been tested as ts-plugin).
• [x] Fix bug where comments are reported as part of unused import statements.
• [x] Include the module specifier of the imported module in the message of the diagnostic.
• [x] Refactor some typescript types. Some types that were intorduced are hard to understand and rather confusing.
• [x] Find solution for the bug described below.
• [x] Add documentation.

[FelixSchuSi]

The rule behaves in a weird way in situations like this:

Note that foo.ts defines foo-element and bar.ts defines bar-element. maxProjectImportDepth is set to Infinity.

As you can see, warnings are created depending on the order of the import statements. This is because because import statements are evaluated from top to bottom. By the time bar.ts is evaluated, the file is already in the cache because it was already imported by foo.ts.

In my optinion it is fine to generate a warning for imports of a file that has already been loaded as a indirect dependecy. However, this should work independent of the order of import statements.

[FelixSchuSi]

Diagnostics are now created independent of the order of import statements. When two import statements are found which load the same file, only one is stored in the dependencyStore. The import statement with the higher depth value gets prefered.

[runem]

This looks awesome @FelixSchuSi !!

In order for you to test the VSCode plugin, you will have to run npm run copylink:watch and npm run watch from the project root. Next, you need to open a VSCode instance inside packages/vscode-lit-plugin and use the VSCode debugger. When you make a change to lit-analyzer you will have to restart the debugger :+1: You can enable verbose logging from VSCode and then run tail -f lit-plugin.log from the project that you test the plugin on (I don't know the equivalent command for Windows if you are running from that OS).

Be aware that I recently merged 1.2.0 into master, so you can safely rebase this PR on top of master and make that branch the target of this PR. I also fixed a caching problem for dependencies that you will need when you test in VSCode.

Have you thought about having a visitSourceFile hook for rules instead of visitImportStatement? I'm not sure if having getImportStatementsInFile in LitAnalyzer becomes too specific if it can be lifted into the "no-unused-import" rule. Having visitSourceFile might also open up for other SourceFile-related rules.

[FelixSchuSi]

Thank you for your help @runem! I like your idea of refactoring the "visitImportStatement" hook into a "visitSourceFile" hook. I will change that later today.

[FelixSchuSi]

Hey @runem,

I implemented all changes I had planned, here are some things you might want to look at:

"visitSourceFile" hook

As previously discussed, I refactored the "visitImportStatement" into a "visitSourceFile" hook. Let me know if this is the solution you had in mind.

Default config

I set the default configuration to "off", when strict mode is enabled it is set to "warning".

Documentation and tests

I tried to stay as close to the documentation and tests of the no-missing-import rule.

Strategy

In my initial implementation, an imported component definition could only be assigned to a single import statement. This strategy resulted in the bug described above. I played around with this strategy for a bit until I switched strategies so that a component definition can be assigned to multiple import statements. Bugs like the one described above should not occur anymore.

Let me know what you think and if there are any changes you would want me to make.

[ephjos]

@FelixSchuSi @runem Is it possible to get this rule merged in? This would be really useful, especially when used in conjunction with no-missing-imports.