Skip to content

Commit

Permalink
fix(ivy): teach template type checker about template attributes
Browse files Browse the repository at this point in the history
For the template type checking to work correctly, it needs to know
what attributes are bound to expressions or directives, which may
require expressions in the template to be evaluated in a different
scope.

In inline templates, there are attributes that are now marked as
"Template" attributes. We need to ensure that the template
type checking code looks at these "bound" attributes as well as the
"input" attributes.
  • Loading branch information
petebacondarwin committed Mar 7, 2019
1 parent de3e2fc commit bc63228
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 40 deletions.
47 changes: 30 additions & 17 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license * found in the LICENSE file at https://angular.io/license
*/ */


import {AST, BindingType, BoundTarget, ImplicitReceiver, PropertyRead, TmplAstBoundText, TmplAstElement, TmplAstNode, TmplAstTemplate, TmplAstVariable} from '@angular/compiler'; import {AST, BindingType, BoundTarget, ImplicitReceiver, PropertyRead, TmplAstBoundAttribute, TmplAstBoundText, TmplAstElement, TmplAstNode, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable} from '@angular/compiler';
import * as ts from 'typescript'; import * as ts from 'typescript';


import {Reference, ReferenceEmitter} from '../../imports'; import {Reference, ReferenceEmitter} from '../../imports';
Expand All @@ -16,6 +16,7 @@ import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta} from './api';
import {astToTypescript} from './expression'; import {astToTypescript} from './expression';





/** /**
* Given a `ts.ClassDeclaration` for a component, and metadata regarding that component, compose a * Given a `ts.ClassDeclaration` for a component, and metadata regarding that component, compose a
* "type check block" function. * "type check block" function.
Expand Down Expand Up @@ -430,7 +431,10 @@ function tcbProcessTemplateDeclaration(tmpl: TmplAstTemplate, tcb: Context, scop
// any template guards, and generate them if needed. // any template guards, and generate them if needed.
dir.ngTemplateGuards.forEach(inputName => { dir.ngTemplateGuards.forEach(inputName => {
// For each template guard function on the directive, look for a binding to that input. // For each template guard function on the directive, look for a binding to that input.
const boundInput = tmpl.inputs.find(i => i.name === inputName); const boundInput = tmpl.inputs.find(i => i.name === inputName) ||
tmpl.templateAttrs.find(
(i: TmplAstTextAttribute | TmplAstBoundAttribute): i is TmplAstBoundAttribute =>
i instanceof TmplAstBoundAttribute && i.name === inputName);
if (boundInput !== undefined) { if (boundInput !== undefined) {
// If there is such a binding, generate an expression for it. // If there is such a binding, generate an expression for it.
const expr = tcbExpression(boundInput.value, tcb, scope); const expr = tcbExpression(boundInput.value, tcb, scope);
Expand Down Expand Up @@ -524,20 +528,28 @@ function tcbGetInputBindingExpressions(
propMatch.set(inputs[key] as string, key); propMatch.set(inputs[key] as string, key);
}); });


// Add a binding expression to the map for each input of the directive that has a el.inputs.forEach(processAttribute);
// matching binding. if (el instanceof TmplAstTemplate) {
el.inputs.filter(input => propMatch.has(input.name)).forEach(input => { el.templateAttrs.forEach(processAttribute);
// Produce an expression representing the value of the binding. }
const expr = tcbExpression(input.value, tcb, scope);

// Call the callback.
bindings.push({
property: input.name,
field: propMatch.get(input.name) !,
expression: expr,
});
});
return bindings; return bindings;

/**
* Add a binding expression to the map for each input/template attribute of the directive that has
* a matching binding.
*/
function processAttribute(attr: TmplAstBoundAttribute | TmplAstTextAttribute): void {
if (attr instanceof TmplAstBoundAttribute && propMatch.has(attr.name)) {
// Produce an expression representing the value of the binding.
const expr = tcbExpression(attr.value, tcb, scope);
// Call the callback.
bindings.push({
property: attr.name,
field: propMatch.get(attr.name) !,
expression: expr,
});
}
}
} }


