From 33fe252c588ee94d6ef99e8070d35c483ec24fda Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 25 Sep 2024 08:51:32 +0200 Subject: [PATCH] fix(compiler-cli): do not report unused declarations coming from an imported array (#57940) Some apps follow a pattern where they have an array of common declarations which is imported in most standalone components, but only some of the declarations are used. Such cases will currently raise the unused imports diagnostic but can be hard to fix, because it would require either removing declarations from the common array which can break other components, or copying only the necessary declarations from the array. Since neither of these solutions is great, this commit tweaks the logic for the diagnostic so that unused imports coming from _exported_ arrays are not reported (either from the same file or another one). PR Close #57940 --- .../rules/unused_standalone_imports_rule.ts | 46 ++- .../test/ngtsc/template_typecheck_spec.ts | 308 ++++++++++++++++++ 2 files changed, 350 insertions(+), 4 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/validation/src/rules/unused_standalone_imports_rule.ts b/packages/compiler-cli/src/ngtsc/validation/src/rules/unused_standalone_imports_rule.ts index 0aca2abcd03..ba45d37ead1 100644 --- a/packages/compiler-cli/src/ngtsc/validation/src/rules/unused_standalone_imports_rule.ts +++ b/packages/compiler-cli/src/ngtsc/validation/src/rules/unused_standalone_imports_rule.ts @@ -105,18 +105,24 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule { usedDirectives: Set, usedPipes: Set, ) { - if (metadata.imports === null || metadata.rawImports === null) { + const {imports, rawImports} = metadata; + + if (imports === null || rawImports === null) { return null; } let unused: [ref: Reference, type: string, name: string][] | null = null; - for (const current of metadata.imports) { + for (const current of imports) { const currentNode = current.node as ts.ClassDeclaration; const dirMeta = this.templateTypeChecker.getDirectiveMetadata(currentNode); if (dirMeta !== null) { - if (dirMeta.isStandalone && (usedDirectives === null || !usedDirectives.has(currentNode))) { + if ( + dirMeta.isStandalone && + !usedDirectives.has(currentNode) && + !this.isPotentialSharedReference(current, rawImports) + ) { unused ??= []; unused.push([current, dirMeta.isComponent ? 'Component' : 'Directive', dirMeta.name]); } @@ -128,7 +134,8 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule { if ( pipeMeta !== null && pipeMeta.isStandalone && - (usedPipes === null || !usedPipes.has(pipeMeta.name)) + !usedPipes.has(pipeMeta.name) && + !this.isPotentialSharedReference(current, rawImports) ) { unused ??= []; unused.push([current, 'Pipe', pipeMeta.ref.node.name.text]); @@ -137,4 +144,35 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule { return unused; } + + /** + * Determines if an import reference *might* be coming from a shared imports array. + * @param reference Reference to be checked. + * @param rawImports AST node that defines the `imports` array. + */ + private isPotentialSharedReference(reference: Reference, rawImports: ts.Expression): boolean { + // If the reference is defined directly in the `imports` array, it cannot be shared. + if (reference.getIdentityInExpression(rawImports) !== null) { + 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()); + + while (current !== null) { + if (ts.isVariableStatement(current)) { + return !!current.modifiers?.some((m) => m.kind === ts.SyntaxKind.ExportKeyword); + } + current = current.parent; + } + + // Otherwise the reference likely comes from an imported + // symbol like an array of shared common components. + return true; + } } diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 289e31e11ae..b94b2d9167f 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -7765,6 +7765,314 @@ suppress 'Pipe "PercentPipe" is not used within the template', ); }); + + it('should report unused imports coming from a nested array from the same file', () => { + env.write( + 'used.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[used]', standalone: true}) + export class UsedDir {} + `, + ); + + env.write( + 'other-used.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[other-used]', standalone: true}) + export class OtherUsedDir {} + `, + ); + + env.write( + 'unused.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[unused]', standalone: true}) + export class UnusedDir {} + `, + ); + + env.write( + 'test.ts', + ` + import {Component} from '@angular/core'; + import {UsedDir} from './used'; + import {OtherUsedDir} from './other-used'; + import {UnusedDir} from './unused'; + + const COMMON = [OtherUsedDir, UnusedDir]; + + @Component({ + template: \` +
+
+ +
+ \`, + standalone: true, + imports: [UsedDir, COMMON] + }) + export class MyComp {} + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toBe('Imports array contains unused imports'); + expect(diags[0].relatedInformation?.length).toBe(1); + expect(diags[0].relatedInformation![0].messageText).toBe( + 'Directive "UnusedDir" is not used within the template', + ); + }); + + it('should report unused imports coming from an array used as the `imports` initializer', () => { + env.write( + 'used.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[used]', standalone: true}) + export class UsedDir {} + `, + ); + + env.write( + 'unused.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[unused]', standalone: true}) + export class UnusedDir {} + `, + ); + + env.write( + 'test.ts', + ` + import {Component} from '@angular/core'; + import {UsedDir} from './used'; + import {UnusedDir} from './unused'; + + const IMPORTS = [UsedDir, UnusedDir]; + + @Component({ + template: \` +
+
+ +
+ \`, + standalone: true, + imports: IMPORTS + }) + export class MyComp {} + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toBe('Imports array contains unused imports'); + expect(diags[0].relatedInformation?.length).toBe(1); + expect(diags[0].relatedInformation![0].messageText).toBe( + 'Directive "UnusedDir" is not used within the template', + ); + }); + + it('should not report unused imports coming from an array through a spread expression from a different file', () => { + env.write( + 'used.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[used]', standalone: true}) + export class UsedDir {} + `, + ); + + env.write( + 'other-used.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[other-used]', standalone: true}) + export class OtherUsedDir {} + `, + ); + + env.write( + 'unused.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[unused]', standalone: true}) + export class UnusedDir {} + `, + ); + + env.write( + 'common.ts', + ` + import {OtherUsedDir} from './other-used'; + import {UnusedDir} from './unused'; + + export const COMMON = [OtherUsedDir, UnusedDir]; + `, + ); + + env.write( + 'test.ts', + ` + import {Component} from '@angular/core'; + import {UsedDir} from './used'; + import {COMMON} from './common'; + + @Component({ + template: \` +
+
+ +
+ \`, + standalone: true, + imports: [UsedDir, ...COMMON] + }) + export class MyComp {} + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should not report unused imports coming from a nested array from a different file', () => { + env.write( + 'used.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[used]', standalone: true}) + export class UsedDir {} + `, + ); + + env.write( + 'other-used.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[other-used]', standalone: true}) + export class OtherUsedDir {} + `, + ); + + env.write( + 'unused.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[unused]', standalone: true}) + export class UnusedDir {} + `, + ); + + env.write( + 'common.ts', + ` + import {OtherUsedDir} from './other-used'; + import {UnusedDir} from './unused'; + + export const COMMON = [OtherUsedDir, UnusedDir]; + `, + ); + + env.write( + 'test.ts', + ` + import {Component} from '@angular/core'; + import {UsedDir} from './used'; + import {COMMON} from './common'; + + @Component({ + template: \` +
+
+ +
+ \`, + standalone: true, + imports: [UsedDir, COMMON] + }) + export class MyComp {} + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should not report unused imports coming from an exported array in the same file', () => { + env.write( + 'used.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[used]', standalone: true}) + export class UsedDir {} + `, + ); + + env.write( + 'other-used.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[other-used]', standalone: true}) + export class OtherUsedDir {} + `, + ); + + env.write( + 'unused.ts', + ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[unused]', standalone: true}) + export class UnusedDir {} + `, + ); + + env.write( + 'test.ts', + ` + import {Component} from '@angular/core'; + import {UsedDir} from './used'; + import {OtherUsedDir} from './other-used'; + import {UnusedDir} from './unused'; + + export const COMMON = [OtherUsedDir, UnusedDir]; + + @Component({ + template: \` +
+
+ +
+ \`, + standalone: true, + imports: [UsedDir, COMMON] + }) + export class MyComp {} + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); }); }); });