Skip to content

refactor(compiler-cli): detect unused standalone imports in local arrays #66620

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.

Sign up for GitHub

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

Open
SkyZeroZx wants to merge 2 commits into
base: main
Choose a base branch
from
+426 −44
Open

refactor(compiler-cli): detect unused standalone imports in local arrays #66620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74ca851
refactor(compiler-cli): detect unused standalone imports in local arrays
SkyZeroZx Jan 19, 2026
13c7e6a
fixup! refactor(compiler-cli): detect unused standalone imports in lo…
SkyZeroZx Feb 10, 2026
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
247 changes: 211 additions & 36 deletions packages/compiler-cli/src/ngtsc/validation/src/rules/unused_standalone_imports_rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,23 +62,25 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule {
return null;
}

const unused = this.getUnusedSymbols(
metadata,
new Set(usedDirectives.map((dir) => dir.ref.node as ts.ClassDeclaration)),
new Set(usedPipes),
const usedDirectivesSet = new Set(
usedDirectives.map((dir) => dir.ref.node as ts.ClassDeclaration),
);

if (unused === null) {
return null;
}
const usedPipesSet = new Set(usedPipes);

const unused = this.getUnusedSymbols(metadata, usedDirectivesSet, usedPipesSet);

const propertyAssignment = closestNode(metadata.rawImports, ts.isPropertyAssignment);
const category =
this.typeCheckingConfig.unusedStandaloneImports === 'error'
? ts.DiagnosticCategory.Error
: ts.DiagnosticCategory.Warning;

if (unused.length === metadata.imports.length && propertyAssignment !== null) {
if (
unused !== null &&
unused.length === metadata.imports.length &&
propertyAssignment !== null
) {
return makeDiagnostic(
ErrorCode.UNUSED_STANDALONE_IMPORTS,
propertyAssignment.name,
Expand All @@ -88,20 +90,37 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule {
);
}

return unused.map((ref) => {
const diagnosticNode =
ref.getIdentityInExpression(metadata.rawImports!) ||
ref.getIdentityIn(node.getSourceFile()) ||
metadata.rawImports!;
const diagnostics: ts.Diagnostic[] = [];

return makeDiagnostic(
ErrorCode.UNUSED_STANDALONE_IMPORTS,
diagnosticNode,
`${ref.node.name.text} is not used within the template of ${metadata.name}`,
undefined,
category,
);
});
const localArrayDiagnostics = this.checkLocalArraysAllUnused(
metadata,
usedDirectivesSet,
usedPipesSet,
category,
);

diagnostics.push(...localArrayDiagnostics);

if (unused !== null) {
for (const ref of unused) {
const diagnosticNode =
ref.getIdentityInExpression(metadata.rawImports!) ||
ref.getIdentityIn(node.getSourceFile()) ||
metadata.rawImports!;

diagnostics.push(
makeDiagnostic(
ErrorCode.UNUSED_STANDALONE_IMPORTS,
diagnosticNode,
`${ref.node.name.text} is not used within the template of ${metadata.name}`,
undefined,
category,
),
);
}
}

return diagnostics.length > 0 ? diagnostics : null;
}

private getUnusedSymbols(
Expand Down Expand Up @@ -161,28 +180,184 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule {
return false;
}

// The reference might be shared if it comes from an exported array. If the variable is local
/// to the file, then it likely isn't shared. Note that this has the potential for false
// positives if a non-exported array of imports is shared between components in the same
// file. This scenario is unlikely and even if we report the diagnostic for it, it would be
// okay since the user only has to refactor components within the same file, rather than the
// entire application.
let current: ts.Node | null = reference.getIdentityIn(rawImports.getSourceFile());
// If the reference comes from any array variable (local or exported), we treat it as
// potentially shared and don't report individual unused items. The checkLocalArraysAllUnused
// method will report "All imports are unused" for local arrays where ALL elements are unused.
// This avoids noisy diagnostics for users who use arrays to group related imports.
return true;
}

while (current !== null) {
if (ts.isVariableStatement(current)) {
return !!current.modifiers?.some((m) => m.kind === ts.SyntaxKind.ExportKeyword);
private checkLocalArraysAllUnused(
metadata: TypeCheckableDirectiveMeta,
usedDirectives: Set<ts.ClassDeclaration>,
usedPipes: Set<string>,
category: ts.DiagnosticCategory,
): ts.Diagnostic[] {
const {rawImports} = metadata;
if (rawImports === null) {
return [];
}

const diagnostics: ts.Diagnostic[] = [];
const arrayIdentifiers = this.collectArrayIdentifiers(rawImports);

for (const {identifier, node: arrayNode} of arrayIdentifiers) {
const sourceFile = identifier.getSourceFile();
const declaration = this.findVariableDeclaration(identifier, sourceFile);

if (declaration === null || !declaration.initializer) {
continue;
}

// Check if this is a local (non-exported) variable
const variableStatement = this.findParentVariableStatement(declaration);
if (variableStatement === null) {
continue;
}

const isExported = variableStatement.modifiers?.some(
(m) => m.kind === ts.SyntaxKind.ExportKeyword,
);

if (isExported) {
continue;
}

if (!ts.isArrayLiteralExpression(declaration.initializer)) {
continue;
}

const allUnused = this.areAllArrayElementsUnused(
declaration.initializer,
usedDirectives,
usedPipes,
);

if (allUnused) {
diagnostics.push(
makeDiagnostic(
ErrorCode.UNUSED_STANDALONE_IMPORTS,
arrayNode,
'All imports are unused',
undefined,
category,
),
);
}
}

return diagnostics;
}

/**
* Collects array identifiers from rawImports (both spread and direct references).
*/
private collectArrayIdentifiers(
rawImports: ts.Expression,
): Array<{identifier: ts.Identifier; node: ts.Node}> {
const result: Array<{identifier: ts.Identifier; node: ts.Node}> = [];

// Handle case where imports is directly an identifier (e.g., imports: importsAsArray)
if (ts.isIdentifier(rawImports)) {
result.push({identifier: rawImports, node: rawImports});
return result;
}

if (!ts.isArrayLiteralExpression(rawImports)) {
return result;
}

for (const element of rawImports.elements) {
if (ts.isSpreadElement(element)) {
// Handle spread syntax: ...ARRAY
if (ts.isIdentifier(element.expression)) {
result.push({identifier: element.expression, node: element});
}
} else if (ts.isIdentifier(element)) {
// Handle direct nested array: ARRAY (without spread)
result.push({identifier: element, node: element});
}
}

// `Node.parent` can be undefined, but the TS types don't reflect it.
// Coerce to null so the value is consitent with the type.
current = current.parent ?? null;
return result;
}

private areAllArrayElementsUnused(
arrayLiteral: ts.ArrayLiteralExpression,
usedDirectives: Set<ts.ClassDeclaration>,
usedPipes: Set<string>,
): boolean {
if (arrayLiteral.elements.length === 0) {
return false;
}

const usedDirectiveNames = new Set<string>();

for (const dir of usedDirectives) {
if (dir.name) {
usedDirectiveNames.add(dir.name.text);
}
}

for (const element of arrayLiteral.elements) {
if (!ts.isIdentifier(element)) {
return false;
}

const elementName = element.text;

if (usedDirectiveNames.has(elementName)) {
return false;
}

if (usedPipes.has(elementName)) {
return false;
}
}

// Otherwise the reference likely comes from an imported
// symbol like an array of shared common components.
return true;
}
Comment on lines +285 to 319

@gemini-code-assist gemini-code-assist Bot Feb 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current implementation uses a nested loop to check if an element is a used directive, which has a time complexity of O(elements * usedDirectives). This can be optimized by first creating a Set of used directive names and then checking for existence in the set, which would reduce the complexity to O(elements + usedDirectives).

  private areAllArrayElementsUnused(
    arrayLiteral: ts.ArrayLiteralExpression,
    usedDirectives: Set<ts.ClassDeclaration>,
    usedPipes: Set<string>,
  ): boolean {
    if (arrayLiteral.elements.length === 0) {
      return false;
    }

    const usedDirectiveNames = new Set<string>();
    for (const dir of usedDirectives) {
      if (dir.name) {
        usedDirectiveNames.add(dir.name.text);
      }
    }

    for (const element of arrayLiteral.elements) {
      if (!ts.isIdentifier(element)) {
        return false;
      }

      const elementName = element.text;

      if (usedDirectiveNames.has(elementName)) {
        return false;
      }

      if (usedPipes.has(elementName)) {
        return false;
      }
    }

    return true;
  }

@SkyZeroZx SkyZeroZx Feb 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done


private findVariableDeclaration(
identifier: ts.Identifier,
sourceFile: ts.SourceFile,
): ts.VariableDeclaration | null {
const targetText = identifier.text;
let foundDeclaration: ts.VariableDeclaration | null = null;

const visit = (node: ts.Node): void => {
if (foundDeclaration) {
return;
}

if (
ts.isVariableDeclaration(node) &&
ts.isIdentifier(node.name) &&
node.name.text === targetText
) {
if (node.name.pos < identifier.pos) {
foundDeclaration = node;
return;
}
}

ts.forEachChild(node, visit);
};

visit(sourceFile);
return foundDeclaration;
}
Comment on lines +321 to +349

@gemini-code-assist gemini-code-assist Bot Feb 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The current implementation for finding a variable declaration by traversing the AST and picking the first match can be brittle and incorrect in the presence of variable shadowing. A more robust approach is to use the TypeScript TypeChecker to resolve the symbol at the identifier's location. This correctly handles scoping and aliasing. Note that the sourceFile parameter becomes unused with this approach and could be removed in a follow-up.

  private findVariableDeclaration(
    identifier: ts.Identifier,
    sourceFile: ts.SourceFile,
  ): ts.VariableDeclaration | null {
    const typeChecker = this.templateTypeChecker.typeChecker;
    const symbol = typeChecker.getSymbolAtLocation(identifier);

    if (!symbol) {
      return null;
    }

    const resolvedSymbol =
      symbol.flags & ts.SymbolFlags.Alias ? typeChecker.getAliasedSymbol(symbol) : symbol;

    if (resolvedSymbol.valueDeclaration && ts.isVariableDeclaration(resolvedSymbol.valueDeclaration)) {
      return resolvedSymbol.valueDeclaration;
    }

    return null;
  }

@SkyZeroZx SkyZeroZx Feb 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The current implementation for finding a variable declaration by traversing the AST and picking the first match can be brittle and incorrect in the presence of variable shadowing.

I don't think this would be necessary considering I don't think it would happen; we would have to write something like this:

function some() {
const SHARED_IMPORTS = [ComponentA]; // Different scope
}

const SHARED_IMPORTS = [ComponentB]; // File level

@Component({
imports: SHARED_IMPORTS // Uses file-level one
})
export class Comp {}

It would also be necessary to make modifications to access type Checker, which would involve more changes that I don't think are necessary in this case.


private findParentVariableStatement(node: ts.Node): ts.VariableStatement | null {
let current: ts.Node | undefined = node;
while (current) {
if (ts.isVariableStatement(current)) {
return current;
}
current = current.parent;
}
return null;
}
}

/** Gets the closest parent node of a certain type. */
Expand Down
Loading