/** /**
Expand Down Expand Up @@ -633,6 +645,7 @@ function tcbResolve(ast: AST, tcb: Context, scope: Scope): ts.Expression|null {
} else if (ast instanceof ImplicitReceiver) { } else if (ast instanceof ImplicitReceiver) {
// AST instances representing variables and references look very similar to property reads from // AST instances representing variables and references look very similar to property reads from
// the component context: both have the shape PropertyRead(ImplicitReceiver, 'propertyName'). // the component context: both have the shape PropertyRead(ImplicitReceiver, 'propertyName').
//
// `tcbExpression` will first try to `tcbResolve` the outer PropertyRead. If this works, it's // `tcbExpression` will first try to `tcbResolve` the outer PropertyRead. If this works, it's
// because the `BoundTarget` found an expression target for the whole expression, and therefore // because the `BoundTarget` found an expression target for the whole expression, and therefore
// `tcbExpression` will never attempt to `tcbResolve` the ImplicitReceiver of that PropertyRead. // `tcbExpression` will never attempt to `tcbResolve` the ImplicitReceiver of that PropertyRead.
Expand Down Expand Up @@ -662,8 +675,8 @@ function tcbResolveVariable(binding: TmplAstVariable, tcb: Context, scope: Scope
if (tmpl === null) { if (tmpl === null) {
throw new Error(`Expected TmplAstVariable to be mapped to a TmplAstTemplate`); throw new Error(`Expected TmplAstVariable to be mapped to a TmplAstTemplate`);
} }
// Look for a context variable for the template. This should've been declared before anything // Look for a context variable for the template. This should've been declared before anything that
// that could reference the template's variables. // could reference the template's variables.
const ctx = scope.getTemplateCtx(tmpl); const ctx = scope.getTemplateCtx(tmpl);
if (ctx === null) { if (ctx === null) {
throw new Error('Expected template context to exist.'); throw new Error('Expected template context to exist.');
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler/src/render3/r3_ast.ts
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -246,4 +246,4 @@ export function transformAll<Result extends Node>(
changed = changed || newNode != node; changed = changed || newNode != node;
} }
return changed ? result : nodes; return changed ? result : nodes;
} }
41 changes: 19 additions & 22 deletions packages/compiler/src/render3/view/t2_binder.ts
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -282,27 +282,21 @@ class DirectiveBinder<DirectiveT extends DirectiveMeta> implements Visitor {
} }
}); });


// Associate bindings on the node with directives or with the node itself. // Associate attributes/bindings on the node with directives or with the node itself.

const processAttribute = (attribute: BoundAttribute | BoundEvent | TextAttribute) => {
// Inputs: let dir = directives.find(dir => dir.inputs.hasOwnProperty(attribute.name));
[...node.attributes, ...node.inputs].forEach(binding => {
let dir = directives.find(dir => dir.inputs.hasOwnProperty(binding.name));
if (dir !== undefined) {
this.bindings.set(binding, dir);
} else {
this.bindings.set(binding, node);
}
});

// Outputs:
node.outputs.forEach(binding => {
let dir = directives.find(dir => dir.outputs.hasOwnProperty(binding.name));
if (dir !== undefined) { if (dir !== undefined) {
this.bindings.set(binding, dir); this.bindings.set(attribute, dir);
} else { } else {
this.bindings.set(binding, node); this.bindings.set(attribute, node);
} }
}); };
node.attributes.forEach(processAttribute);
node.inputs.forEach(processAttribute);
node.outputs.forEach(processAttribute);
if (node instanceof Template) {
node.templateAttrs.forEach(processAttribute);
}


// Recurse into the node's children. // Recurse into the node's children.
node.children.forEach(child => child.visit(this)); node.children.forEach(child => child.visit(this));
Expand Down Expand Up @@ -378,10 +372,12 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor {


private ingest(template: Template|Node[]): void { private ingest(template: Template|Node[]): void {
if (template instanceof Template) { if (template instanceof Template) {
// For <ng-template>s, process inputs, outputs, variables, and child nodes. References were // For <ng-template>s, process inputs, outputs, template attributes,
// processed in the scope of the containing template. // variables, and child nodes.
// References were processed in the scope of the containing template.
template.inputs.forEach(this.visitNode); template.inputs.forEach(this.visitNode);
template.outputs.forEach(this.visitNode); template.outputs.forEach(this.visitNode);
template.templateAttrs.forEach(this.visitNode);
template.variables.forEach(this.visitNode); template.variables.forEach(this.visitNode);
template.children.forEach(this.visitNode); template.children.forEach(this.visitNode);


Expand All @@ -394,16 +390,17 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor {
} }


visitElement(element: Element) { visitElement(element: Element) {
// Vist the inputs, outputs, and children of the element. // Visit the inputs, outputs, and children of the element.
element.inputs.forEach(this.visitNode); element.inputs.forEach(this.visitNode);
element.outputs.forEach(this.visitNode); element.outputs.forEach(this.visitNode);
element.children.forEach(this.visitNode); element.children.forEach(this.visitNode);
} }


visitTemplate(template: Template) { visitTemplate(template: Template) {
// First, visit the inputs, outputs of the template node. // First, visit inputs, outputs and template attributes of the template node.
template.inputs.forEach(this.visitNode); template.inputs.forEach(this.visitNode);
template.outputs.forEach(this.visitNode); template.outputs.forEach(this.visitNode);
template.templateAttrs.forEach(this.visitNode);


// References are also evaluated in the outer context. // References are also evaluated in the outer context.
template.references.forEach(this.visitNode); template.references.forEach(this.visitNode);
Expand Down

0 comments on commit bc63228

Please sign in to comment.