feat(compiler): Add extended diagnostic to warn when text attributes …

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)

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)

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:
+   * ```
+   * <div
+   *   attr.x="value"
+   *   class.blue="true"
+   *   style.margin-right.px="5">
+   * </div>
+   * ```
+   *
+   * 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.

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',
 }

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",
     ],
 )

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",
+    ],
+)

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, `<div attr.id="my-id"></div>` 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<ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING> {
+  override code = ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING as const;
+
+  override visitNode(
+      ctx: TemplateContext<ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING>,
+      component: ts.ClassDeclaration,
+      node: TmplAstNode|AST,
+      ): NgTemplateDiagnostic<ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING>[] {
+    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(),
+};

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,
     ];

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",
+    ],
+)

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': `<div class.blue="true"></div>`,
+        },
+        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': `<div attr.id="bar"></div>`,
+        },
+        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': `<div style.margin-right.px="5"></div>`,
+        },
+        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': `<div attr.readonly></div>`,
+        },
+        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`);
+    });
+  });
+});