honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(compiler): evaluate safe navigation expressions in correct binding order (#37911) 

When using the safe navigation operator in a binding expression, a temporary
variable may be used for storing the result of a side-effectful call.
For example, the following template uses a pipe and a safe property access:

```html
<app-person-view [enabled]="enabled" [firstName]="(person$ | async)?.name"></app-person-view>
```

The result of the pipe evaluation is stored in a temporary to be able to check
whether it is present. The temporary variable needs to be declared in a separate
statement and this would also cause the full expression itself to be pulled out
into a separate statement. This would compile into the following
pseudo-code instructions:

```js
var temp = null;
var firstName = (temp = pipe('async', ctx.person$)) == null ? null : temp.name;
property('enabled', ctx.enabled)('firstName', firstName);
```

Notice that the pipe evaluation happens before evaluating the `enabled` binding,
such that the runtime's internal binding index would correspond with `enabled`,
not `firstName`. This introduces a problem when the pipe uses `WrappedValue` to
force a change to be detected, as the runtime would then mark the binding slot
corresponding with `enabled` as dirty, instead of `firstName`. This results
in the `enabled` binding to be updated, triggering setters and affecting how
`OnChanges` is called.

In the pseudo-code above, the intermediate `firstName` variable is not strictly
necessary---it only improved readability a bit---and emitting it inline with
the binding itself avoids the out-of-order execution of the pipe:

```js
var temp = null;
property('enabled', ctx.enabled)
  ('firstName', (temp = pipe('async', ctx.person$)) == null ? null : temp.name);
```

This commit introduces a new `BindingForm` that results in the above code to be
generated and adds compiler and acceptance tests to verify the proper behavior.

Fixes #37194

PR Close #37911
This commit is contained in:
JoostK 2020-07-03 19:35:44 +02:00 committed by Andrew Kushnir
parent 2e9fdbde9e
commit 9514fd9080
6 changed files with 219 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

78
packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts
View File

@ -180,6 +180,44 @@ describe('compiler compliance: bindings', () => {
expectEmit(result.source, template, 'Incorrect template');
});
it('should emit temporary evaluation within the binding expression for in-order execution',
() => {
// https://github.com/angular/angular/issues/37194
// Verifies that temporary expressions used for expressions with potential side-effects in
// the LHS of a safe navigation access are emitted within the binding expression itself, to
// ensure that these temporaries are evaluated during the evaluation of the binding. This
// is important for when the LHS contains a pipe, as pipe evaluation depends on the current
// binding index.
const files = {
app: {
'example.ts': `
import {Component} from '@angular/core';
@Component({
template: '<button [title]="myTitle" [id]="(auth()?.identity() | async)?.id" [tabindex]="1"></button>'
})
export class MyComponent {
myTitle = 'hello';
auth?: () => { identity(): any; };
}`
}
};
const result = compile(files, angularFiles);
const template = `
…
template: function MyComponent_Template(rf, ctx) {
…
if (rf & 2) {
var $tmp0$ = null;
$r3$.ɵɵproperty("title", ctx.myTitle)("id", ($tmp0$ = $r3$.ɵɵpipeBind1(1, 3, ($tmp0$ = ctx.auth()) == null ? null : $tmp0$.identity())) == null ? null : $tmp0$.id)("tabindex", 1);
}
}
`;
expectEmit(result.source, template, 'Incorrect template');
});
it('should chain multiple property bindings into a single instruction', () => {
const files = {
app: {
@ -685,6 +723,46 @@ describe('compiler compliance: bindings', () => {
expectEmit(source, HostBindingDirDeclaration, 'Invalid host binding code');
});
it('should support host bindings with temporary expressions', () => {
const files = {
app: {
'spec.ts': `
import {Directive, NgModule} from '@angular/core';
@Directive({
selector: '[hostBindingDir]',
host: {'[id]': 'getData()?.id'}
})
export class HostBindingDir {
getData?: () => { id: number };
}
@NgModule({declarations: [HostBindingDir]})
export class MyModule {}
`
}
};
const HostBindingDirDeclaration = `
HostBindingDir.ɵdir = $r3$.ɵɵdefineDirective({
type: HostBindingDir,
selectors: [["", "hostBindingDir", ""]],
hostVars: 1,
hostBindings: function HostBindingDir_HostBindings(rf, ctx) {
if (rf & 2) {
var $tmp0$ = null;
$r3$.ɵɵhostProperty("id", ($tmp0$ = ctx.getData()) == null ? null : $tmp0$.id);
}
}
});
`;
const result = compile(files, angularFiles);
const source = result.source;
expectEmit(source, HostBindingDirDeclaration, 'Invalid host binding code');
});
it('should support host bindings with pure functions', () => {
const files = {
app: {

6
packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts
View File

@ -805,8 +805,7 @@ describe('i18n support in the template compiler', () => {
}
if (rf & 2) {
var $tmp_0_0$ = null;
const $currVal_0$ = ($tmp_0_0$ = ctx.valueA.getRawValue()) == null ? null : $tmp_0_0$.getTitle();
$r3$.ɵɵi18nExp($currVal_0$);
$r3$.ɵɵi18nExp(($tmp_0_0$ = ctx.valueA.getRawValue()) == null ? null : $tmp_0_0$.getTitle());
$r3$.ɵɵi18nApply(1);
}
}
@ -1320,9 +1319,8 @@ describe('i18n support in the template compiler', () => {
}
if (rf & 2) {
var $tmp_2_0$ = null;
const $currVal_2$ = ($tmp_2_0$ = ctx.valueA.getRawValue()) == null ? null : $tmp_2_0$.getTitle();
$r3$.ɵɵadvance(2);
$r3$.ɵɵi18nExp($r3$.ɵɵpipeBind1(2, 3, ctx.valueA))(ctx.valueA == null ? null : ctx.valueA.a == null ? null : ctx.valueA.a.b)($currVal_2$);
$r3$.ɵɵi18nExp($r3$.ɵɵpipeBind1(2, 3, ctx.valueA))(ctx.valueA == null ? null : ctx.valueA.a == null ? null : ctx.valueA.a.b)(($tmp_2_0$ = ctx.valueA.getRawValue()) == null ? null : $tmp_2_0$.getTitle());
$r3$.ɵɵi18nApply(1);
}
}

9
packages/compiler/src/compiler_util/expression_converter.ts
View File

@ -154,6 +154,11 @@ export enum BindingForm {
// Try to generate a simple binding (no temporaries or statements)
// otherwise generate a general binding
TrySimple,
// Inlines assignment of temporaries into the generated expression. The result may still
// have statements attached for declarations of temporary variables.
// This is the only relevant form for Ivy, the other forms are only used in ViewEngine.
Expression,
}
/**
@ -168,7 +173,6 @@ export function convertPropertyBinding(
if (!localResolver) {
localResolver = new DefaultLocalResolver();
}
const currValExpr = createCurrValueExpr(bindingId);
const visitor =
new _AstToIrVisitor(localResolver, implicitReceiver, bindingId, interpolationFunction);
const outputExpr: o.Expression = expressionWithoutBuiltins.visit(visitor, _Mode.Expression);
@ -180,8 +184,11 @@ export function convertPropertyBinding(
if (visitor.temporaryCount === 0 && form == BindingForm.TrySimple) {
return new ConvertPropertyBindingResult([], outputExpr);
} else if (form === BindingForm.Expression) {
return new ConvertPropertyBindingResult(stmts, outputExpr);
}
const currValExpr = createCurrValueExpr(bindingId);
stmts.push(currValExpr.set(outputExpr).toDeclStmt(o.DYNAMIC_TYPE, [o.StmtModifier.Final]));
return new ConvertPropertyBindingResult(stmts, currValExpr);
}

2
packages/compiler/src/render3/view/compiler.ts
View File

@ -714,7 +714,7 @@ function createHostBindingsFunction(
function bindingFn(implicit: any, value: AST) {
return convertPropertyBinding(
null, implicit, value, 'b', BindingForm.TrySimple, () => error('Unexpected interpolation'));
null, implicit, value, 'b', BindingForm.Expression, () => error('Unexpected interpolation'));
}
function convertStylingCall(

2
packages/compiler/src/render3/view/template.ts
View File

@ -1213,7 +1213,7 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
private convertPropertyBinding(value: AST): o.Expression {
const convertedPropertyBinding = convertPropertyBinding(
this, this.getImplicitReceiverExpr(), value, this.bindingContext(), BindingForm.TrySimple,
this, this.getImplicitReceiverExpr(), value, this.bindingContext(), BindingForm.Expression,
() => error('Unexpected interpolation'));
const valExpr = convertedPropertyBinding.currValExpr;
this._tempVariables.push(...convertedPropertyBinding.stmts);

130
packages/core/test/acceptance/pipe_spec.ts
View File

@ -6,9 +6,10 @@
* found in the LICENSE file at https://angular.io/license
*/
import {ChangeDetectionStrategy, ChangeDetectorRef, Component, Directive, Inject, Injectable, InjectionToken, Input, NgModule, OnDestroy, Pipe, PipeTransform, ViewChild} from '@angular/core';
import {ChangeDetectionStrategy, ChangeDetectorRef, Component, Directive, Inject, Injectable, InjectionToken, Input, NgModule, OnChanges, OnDestroy, Pipe, PipeTransform, SimpleChanges, ViewChild, WrappedValue} from '@angular/core';
import {TestBed} from '@angular/core/testing';
import {expect} from '@angular/platform-browser/testing/src/matchers';
import {ivyEnabled} from '@angular/private/testing';
describe('pipe', () => {
@Pipe({name: 'countingPipe'})
@ -285,6 +286,133 @@ describe('pipe', () => {
expect(fixture.nativeElement).toHaveText('a');
});
describe('pipes within an optional chain', () => {
it('should not dirty unrelated inputs', () => {
// https://github.com/angular/angular/issues/37194
// https://github.com/angular/angular/issues/37591
// Using a pipe in the LHS of safe navigation operators would clobber unrelated bindings
// iff the pipe returns WrappedValue, incorrectly causing the unrelated binding
// to be considered changed.
const log: string[] = [];
@Component({template: `<my-cmp [value1]="1" [value2]="(value2 | pipe)?.id"></my-cmp>`})
class App {
value2 = {id: 2};
}
@Component({selector: 'my-cmp', template: ''})
class MyCmp {
@Input()
set value1(value1: number) {
log.push(`set value1=${value1}`);
}
@Input()
set value2(value2: number) {
log.push(`set value2=${value2}`);
}
}
@Pipe({name: 'pipe'})
class MyPipe implements PipeTransform {
transform(value: any): any {
log.push('pipe');
return WrappedValue.wrap(value);
}
}
TestBed.configureTestingModule({declarations: [App, MyCmp, MyPipe]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges(/* checkNoChanges */ false);
// Both bindings should have been set. Note: ViewEngine evaluates the pipe out-of-order,
// before setting inputs.
expect(log).toEqual(
ivyEnabled ?
[
'set value1=1',
'pipe',
'set value2=2',
] :
[
'pipe',
'set value1=1',
'set value2=2',
]);
log.length = 0;
fixture.componentInstance.value2 = {id: 3};
fixture.detectChanges(/* checkNoChanges */ false);
// value1 did not change, so it should not have been set.
expect(log).toEqual([
'pipe',
'set value2=3',
]);
});
it('should not include unrelated inputs in ngOnChanges', () => {
// https://github.com/angular/angular/issues/37194
// https://github.com/angular/angular/issues/37591
// Using a pipe in the LHS of safe navigation operators would clobber unrelated bindings
// iff the pipe returns WrappedValue, incorrectly causing the unrelated binding
// to be considered changed.
const log: string[] = [];
@Component({template: `<my-cmp [value1]="1" [value2]="(value2 | pipe)?.id"></my-cmp>`})
class App {
value2 = {id: 2};
}
@Component({selector: 'my-cmp', template: ''})
class MyCmp implements OnChanges {
@Input() value1!: number;
@Input() value2!: number;
ngOnChanges(changes: SimpleChanges): void {
if (changes.value1) {
const {previousValue, currentValue, firstChange} = changes.value1;
log.push(`change value1: ${previousValue} -> ${currentValue} (${firstChange})`);
}
if (changes.value2) {
const {previousValue, currentValue, firstChange} = changes.value2;
log.push(`change value2: ${previousValue} -> ${currentValue} (${firstChange})`);
}
}
}
@Pipe({name: 'pipe'})
class MyPipe implements PipeTransform {
transform(value: any): any {
log.push('pipe');
return WrappedValue.wrap(value);
}
}
TestBed.configureTestingModule({declarations: [App, MyCmp, MyPipe]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges(/* checkNoChanges */ false);
// Both bindings should have been included in ngOnChanges.
expect(log).toEqual([
'pipe',
'change value1: undefined -> 1 (true)',
'change value2: undefined -> 2 (true)',
]);
log.length = 0;
fixture.componentInstance.value2 = {id: 3};
fixture.detectChanges(/* checkNoChanges */ false);
// value1 did not change, so it should not have been included in ngOnChanges
expect(log).toEqual([
'pipe',
'change value2: 2 -> 3 (false)',
]);
});
});
describe('pure', () => {
it('should call pure pipes only if the arguments change', () => {
@Component({