Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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
- Loading branch information
Showing
10 changed files
with
248 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
17 changes: 17 additions & 0 deletions
17
...s/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding/BUILD.bazel
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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", | ||
], | ||
) |
68 changes: 68 additions & 0 deletions
68
...ages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding/index.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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(), | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
27 changes: 27 additions & 0 deletions
27
...piler-cli/src/ngtsc/typecheck/extended/test/checks/text_attribute_not_binding/BUILD.bazel
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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", | ||
], | ||
) |
111 changes: 111 additions & 0 deletions
111
...echeck/extended/test/checks/text_attribute_not_binding/text_attribute_not_binding_spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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`); | ||
}); | ||
}); | ||
}); |