From 6ba5fdc2082cc668f53f82afc36776f2cb24dcd6 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 10 Dec 2019 17:34:11 -0800 Subject: [PATCH] fix(ivy): generate a better error for template var writes (#34339) In Ivy it's illegal for a template to write to a template variable. So the template: ```html ``` is erroneous and previously would fail to compile with an assertion error from the `TemplateDefinitionBuilder`. This error wasn't particularly user- friendly, though, as it lacked the context of which template or where the error occurred. In this commit, a new check in template type-checking is added which detects such erroneous writes and produces a true diagnostic with the appropriate context information. Closes #33674 PR Close #34339 --- .../src/ngtsc/diagnostics/src/code.ts | 14 +++++ .../src/ngtsc/typecheck/src/diagnostics.ts | 52 ++++++++++++++----- .../src/ngtsc/typecheck/src/oob.ts | 26 +++++++++- .../ngtsc/typecheck/src/template_semantics.ts | 44 ++++++++++++++++ .../ngtsc/typecheck/src/type_check_block.ts | 21 +++++++- .../src/ngtsc/typecheck/test/test_utils.ts | 1 + .../typecheck/test/type_check_block_spec.ts | 6 +++ .../test/ngtsc/template_typecheck_spec.ts | 28 ++++++++++ 8 files changed, 178 insertions(+), 14 deletions(-) create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/src/template_semantics.ts diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts index 9e9c7cd8ae..10e3c3ee44 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts @@ -95,6 +95,20 @@ export enum ErrorCode { */ MISSING_PIPE = 8004, + /** + * The left-hand side of an assignment expression was a template variable. Effectively, the + * template looked like: + * + * ``` + * + * + * + * ``` + * + * Template variables are read-only. + */ + WRITE_TO_READ_ONLY_VARIABLE = 8005, + /** * An injectable already has a `ɵprov` property. */ diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts index 6f761c843c..8d9dd5a266 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts @@ -158,8 +158,22 @@ export function translateDiagnostic( */ export function makeTemplateDiagnostic( mapping: TemplateSourceMapping, span: ParseSourceSpan, category: ts.DiagnosticCategory, - code: number, messageText: string | ts.DiagnosticMessageChain): ts.Diagnostic { + code: number, messageText: string | ts.DiagnosticMessageChain, relatedMessage?: { + text: string, + span: ParseSourceSpan, + }): ts.Diagnostic { if (mapping.type === 'direct') { + let relatedInformation: ts.DiagnosticRelatedInformation[]|undefined = undefined; + if (relatedMessage !== undefined) { + relatedInformation = [{ + category: ts.DiagnosticCategory.Message, + code: 0, + file: mapping.node.getSourceFile(), + start: relatedMessage.span.start.offset, + length: relatedMessage.span.end.offset - relatedMessage.span.start.offset, + messageText: relatedMessage.text, + }]; + } // For direct mappings, the error is shown inline as ngtsc was able to pinpoint a string // constant within the `@Component` decorator for the template. This allows us to map the error // directly into the bytes of the source file. @@ -170,7 +184,7 @@ export function makeTemplateDiagnostic( messageText, file: mapping.node.getSourceFile(), start: span.start.offset, - length: span.end.offset - span.start.offset, + length: span.end.offset - span.start.offset, relatedInformation, }; } else if (mapping.type === 'indirect' || mapping.type === 'external') { // For indirect mappings (template was declared inline, but ngtsc couldn't map it directly @@ -189,6 +203,29 @@ export function makeTemplateDiagnostic( const sf = ts.createSourceFile( fileName, mapping.template, ts.ScriptTarget.Latest, false, ts.ScriptKind.JSX); + let relatedInformation: ts.DiagnosticRelatedInformation[] = []; + if (relatedMessage !== undefined) { + relatedInformation.push({ + category: ts.DiagnosticCategory.Message, + code: 0, + file: sf, + start: relatedMessage.span.start.offset, + length: relatedMessage.span.end.offset - relatedMessage.span.start.offset, + messageText: relatedMessage.text, + }); + } + + relatedInformation.push({ + category: ts.DiagnosticCategory.Message, + code: 0, + file: componentSf, + // mapping.node represents either the 'template' or 'templateUrl' expression. getStart() + // and getEnd() are used because they don't include surrounding whitespace. + start: mapping.node.getStart(), + length: mapping.node.getEnd() - mapping.node.getStart(), + messageText: `Error occurs in the template of component ${componentName}.`, + }); + return { source: 'ngtsc', category, @@ -198,16 +235,7 @@ export function makeTemplateDiagnostic( start: span.start.offset, length: span.end.offset - span.start.offset, // Show a secondary message indicating the component whose template contains the error. - relatedInformation: [{ - category: ts.DiagnosticCategory.Message, - code: 0, - file: componentSf, - // mapping.node represents either the 'template' or 'templateUrl' expression. getStart() - // and getEnd() are used because they don't include surrounding whitespace. - start: mapping.node.getStart(), - length: mapping.node.getEnd() - mapping.node.getStart(), - messageText: `Error occurs in the template of component ${componentName}.`, - }], + relatedInformation, }; } else { throw new Error(`Unexpected source mapping type: ${(mapping as {type: string}).type}`); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts index 4204f75b01..3cd5b21fa1 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {AbsoluteSourceSpan, BindingPipe, TmplAstReference} from '@angular/compiler'; +import {AbsoluteSourceSpan, BindingPipe, PropertyWrite, TmplAstReference, TmplAstVariable} from '@angular/compiler'; import * as ts from 'typescript'; import {ErrorCode, ngErrorCode} from '../../diagnostics'; @@ -49,6 +49,10 @@ export interface OutOfBandDiagnosticRecorder { * plus span of the larger expression context. */ missingPipe(templateId: string, ast: BindingPipe, sourceSpan: AbsoluteSourceSpan): void; + + illegalAssignmentToTemplateVar( + templateId: string, assignment: PropertyWrite, assignmentSpan: AbsoluteSourceSpan, + target: TmplAstVariable): void; } export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder { @@ -82,4 +86,24 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor mapping, sourceSpan, ts.DiagnosticCategory.Error, ngErrorCode(ErrorCode.MISSING_PIPE), errorMsg)); } + + illegalAssignmentToTemplateVar( + templateId: string, assignment: PropertyWrite, assignmentSpan: AbsoluteSourceSpan, + target: TmplAstVariable): void { + const mapping = this.resolver.getSourceMapping(templateId); + const errorMsg = + `Cannot use variable '${assignment.name}' as the left-hand side of an assignment expression. Template variables are read-only.`; + + const location = absoluteSourceSpanToSourceLocation(templateId, assignmentSpan); + const sourceSpan = this.resolver.sourceLocationToSpan(location); + if (sourceSpan === null) { + throw new Error(`Assertion failure: no SourceLocation found for property binding.`); + } + this._diagnostics.push(makeTemplateDiagnostic( + mapping, sourceSpan, ts.DiagnosticCategory.Error, + ngErrorCode(ErrorCode.WRITE_TO_READ_ONLY_VARIABLE), errorMsg, { + text: `The variable ${assignment.name} is declared here.`, + span: target.valueSpan || target.sourceSpan, + })); + } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/template_semantics.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/template_semantics.ts new file mode 100644 index 0000000000..24a294bca3 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/template_semantics.ts @@ -0,0 +1,44 @@ +/** + * @license + * Copyright Google Inc. 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, BoundTarget, ImplicitReceiver, ParseSourceSpan, PropertyWrite, RecursiveAstVisitor, TmplAstVariable} from '@angular/compiler'; + +import {toAbsoluteSpan} from './diagnostics'; +import {OutOfBandDiagnosticRecorder} from './oob'; + +/** + * Visits a template and records any semantic errors within its expressions. + */ +export class ExpressionSemanticVisitor extends RecursiveAstVisitor { + constructor( + private templateId: string, private boundTarget: BoundTarget, + private oob: OutOfBandDiagnosticRecorder, private sourceSpan: ParseSourceSpan) { + super(); + } + + visitPropertyWrite(ast: PropertyWrite, context: any): void { + super.visitPropertyWrite(ast, context); + + if (!(ast.receiver instanceof ImplicitReceiver)) { + return; + } + + const target = this.boundTarget.getExpressionTarget(ast); + if (target instanceof TmplAstVariable) { + // Template variables are read-only. + const astSpan = toAbsoluteSpan(ast.span, this.sourceSpan); + this.oob.illegalAssignmentToTemplateVar(this.templateId, ast, astSpan, target); + } + } + + static visit( + ast: AST, sourceSpan: ParseSourceSpan, id: string, boundTarget: BoundTarget, + oob: OutOfBandDiagnosticRecorder): void { + ast.visit(new ExpressionSemanticVisitor(id, boundTarget, oob, sourceSpan)); + } +} diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index 86be2c6bee..802dc61b12 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {AST, BindingPipe, BindingType, BoundTarget, DYNAMIC_TYPE, ImplicitReceiver, MethodCall, ParseSourceSpan, ParseSpan, ParsedEventType, PropertyRead, SchemaMetadata, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable} from '@angular/compiler'; +import {AST, BindingPipe, BindingType, BoundTarget, DYNAMIC_TYPE, ImplicitReceiver, MethodCall, ParseSourceSpan, ParseSpan, ParsedEventType, PropertyRead, PropertyWrite, SchemaMetadata, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable} from '@angular/compiler'; import * as ts from 'typescript'; import {Reference} from '../../imports'; @@ -18,6 +18,7 @@ import {DomSchemaChecker} from './dom'; import {Environment} from './environment'; import {NULL_AS_ANY, astToTypescript} from './expression'; import {OutOfBandDiagnosticRecorder} from './oob'; +import {ExpressionSemanticVisitor} from './template_semantics'; import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsCastToAny, tsCreateElement, tsCreateVariable, tsDeclareVariable} from './ts_util'; @@ -491,6 +492,10 @@ class TcbDirectiveOutputsOp extends TcbOp { const handler = tcbCreateEventHandler(output, this.tcb, this.scope, EventParamType.Any); this.scope.addStatement(ts.createExpressionStatement(handler)); } + + ExpressionSemanticVisitor.visit( + output.handler, output.handlerSpan, this.tcb.id, this.tcb.boundTarget, + this.tcb.oobRecorder); } return null; @@ -549,6 +554,10 @@ class TcbUnclaimedOutputsOp extends TcbOp { const handler = tcbCreateEventHandler(output, this.tcb, this.scope, EventParamType.Any); this.scope.addStatement(ts.createExpressionStatement(handler)); } + + ExpressionSemanticVisitor.visit( + output.handler, output.handlerSpan, this.tcb.id, this.tcb.boundTarget, + this.tcb.oobRecorder); } return null; @@ -972,6 +981,16 @@ class TcbExpressionTranslator { // returned here to let it fall through resolution so it will be caught when the // `ImplicitReceiver` is resolved in the branch below. return this.resolveTarget(ast); + } else if (ast instanceof PropertyWrite && ast.receiver instanceof ImplicitReceiver) { + const target = this.resolveTarget(ast); + if (target === null) { + return null; + } + + const expr = this.translate(ast.value); + const result = ts.createParen(ts.createBinary(target, ts.SyntaxKind.EqualsToken, expr)); + addParseSpanInfo(result, toAbsoluteSpan(ast.span, this.sourceSpan)); + return result; } else if (ast instanceof ImplicitReceiver) { // AST instances representing variables and references look very similar to property reads // or method calls from the component context: both have the shape diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index 5d816acb4a..003914282d 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -389,4 +389,5 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder { get diagnostics(): ReadonlyArray { return []; } missingReferenceTarget(): void {} missingPipe(): void {} + illegalAssignmentToTemplateVar(): void {} } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts index 166f68a26c..6e35fb68f7 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts @@ -280,6 +280,12 @@ describe('type check blocks', () => { '_t1.addEventListener("event", ($event): any => (ctx).foo(($event as any)));'); }); + it('should detect writes to template variables', () => { + const TEMPLATE = `
`; + const block = tcb(TEMPLATE); + expect(block).toContain('_t3.addEventListener("event", ($event): any => (_t2 = 3))'); + }); + }); describe('config', () => { diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 76da4e324a..49f31c2ad4 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -1121,6 +1121,34 @@ export declare class AnimationEvent { expect(getSourceCodeForDiagnostic(diags[2])).toEqual('[fromChild]="4"'); }); + it('should detect an illegal write to a template variable', () => { + env.write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + import {CommonModule} from '@angular/common'; + + @Component({ + selector: 'test', + template: \` +
+ +
+ \`, + }) + export class TestCmp { + x!: boolean; + } + + @NgModule({ + declarations: [TestCmp], + imports: [CommonModule], + }) + export class Module {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toEqual(1); + expect(getSourceCodeForDiagnostic(diags[0])).toEqual('y = !y'); + }); + describe('input coercion', () => { beforeEach(() => { env.tsconfig({fullTemplateTypeCheck: true, strictInputTypes: true});