From 54ba7969fec538cd730c7e982d8b1a131a797b50 Mon Sep 17 00:00:00 2001 From: robertIsaac Date: Sat, 7 Jan 2023 15:31:33 +0200 Subject: [PATCH 1/2] feat(compiler-cli): add extended diagnostics to check for component variable shadowing template reference for example ``` @COMPONENT({ template: '
' }) export class FooComponent { var1: string; } ``` right now this goes without error, but it can lead to mistakenly inside the template to use the template reference as if it was the component variable Fixes #45227 --- goldens/public-api/compiler-cli/error_code.md | 3 +- .../extended_template_diagnostic_name.md | 2 + .../src/ngtsc/diagnostics/src/error_code.ts | 15 ++++ .../src/extended_template_diagnostic_name.ts | 1 + .../BUILD.bazel | 17 +++++ .../index.ts | 53 ++++++++++++++ .../BUILD.bazel | 27 ++++++++ ...ariable_shadows_template_reference_spec.ts | 69 +++++++++++++++++++ 8 files changed, 186 insertions(+), 1 deletion(-) create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadows_template_reference/BUILD.bazel create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadows_template_reference/index.ts create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/component_variable_shadows_template_reference/BUILD.bazel create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/component_variable_shadows_template_reference/component_variable_shadows_template_reference_spec.ts diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index d27864e5138d..bacde29773a4 100644 --- a/goldens/public-api/compiler-cli/error_code.md +++ b/goldens/public-api/compiler-cli/error_code.md @@ -14,6 +14,7 @@ export enum ErrorCode { COMPONENT_NOT_STANDALONE = 2010, COMPONENT_RESOURCE_NOT_FOUND = 2008, COMPONENT_UNKNOWN_IMPORT = 2012, + COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE = 8109, // (undocumented) CONFIG_EXTENDED_DIAGNOSTICS_IMPLIES_STRICT_TEMPLATES = 4003, // (undocumented) @@ -99,4 +100,4 @@ export enum ErrorCode { // (No @packageDocumentation comment for this package) -``` +``` \ No newline at end of file diff --git a/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md b/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md index 6b6ca9e263d0..deeef96e5e32 100644 --- a/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md +++ b/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md @@ -6,6 +6,8 @@ // @public export enum ExtendedTemplateDiagnosticName { + // (undocumented) + COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE = "ComponentVariableShadowsTemplateReference", // (undocumented) INVALID_BANANA_IN_BOX = "invalidBananaInBox", // (undocumented) 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 4df3d3d91e4d..ae1ddfe67cf0 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -377,6 +377,21 @@ export enum ErrorCode { */ SKIP_HYDRATION_NOT_STATIC = 8108, + /** + * A template reference shadows a component class property or method. + * For example: + * + * ``` + * @Component({ + * template: '
' + * }) + * export class FooComponent { + * var1: string; + * } + * ``` + */ + COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE = 8109, + /** * 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 6b3a7834d12a..10a83f692fae 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 @@ -24,4 +24,5 @@ export enum ExtendedTemplateDiagnosticName { MISSING_NGFOROF_LET = 'missingNgForOfLet', SUFFIX_NOT_SUPPORTED = 'suffixNotSupported', SKIP_HYDRATION_NOT_STATIC = 'skipHydrationNotStatic', + COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE = 'ComponentVariableShadowsTemplateReference', } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadows_template_reference/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadows_template_reference/BUILD.bazel new file mode 100644 index 000000000000..78298716031c --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadows_template_reference/BUILD.bazel @@ -0,0 +1,17 @@ +load("//tools:defaults.bzl", "ts_library") + +ts_library( + name = "component_variable_shadows_template_reference", + 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/component_variable_shadows_template_reference/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadows_template_reference/index.ts new file mode 100644 index 000000000000..1bdb366ce8b5 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadows_template_reference/index.ts @@ -0,0 +1,53 @@ +/** + * @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, TmplAstNode, TmplAstReference as Reference} from '@angular/compiler'; +import ts from 'typescript'; + +import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics'; +import {NgTemplateDiagnostic} from '../../../api'; +import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api'; + +/** + * Ensures component variables don't shadow any template reference. + * Will return diagnostic information when any shadow is found. + */ +class ComponentVariableShadowsTemplateReference extends + TemplateCheckWithVisitor { + override code = ErrorCode.COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE as const; + + override visitNode( + ctx: TemplateContext, + component: ts.ClassDeclaration, + node: TmplAstNode|AST, + ): NgTemplateDiagnostic[] { + if (!(node instanceof Reference)) return []; + + const templateReference = node.name; + const componentVariables = + component.members.map(member => (member.name as ts.Identifier)?.escapedText.toString()); + + if (!componentVariables.includes(templateReference)) { + return []; + } + + const diagnostic = ctx.makeTemplateDiagnostic( + node.sourceSpan, + `you have a component variable and template reference with the same name "${ + templateReference}"`); + + return [diagnostic]; + } +} +export const factory: TemplateCheckFactory< + ErrorCode.COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE, + ExtendedTemplateDiagnosticName.COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE> = { + code: ErrorCode.COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE, + name: ExtendedTemplateDiagnosticName.COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE, + create: () => new ComponentVariableShadowsTemplateReference(), +}; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/component_variable_shadows_template_reference/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/component_variable_shadows_template_reference/BUILD.bazel new file mode 100644 index 000000000000..b30ea55c17aa --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/component_variable_shadows_template_reference/BUILD.bazel @@ -0,0 +1,27 @@ +load("//tools:defaults.bzl", "jasmine_node_test", "ts_library") + +ts_library( + name = "test_lib", + testonly = True, + srcs = ["component_variable_shadows_template_reference_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/component_variable_shadows_template_reference", + "//packages/compiler-cli/src/ngtsc/typecheck/testing", + "@npm//typescript", + ], +) + +jasmine_node_test( + name = "test", + bootstrap = ["//tools/testing:node_no_angular"], + deps = [ + ":test_lib", + ], +) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/component_variable_shadows_template_reference/component_variable_shadows_template_reference_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/component_variable_shadows_template_reference/component_variable_shadows_template_reference_spec.ts new file mode 100644 index 000000000000..e6efe2514b84 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/component_variable_shadows_template_reference/component_variable_shadows_template_reference_spec.ts @@ -0,0 +1,69 @@ +/** + * @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 componentVariableShadowsTemplateReference} from '../../../checks/component_variable_shadows_template_reference'; +import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker'; + +runInEachFileSystem(() => { + describe('TemplateChecks', () => { + it('binds the error code to its extended template diagnostic name', () => { + expect(componentVariableShadowsTemplateReference.code) + .toBe(ErrorCode.COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE); + expect(componentVariableShadowsTemplateReference.name) + .toBe(ExtendedTemplateDiagnosticName.COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE); + }); + + it('should produce component variable shadow template reference warning', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': '
', + }, + source: `export class TestCmp { var3 = 'text'; var2: string = 'text'; }` + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), + [componentVariableShadowsTemplateReference], {} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(1); + expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); + expect(diags[0].code) + .toBe(ngErrorCode(ErrorCode.COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE)); + expect(getSourceCodeForDiagnostic(diags[0])).toBe('#var2'); + }); + + it('should not produce component variable shadow template reference warning if written correctly', + () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': '
', + }, + source: `export class TestCmp { var3 = 'text'; var4 = 'text'; }` + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), + [componentVariableShadowsTemplateReference], {} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(0); + }); + }); +}); From fd2b2ecad651c94fdd457ea9efa457a76c471552 Mon Sep 17 00:00:00 2001 From: robertIsaac Date: Tue, 10 Jan 2023 15:08:59 +0200 Subject: [PATCH 2/2] feat(compiler-cli): add extended diagnostics to check for component variable shadowing template reference for example ``` @COMPONENT({ template: '
' }) export class FooComponent { var1: string; } ``` right now this goes without error, but it can lead to mistakenly inside the template to use the template reference as if it was the component variable Fixes #45227 --- goldens/public-api/compiler-cli/error_code.md | 2 +- .../index.ts | 18 ++-- ...ariable_shadows_template_reference_spec.ts | 93 +++++++++++++++++++ 3 files changed, 106 insertions(+), 7 deletions(-) diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index bacde29773a4..1fa487388a70 100644 --- a/goldens/public-api/compiler-cli/error_code.md +++ b/goldens/public-api/compiler-cli/error_code.md @@ -100,4 +100,4 @@ export enum ErrorCode { // (No @packageDocumentation comment for this package) -``` \ No newline at end of file +``` diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadows_template_reference/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadows_template_reference/index.ts index 1bdb366ce8b5..f43fd383a7e5 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadows_template_reference/index.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadows_template_reference/index.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {AST, TmplAstNode, TmplAstReference as Reference} from '@angular/compiler'; +import {AST, TmplAstNode as Node, TmplAstReference as Reference, TmplAstVariable as Variable} from '@angular/compiler'; import ts from 'typescript'; import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics'; @@ -24,15 +24,21 @@ class ComponentVariableShadowsTemplateReference extends override visitNode( ctx: TemplateContext, component: ts.ClassDeclaration, - node: TmplAstNode|AST, + node: Node|AST, ): NgTemplateDiagnostic[] { - if (!(node instanceof Reference)) return []; + if (!(node instanceof Reference || node instanceof Variable)) return []; const templateReference = node.name; - const componentVariables = - component.members.map(member => (member.name as ts.Identifier)?.escapedText.toString()); + const parent = component.parent as unknown as + {classifiableNames: Set, identifiers: Map}; + const componentVariables = new Set(); + for (let id of parent.identifiers.keys()) { + if (!parent.classifiableNames.has(id)) { + componentVariables.add(id); + } + } - if (!componentVariables.includes(templateReference)) { + if (!componentVariables.has(templateReference)) { return []; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/component_variable_shadows_template_reference/component_variable_shadows_template_reference_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/component_variable_shadows_template_reference/component_variable_shadows_template_reference_spec.ts index e6efe2514b84..9ae4d4d33645 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/component_variable_shadows_template_reference/component_variable_shadows_template_reference_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/component_variable_shadows_template_reference/component_variable_shadows_template_reference_spec.ts @@ -65,5 +65,98 @@ runInEachFileSystem(() => { const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); expect(diags.length).toBe(0); }); + + it('should produce component variable shadow template reference warning for inherited properties as well', + () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': '
', + }, + source: + `export abstract class TestAbsCls { var1 = 'text'; } export class TestCmp extends TestAbsCls { var2: string = 'text'; }` + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), + [componentVariableShadowsTemplateReference], {} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(1); + expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); + expect(diags[0].code) + .toBe(ngErrorCode(ErrorCode.COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE)); + expect(getSourceCodeForDiagnostic(diags[0])).toBe('#var1'); + }); + + it('should not produce component variable shadow template reference warning if used class name as variable', + () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': '
', + }, + source: + `export abstract class TestAbsCls { var1 = 'text'; } export class TestCmp extends TestAbsCls { var2: string = 'text'; }` + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), + [componentVariableShadowsTemplateReference], {} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(0); + }); + + it('should produce component variable shadow template reference warning for ngFor variable', + () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': '
', + }, + source: + `export class TestCmp extends TestAbsCls { item = 'text'; array: string[] = []; }` + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), + [componentVariableShadowsTemplateReference], {} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(1); + expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); + expect(diags[0].code) + .toBe(ngErrorCode(ErrorCode.COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE)); + expect(getSourceCodeForDiagnostic(diags[0]).trim()).toBe('let item'); + }); + + it('should produce component variable shadow template reference warning for ngIf as variable', + () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': '
', + }, + source: + `export class TestCmp extends TestAbsCls { result = 'text'; result$: Observable; }` + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), + [componentVariableShadowsTemplateReference], {} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(1); + expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); + expect(diags[0].code) + .toBe(ngErrorCode(ErrorCode.COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE)); + expect(getSourceCodeForDiagnostic(diags[0]).trim()) + .toBe('ngIf="result$ | async as result'); + }); }); });