Skip to content
Permalink
Browse files

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
<ng-template let-somevar>
  <button (click)="somevar = 3">Set var to 3</button>
</ng-template>
```

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
  • Loading branch information
alxhub authored and kara committed Dec 11, 2019
1 parent c387952 commit 418d5869086715d8a54a23467d6b0ec7be00dca1
@@ -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:
*
* ```
* <ng-template let-something>
* <button (click)="something = ...">...</button>
* </ng-template>
* ```
*
* Template variables are read-only.
*/
WRITE_TO_READ_ONLY_VARIABLE = 8005,

/**
* An injectable already has a `ɵprov` property.
*/
@@ -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}`);
@@ -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,
}));
}
}
@@ -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<any>,
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<any>,
oob: OutOfBandDiagnosticRecorder): void {
ast.visit(new ExpressionSemanticVisitor(id, boundTarget, oob, sourceSpan));
}
}
@@ -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
@@ -389,4 +389,5 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
get diagnostics(): ReadonlyArray<ts.Diagnostic> { return []; }
missingReferenceTarget(): void {}
missingPipe(): void {}
illegalAssignmentToTemplateVar(): void {}
}
@@ -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 = `<ng-template let-v><div (event)="v = 3"></div></ng-template>`;
const block = tcb(TEMPLATE);
expect(block).toContain('_t3.addEventListener("event", ($event): any => (_t2 = 3))');
});

});

describe('config', () => {
@@ -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: \`
<div *ngIf="x as y">
<button (click)="y = !y">Toggle</button>
</div>
\`,
})
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});

0 comments on commit 418d586

Please sign in to comment.
You can’t perform that action at this time.