From 6f11a580406877e440c43df31fae3d5f120cafed Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 26 May 2022 13:12:31 -0700 Subject: [PATCH] feat(compiler): Add extended diagnostic to warn when text attributes are intended to be bindings (#46161) https://angular.io/guide/attribute-binding#attribute-class-and-style-bindings Angular supports `attr.`, `style.`, and `class.` binding prefixes to bind attributes, styles, and classes. If the key does not have the binding syntax `[]` or the value does not have an interpolation `{{}}`, the attribute will not be interpreted as a binding. This diagnostic warns the user when the attributes listed above will not be interpreted as bindings. resolves #46137 PR Close #46161 --- goldens/public-api/compiler-cli/error_code.md | 1 + .../extended_template_diagnostic_name.md | 4 +- .../src/ngtsc/diagnostics/src/error_code.ts | 17 +++ .../src/extended_template_diagnostic_name.ts | 1 + .../src/ngtsc/typecheck/extended/BUILD.bazel | 1 + .../text_attribute_not_binding/BUILD.bazel | 17 +++ .../text_attribute_not_binding/index.ts | 68 +++++++++++ .../src/ngtsc/typecheck/extended/index.ts | 2 + .../text_attribute_not_binding/BUILD.bazel | 27 +++++ .../text_attribute_not_binding_spec.ts | 111 ++++++++++++++++++ 10 files changed, 248 insertions(+), 1 deletion(-) create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding/BUILD.bazel create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding/index.ts create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/text_attribute_not_binding/BUILD.bazel create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/text_attribute_not_binding/text_attribute_not_binding_spec.ts diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index 46612ddeac3b3..a98edb2a7f8cf 100644 --- a/goldens/public-api/compiler-cli/error_code.md +++ b/goldens/public-api/compiler-cli/error_code.md @@ -73,6 +73,7 @@ export enum ErrorCode { // (undocumented) SYMBOL_NOT_EXPORTED = 3001, TEMPLATE_PARSE_ERROR = 5002, + TEXT_ATTRIBUTE_NOT_BINDING = 8104, UNDECORATED_CLASS_USING_ANGULAR_FEATURES = 2007, UNDECORATED_PROVIDER = 2005, // (undocumented) 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 14c2369348ea1..8241cb63a5739 100644 --- a/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md +++ b/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md @@ -11,7 +11,9 @@ export enum ExtendedTemplateDiagnosticName { // (undocumented) MISSING_CONTROL_FLOW_DIRECTIVE = "missingControlFlowDirective", // (undocumented) - NULLISH_COALESCING_NOT_NULLABLE = "nullishCoalescingNotNullable" + NULLISH_COALESCING_NOT_NULLABLE = "nullishCoalescingNotNullable", + // (undocumented) + TEXT_ATTRIBUTE_NOT_BINDING = "textAttributeNotBinding" } // (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 486b8c3dfc22f..9b11235296796 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -239,6 +239,23 @@ export enum ErrorCode { */ MISSING_CONTROL_FLOW_DIRECTIVE = 8103, + /** + * A text attribute is not interpreted as a binding but likely intended to be. + * + * For example: + * ``` + *
+ *
+ * ``` + * + * All of the above attributes will just be static text attributes and will not be interpreted as + * bindings by the compiler. + */ + TEXT_ATTRIBUTE_NOT_BINDING = 8104, + /** * 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 4f58aeb142103..8002e7a9ffbf7 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 @@ -19,4 +19,5 @@ export enum ExtendedTemplateDiagnosticName { INVALID_BANANA_IN_BOX = 'invalidBananaInBox', NULLISH_COALESCING_NOT_NULLABLE = 'nullishCoalescingNotNullable', MISSING_CONTROL_FLOW_DIRECTIVE = 'missingControlFlowDirective', + TEXT_ATTRIBUTE_NOT_BINDING = 'textAttributeNotBinding', } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel index 1490aaaaf4877..9109bdd8b6a80 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel @@ -15,6 +15,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/invalid_banana_in_box", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable", + "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding", "@npm//typescript", ], ) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding/BUILD.bazel new file mode 100644 index 0000000000000..45e05ab4f3383 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding/BUILD.bazel @@ -0,0 +1,17 @@ +load("//tools:defaults.bzl", "ts_library") + +ts_library( + name = "text_attribute_not_binding", + 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/text_attribute_not_binding/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding/index.ts new file mode 100644 index 0000000000000..5b3c2db077119 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding/index.ts @@ -0,0 +1,68 @@ +/** + * @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, TmplAstTextAttribute} from '@angular/compiler'; +import ts from 'typescript'; + +import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics'; +import {NgTemplateDiagnostic} from '../../../api'; +import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api'; + +/** + * Ensures that attributes that have the "special" angular binding prefix (attr., style., and + * class.) are interpreted as bindings. For example, `
` will not + * interperet this as an `AttributeBinding` to `id` but rather just a `TmplAstTextAttribute`. This + * is likely not the intent of the developer. Instead, the intent is likely to have the `id` be set + * to 'my-id'. + */ +class TextAttributeNotBindingSpec extends + TemplateCheckWithVisitor { + override code = ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING as const; + + override visitNode( + ctx: TemplateContext, + component: ts.ClassDeclaration, + node: TmplAstNode|AST, + ): NgTemplateDiagnostic[] { + if (!(node instanceof TmplAstTextAttribute)) return []; + + const name = node.name; + if ((!name.startsWith('attr.') && !name.startsWith('style.') && !name.startsWith('class.'))) { + return []; + } + + let errorString: string; + if (name.startsWith('attr.')) { + const staticAttr = name.replace('attr.', ''); + errorString = `Static attributes should be written without the 'attr.' prefix.`; + if (node.value) { + errorString += ` For example, ${staticAttr}="${node.value}".`; + } + } else { + const expectedKey = `[${name}]`; + const expectedValue = + // true/false are special cases because we don't want to convert them to strings but + // rather maintain the logical true/false when bound. + (node.value === 'true' || node.value === 'false') ? node.value : `'${node.value}'`; + errorString = 'Attribute, style, and class bindings should be enclosed with square braces.'; + if (node.value) { + errorString += ` For example, '${expectedKey}="${expectedValue}"'.`; + } + } + const diagnostic = ctx.makeTemplateDiagnostic(node.sourceSpan, errorString); + return [diagnostic]; + } +} + +export const factory: TemplateCheckFactory< + ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING, + ExtendedTemplateDiagnosticName.TEXT_ATTRIBUTE_NOT_BINDING> = { + code: ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING, + name: ExtendedTemplateDiagnosticName.TEXT_ATTRIBUTE_NOT_BINDING, + create: () => new TextAttributeNotBindingSpec(), +}; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts index 9378767b36078..5823b60bcfeef 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts @@ -12,6 +12,7 @@ import {TemplateCheckFactory} from './api'; import {factory as invalidBananaInBoxFactory} from './checks/invalid_banana_in_box'; import {factory as missingControlFlowDirectiveFactory} from './checks/missing_control_flow_directive'; import {factory as nullishCoalescingNotNullableFactory} from './checks/nullish_coalescing_not_nullable'; +import {factory as textAttributeNotBindingFactory} from './checks/text_attribute_not_binding'; export {ExtendedTemplateCheckerImpl} from './src/extended_template_checker'; @@ -20,4 +21,5 @@ export const ALL_DIAGNOSTIC_FACTORIES: invalidBananaInBoxFactory, nullishCoalescingNotNullableFactory, missingControlFlowDirectiveFactory, + textAttributeNotBindingFactory, ]; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/text_attribute_not_binding/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/text_attribute_not_binding/BUILD.bazel new file mode 100644 index 0000000000000..41aa28514add0 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/text_attribute_not_binding/BUILD.bazel @@ -0,0 +1,27 @@ +load("//tools:defaults.bzl", "jasmine_node_test", "ts_library") + +ts_library( + name = "test_lib", + testonly = True, + srcs = ["text_attribute_not_binding_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/text_attribute_not_binding", + "//packages/compiler-cli/src/ngtsc/typecheck/testing", + "@npm//typescript", + ], +) + +jasmine_node_test( + name = "test", + bootstrap = ["//tools/testing:node_no_angular_es2015"], + deps = [ + ":test_lib", + ], +) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/text_attribute_not_binding/text_attribute_not_binding_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/text_attribute_not_binding/text_attribute_not_binding_spec.ts new file mode 100644 index 0000000000000..b940a36df5c00 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/text_attribute_not_binding/text_attribute_not_binding_spec.ts @@ -0,0 +1,111 @@ +/** + * @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 {DiagnosticCategoryLabel} from '@angular/compiler-cli/src/ngtsc/core/api'; +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 textAttributeNotBindingFactory} from '../../../checks/text_attribute_not_binding'; +import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker'; + +runInEachFileSystem(() => { + describe('TextAttributeNotBindingCheck', () => { + it('binds the error code to its extended template diagnostic name', () => { + expect(textAttributeNotBindingFactory.code).toBe(ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING); + expect(textAttributeNotBindingFactory.name) + .toBe(ExtendedTemplateDiagnosticName.TEXT_ATTRIBUTE_NOT_BINDING); + }); + + it('should produce class binding warning', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': `
`, + }, + source: 'export class TestCmp { }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [textAttributeNotBindingFactory], + {} /* 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.TEXT_ATTRIBUTE_NOT_BINDING)); + expect(getSourceCodeForDiagnostic(diags[0])).toBe(`class.blue="true"`); + }); + + it('should produce an attribute binding warning', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': `
`, + }, + source: 'export class TestCmp { }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [textAttributeNotBindingFactory], + {} /* 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.TEXT_ATTRIBUTE_NOT_BINDING)); + expect(getSourceCodeForDiagnostic(diags[0])).toBe(`attr.id="bar"`); + }); + + it('should produce a style binding warning', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': `
`, + }, + source: 'export class TestCmp { }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [textAttributeNotBindingFactory], + {} /* 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.TEXT_ATTRIBUTE_NOT_BINDING)); + expect(getSourceCodeForDiagnostic(diags[0])).toBe(`style.margin-right.px="5"`); + }); + + it('should not produce a warning when there is no value', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': `
`, + }, + source: 'export class TestCmp { }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [textAttributeNotBindingFactory], + {} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(1); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING)); + expect(getSourceCodeForDiagnostic(diags[0])).toBe(`attr.readonly`); + }); + }); +});