diff --git a/goldens/public-api/compiler-cli/error_code.api.md b/goldens/public-api/compiler-cli/error_code.api.md index e1ad1d87bde..7c307e96f2f 100644 --- a/goldens/public-api/compiler-cli/error_code.api.md +++ b/goldens/public-api/compiler-cli/error_code.api.md @@ -105,6 +105,7 @@ export enum ErrorCode { UNDECORATED_PROVIDER = 2005, UNINVOKED_FUNCTION_IN_EVENT_BINDING = 8111, UNSUPPORTED_INITIALIZER_API_USAGE = 8110, + UNUSED_LET_DECLARATION = 8112, // (undocumented) VALUE_HAS_WRONG_TYPE = 1010, // (undocumented) diff --git a/goldens/public-api/compiler-cli/extended_template_diagnostic_name.api.md b/goldens/public-api/compiler-cli/extended_template_diagnostic_name.api.md index 2f57f98462b..36777b64fa9 100644 --- a/goldens/public-api/compiler-cli/extended_template_diagnostic_name.api.md +++ b/goldens/public-api/compiler-cli/extended_template_diagnostic_name.api.md @@ -27,7 +27,9 @@ export enum ExtendedTemplateDiagnosticName { // (undocumented) TEXT_ATTRIBUTE_NOT_BINDING = "textAttributeNotBinding", // (undocumented) - UNINVOKED_FUNCTION_IN_EVENT_BINDING = "uninvokedFunctionInEventBinding" + UNINVOKED_FUNCTION_IN_EVENT_BINDING = "uninvokedFunctionInEventBinding", + // (undocumented) + UNUSED_LET_DECLARATION = "unusedLetDeclaration" } // (No @packageDocumentation comment for this package) diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts index 9b3fb28331e..14584d91f77 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -495,6 +495,19 @@ export enum ErrorCode { */ UNINVOKED_FUNCTION_IN_EVENT_BINDING = 8111, + /** + * A `@let` declaration in a template isn't used. + * + * For example: + * ``` + * @let used = 1; + * @let notUsed = 2; + * + * {{used}} + * ``` + */ + UNUSED_LET_DECLARATION = 8112, + /** * The template type-checking engine would need to generate an inline type check block for a * component, but the current type-checking environment doesn't support it. diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts index c04d2b4f959..1b968e610b1 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts @@ -27,4 +27,5 @@ export enum ExtendedTemplateDiagnosticName { SKIP_HYDRATION_NOT_STATIC = 'skipHydrationNotStatic', INTERPOLATED_SIGNAL_NOT_INVOKED = 'interpolatedSignalNotInvoked', CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = 'controlFlowPreventingContentProjection', + UNUSED_LET_DECLARATION = 'unusedLetDeclaration', } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel index d49bed313ec..f4527da5c18 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel @@ -22,6 +22,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/suffix_not_supported", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/uninvoked_function_in_event_binding", + "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration", "@npm//typescript", ], ) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration/BUILD.bazel new file mode 100644 index 00000000000..75824d705df --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration/BUILD.bazel @@ -0,0 +1,17 @@ +load("//tools:defaults.bzl", "ts_library") + +ts_library( + name = "unused_let_declaration", + srcs = ["index.ts"], + visibility = [ + "//packages/compiler-cli/src/ngtsc:__subpackages__", + "//packages/compiler-cli/test/ngtsc:__pkg__", + ], + deps = [ + "//packages/compiler", + "//packages/compiler-cli/src/ngtsc/diagnostics", + "//packages/compiler-cli/src/ngtsc/typecheck/api", + "//packages/compiler-cli/src/ngtsc/typecheck/extended/api", + "@npm//typescript", + ], +) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration/index.ts new file mode 100644 index 00000000000..e670f7f3464 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration/index.ts @@ -0,0 +1,87 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {AST, ASTWithSource, TmplAstLetDeclaration, TmplAstNode} from '@angular/compiler'; +import ts from 'typescript'; + +import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics'; +import {NgTemplateDiagnostic} from '../../../api'; +import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api'; + +interface ClassAnalysis { + allLetDeclarations: Set; + usedLetDeclarations: Set; +} + +/** + * Ensures that all `@let` declarations in a template are used. + */ +class UnusedLetDeclarationCheck extends TemplateCheckWithVisitor { + override code = ErrorCode.UNUSED_LET_DECLARATION as const; + private analysis = new Map(); + + override run( + ctx: TemplateContext, + component: ts.ClassDeclaration, + template: TmplAstNode[], + ): NgTemplateDiagnostic[] { + super.run(ctx, component, template); + + const diagnostics: NgTemplateDiagnostic[] = []; + const {allLetDeclarations, usedLetDeclarations} = this.getAnalysis(component); + + for (const decl of allLetDeclarations) { + if (!usedLetDeclarations.has(decl)) { + diagnostics.push( + ctx.makeTemplateDiagnostic( + decl.sourceSpan, + `@let ${decl.name} is declared but its value is never read.`, + ), + ); + } + } + + this.analysis.clear(); + return diagnostics; + } + + override visitNode( + ctx: TemplateContext, + component: ts.ClassDeclaration, + node: TmplAstNode | AST, + ): NgTemplateDiagnostic[] { + if (node instanceof TmplAstLetDeclaration) { + this.getAnalysis(component).allLetDeclarations.add(node); + } else if (node instanceof AST) { + const unwrappedNode = node instanceof ASTWithSource ? node.ast : node; + const target = ctx.templateTypeChecker.getExpressionTarget(unwrappedNode, component); + + if (target !== null && target instanceof TmplAstLetDeclaration) { + this.getAnalysis(component).usedLetDeclarations.add(target); + } + } + + return []; + } + + private getAnalysis(node: ts.ClassDeclaration): ClassAnalysis { + if (!this.analysis.has(node)) { + this.analysis.set(node, {allLetDeclarations: new Set(), usedLetDeclarations: new Set()}); + } + return this.analysis.get(node)!; + } +} + +export const factory: TemplateCheckFactory< + ErrorCode.UNUSED_LET_DECLARATION, + ExtendedTemplateDiagnosticName.UNUSED_LET_DECLARATION +> = { + code: ErrorCode.UNUSED_LET_DECLARATION, + name: ExtendedTemplateDiagnosticName.UNUSED_LET_DECLARATION, + create: () => new UnusedLetDeclarationCheck(), +}; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts index 0dc597df050..c56e9d945aa 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts @@ -18,6 +18,7 @@ import {factory as optionalChainNotNullableFactory} from './checks/optional_chai import {factory as suffixNotSupportedFactory} from './checks/suffix_not_supported'; import {factory as textAttributeNotBindingFactory} from './checks/text_attribute_not_binding'; import {factory as uninvokedFunctionInEventBindingFactory} from './checks/uninvoked_function_in_event_binding'; +import {factory as unusedLetDeclarationFactory} from './checks/unused_let_declaration'; export {ExtendedTemplateCheckerImpl} from './src/extended_template_checker'; @@ -34,6 +35,7 @@ export const ALL_DIAGNOSTIC_FACTORIES: readonly TemplateCheckFactory< suffixNotSupportedFactory, interpolatedSignalNotInvoked, uninvokedFunctionInEventBindingFactory, + unusedLetDeclarationFactory, ]; export const SUPPORTED_DIAGNOSTIC_NAMES = new Set([ diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unused_let_declaration/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unused_let_declaration/BUILD.bazel new file mode 100644 index 00000000000..a194ca51768 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unused_let_declaration/BUILD.bazel @@ -0,0 +1,30 @@ +load("//tools:defaults.bzl", "jasmine_node_test", "ts_library") + +ts_library( + name = "test_lib", + testonly = True, + srcs = ["unused_let_declaration_spec.ts"], + deps = [ + "//packages/compiler", + "//packages/compiler-cli/src/ngtsc/core:api", + "//packages/compiler-cli/src/ngtsc/diagnostics", + "//packages/compiler-cli/src/ngtsc/file_system", + "//packages/compiler-cli/src/ngtsc/file_system/testing", + "//packages/compiler-cli/src/ngtsc/testing", + "//packages/compiler-cli/src/ngtsc/typecheck/extended", + "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration", + "//packages/compiler-cli/src/ngtsc/typecheck/testing", + "@npm//typescript", + ], +) + +jasmine_node_test( + name = "test", + bootstrap = ["//tools/testing:node_no_angular"], + data = [ + "//packages/core:npm_package", + ], + deps = [ + ":test_lib", + ], +) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unused_let_declaration/unused_let_declaration_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unused_let_declaration/unused_let_declaration_spec.ts new file mode 100644 index 00000000000..3afced6bf38 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unused_let_declaration/unused_let_declaration_spec.ts @@ -0,0 +1,112 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import ts from 'typescript'; + +import {ErrorCode, ExtendedTemplateDiagnosticName, ngErrorCode} from '../../../../../diagnostics'; +import {absoluteFrom, getSourceFileOrError} from '../../../../../file_system'; +import {runInEachFileSystem} from '../../../../../file_system/testing'; +import {getSourceCodeForDiagnostic} from '../../../../../testing'; +import {getClass, setup} from '../../../../testing'; +import {factory as unusedLetDeclarationFactory} from '../../../checks/unused_let_declaration/index'; +import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker'; + +runInEachFileSystem(() => { + describe('UnusedLetDeclarationCheck', () => { + function diagnose(template: string) { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([ + { + fileName, + templates: { + 'TestCmp': template, + }, + source: ` + export class TestCmp { + eventCallback(value: any) {} + } + `, + }, + ]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, + program.getTypeChecker(), + [unusedLetDeclarationFactory], + {}, + ); + return extendedTemplateChecker.getDiagnosticsForComponent(component); + } + + it('binds the error code to its extended template diagnostic name', () => { + expect(unusedLetDeclarationFactory.code).toBe(ErrorCode.UNUSED_LET_DECLARATION); + expect(unusedLetDeclarationFactory.name).toBe( + ExtendedTemplateDiagnosticName.UNUSED_LET_DECLARATION, + ); + }); + + it('should report a @let declaration that is not used', () => { + const diags = diagnose(` + @let used = 1; + @let unused = 2; + {{used}} + `); + + expect(diags.length).toBe(1); + expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.UNUSED_LET_DECLARATION)); + expect(getSourceCodeForDiagnostic(diags[0])).toBe('@let unused = 2'); + }); + + it('should report a @let declaration that is not used', () => { + const diags = diagnose(` + @let foo = 1; + + @if (true) { + @let foo = 2; + {{foo}} + } + `); + + expect(diags.length).toBe(1); + expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.UNUSED_LET_DECLARATION)); + expect(getSourceCodeForDiagnostic(diags[0])).toBe('@let foo = 1'); + }); + + it('should not report a @let declaration that is only used in other @let declarations', () => { + const diags = diagnose(` + @let one = 1; + @let two = 2; + @let three = one + two; + {{three}} + `); + + expect(diags.length).toBe(0); + }); + + it('should not report a @let declaration that is only used in an event listener', () => { + const diags = diagnose(` + @let foo = 1; + + `); + + expect(diags.length).toBe(0); + }); + + it('should not report a @let declaration that is only used in a structural directive', () => { + const diags = diagnose(` + @let foo = null; +
+ `); + + expect(diags.length).toBe(0); + }); + }); +}); diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 930844c75ce..fd7093d0767 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -6611,7 +6611,18 @@ suppress }); describe('@let declarations', () => { - beforeEach(() => env.tsconfig({strictTemplates: true})); + beforeEach(() => + env.tsconfig({ + strictTemplates: true, + extendedDiagnostics: { + checks: { + // Suppress the diagnostic for unused @let since some of the error cases + // we're checking for here also qualify as being unused which adds noise. + unusedLetDeclaration: 'suppress', + }, + }, + }), + ); it('should infer the type of a let declaration', () => { env.write( diff --git a/packages/language-service/test/quick_info_spec.ts b/packages/language-service/test/quick_info_spec.ts index 899fdc40b2a..c4183d2b6b3 100644 --- a/packages/language-service/test/quick_info_spec.ts +++ b/packages/language-service/test/quick_info_spec.ts @@ -708,7 +708,7 @@ describe('quick info', () => { describe('let declarations', () => { it('should get quick info for a let declaration', () => { expectQuickInfo({ - templateOverride: `@let na¦me = 'Frodo';`, + templateOverride: `@let na¦me = 'Frodo'; {{name}}`, expectedSpanText: `@let name = 'Frodo'`, expectedDisplayString: `(let) name: "Frodo"`, });