Skip to content

Commit ba9ddd7

Browse files
crisbetoatscott
crisbeto
authored and
atscott
committed
refactor(compiler-cli): move illegal template assignment check into template semantics checker (#54714)
Moves the check which ensures that there are no writes to template variables into the `TemplateSemanticsChecker` to prepare for the upcoming changes. PR Close #54714
1 parent 5d23e60 commit ba9ddd7

File tree

8 files changed

+77
-82
lines changed

8 files changed

+77
-82
lines changed

‎packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {AST, LiteralPrimitive, ParseSourceSpan, PropertyRead, SafePropertyRead, TmplAstElement, TmplAstNode, TmplAstTemplate, TmplAstTextAttribute} from '@angular/compiler';
9+
import {AST, LiteralPrimitive, ParseSourceSpan, PropertyRead, SafePropertyRead, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable} from '@angular/compiler';
1010
import ts from 'typescript';
1111

1212
import {AbsoluteFsPath} from '../../../../src/ngtsc/file_system';
@@ -213,6 +213,12 @@ export interface TemplateTypeChecker {
213213
*/
214214
invalidateClass(clazz: ts.ClassDeclaration): void;
215215

216+
/**
217+
* Gets the target of a template expression, if possible.
218+
*/
219+
getExpressionTarget(expression: AST, clazz: ts.ClassDeclaration): TmplAstReference|TmplAstVariable
220+
|null;
221+
216222
/**
217223
* Constructs a `ts.Diagnostic` for a given `ParseSourceSpan` within a template.
218224
*/

‎packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {AST, CssSelector, DomElementSchemaRegistry, ExternalExpr, LiteralPrimitive, ParseSourceSpan, PropertyRead, SafePropertyRead, TmplAstElement, TmplAstNode, TmplAstTemplate, TmplAstTextAttribute, WrappedNodeExpr} from '@angular/compiler';
9+
import {AST, CssSelector, DomElementSchemaRegistry, ExternalExpr, LiteralPrimitive, ParseSourceSpan, PropertyRead, SafePropertyRead, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable, WrappedNodeExpr} from '@angular/compiler';
1010
import ts from 'typescript';
1111

1212
import {ErrorCode, ngErrorCode} from '../../diagnostics';
@@ -345,6 +345,12 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker {
345345
this.isComplete = false;
346346
}
347347

348+
getExpressionTarget(expression: AST, clazz: ts.ClassDeclaration): TmplAstReference|TmplAstVariable
349+
|null {
350+
return this.getLatestComponentState(clazz).data?.boundTarget.getExpressionTarget(expression) ||
351+
null;
352+
}
353+
348354
makeTemplateDiagnostic<T extends ErrorCode>(
349355
clazz: ts.ClassDeclaration, sourceSpan: ParseSourceSpan, category: ts.DiagnosticCategory,
350356
errorCode: T, message: string, relatedInformation?: {

‎packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,6 @@ export interface OutOfBandDiagnosticRecorder {
7070
*/
7171
deferredComponentUsedEagerly(templateId: TemplateId, element: TmplAstElement): void;
7272

73-
illegalAssignmentToTemplateVar(
74-
templateId: TemplateId, assignment: PropertyWrite, target: TmplAstVariable): void;
75-
7673
/**
7774
* Reports a duplicate declaration of a template variable.
7875
*
@@ -217,27 +214,6 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
217214
ngErrorCode(ErrorCode.DEFERRED_DIRECTIVE_USED_EAGERLY), errorMsg));
218215
}
219216

220-
illegalAssignmentToTemplateVar(
221-
templateId: TemplateId, assignment: PropertyWrite, target: TmplAstVariable): void {
222-
const mapping = this.resolver.getSourceMapping(templateId);
223-
const errorMsg = `Cannot use variable '${
224-
assignment
225-
.name}' as the left-hand side of an assignment expression. Template variables are read-only.`;
226-
227-
const sourceSpan = this.resolver.toParseSourceSpan(templateId, assignment.sourceSpan);
228-
if (sourceSpan === null) {
229-
throw new Error(`Assertion failure: no SourceLocation found for property binding.`);
230-
}
231-
this._diagnostics.push(makeTemplateDiagnostic(
232-
templateId, mapping, sourceSpan, ts.DiagnosticCategory.Error,
233-
ngErrorCode(ErrorCode.WRITE_TO_READ_ONLY_VARIABLE), errorMsg, [{
234-
text: `The variable ${assignment.name} is declared here.`,
235-
start: target.valueSpan?.start.offset || target.sourceSpan.start.offset,
236-
end: target.valueSpan?.end.offset || target.sourceSpan.end.offset,
237-
sourceFile: mapping.node.getSourceFile(),
238-
}]));
239-
}
240-
241217
duplicateTemplateVar(
242218
templateId: TemplateId, variable: TmplAstVariable, firstDecl: TmplAstVariable): void {
243219
const mapping = this.resolver.getSourceMapping(templateId);

‎packages/compiler-cli/src/ngtsc/typecheck/src/template_semantics.ts‎

Lines changed: 0 additions & 44 deletions
This file was deleted.

‎packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import {DomSchemaChecker} from './dom';
2020
import {Environment} from './environment';
2121
import {astToTypescript, NULL_AS_ANY} from './expression';
2222
import {OutOfBandDiagnosticRecorder} from './oob';
23-
import {ExpressionSemanticVisitor} from './template_semantics';
2423
import {tsCallMethod, tsCastToAny, tsCreateElement, tsCreateTypeQueryForCoercedInput, tsCreateVariable, tsDeclareVariable} from './ts_util';
2524
import {requiresInlineTypeCtor} from './type_constructor';
2625
import {TypeParameterEmitter} from './type_parameter_emitter';
@@ -1214,9 +1213,6 @@ export class TcbDirectiveOutputsOp extends TcbOp {
12141213
const handler = tcbCreateEventHandler(output, this.tcb, this.scope, EventParamType.Any);
12151214
this.scope.addStatement(ts.factory.createExpressionStatement(handler));
12161215
}
1217-
1218-
ExpressionSemanticVisitor.visit(
1219-
output.handler, this.tcb.id, this.tcb.boundTarget, this.tcb.oobRecorder);
12201216
}
12211217

12221218
return null;
@@ -1292,9 +1288,6 @@ class TcbUnclaimedOutputsOp extends TcbOp {
12921288
const handler = tcbCreateEventHandler(output, this.tcb, this.scope, EventParamType.Any);
12931289
this.scope.addStatement(ts.factory.createExpressionStatement(handler));
12941290
}
1295-
1296-
ExpressionSemanticVisitor.visit(
1297-
output.handler, this.tcb.id, this.tcb.boundTarget, this.tcb.oobRecorder);
12981291
}
12991292

13001293
return null;

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ ts_library(
1010
"//packages/compiler",
1111
"//packages/compiler-cli/src/ngtsc/core:api",
1212
"//packages/compiler-cli/src/ngtsc/diagnostics",
13+
"//packages/compiler-cli/src/ngtsc/typecheck",
1314
"//packages/compiler-cli/src/ngtsc/typecheck/api",
1415
"//packages/compiler-cli/src/ngtsc/typecheck/template_semantics/api",
1516
"@npm//typescript",

‎packages/compiler-cli/src/ngtsc/typecheck/template_semantics/src/template_semantics_checker.ts‎

Lines changed: 62 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,10 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9+
import {ImplicitReceiver, PropertyWrite, RecursiveAstVisitor, TmplAstBoundEvent, TmplAstNode, TmplAstRecursiveVisitor, TmplAstVariable} from '@angular/compiler';
910
import ts from 'typescript';
1011

11-
import {DiagnosticCategoryLabel} from '../../../core/api';
12+
import {ErrorCode, ngErrorCode} from '../../../diagnostics';
1213
import {TemplateDiagnostic, TemplateTypeChecker} from '../../api';
1314
import {TemplateSemanticsChecker} from '../api/api';
1415

@@ -17,12 +18,69 @@ export class TemplateSemanticsCheckerImpl implements TemplateSemanticsChecker {
1718

1819
getDiagnosticsForComponent(component: ts.ClassDeclaration): TemplateDiagnostic[] {
1920
const template = this.templateTypeChecker.getTemplate(component);
21+
return template !== null ?
22+
TemplateSemanticsVisitor.visit(template, component, this.templateTypeChecker) :
23+
[];
24+
}
25+
}
2026

21-
if (template === null) {
22-
return [];
23-
}
27+
/** Visitor that verifies the semantics of a template. */
28+
class TemplateSemanticsVisitor extends TmplAstRecursiveVisitor {
29+
private constructor(private expressionVisitor: ExpressionsSemanticsVisitor) {
30+
super();
31+
}
2432

33+
static visit(
34+
nodes: TmplAstNode[], component: ts.ClassDeclaration,
35+
templateTypeChecker: TemplateTypeChecker) {
2536
const diagnostics: TemplateDiagnostic[] = [];
37+
const expressionVisitor =
38+
new ExpressionsSemanticsVisitor(templateTypeChecker, component, diagnostics);
39+
const templateVisitor = new TemplateSemanticsVisitor(expressionVisitor);
40+
nodes.forEach(node => node.visit(templateVisitor));
2641
return diagnostics;
2742
}
43+
44+
override visitBoundEvent(event: TmplAstBoundEvent): void {
45+
super.visitBoundEvent(event);
46+
event.handler.visit(this.expressionVisitor, event);
47+
}
48+
}
49+
50+
/** Visitor that verifies the semantics of the expressions within a template. */
51+
class ExpressionsSemanticsVisitor extends RecursiveAstVisitor {
52+
constructor(
53+
private templateTypeChecker: TemplateTypeChecker, private component: ts.ClassDeclaration,
54+
private diagnostics: TemplateDiagnostic[]) {
55+
super();
56+
}
57+
58+
override visitPropertyWrite(ast: PropertyWrite, context: TmplAstNode): void {
59+
super.visitPropertyWrite(ast, context);
60+
61+
if (!(context instanceof TmplAstBoundEvent) || !(ast.receiver instanceof ImplicitReceiver)) {
62+
return;
63+
}
64+
65+
const target = this.templateTypeChecker.getExpressionTarget(ast, this.component);
66+
if (target instanceof TmplAstVariable) {
67+
const errorMessage = `Cannot use variable '${
68+
target
69+
.name}' as the left-hand side of an assignment expression. Template variables are read-only.`;
70+
this.diagnostics.push(this.makeIllegalTemplateVarDiagnostic(target, context, errorMessage));
71+
}
72+
}
73+
74+
private makeIllegalTemplateVarDiagnostic(
75+
target: TmplAstVariable, expressionNode: TmplAstNode,
76+
errorMessage: string): TemplateDiagnostic {
77+
return this.templateTypeChecker.makeTemplateDiagnostic(
78+
this.component, expressionNode.sourceSpan, ts.DiagnosticCategory.Error,
79+
ngErrorCode(ErrorCode.WRITE_TO_READ_ONLY_VARIABLE), errorMessage, [{
80+
text: `The variable ${target.name} is declared here.`,
81+
start: target.valueSpan?.start.offset || target.sourceSpan.start.offset,
82+
end: target.valueSpan?.end.offset || target.sourceSpan.end.offset,
83+
sourceFile: this.component.getSourceFile(),
84+
}]);
85+
}
2886
}

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -859,7 +859,6 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
859859
missingPipe(): void {}
860860
deferredPipeUsedEagerly(templateId: TemplateId, ast: BindingPipe): void {}
861861
deferredComponentUsedEagerly(templateId: TemplateId, element: TmplAstElement): void {}
862-
illegalAssignmentToTemplateVar(): void {}
863862
duplicateTemplateVar(): void {}
864863
requiresInlineTcb(): void {}
865864
requiresInlineTypeConstructors(): void {}

0 commit comments

Comments
 (0)