Skip to content
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

Implement "no-unused-import" rule #118

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
df65add
WIP: Extend dependency traversal to gather data for 'no-unused-import…
FelixSchuSi Jul 14, 2020
4904054
WIP: Implement 'no-missing-import' rule.
FelixSchuSi Jul 14, 2020
42797af
Implement import-analyzer.
FelixSchuSi Jul 15, 2020
9bc6c1b
WIP: try to solve bug by prefering indirect imports over direct imports
FelixSchuSi Jul 15, 2020
dfc0153
Solve Bug by prefering imports with higher depth.
FelixSchuSi Jul 15, 2020
d0275fd
Avoid additional loop.
FelixSchuSi Jul 15, 2020
3aa7019
Refactor types.
FelixSchuSi Jul 15, 2020
ecb8591
Inlcude path of module in diagnostic message.
FelixSchuSi Jul 15, 2020
4b8175f
Solve bug by allowing to emit files multipe times if they come from a…
FelixSchuSi Jul 16, 2020
4628975
Fix bug with circular dependencies; Refactor 'parse-dependencies.ts'.
FelixSchuSi Jul 20, 2020
e3af6be
Refactor 'import-analyzer' into 'sourcefile-analyzer'.
FelixSchuSi Jul 20, 2020
31e4064
Use importDeclaration objects instead of ranges.
FelixSchuSi Jul 20, 2020
97542fb
Fix bug where comments were reported as part of unused import stateme…
FelixSchuSi Jul 20, 2020
f73d6ea
Implement codefix that removes the unused import.
FelixSchuSi Jul 20, 2020
cee8f20
Fix test for recursive dependencies.
FelixSchuSi Jul 20, 2020
550572b
Add 'no-unused-import' option for vscode extension; Include trailing …
FelixSchuSi Jul 21, 2020
724a0cb
Fix bug where imports where falsely reported as unused.
FelixSchuSi Jul 21, 2020
32b1085
Add tests for 'no-unused-import' rule.
FelixSchuSi Jul 21, 2020
19bb32f
Add documentation for 'no-unused-import' rule.
FelixSchuSi Jul 21, 2020
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
22 changes: 22 additions & 0 deletions docs/readme/rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Each rule can have severity of `off`, `warning` or `error`. You can toggle rules
| :------ | ----------- | --------------- | --------------- |
| [no-unknown-tag-name](#-no-unknown-tag-name) | The existence of tag names are checked. Be aware that not all custom elements from libraries will be found out of the box. | off | warning |
| [no-missing-import](#-no-missing-import) | When using custom elements in HTML it is checked if the element has been imported and is available in the current context. | off | warning |
| [no-unused-import](#-no-unused-import) | When an import statement loads modules that define custom elements, it is checked whether any of the imported custom element definitions are used in the current file. | off | warning |
| [no-unclosed-tag](#-no-unclosed-tag) | Unclosed tags, and invalid self closing tags like custom elements tags, are checked. | warning | error |
| [no-missing-element-type-definition](#no-missing-element-type-definition) | This rule will ensure that custom elements are registered on the `HTMLElementTagNameMap` Typescript interface. | off | off |

Expand Down Expand Up @@ -95,6 +96,27 @@ import "my-element.js";
html`<my-element></my-element>`
```

#### 🚮 no-unused-import

When an import statement loads modules that define custom elements, it is checked whether any of the imported custom element definitions are used in the current file.
An import statement is considered unused if none of its imported custom element definitions are used in a `lit-html` template. Currently, only import statements without import clauses (e. g. `import "my-element.js"`) are evaluated.

The following example is considered a warning:

<!-- prettier-ignore -->
```js
import "my-element.js";
// "my-element" is not being used
```

The following example is not considered a warning:

<!-- prettier-ignore -->
```js
import "my-element.js";
html`<my-element></my-element>`
```

#### ☯ no-unclosed-tag

Unclosed tags, and invalid self closing tags like custom elements tags, are checked.
Expand Down
22 changes: 22 additions & 0 deletions packages/lit-analyzer/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ Each rule can have severity of `off`, `warning` or `error`. You can toggle rules
| :------ | ----------- | --------------- | --------------- |
| [no-unknown-tag-name](#-no-unknown-tag-name) | The existence of tag names are checked. Be aware that not all custom elements from libraries will be found out of the box. | off | warning |
| [no-missing-import](#-no-missing-import) | When using custom elements in HTML it is checked if the element has been imported and is available in the current context. | off | warning |
| [no-unused-import](#-no-unused-import) | When an import statement loads modules that define custom elements, it is checked whether any of the imported custom element definitions are used in the current file. | off | warning |
| [no-unclosed-tag](#-no-unclosed-tag) | Unclosed tags, and invalid self closing tags like custom elements tags, are checked. | warning | error |
| [no-missing-element-type-definition](#no-missing-element-type-definition) | This rule will ensure that custom elements are registered on the `HTMLElementTagNameMap` Typescript interface. | off | off |

Expand Down Expand Up @@ -168,6 +169,27 @@ import "my-element.js";
html`<my-element></my-element>`
```

#### 🚮 no-unused-import

When an import statement loads modules that define custom elements, it is checked whether any of the imported custom element definitions are used in the current file.
An import statement is considered unused if none of its imported custom element definitions are used in a `lit-html` template. Currently, only import statements without import clauses (e. g. `import "my-element.js"`) are evaluated.

The following example is considered a warning:

<!-- prettier-ignore -->
```js
import "my-element.js";
// "my-element" is not being used
```

The following example is not considered a warning:

<!-- prettier-ignore -->
```js
import "my-element.js";
html`<my-element></my-element>`
```

#### ☯ no-unclosed-tag

Unclosed tags, and invalid self closing tags like custom elements tags, are checked.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ export class DefaultLitAnalyzerContext implements LitAnalyzerContext {
}

private findDependenciesInFile(file: SourceFile) {
if (isRuleDisabled(this.config, "no-missing-import")) return;
if (isRuleDisabled(this.config, "no-missing-import") && isRuleDisabled(this.config, "no-unused-import")) return;

// Build a graph of component dependencies
const res = parseDependencies(file, this);
Expand Down
2 changes: 2 additions & 0 deletions packages/lit-analyzer/src/analyze/lit-analyzer-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export type LitAnalyzerRuleSeverity = "on" | "off" | "warn" | "warning" | "error
export type LitAnalyzerRuleId =
| "no-unknown-tag-name"
| "no-missing-import"
| "no-unused-import"
| "no-unclosed-tag"
| "no-unknown-attribute"
| "no-unknown-property"
Expand Down Expand Up @@ -37,6 +38,7 @@ export type LitAnalyzerRules = Partial<Record<LitAnalyzerRuleId, LitAnalyzerRule
const DEFAULT_RULES_SEVERITY: Record<LitAnalyzerRuleId, [LitAnalyzerRuleSeverity, LitAnalyzerRuleSeverity]> = {
"no-unknown-tag-name": ["off", "warn"],
"no-missing-import": ["off", "warn"],
"no-unused-import": ["off", "warn"],
"no-unclosed-tag": ["warn", "error"],
"no-unknown-attribute": ["off", "warn"],
"no-unknown-property": ["off", "warn"],
Expand Down
7 changes: 6 additions & 1 deletion packages/lit-analyzer/src/analyze/lit-analyzer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,13 @@ import { arrayFlat } from "./util/array-util";
import { getNodeAtPosition, nodeIntersects } from "./util/ast-util";
import { iterableFirst } from "./util/iterable-util";
import { makeSourceFileRange, sfRangeToDocumentRange } from "./util/range-util";
import { SourceFileAnalyzer } from "./sourcefile-analyzer/sourcefile-analyzer";

export class LitAnalyzer {
private litHtmlDocumentAnalyzer = new LitHtmlDocumentAnalyzer();
private litCssDocumentAnalyzer = new LitCssDocumentAnalyzer();
private componentAnalyzer = new ComponentAnalyzer();
private sourceFileAnalyzer = new SourceFileAnalyzer();

constructor(private context: LitAnalyzerContext) {
// Set the Typescript module
Expand Down Expand Up @@ -233,6 +235,9 @@ export class LitAnalyzer {
}
}

// Get general diagnostics for this file.
diagnostics.push(...this.sourceFileAnalyzer.getDiagnostics(this.context));

return diagnostics;
}

Expand Down Expand Up @@ -268,7 +273,7 @@ export class LitAnalyzer {
}
}

return [];
return this.sourceFileAnalyzer.getCodeFixesAtOffsetRange(sourceFileRange, this.context);
}

getFormatEditsInFile(file: SourceFile, settings: ts.FormatCodeSettings): LitFormatEdit[] {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,26 +1,30 @@
import { SourceFile } from "typescript";
import { SourceFile, ImportDeclaration } from "typescript";
import { ComponentDefinition } from "web-component-analyzer";
import { LitAnalyzerContext } from "../../lit-analyzer-context";
import { visitIndirectImportsFromSourceFile } from "./visit-dependencies";

export type ComponentDefinitionWithImport = { definition: ComponentDefinition; importDeclaration: ImportDeclaration | "rootSourceFile" };
export type SourceFileWithImport = { sourceFile: SourceFile; importDeclaration: ImportDeclaration | "rootSourceFile" };

// A cache used to prevent traversing through entire source files multiple times to find direct imports
const DIRECT_IMPORT_CACHE = new WeakMap<SourceFile, Set<SourceFile>>();

// Two caches used to return the result of of a known source file right away
const RESULT_CACHE = new WeakMap<SourceFile, ComponentDefinition[]>();
const IMPORTED_SOURCE_FILES_CACHE = new WeakMap<SourceFile, Set<SourceFile>>();
const RESULT_CACHE = new WeakMap<SourceFile, ComponentDefinitionWithImport[]>();
const IMPORTED_SOURCE_FILES_CACHE = new WeakMap<SourceFile, Set<SourceFileWithImport>>();

/**
* Returns a map of imported component definitions in each file encountered from a source file recursively.
* @param sourceFile
* @param context
*/
export function parseDependencies(sourceFile: SourceFile, context: LitAnalyzerContext): ComponentDefinition[] {
export function parseDependencies(sourceFile: SourceFile, context: LitAnalyzerContext): ComponentDefinitionWithImport[] {
if (RESULT_CACHE.has(sourceFile)) {
let invalidate = false;

// Check if the cache has been invalidated
for (const file of IMPORTED_SOURCE_FILES_CACHE.get(sourceFile) || []) {
for (const fileWithImport of IMPORTED_SOURCE_FILES_CACHE.get(sourceFile) || []) {
const { sourceFile: file } = fileWithImport;
// If we get a SourceFile with a certain fileName but it's not the same reference, the file has been updated
if (context.program.getSourceFile(file.fileName) !== file) {
invalidate = true;
Expand All @@ -41,10 +45,11 @@ export function parseDependencies(sourceFile: SourceFile, context: LitAnalyzerCo
IMPORTED_SOURCE_FILES_CACHE.set(sourceFile, importedSourceFiles);

// Get component definitions from all these source files
const definitions = new Set<ComponentDefinition>();
for (const file of importedSourceFiles) {
for (const def of context.definitionStore.getDefinitionsInFile(file)) {
definitions.add(def);
const definitions = new Set<ComponentDefinitionWithImport>();
for (const importedSourceFile of importedSourceFiles) {
const { sourceFile, importDeclaration } = importedSourceFile;
for (const definition of context.definitionStore.getDefinitionsInFile(sourceFile)) {
definitions.add({ definition, importDeclaration });
}
}

Expand All @@ -56,7 +61,7 @@ export function parseDependencies(sourceFile: SourceFile, context: LitAnalyzerCo
}

/**
* Returns a map of component declarations in each file encountered from a source file recursively.
* Returns a set of component declarations in each file encountered from a source file recursively.
* @param sourceFile
* @param context
* @param maxExternalDepth
Expand All @@ -66,26 +71,44 @@ export function parseAllIndirectImports(
sourceFile: SourceFile,
context: LitAnalyzerContext,
{ maxExternalDepth, maxInternalDepth }: { maxExternalDepth?: number; maxInternalDepth?: number } = {}
): Set<SourceFile> {
const importedSourceFiles = new Set<SourceFile>();

): Set<SourceFileWithImport> {
const importedSourceFiles = new Map<SourceFile, Set<ImportDeclaration | "rootSourceFile">>();
visitIndirectImportsFromSourceFile(sourceFile, {
project: context.project,
program: context.program,
ts: context.ts,
directImportCache: DIRECT_IMPORT_CACHE,
maxExternalDepth: maxExternalDepth ?? context.config.maxNodeModuleImportDepth,
maxInternalDepth: maxInternalDepth ?? context.config.maxProjectImportDepth,
emitIndirectImport(file: SourceFile): boolean {
if (importedSourceFiles.has(file)) {
return false;
}

importedSourceFiles.add(file);
emitIndirectImport(sourceFileWithImport: SourceFileWithImport): boolean {
const { sourceFile, importDeclaration } = sourceFileWithImport;

if (importedSourceFiles.has(sourceFile)) {
const importDeclarations = importedSourceFiles.get(sourceFile)!;
if (importDeclarations.has(importDeclaration) || importDeclarations.has("rootSourceFile")) {
// Sourcefile has already been visited from this importDeclaration or is the rootSourceFile.
return false;
} else {
// Sourcefile has already been visited from ANOTHER importDeclaration.
// Adding this importDeclaration to the Set of importDeclarations.
importDeclarations.add(importDeclaration);
importedSourceFiles.set(sourceFile, importDeclarations);
return true;
}
}
// Sourcefile has not been visited yet.
const importDeclarations = new Set<ImportDeclaration | "rootSourceFile">();
importDeclarations.add(importDeclaration);
importedSourceFiles.set(sourceFile, importDeclarations);
return true;
}
});

return importedSourceFiles;
const result = new Set<SourceFileWithImport>();
for (const [sourceFile, importDeclarations] of importedSourceFiles) {
for (const importDeclaration of importDeclarations) {
result.add({ sourceFile, importDeclaration });
}
}
return result;
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
import * as tsModule from "typescript";
import { Node, Program, SourceFile } from "typescript";
import { Node, Program, SourceFile, ImportDeclaration } from "typescript";
import { SourceFileWithImport } from "./parse-dependencies";

interface IVisitDependenciesContext {
program: Program;
ts: typeof tsModule;
project: ts.server.Project | undefined;
directImportCache: WeakMap<SourceFile, Set<SourceFile>>;
emitIndirectImport(file: SourceFile, importedFrom?: SourceFile): boolean;
emitDirectImport?(file: SourceFile): void;
emitIndirectImport(sourceFileWithImport: SourceFileWithImport): boolean;
emitDirectImport?(file: SourceFile, importDeclaration: ImportDeclaration): void;
depth?: number;
maxExternalDepth?: number;
maxInternalDepth?: number;
importDeclaration?: ImportDeclaration;
}

/**
Expand All @@ -21,9 +23,8 @@ interface IVisitDependenciesContext {
*/
export function visitIndirectImportsFromSourceFile(sourceFile: SourceFile, context: IVisitDependenciesContext): void {
const currentDepth = context.depth ?? 0;

// Emit a visit. If this file has been seen already, the function will return false, and traversal will stop
if (!context.emitIndirectImport(sourceFile)) {
if (!context.emitIndirectImport({ sourceFile, importDeclaration: context.importDeclaration ?? "rootSourceFile" })) {
return;
}

Expand All @@ -38,15 +39,17 @@ export function visitIndirectImportsFromSourceFile(sourceFile: SourceFile, conte

// Get all direct imports from the cache
let directImports = context.directImportCache.get(sourceFile);
const importDeclarations = new Map<SourceFile, ImportDeclaration>();

if (directImports == null) {
if (directImports == null || context.importDeclaration == null) {
// If the cache didn't have all direct imports, build up using the visitor function
directImports = new Set<SourceFile>();

const newContext = {
...context,
emitDirectImport(file: SourceFile) {
emitDirectImport(file: SourceFile, importDeclaration: ImportDeclaration) {
directImports!.add(file);
importDeclarations.set(file, importDeclaration);
}
};

Expand Down Expand Up @@ -91,9 +94,12 @@ export function visitIndirectImportsFromSourceFile(sourceFile: SourceFile, conte
newDepth--;
}

const importDeclaration = context.importDeclaration ?? importDeclarations.get(file);

// Visit direct imported source files recursively
visitIndirectImportsFromSourceFile(file, {
...context,
importDeclaration,
depth: newDepth
});
}
Expand Down Expand Up @@ -154,7 +160,8 @@ function emitDirectModuleImportWithName(moduleSpecifier: string, node: Node, con
const resolvedModule = result.resolvedModule;
const sourceFile = context.program.getSourceFile(resolvedModule.resolvedFileName);
if (sourceFile != null) {
context.emitDirectImport?.(sourceFile);
const importDeclaration = node as ImportDeclaration;
context.emitDirectImport?.(sourceFile, importDeclaration);
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions packages/lit-analyzer/src/analyze/rule-collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { HtmlNode, HtmlNodeKind } from "./types/html-node/html-node-types";
import { RuleDiagnostic } from "./types/rule/rule-diagnostic";
import { RuleModule, RuleModuleImplementation } from "./types/rule/rule-module";
import { RuleModuleContext } from "./types/rule/rule-module-context";
import { SourceFile } from "typescript";

export interface ReportedRuleDiagnostic {
source: LitAnalyzerRuleId;
Expand Down Expand Up @@ -133,6 +134,12 @@ export class RuleCollection {

return diagnostics;
}

getDiagnosticsFromSourceFile(sourceFile: SourceFile, baseContext: LitAnalyzerContext): ReportedRuleDiagnostic[] {
const diagnostics: ReportedRuleDiagnostic[] = [];
this.invokeRules("visitSourceFile", sourceFile, d => diagnostics.push(d), baseContext);
return diagnostics;
}
}

function getPriorityValue(rule: RuleModule): number {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { LitAnalyzerContext } from "../lit-analyzer-context";
import { ReportedRuleDiagnostic } from "../rule-collection";
import { LitDiagnostic } from "../types/lit-diagnostic";
import { convertRuleDiagnosticToLitDiagnostic } from "../util/rule-diagnostic-util";
import { LitCodeFix } from "../types/lit-code-fix";
import { arrayDefined, arrayFlat } from "../util/array-util";
import { converRuleFixToLitCodeFix } from "../util/rule-fix-util";
import { Range } from "../types/range";
import { intersects } from "../util/range-util";

export class SourceFileAnalyzer {
getDiagnostics(context: LitAnalyzerContext): LitDiagnostic[] {
return this.getRuleDiagnostics(context).map(diagnostic => convertRuleDiagnosticToLitDiagnostic(diagnostic, context));
}

getCodeFixesAtOffsetRange(range: Range, context: LitAnalyzerContext): LitCodeFix[] {
return arrayFlat(
arrayDefined(
this.getRuleDiagnostics(context)
.filter(({ diagnostic }) => intersects(range, diagnostic.location))
.map(({ diagnostic }) => diagnostic.fix?.())
)
).map(ruleFix => converRuleFixToLitCodeFix(ruleFix));
}

private getRuleDiagnostics(context: LitAnalyzerContext): ReportedRuleDiagnostic[] {
return context.rules.getDiagnosticsFromSourceFile(context.currentFile, context);
}
}
Original file line number Diff line number Diff line change
@@ -1,20 +1,7 @@
import { ImportDeclaration } from "typescript";
import { ComponentDefinition } from "web-component-analyzer";

export interface AnalyzerDependencyStore {
hasTagNameBeenImported(fileName: string, tagName: string): boolean;
getImportedComponentDefinitionsByImportDeclaration(importDeclaration: ImportDeclaration): ComponentDefinition[];
}

//importedComponentDefinitionsInFile = new Map<string, ComponentDefinition[]>();

/**
* Returns if a component for a specific file has been imported.
* @param fileName
* @param tagName
*/
/*hasTagNameBeenImported(fileName: string, tagName: string): boolean {
for (const file of this.importedComponentDefinitionsInFile.get(fileName) || []) {
if (file.tagName === tagName) {
return true;
}
}

return false;
}*/
Loading