Skip to content

Commit d4ff6bc

Browse files
crisbetoatscott
crisbeto
authored and
atscott
committed
fix(compiler-cli): add warning for unused let declarations (#57033)
Adds a new extended diagnostic that will flag `@let` declarations that aren't used within the template. The diagnostic can be turned off through the `extendedDiagnostics` compiler option. PR Close #57033
1 parent 6c2fbda commit d4ff6bc

File tree

12 files changed

+280
-3
lines changed

12 files changed

+280
-3
lines changed

‎goldens/public-api/compiler-cli/error_code.api.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ export enum ErrorCode {
105105
UNDECORATED_PROVIDER = 2005,
106106
UNINVOKED_FUNCTION_IN_EVENT_BINDING = 8111,
107107
UNSUPPORTED_INITIALIZER_API_USAGE = 8110,
108+
UNUSED_LET_DECLARATION = 8112,
108109
// (undocumented)
109110
VALUE_HAS_WRONG_TYPE = 1010,
110111
// (undocumented)

‎goldens/public-api/compiler-cli/extended_template_diagnostic_name.api.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@ export enum ExtendedTemplateDiagnosticName {
2727
// (undocumented)
2828
TEXT_ATTRIBUTE_NOT_BINDING = "textAttributeNotBinding",
2929
// (undocumented)
30-
UNINVOKED_FUNCTION_IN_EVENT_BINDING = "uninvokedFunctionInEventBinding"
30+
UNINVOKED_FUNCTION_IN_EVENT_BINDING = "uninvokedFunctionInEventBinding",
31+
// (undocumented)
32+
UNUSED_LET_DECLARATION = "unusedLetDeclaration"
3133
}
3234

3335
// (No @packageDocumentation comment for this package)

‎packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -495,6 +495,19 @@ export enum ErrorCode {
495495
*/
496496
UNINVOKED_FUNCTION_IN_EVENT_BINDING = 8111,
497497

498+
/**
499+
* A `@let` declaration in a template isn't used.
500+
*
501+
* For example:
502+
* ```
503+
* @let used = 1; <!-- Not an error -->
504+
* @let notUsed = 2; <!-- Error -->
505+
*
506+
* {{used}}
507+
* ```
508+
*/
509+
UNUSED_LET_DECLARATION = 8112,
510+
498511
/**
499512
* The template type-checking engine would need to generate an inline type check block for a
500513
* component, but the current type-checking environment doesn't support it.

‎packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,4 +27,5 @@ export enum ExtendedTemplateDiagnosticName {
2727
SKIP_HYDRATION_NOT_STATIC = 'skipHydrationNotStatic',
2828
INTERPOLATED_SIGNAL_NOT_INVOKED = 'interpolatedSignalNotInvoked',
2929
CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = 'controlFlowPreventingContentProjection',
30+
UNUSED_LET_DECLARATION = 'unusedLetDeclaration',
3031
}

‎packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ ts_library(
2222
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/suffix_not_supported",
2323
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding",
2424
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/uninvoked_function_in_event_binding",
25+
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration",
2526
"@npm//typescript",
2627
],
2728
)

‎packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration/BUILD.bazel

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
load("//tools:defaults.bzl", "ts_library")
2+
3+
ts_library(
4+
name = "unused_let_declaration",
5+
srcs = ["index.ts"],
6+
visibility = [
7+
"//packages/compiler-cli/src/ngtsc:__subpackages__",
8+
"//packages/compiler-cli/test/ngtsc:__pkg__",
9+
],
10+
deps = [
11+
"//packages/compiler",
12+
"//packages/compiler-cli/src/ngtsc/diagnostics",
13+
"//packages/compiler-cli/src/ngtsc/typecheck/api",
14+
"//packages/compiler-cli/src/ngtsc/typecheck/extended/api",
15+
"@npm//typescript",
16+
],
17+
)

‎packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration/index.ts

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {AST, ASTWithSource, TmplAstLetDeclaration, TmplAstNode} from '@angular/compiler';
10+
import ts from 'typescript';
11+
12+
import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics';
13+
import {NgTemplateDiagnostic} from '../../../api';
14+
import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api';
15+
16+
interface ClassAnalysis {
17+
allLetDeclarations: Set<TmplAstLetDeclaration>;
18+
usedLetDeclarations: Set<TmplAstLetDeclaration>;
19+
}
20+
21+
/**
22+
* Ensures that all `@let` declarations in a template are used.
23+
*/
24+
class UnusedLetDeclarationCheck extends TemplateCheckWithVisitor<ErrorCode.UNUSED_LET_DECLARATION> {
25+
override code = ErrorCode.UNUSED_LET_DECLARATION as const;
26+
private analysis = new Map<ts.ClassDeclaration, ClassAnalysis>();
27+
28+
override run(
29+
ctx: TemplateContext<ErrorCode.UNUSED_LET_DECLARATION>,
30+
component: ts.ClassDeclaration,
31+
template: TmplAstNode[],
32+
): NgTemplateDiagnostic<ErrorCode.UNUSED_LET_DECLARATION>[] {
33+
super.run(ctx, component, template);
34+
35+
const diagnostics: NgTemplateDiagnostic<ErrorCode.UNUSED_LET_DECLARATION>[] = [];
36+
const {allLetDeclarations, usedLetDeclarations} = this.getAnalysis(component);
37+
38+
for (const decl of allLetDeclarations) {
39+
if (!usedLetDeclarations.has(decl)) {
40+
diagnostics.push(
41+
ctx.makeTemplateDiagnostic(
42+
decl.sourceSpan,
43+
`@let ${decl.name} is declared but its value is never read.`,
44+
),
45+
);
46+
}
47+
}
48+
49+
this.analysis.clear();
50+
return diagnostics;
51+
}
52+
53+
override visitNode(
54+
ctx: TemplateContext<ErrorCode.UNUSED_LET_DECLARATION>,
55+
component: ts.ClassDeclaration,
56+
node: TmplAstNode | AST,
57+
): NgTemplateDiagnostic<ErrorCode.UNUSED_LET_DECLARATION>[] {
58+
if (node instanceof TmplAstLetDeclaration) {
59+
this.getAnalysis(component).allLetDeclarations.add(node);
60+
} else if (node instanceof AST) {
61+
const unwrappedNode = node instanceof ASTWithSource ? node.ast : node;
62+
const target = ctx.templateTypeChecker.getExpressionTarget(unwrappedNode, component);
63+
64+
if (target !== null && target instanceof TmplAstLetDeclaration) {
65+
this.getAnalysis(component).usedLetDeclarations.add(target);
66+
}
67+
}
68+
69+
return [];
70+
}
71+
72+
private getAnalysis(node: ts.ClassDeclaration): ClassAnalysis {
73+
if (!this.analysis.has(node)) {
74+
this.analysis.set(node, {allLetDeclarations: new Set(), usedLetDeclarations: new Set()});
75+
}
76+
return this.analysis.get(node)!;
77+
}
78+
}
79+
80+
export const factory: TemplateCheckFactory<
81+
ErrorCode.UNUSED_LET_DECLARATION,
82+
ExtendedTemplateDiagnosticName.UNUSED_LET_DECLARATION
83+
> = {
84+
code: ErrorCode.UNUSED_LET_DECLARATION,
85+
name: ExtendedTemplateDiagnosticName.UNUSED_LET_DECLARATION,
86+
create: () => new UnusedLetDeclarationCheck(),
87+
};

‎packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {factory as optionalChainNotNullableFactory} from './checks/optional_chai
1818
import {factory as suffixNotSupportedFactory} from './checks/suffix_not_supported';
1919
import {factory as textAttributeNotBindingFactory} from './checks/text_attribute_not_binding';
2020
import {factory as uninvokedFunctionInEventBindingFactory} from './checks/uninvoked_function_in_event_binding';
21+
import {factory as unusedLetDeclarationFactory} from './checks/unused_let_declaration';
2122

2223
export {ExtendedTemplateCheckerImpl} from './src/extended_template_checker';
2324

@@ -34,6 +35,7 @@ export const ALL_DIAGNOSTIC_FACTORIES: readonly TemplateCheckFactory<
3435
suffixNotSupportedFactory,
3536
interpolatedSignalNotInvoked,
3637
uninvokedFunctionInEventBindingFactory,
38+
unusedLetDeclarationFactory,
3739
];
3840

3941
export const SUPPORTED_DIAGNOSTIC_NAMES = new Set<string>([

‎packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unused_let_declaration/BUILD.bazel

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
load("//tools:defaults.bzl", "jasmine_node_test", "ts_library")
2+
3+
ts_library(
4+
name = "test_lib",
5+
testonly = True,
6+
srcs = ["unused_let_declaration_spec.ts"],
7+
deps = [
8+
"//packages/compiler",
9+
"//packages/compiler-cli/src/ngtsc/core:api",
10+
"//packages/compiler-cli/src/ngtsc/diagnostics",
11+
"//packages/compiler-cli/src/ngtsc/file_system",
12+
"//packages/compiler-cli/src/ngtsc/file_system/testing",
13+
"//packages/compiler-cli/src/ngtsc/testing",
14+
"//packages/compiler-cli/src/ngtsc/typecheck/extended",
15+
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration",
16+
"//packages/compiler-cli/src/ngtsc/typecheck/testing",
17+
"@npm//typescript",
18+
],
19+
)
20+
21+
jasmine_node_test(
22+
name = "test",
23+
bootstrap = ["//tools/testing:node_no_angular"],
24+
data = [
25+
"//packages/core:npm_package",
26+
],
27+
deps = [
28+
":test_lib",
29+
],
30+
)

‎packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unused_let_declaration/unused_let_declaration_spec.ts

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import ts from 'typescript';
10+
11+
import {ErrorCode, ExtendedTemplateDiagnosticName, ngErrorCode} from '../../../../../diagnostics';
12+
import {absoluteFrom, getSourceFileOrError} from '../../../../../file_system';
13+
import {runInEachFileSystem} from '../../../../../file_system/testing';
14+
import {getSourceCodeForDiagnostic} from '../../../../../testing';
15+
import {getClass, setup} from '../../../../testing';
16+
import {factory as unusedLetDeclarationFactory} from '../../../checks/unused_let_declaration/index';
17+
import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker';
18+
19+
runInEachFileSystem(() => {
20+
describe('UnusedLetDeclarationCheck', () => {
21+
function diagnose(template: string) {
22+
const fileName = absoluteFrom('/main.ts');
23+
const {program, templateTypeChecker} = setup([
24+
{
25+
fileName,
26+
templates: {
27+
'TestCmp': template,
28+
},
29+
source: `
30+
export class TestCmp {
31+
eventCallback(value: any) {}
32+
}
33+
`,
34+
},
35+
]);
36+
const sf = getSourceFileOrError(program, fileName);
37+
const component = getClass(sf, 'TestCmp');
38+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
39+
templateTypeChecker,
40+
program.getTypeChecker(),
41+
[unusedLetDeclarationFactory],
42+
{},
43+
);
44+
return extendedTemplateChecker.getDiagnosticsForComponent(component);
45+
}
46+
47+
it('binds the error code to its extended template diagnostic name', () => {
48+
expect(unusedLetDeclarationFactory.code).toBe(ErrorCode.UNUSED_LET_DECLARATION);
49+
expect(unusedLetDeclarationFactory.name).toBe(
50+
ExtendedTemplateDiagnosticName.UNUSED_LET_DECLARATION,
51+
);
52+
});
53+
54+
it('should report a @let declaration that is not used', () => {
55+
const diags = diagnose(`
56+
@let used = 1;
57+
@let unused = 2;
58+
{{used}}
59+
`);
60+
61+
expect(diags.length).toBe(1);
62+
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
63+
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.UNUSED_LET_DECLARATION));
64+
expect(getSourceCodeForDiagnostic(diags[0])).toBe('@let unused = 2');
65+
});
66+
67+
it('should report a @let declaration that is not used', () => {
68+
const diags = diagnose(`
69+
@let foo = 1;
70+
71+
@if (true) {
72+
@let foo = 2;
73+
{{foo}}
74+
}
75+
`);
76+
77+
expect(diags.length).toBe(1);
78+
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
79+
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.UNUSED_LET_DECLARATION));
80+
expect(getSourceCodeForDiagnostic(diags[0])).toBe('@let foo = 1');
81+
});
82+
83+
it('should not report a @let declaration that is only used in other @let declarations', () => {
84+
const diags = diagnose(`
85+
@let one = 1;
86+
@let two = 2;
87+
@let three = one + two;
88+
{{three}}
89+
`);
90+
91+
expect(diags.length).toBe(0);
92+
});
93+
94+
it('should not report a @let declaration that is only used in an event listener', () => {
95+
const diags = diagnose(`
96+
@let foo = 1;
97+
<button (click)="eventCallback(foo + 1)">Click me</button>
98+
`);
99+
100+
expect(diags.length).toBe(0);
101+
});
102+
103+
it('should not report a @let declaration that is only used in a structural directive', () => {
104+
const diags = diagnose(`
105+
@let foo = null;
106+
<div *ngIf="foo"></div>
107+
`);
108+
109+
expect(diags.length).toBe(0);
110+
});
111+
});
112+
});

0 commit comments

Comments
 (0)