fix(ivy): correctly bind to component context (#23168)

When compiling templates the compiler would often bind to
closest context rather than the component context.

The only time one should be binding to the cont component is
in explicit cases where the inner template declares local variable.

PR Close #23168
This commit is contained in:
Miško Hevery 2018-04-05 11:38:06 -07:00 committed by Igor Minar
parent 4f7fac0e03
commit 3fb4e190a8
7 changed files with 175 additions and 80 deletions

View File

@ -15,7 +15,46 @@ export class EventHandlerVars { static event = o.variable('$event'); }
export interface LocalResolver { getLocal(name: string): o.Expression|null; }
export class ConvertActionBindingResult {
constructor(public stmts: o.Statement[], public allowDefault: o.ReadVarExpr) {}
/**
* Store statements which are render3 compatible.
*/
render3Stmts: o.Statement[];
constructor(
/**
* Render2 compatible statements,
*/
public stmts: o.Statement[],
/**
* Variable name used with render2 compatible statements.
*/
public allowDefault: o.ReadVarExpr) {
/**
* This is bit of a hack. It converts statements which render2 expects to statements which are
* expected by render3.
*
* Example: `<div click="doSomething($event)">` will generate:
*
* Render3:
* ```
* const pd_b:any = ((<any>ctx.doSomething($event)) !== false);
* return pd_b;
* ```
*
* but render2 expects:
* ```
* return ctx.doSomething($event);
* ```
*/
// TODO(misko): remove this hack once we no longer support ViewEngine.
this.render3Stmts = stmts.map((statement: o.Statement) => {
if (statement instanceof o.DeclareVarStmt && statement.name == allowDefault.name &&
statement.value instanceof o.BinaryOperatorExpr) {
const lhs = statement.value.lhs as o.CastExpr;
return new o.ReturnStatement(lhs.value);
}
return statement;
});
}
}
export type InterpolationFunction = (args: o.Expression[]) => o.Expression;

View File

@ -73,10 +73,10 @@ export function compileDirective(
field('attributes', createHostAttributesArray(directive, outputCtx));
// e.g 'inputs: {a: 'a'}`
field('inputs', createMapObjectLiteral(directive.inputs, outputCtx));
field('inputs', conditionallyCreateMapObjectLiteral(directive.inputs, outputCtx));
// e.g 'outputs: {a: 'a'}`
field('outputs', createMapObjectLiteral(directive.outputs, outputCtx));
field('outputs', conditionallyCreateMapObjectLiteral(directive.outputs, outputCtx));
const className = identifierName(directive.type) !;
className || error(`Cannot resolver the name of ${directive.type}`);
@ -180,7 +180,7 @@ export function compileComponent(
const pipeMap = new Map(pipes.map<[string, CompilePipeSummary]>(pipe => [pipe.name, pipe]));
const templateFunctionExpression =
new TemplateDefinitionBuilder(
outputCtx, outputCtx.constantPool, reflector, CONTEXT_NAME, ROOT_SCOPE.nestedScope(), 0,
outputCtx, outputCtx.constantPool, reflector, CONTEXT_NAME, BindingScope.ROOT_SCOPE, 0,
component.template !.ngContentSelectors, templateTypeName, templateName, pipeMap,
component.viewQueries, addDirectiveDependency, addPipeDependency)
.buildTemplateFunction(template, []);
@ -196,10 +196,10 @@ export function compileComponent(
}
// e.g `inputs: {a: 'a'}`
field('inputs', createMapObjectLiteral(component.inputs, outputCtx));
field('inputs', conditionallyCreateMapObjectLiteral(component.inputs, outputCtx));
// e.g 'outputs: {a: 'a'}`
field('outputs', createMapObjectLiteral(component.outputs, outputCtx));
field('outputs', conditionallyCreateMapObjectLiteral(component.outputs, outputCtx));
// e.g. `features: [NgOnChangesFeature(MyComponent)]`
const features: o.Expression[] = [];
@ -331,34 +331,86 @@ function getLiteralFactory(
return o.importExpr(pureFunctionIdent).callFn([literalFactory, ...literalFactoryArguments]);
}
class BindingScope {
private map = new Map<string, o.Expression>();
function noop() {}
/**
* Function which is executed whenever a variable is referenced for the first time in a given
* scope.
*
* It is expected that the function creates the `const localName = expression`; statement.
*/
type DeclareLocalVarCallback = (lhsVar: o.ReadVarExpr, rhsExpression: o.Expression) => void;
class BindingScope implements LocalResolver {
/**
* Keeps a map from local variables to their expressions.
*
* This is used when one refers to variable such as: 'let abc = a.b.c`.
* - key to the map is the string literal `"abc"`.
* - value `lhs` is the left hand side which is an AST representing `abc`.
* - value `rhs` is the right hand side which is an AST representing `a.b.c`.
* - value `declared` is true if the `declareLocalVarCallback` has been called for this scope
* already.
*/
private map = new Map < string, {
lhs: o.ReadVarExpr;
rhs: o.Expression|undefined;
declared: boolean;
}
> ();
private referenceNameIndex = 0;
constructor(private parent: BindingScope|null) {}
static ROOT_SCOPE = new BindingScope().set('$event', o.variable('$event'));
private constructor(
private parent: BindingScope|null = null,
private declareLocalVarCallback: DeclareLocalVarCallback = noop) {}
get(name: string): o.Expression|null {
let current: BindingScope|null = this;
while (current) {
const value = current.map.get(name);
let value = current.map.get(name);
if (value != null) {
// Cache the value locally.
this.map.set(name, value);
return value;
if (current !== this) {
// make a local copy and reset the `declared` state.
value = {lhs: value.lhs, rhs: value.rhs, declared: false};
// Cache the value locally.
this.map.set(name, value);
}
if (value.rhs && !value.declared) {
// if it is first time we are referencing the variable in the scope
// than invoke the callback to insert variable declaration.
this.declareLocalVarCallback(value.lhs, value.rhs);
value.declared = true;
}
return value.lhs;
}
current = current.parent;
}
return null;
}
set(name: string, value: o.Expression): BindingScope {
/**
* Create a local variable for later reference.
*
* @param name Name of the variable.
* @param lhs AST representing the left hand side of the `let lhs = rhs;`.
* @param rhs AST representing the right hand side of the `let lhs = rhs;`. The `rhs` can be
* `undefined` for variable that are ambient such as `$event` and which don't have `rhs`
* declaration.
*/
set(name: string, lhs: o.ReadVarExpr, rhs?: o.Expression): BindingScope {
!this.map.has(name) ||
error(`The name ${name} is already defined in scope to be ${this.map.get(name)}`);
this.map.set(name, value);
this.map.set(name, {lhs: lhs, rhs: rhs, declared: false});
return this;
}
nestedScope(): BindingScope { return new BindingScope(this); }
getLocal(name: string): (o.Expression|null) { return this.get(name); }
nestedScope(declareCallback: DeclareLocalVarCallback): BindingScope {
return new BindingScope(this, declareCallback);
}
freshReferenceName(): string {
let current: BindingScope = this;
@ -369,8 +421,6 @@ class BindingScope {
}
}
const ROOT_SCOPE = new BindingScope(null).set('$event', o.variable('$event'));
class TemplateDefinitionBuilder implements TemplateAstVisitor, LocalResolver {
private _dataIndex = 0;
private _bindingContext = 0;
@ -386,6 +436,7 @@ class TemplateDefinitionBuilder implements TemplateAstVisitor, LocalResolver {
private _valueConverter: ValueConverter;
private unsupported = unsupported;
private invalid = invalid;
private bindingScope: BindingScope;
// Whether we are inside a translatable element (`<p i18n>... somewhere here ... </p>)
private _inI18nSection: boolean = false;
@ -396,14 +447,19 @@ class TemplateDefinitionBuilder implements TemplateAstVisitor, LocalResolver {
constructor(
private outputCtx: OutputContext, private constantPool: ConstantPool,
private reflector: CompileReflector, private contextParameter: string,
private bindingScope: BindingScope, private level = 0, private ngContentSelectors: string[],
parentBindingScope: BindingScope, private level = 0, private ngContentSelectors: string[],
private contextName: string|null, private templateName: string|null,
private pipes: Map<string, CompilePipeSummary>, private viewQueries: CompileQueryMetadata[],
private addDirectiveDependency: (ast: DirectiveAst) => void,
private addPipeDependency: (summary: CompilePipeSummary) => void) {
this.bindingScope =
parentBindingScope.nestedScope((lhsVar: o.ReadVarExpr, expression: o.Expression) => {
this._bindingMode.push(
lhsVar.set(expression).toDeclStmt(o.INFERRED_TYPE, [o.StmtModifier.Final]));
});
this._valueConverter = new ValueConverter(
outputCtx, () => this.allocateDataSlot(), (name, localName, slot, value) => {
bindingScope.set(localName, value);
outputCtx, () => this.allocateDataSlot(), (name, localName, slot, value: o.ReadVarExpr) => {
this.bindingScope.set(localName, value);
const pipe = pipes.get(name) !;
pipe || error(`Could not find pipe ${name}`);
this.addPipeDependency(pipe);
@ -419,15 +475,8 @@ class TemplateDefinitionBuilder implements TemplateAstVisitor, LocalResolver {
const expression =
o.variable(this.contextParameter).prop(variable.value || IMPLICIT_REFERENCE);
const scopedName = this.bindingScope.freshReferenceName();
const declaration = o.variable(scopedName).set(expression).toDeclStmt(o.INFERRED_TYPE, [
o.StmtModifier.Final
]);
// Add the reference to the local scope.
this.bindingScope.set(variableName, o.variable(scopedName));
// Declare the local variable in binding mode
this._bindingMode.push(declaration);
this.bindingScope.set(variableName, o.variable(variableName + scopedName), expression);
}
// Collect content projections
@ -635,17 +684,23 @@ class TemplateDefinitionBuilder implements TemplateAstVisitor, LocalResolver {
this.instruction(
this._creationMode, element.sourceSpan, R3.createElement, ...trimTrailingNulls(parameters));
const implicit = o.variable(this.contextParameter);
const implicit = o.variable(CONTEXT_NAME);
// Generate Listeners (outputs)
element.outputs.forEach((outputAst: BoundEventAst) => {
const functionName = `${this.templateName}_${element.name}_${outputAst.name}_listener`;
const localVars: o.Statement[] = [];
const bindingScope =
this.bindingScope.nestedScope((lhsVar: o.ReadVarExpr, rhsExpression: o.Expression) => {
localVars.push(
lhsVar.set(rhsExpression).toDeclStmt(o.INFERRED_TYPE, [o.StmtModifier.Final]));
});
const bindingExpr = convertActionBinding(
this, o.variable('ctx'), outputAst.handler, 'b', () => error('Unexpected interpolation'));
bindingScope, o.variable(CONTEXT_NAME), outputAst.handler, 'b',
() => error('Unexpected interpolation'));
const handler = o.fn(
[new o.FnParam('$event', o.DYNAMIC_TYPE)],
[...bindingExpr.stmts, new o.ReturnStatement(bindingExpr.allowDefault)], o.INFERRED_TYPE,
null, functionName);
[new o.FnParam('$event', o.DYNAMIC_TYPE)], [...localVars, ...bindingExpr.render3Stmts],
o.INFERRED_TYPE, null, functionName);
this.instruction(
this._creationMode, outputAst.sourceSpan, R3.listener, o.literal(outputAst.name),
handler);
@ -756,13 +811,13 @@ class TemplateDefinitionBuilder implements TemplateAstVisitor, LocalResolver {
...trimTrailingNulls(parameters));
// Generate directives
this._visitDirectives(ast.directives, o.variable(this.contextParameter), templateIndex);
this._visitDirectives(ast.directives, o.variable(CONTEXT_NAME), templateIndex);
// Create the template function
const templateVisitor = new TemplateDefinitionBuilder(
this.outputCtx, this.constantPool, this.reflector, templateContext,
this.bindingScope.nestedScope(), this.level + 1, this.ngContentSelectors, contextName,
templateName, this.pipes, [], this.addDirectiveDependency, this.addPipeDependency);
this.outputCtx, this.constantPool, this.reflector, templateContext, this.bindingScope,
this.level + 1, this.ngContentSelectors, contextName, templateName, this.pipes, [],
this.addDirectiveDependency, this.addPipeDependency);
const templateFunctionExpr = templateVisitor.buildTemplateFunction(ast.children, ast.variables);
this._postfix.push(templateFunctionExpr.toDeclStmt(templateName, null));
}
@ -784,7 +839,7 @@ class TemplateDefinitionBuilder implements TemplateAstVisitor, LocalResolver {
// Refresh mode
this.instruction(
this._refreshMode, ast.sourceSpan, R3.textCreateBound, o.literal(nodeIndex),
this.bind(o.variable(CONTEXT_NAME), ast.value, ast.sourceSpan));
this.convertPropertyBinding(o.variable(CONTEXT_NAME), ast.value));
}
// TemplateAstVisitor
@ -847,10 +902,6 @@ class TemplateDefinitionBuilder implements TemplateAstVisitor, LocalResolver {
this._refreshMode.push(...convertedPropertyBinding.stmts);
return convertedPropertyBinding.currValExpr;
}
private bind(implicit: o.Expression, value: AST, sourceSpan: ParseSourceSpan): o.Expression {
return this.convertPropertyBinding(implicit, value);
}
}
function getQueryPredicate(query: CompileQueryMetadata, outputCtx: OutputContext): o.Expression {
@ -1058,7 +1109,7 @@ function createHostBindingsFunction(
return null;
}
function createMapObjectLiteral(
function conditionallyCreateMapObjectLiteral(
keys: {[key: string]: string}, outputCtx: OutputContext): o.Expression|null {
if (Object.getOwnPropertyNames(keys).length > 0) {
return mapToExpression(keys);

View File

@ -1153,6 +1153,7 @@ describe('compiler compliance', () => {
$r3$.ɵT(1);
$r3$.ɵe();
}
const $item$ = ctx0.$implicit;
const $info$ = ctx1.$implicit;
$r3$.ɵt(1, $r3$.ɵi2(' ', $item$.name, ': ', $info$.description, ' '));
}

View File

@ -53,7 +53,7 @@ describe('compiler compliance: listen()', () => {
// The template should look like this (where IDENT is a wild card for an identifier):
const template = `
static ngComponentDef = i0.ɵdefineComponent({
static ngComponentDef = IDENT.ɵdefineComponent({
inputs:{
componentInput: 'componentInput',
@ -65,7 +65,7 @@ describe('compiler compliance: listen()', () => {
}
});
static ngDirectiveDef = i0.ɵdefineDirective({
static ngDirectiveDef = IDENT.ɵdefineDirective({
inputs:{
directiveInput: 'directiveInput',

View File

@ -27,7 +27,7 @@ describe('compiler compliance: listen()', () => {
@Component({
selector: 'my-component',
template: \`<div (click)="onClick($event)"></div>\`
template: \`<div (click)="onClick($event); 1 == 2"></div>\`
})
export class MyComponent {
onClick(event: any) {}
@ -45,8 +45,8 @@ describe('compiler compliance: listen()', () => {
if (cm) {
$r3$.ɵE(0, 'div');
$r3$.ɵL('click', function MyComponent_Template_div_click_listener($event: $any$) {
const $return_value$:$any$ = ctx.onClick($event) ;
return $return_value$;
ctx.onClick($event);
return (1 == 2);
});
$r3$.ɵe();
}

View File

@ -9,10 +9,7 @@
import {MockDirectory, setup} from '../aot/test_util';
import {compile, expectEmit} from './mock_compile';
/* These tests are codified version of the tests in compiler_canonical_spec.ts. Every
* test in compiler_canonical_spec.ts should have a corresponding test here.
*/
describe('compiler compliance: listen()', () => {
describe('compiler compliance: template', () => {
const angularFiles = setup({
compileAngular: true,
compileAnimations: false,
@ -31,13 +28,17 @@ describe('compiler compliance: listen()', () => {
template: \`
<ul *ngFor="let outer of items">
<li *ngFor="let middle of outer.items">
<div *ngFor="let inner of middle.items" (click)="onClick(outer, middle, inner)">
{{format(outer, middle, inner)}}
<div *ngFor="let inner of items"
(click)="onClick(outer, middle, inner)"
[title]="format(outer, middle, inner, component)"
>
{{format(outer, middle, inner, component)}}
</div>
</li>
</ul>\`
})
export class MyComponent {
component = this;
format(outer: any, middle: any, inner: any) { }
onClick(outer: any, middle: any, inner: any) { }
}
@ -50,39 +51,43 @@ describe('compiler compliance: listen()', () => {
// The template should look like this (where IDENT is a wild card for an identifier):
const template = `
template:function MyComponent_Template(ctx:any,cm:boolean){
template:function MyComponent_Template(ctx:any, cm:boolean){
if (cm) {
i0.ɵC(0,MyComponent_NgForOf_Template_0,null,_c0);
$i0$.ɵC(0, MyComponent_NgForOf_Template_0, null, _c0);
}
i0.ɵp(0,'ngForOf',i0.ɵb(ctx.items));
function MyComponent_NgForOf_Template_0(ctx0:any,cm:boolean) {
$i0$.ɵp(0, 'ngForOf', $i0$.ɵb(ctx.items));
function MyComponent_NgForOf_Template_0(ctx0:any, cm:boolean) {
if (cm) {
i0.ɵE(0,'ul');
i0.ɵC(1,MyComponent_NgForOf_NgForOf_Template_1,null,_c0);
i0.ɵe();
$i0$.ɵE(0, 'ul');
$i0$.ɵC(1, MyComponent_NgForOf_NgForOf_Template_1, null, _c0);
$i0$.ɵe();
}
const $_r0$ = ctx0.$implicit;
i0.ɵp(1,'ngForOf',i0.ɵb($_r0$.items));
function MyComponent_NgForOf_NgForOf_Template_1(ctx1:any,cm:boolean) {
const $outer$ = ctx0.$implicit;
$i0$.ɵp(1, 'ngForOf', $i0$.ɵb($outer$.items));
function MyComponent_NgForOf_NgForOf_Template_1(ctx1:any, cm:boolean) {
if (cm) {
i0.ɵE(0,'li');
i0.ɵC(1,MyComponent_NgForOf_NgForOf_NgForOf_Template_1,null,_c0);
i0.ɵe();
$i0$.ɵE(0, 'li');
$i0$.ɵC(1, MyComponent_NgForOf_NgForOf_NgForOf_Template_1, null, _c0);
$i0$.ɵe();
}
const $_r1$ = ctx1.$implicit;
i0.ɵp(1,'ngForOf',i0.ɵb($_r1$.items));
function MyComponent_NgForOf_NgForOf_NgForOf_Template_1(ctx2:any,cm:boolean) {
$i0$.ɵp(1, 'ngForOf', $i0$.ɵb(ctx.items));
function MyComponent_NgForOf_NgForOf_NgForOf_Template_1(ctx2:any, cm:boolean) {
if (cm) {
i0.ɵE(0,'div');
i0.ɵL('click',function MyComponent_NgForOf_NgForOf_NgForOf_Template_1_div_click_listener($event:any){
const pd_b:any = ((<any>ctx.onClick($_r0$,$_r1$,$_r2$)) !== false);
return pd_b;
$i0$.ɵE(0, 'div');
$i0$.ɵL('click', function MyComponent_NgForOf_NgForOf_NgForOf_Template_1_div_click_listener($event:any){
const $outer$ = ctx0.$implicit;
const $middle$ = ctx1.$implicit;
const $inner$ = ctx2.$implicit;
return ctx.onClick($outer$, $middle$, $inner$);
});
i0.ɵT(1);
i0.ɵe();
$i0$.ɵT(1);
$i0$.ɵe();
}
const $_r2$ = ctx2.$implicit;
i0.ɵt(1,i0.ɵi1(' ',ctx.format($_r0$,$_r1$,$_r2$),' '));
const $outer$ = ctx0.$implicit;
const $middle$ = ctx1.$implicit;
const $inner$ = ctx2.$implicit;
$i0$.ɵp(0, 'title', ctx.format($outer$, $middle$, $inner$, ctx.component));
$i0$.ɵt(1, $i0$.ɵi1(' ', ctx.format($outer$, $middle$, $inner$, ctx.component), ' '));
}
}
}

View File

@ -61,7 +61,6 @@ export class TodoComponent {
</div>
<span>count: {{appState.todos.length}}.</span>
`,
changeDetection: ChangeDetectionStrategy.OnPush
})
export class ToDoAppComponent {
public appState: AppState;