Skip to content

Commit 6f11a58

Browse files
atscottatscott
committed
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
1 parent 5dd9d4a commit 6f11a58

File tree

10 files changed

+248
-1
lines changed

10 files changed

+248
-1
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ export enum ErrorCode {
7373
// (undocumented)
7474
SYMBOL_NOT_EXPORTED = 3001,
7575
TEMPLATE_PARSE_ERROR = 5002,
76+
TEXT_ATTRIBUTE_NOT_BINDING = 8104,
7677
UNDECORATED_CLASS_USING_ANGULAR_FEATURES = 2007,
7778
UNDECORATED_PROVIDER = 2005,
7879
// (undocumented)

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@ export enum ExtendedTemplateDiagnosticName {
1111
// (undocumented)
1212
MISSING_CONTROL_FLOW_DIRECTIVE = "missingControlFlowDirective",
1313
// (undocumented)
14-
NULLISH_COALESCING_NOT_NULLABLE = "nullishCoalescingNotNullable"
14+
NULLISH_COALESCING_NOT_NULLABLE = "nullishCoalescingNotNullable",
15+
// (undocumented)
16+
TEXT_ATTRIBUTE_NOT_BINDING = "textAttributeNotBinding"
1517
}
1618

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

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,23 @@ export enum ErrorCode {
239239
*/
240240
MISSING_CONTROL_FLOW_DIRECTIVE = 8103,
241241

242+
/**
243+
* A text attribute is not interpreted as a binding but likely intended to be.
244+
*
245+
* For example:
246+
* ```
247+
* <div
248+
* attr.x="value"
249+
* class.blue="true"
250+
* style.margin-right.px="5">
251+
* </div>
252+
* ```
253+
*
254+
* All of the above attributes will just be static text attributes and will not be interpreted as
255+
* bindings by the compiler.
256+
*/
257+
TEXT_ATTRIBUTE_NOT_BINDING = 8104,
258+
242259
/**
243260
* The template type-checking engine would need to generate an inline type check block for a
244261
* 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
@@ -19,4 +19,5 @@ export enum ExtendedTemplateDiagnosticName {
1919
INVALID_BANANA_IN_BOX = 'invalidBananaInBox',
2020
NULLISH_COALESCING_NOT_NULLABLE = 'nullishCoalescingNotNullable',
2121
MISSING_CONTROL_FLOW_DIRECTIVE = 'missingControlFlowDirective',
22+
TEXT_ATTRIBUTE_NOT_BINDING = 'textAttributeNotBinding',
2223
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ ts_library(
1515
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/invalid_banana_in_box",
1616
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive",
1717
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable",
18+
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding",
1819
"@npm//typescript",
1920
],
2021
)

‎packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding/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 = "text_attribute_not_binding",
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/text_attribute_not_binding/index.ts‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
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, TmplAstNode, TmplAstTextAttribute} 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+
/**
17+
* Ensures that attributes that have the "special" angular binding prefix (attr., style., and
18+
* class.) are interpreted as bindings. For example, `<div attr.id="my-id"></div>` will not
19+
* interperet this as an `AttributeBinding` to `id` but rather just a `TmplAstTextAttribute`. This
20+
* is likely not the intent of the developer. Instead, the intent is likely to have the `id` be set
21+
* to 'my-id'.
22+
*/
23+
class TextAttributeNotBindingSpec extends
24+
TemplateCheckWithVisitor<ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING> {
25+
override code = ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING as const;
26+
27+
override visitNode(
28+
ctx: TemplateContext<ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING>,
29+
component: ts.ClassDeclaration,
30+
node: TmplAstNode|AST,
31+
): NgTemplateDiagnostic<ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING>[] {
32+
if (!(node instanceof TmplAstTextAttribute)) return [];
33+
34+
const name = node.name;
35+
if ((!name.startsWith('attr.') && !name.startsWith('style.') && !name.startsWith('class.'))) {
36+
return [];
37+
}
38+
39+
let errorString: string;
40+
if (name.startsWith('attr.')) {
41+
const staticAttr = name.replace('attr.', '');
42+
errorString = `Static attributes should be written without the 'attr.' prefix.`;
43+
if (node.value) {
44+
errorString += ` For example, ${staticAttr}="${node.value}".`;
45+
}
46+
} else {
47+
const expectedKey = `[${name}]`;
48+
const expectedValue =
49+
// true/false are special cases because we don't want to convert them to strings but
50+
// rather maintain the logical true/false when bound.
51+
(node.value === 'true' || node.value === 'false') ? node.value : `'${node.value}'`;
52+
errorString = 'Attribute, style, and class bindings should be enclosed with square braces.';
53+
if (node.value) {
54+
errorString += ` For example, '${expectedKey}="${expectedValue}"'.`;
55+
}
56+
}
57+
const diagnostic = ctx.makeTemplateDiagnostic(node.sourceSpan, errorString);
58+
return [diagnostic];
59+
}
60+
}
61+
62+
export const factory: TemplateCheckFactory<
63+
ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING,
64+
ExtendedTemplateDiagnosticName.TEXT_ATTRIBUTE_NOT_BINDING> = {
65+
code: ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING,
66+
name: ExtendedTemplateDiagnosticName.TEXT_ATTRIBUTE_NOT_BINDING,
67+
create: () => new TextAttributeNotBindingSpec(),
68+
};

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {TemplateCheckFactory} from './api';
1212
import {factory as invalidBananaInBoxFactory} from './checks/invalid_banana_in_box';
1313
import {factory as missingControlFlowDirectiveFactory} from './checks/missing_control_flow_directive';
1414
import {factory as nullishCoalescingNotNullableFactory} from './checks/nullish_coalescing_not_nullable';
15+
import {factory as textAttributeNotBindingFactory} from './checks/text_attribute_not_binding';
1516

1617
export {ExtendedTemplateCheckerImpl} from './src/extended_template_checker';
1718

@@ -20,4 +21,5 @@ export const ALL_DIAGNOSTIC_FACTORIES:
2021
invalidBananaInBoxFactory,
2122
nullishCoalescingNotNullableFactory,
2223
missingControlFlowDirectiveFactory,
24+
textAttributeNotBindingFactory,
2325
];

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
load("//tools:defaults.bzl", "jasmine_node_test", "ts_library")
2+
3+
ts_library(
4+
name = "test_lib",
5+
testonly = True,
6+
srcs = ["text_attribute_not_binding_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/text_attribute_not_binding",
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_es2015"],
24+
deps = [
25+
":test_lib",
26+
],
27+
)

‎packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/text_attribute_not_binding/text_attribute_not_binding_spec.ts‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
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 {DiagnosticCategoryLabel} from '@angular/compiler-cli/src/ngtsc/core/api';
10+
import ts from 'typescript';
11+
12+
import {ErrorCode, ExtendedTemplateDiagnosticName, ngErrorCode} from '../../../../../diagnostics';
13+
import {absoluteFrom, getSourceFileOrError} from '../../../../../file_system';
14+
import {runInEachFileSystem} from '../../../../../file_system/testing';
15+
import {getSourceCodeForDiagnostic} from '../../../../../testing';
16+
import {getClass, setup} from '../../../../testing';
17+
import {factory as textAttributeNotBindingFactory} from '../../../checks/text_attribute_not_binding';
18+
import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker';
19+
20+
runInEachFileSystem(() => {
21+
describe('TextAttributeNotBindingCheck', () => {
22+
it('binds the error code to its extended template diagnostic name', () => {
23+
expect(textAttributeNotBindingFactory.code).toBe(ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING);
24+
expect(textAttributeNotBindingFactory.name)
25+
.toBe(ExtendedTemplateDiagnosticName.TEXT_ATTRIBUTE_NOT_BINDING);
26+
});
27+
28+
it('should produce class binding warning', () => {
29+
const fileName = absoluteFrom('/main.ts');
30+
const {program, templateTypeChecker} = setup([{
31+
fileName,
32+
templates: {
33+
'TestCmp': `<div class.blue="true"></div>`,
34+
},
35+
source: 'export class TestCmp { }'
36+
}]);
37+
const sf = getSourceFileOrError(program, fileName);
38+
const component = getClass(sf, 'TestCmp');
39+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
40+
templateTypeChecker, program.getTypeChecker(), [textAttributeNotBindingFactory],
41+
{} /* options */);
42+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
43+
expect(diags.length).toBe(1);
44+
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
45+
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING));
46+
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`class.blue="true"`);
47+
});
48+
49+
it('should produce an attribute binding warning', () => {
50+
const fileName = absoluteFrom('/main.ts');
51+
const {program, templateTypeChecker} = setup([{
52+
fileName,
53+
templates: {
54+
'TestCmp': `<div attr.id="bar"></div>`,
55+
},
56+
source: 'export class TestCmp { }'
57+
}]);
58+
const sf = getSourceFileOrError(program, fileName);
59+
const component = getClass(sf, 'TestCmp');
60+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
61+
templateTypeChecker, program.getTypeChecker(), [textAttributeNotBindingFactory],
62+
{} /* options */);
63+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
64+
expect(diags.length).toBe(1);
65+
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
66+
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING));
67+
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`attr.id="bar"`);
68+
});
69+
70+
it('should produce a style binding warning', () => {
71+
const fileName = absoluteFrom('/main.ts');
72+
const {program, templateTypeChecker} = setup([{
73+
fileName,
74+
templates: {
75+
'TestCmp': `<div style.margin-right.px="5"></div>`,
76+
},
77+
source: 'export class TestCmp { }'
78+
}]);
79+
const sf = getSourceFileOrError(program, fileName);
80+
const component = getClass(sf, 'TestCmp');
81+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
82+
templateTypeChecker, program.getTypeChecker(), [textAttributeNotBindingFactory],
83+
{} /* options */);
84+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
85+
expect(diags.length).toBe(1);
86+
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
87+
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING));
88+
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`style.margin-right.px="5"`);
89+
});
90+
91+
it('should not produce a warning when there is no value', () => {
92+
const fileName = absoluteFrom('/main.ts');
93+
const {program, templateTypeChecker} = setup([{
94+
fileName,
95+
templates: {
96+
'TestCmp': `<div attr.readonly></div>`,
97+
},
98+
source: 'export class TestCmp { }'
99+
}]);
100+
const sf = getSourceFileOrError(program, fileName);
101+
const component = getClass(sf, 'TestCmp');
102+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
103+
templateTypeChecker, program.getTypeChecker(), [textAttributeNotBindingFactory],
104+
{} /* options */);
105+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
106+
expect(diags.length).toBe(1);
107+
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.TEXT_ATTRIBUTE_NOT_BINDING));
108+
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`attr.readonly`);
109+
});
110+
});
111+
});

0 commit comments

Comments
 (0)