From fcdada53f1afba5aed2180e260bebccf6750cf0d Mon Sep 17 00:00:00 2001 From: crisbeto Date: Wed, 13 Nov 2019 22:26:34 +0900 Subject: [PATCH] fix(ivy): constant object literals shared across element and component instances (#33705) Currently if a consumer does something like the following, the object literal will be shared across the two elements and any instances of the component template. The same applies to array literals: ```
``` These changes make it so that we generate a pure function even if an object is constant so that each instance gets its own object. Note that the original design for this fix included moving the pure function factories into the `consts` array. In the process of doing so I realized that pure function are also used inside of directive host bindings which means that we don't have access to the `consts`. These changes also: * Fix an issue that meant that the `pureFunction0` instruction could only be run during creation mode. * Make the `getConstant` utility slightly more convenient to use. This isn't strictly required for these changes to work, but I had made it as a part of a larger refactor that I ended up reverting. PR Close #33705 --- .../compliance/r3_compiler_compliance_spec.ts | 97 +++++++++++++++++-- .../r3_view_compiler_styling_spec.ts | 2 +- .../compiler/src/render3/view/template.ts | 18 +--- .../src/render3/instructions/container.ts | 4 +- .../core/src/render3/instructions/element.ts | 4 +- .../render3/instructions/element_container.ts | 4 +- packages/core/src/render3/pure_function.ts | 4 +- packages/core/src/render3/util/view_utils.ts | 5 +- .../test/acceptance/pure_function_spec.ts | 83 +++++++++++++++- 9 files changed, 188 insertions(+), 33 deletions(-) diff --git a/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts b/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts index f2c3344fbf..9c54be9169 100644 --- a/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts @@ -365,10 +365,10 @@ describe('compiler compliance', () => { template: function MyComponent_Template(rf, ctx) { if (rf & 1) { $r3$.ɵɵelement(0, "div", 0); - $r3$.ɵɵpipe(1,"pipe"); + $r3$.ɵɵpipe(1, "pipe"); } if (rf & 2) { - $r3$.ɵɵproperty("ternary", ctx.cond ? $r3$.ɵɵpureFunction1(8, $c0$, ctx.a): $c1$)("pipe", $r3$.ɵɵpipeBind3(1, 4, ctx.value, 1, 2))("and", ctx.cond && $r3$.ɵɵpureFunction1(10, $c0$, ctx.b))("or", ctx.cond || $r3$.ɵɵpureFunction1(12, $c0$, ctx.c)); + $r3$.ɵɵproperty("ternary", ctx.cond ? $r3$.ɵɵpureFunction1(8, $c0$, ctx.a): $r3$.ɵɵpureFunction0(10, $c1$))("pipe", $r3$.ɵɵpipeBind3(1, 4, ctx.value, 1, 2))("and", ctx.cond && $r3$.ɵɵpureFunction1(11, $c0$, ctx.b))("or", ctx.cond || $r3$.ɵɵpureFunction1(13, $c0$, ctx.c)); } } `; @@ -1082,25 +1082,24 @@ describe('compiler compliance', () => { }; const MyAppDefinition = ` - const $c0$ = {opacity: 0, duration: 0}; + const $c0$ = function () { return {opacity: 0, duration: 0}; }; const $e0_ff$ = function ($v$) { return {opacity: 1, duration: $v$}; }; - const $e0_ff_1$ = function ($v$) { return [$c0$, $v$]; }; + const $e0_ff_1$ = function ($v1$, $v2$) { return [$v1$, $v2$]; }; const $e0_ff_2$ = function ($v1$, $v2$) { return {animation: $v1$, actions: $v2$}; }; … MyApp.ɵcmp = $r3$.ɵɵdefineComponent({ type: MyApp, selectors: [["my-app"]], decls: 1, - vars: 8, + vars: 10, consts: [[${AttributeMarker.Bindings}, "config"]], template: function MyApp_Template(rf, ctx) { if (rf & 1) { $r3$.ɵɵelement(0, "nested-comp", 0); } if (rf & 2) { - $r3$.ɵɵproperty( - "config", - $r3$.ɵɵpureFunction2(5, $e0_ff_2$, ctx.name, $r3$.ɵɵpureFunction1(3, $e0_ff_1$, $r3$.ɵɵpureFunction1(1, $e0_ff$, ctx.duration)))); + $r3$.ɵɵproperty("config", + $r3$.ɵɵpureFunction2(7, $e0_ff_2$, ctx.name, $r3$.ɵɵpureFunction2(4, $e0_ff_1$, $r3$.ɵɵpureFunction0(1, $c0$), $r3$.ɵɵpureFunction1(2, $e0_ff$, ctx.duration)))); } }, directives: [NestedComp], @@ -2975,6 +2974,88 @@ describe('compiler compliance', () => { expectEmit(result.source, expectedOutput, 'Invalid directive definition'); }); + it('should generate a pure function for constant object literals', () => { + const files = { + app: { + 'spec.ts': ` + import {Component} from '@angular/core'; + + @Component({ + template: '' + }) + export class MyApp { + } + ` + } + }; + + const MyAppDeclaration = ` + const $c0$ = function () { return {}; }; + const $c1$ = function () { return { a: 1, b: 2 }; }; + … + MyApp.ɵcmp = $r3$.ɵɵdefineComponent({ + type: MyApp, + selectors: [["ng-component"]], + decls: 1, + vars: 4, + consts: [[${AttributeMarker.Bindings}, "prop", "otherProp"]], + template: function MyApp_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵelement(0, "some-comp", 0); + } + if (rf & 2) { + $r3$.ɵɵproperty("prop", $r3$.ɵɵpureFunction0(2, $c0$))("otherProp", $r3$.ɵɵpureFunction0(3, $c1$)); + } + }, + encapsulation: 2 + }); + `; + + const result = compile(files, angularFiles); + expectEmit(result.source, MyAppDeclaration, 'Invalid component definition'); + }); + + it('should generate a pure function for constant array literals', () => { + const files = { + app: { + 'spec.ts': ` + import {Component} from '@angular/core'; + + @Component({ + template: '' + }) + export class MyApp { + } + ` + } + }; + + const MyAppDeclaration = ` + const $c0$ = function () { return []; }; + const $c1$ = function () { return [0, 1, 2]; }; + … + MyApp.ɵcmp = $r3$.ɵɵdefineComponent({ + type: MyApp, + selectors: [["ng-component"]], + decls: 1, + vars: 4, + consts: [[${AttributeMarker.Bindings}, "prop", "otherProp"]], + template: function MyApp_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵelement(0, "some-comp", 0); + } + if (rf & 2) { + $r3$.ɵɵproperty("prop", $r3$.ɵɵpureFunction0(2, $c0$))("otherProp", $r3$.ɵɵpureFunction0(3, $c1$)); + } + }, + encapsulation: 2 + }); + `; + + const result = compile(files, angularFiles); + expectEmit(result.source, MyAppDeclaration, 'Invalid component definition'); + }); + }); describe('inherited base classes', () => { diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts index a5413bd41f..23fff07a73 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts @@ -915,7 +915,7 @@ describe('compiler compliance: styling', () => { if (rf & 2) { $r3$.ɵɵstyleSanitizer($r3$.ɵɵdefaultStyleSanitizer); $r3$.ɵɵstyleMap($r3$.ɵɵpipeBind2(1, 8, $ctx$.myStyleExp, 1000)); - $r3$.ɵɵclassMap($e2_styling$); + $r3$.ɵɵclassMap($r3$.ɵɵpureFunction0(20, _c0)); $r3$.ɵɵstyleProp("bar", $r3$.ɵɵpipeBind2(2, 11, $ctx$.barExp, 3000)); $r3$.ɵɵstyleProp("baz", $r3$.ɵɵpipeBind2(3, 14, $ctx$.bazExp, 4000)); $r3$.ɵɵclassProp("foo", $r3$.ɵɵpipeBind2(4, 17, $ctx$.fooExp, 2000)); diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 9eba6ff72f..9b0b5bd9ea 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -1420,11 +1420,9 @@ export class ValueConverter extends AstMemoryEfficientTransformer { array.span, array.sourceSpan, this.visitAll(array.expressions), values => { // If the literal has calculated (non-literal) elements transform it into // calls to literal factories that compose the literal and will cache intermediate - // values. Otherwise, just return an literal array that contains the values. + // values. const literal = o.literalArr(values); - return values.every(a => a.isConstant()) ? - this.constantPool.getConstLiteral(literal, true) : - getLiteralFactory(this.constantPool, literal, this.allocatePureFunctionSlots); + return getLiteralFactory(this.constantPool, literal, this.allocatePureFunctionSlots); }); } @@ -1432,12 +1430,10 @@ export class ValueConverter extends AstMemoryEfficientTransformer { return new BuiltinFunctionCall(map.span, map.sourceSpan, this.visitAll(map.values), values => { // If the literal has calculated (non-literal) elements transform it into // calls to literal factories that compose the literal and will cache intermediate - // values. Otherwise, just return an literal array that contains the values. + // values. const literal = o.literalMap(values.map( (value, index) => ({key: map.keys[index].key, value, quoted: map.keys[index].quoted}))); - return values.every(a => a.isConstant()) ? - this.constantPool.getConstLiteral(literal, true) : - getLiteralFactory(this.constantPool, literal, this.allocatePureFunctionSlots); + return getLiteralFactory(this.constantPool, literal, this.allocatePureFunctionSlots); }); } } @@ -1484,15 +1480,11 @@ function getLiteralFactory( const {literalFactory, literalFactoryArguments} = constantPool.getLiteralFactory(literal); // Allocate 1 slot for the result plus 1 per argument const startSlot = allocateSlots(1 + literalFactoryArguments.length); - literalFactoryArguments.length > 0 || error(`Expected arguments to a literal factory function`); const {identifier, isVarLength} = pureFunctionCallInfo(literalFactoryArguments); // Literal factories are pure functions that only need to be re-invoked when the parameters // change. - const args = [ - o.literal(startSlot), - literalFactory, - ]; + const args = [o.literal(startSlot), literalFactory]; if (isVarLength) { args.push(o.literalArr(literalFactoryArguments)); diff --git a/packages/core/src/render3/instructions/container.ts b/packages/core/src/render3/instructions/container.ts index d481f8ebe2..40e6a50bf6 100644 --- a/packages/core/src/render3/instructions/container.ts +++ b/packages/core/src/render3/instructions/container.ts @@ -73,8 +73,8 @@ export function ɵɵtemplate( // TODO: consider a separate node type for templates const tContainerNode = containerInternal( - lView, index, tagName || null, getConstant(tViewConsts, attrsIndex) as TAttributes); - const localRefs = getConstant(tViewConsts, localRefsIndex) as string[]; + lView, index, tagName || null, getConstant(tViewConsts, attrsIndex)); + const localRefs = getConstant(tViewConsts, localRefsIndex); if (tView.firstCreatePass) { ngDevMode && ngDevMode.firstCreatePass++; resolveDirectives(tView, lView, tContainerNode, localRefs); diff --git a/packages/core/src/render3/instructions/element.ts b/packages/core/src/render3/instructions/element.ts index 14b31cea00..4dda458a90 100644 --- a/packages/core/src/render3/instructions/element.ts +++ b/packages/core/src/render3/instructions/element.ts @@ -46,8 +46,8 @@ export function ɵɵelementStart( const lView = getLView(); const tView = lView[TVIEW]; const tViewConsts = tView.consts; - const attrs = getConstant(tViewConsts, attrsIndex) as TAttributes; - const localRefs = getConstant(tViewConsts, localRefsIndex) as string[]; + const attrs = getConstant(tViewConsts, attrsIndex); + const localRefs = getConstant(tViewConsts, localRefsIndex); ngDevMode && assertEqual( getBindingIndex(), tView.bindingStartIndex, 'elements should be created before any bindings'); diff --git a/packages/core/src/render3/instructions/element_container.ts b/packages/core/src/render3/instructions/element_container.ts index 47565f7c51..4752b22097 100644 --- a/packages/core/src/render3/instructions/element_container.ts +++ b/packages/core/src/render3/instructions/element_container.ts @@ -43,8 +43,8 @@ export function ɵɵelementContainerStart( const renderer = lView[RENDERER]; const tagName = 'ng-container'; const tViewConsts = tView.consts; - const attrs = getConstant(tViewConsts, attrsIndex) as TAttributes; - const localRefs = getConstant(tViewConsts, localRefsIndex) as string[]; + const attrs = getConstant(tViewConsts, attrsIndex); + const localRefs = getConstant(tViewConsts, localRefsIndex); ngDevMode && assertEqual( getBindingIndex(), tView.bindingStartIndex, 'element containers should be created before any bindings'); diff --git a/packages/core/src/render3/pure_function.ts b/packages/core/src/render3/pure_function.ts index d0c5cc59ac..980e0620bb 100644 --- a/packages/core/src/render3/pure_function.ts +++ b/packages/core/src/render3/pure_function.ts @@ -8,7 +8,7 @@ import {bindingUpdated, bindingUpdated2, bindingUpdated3, bindingUpdated4, getBinding, updateBinding} from './bindings'; import {getBindingRoot, getLView} from './state'; -import {isCreationMode} from './util/view_utils'; +import {NO_CHANGE} from './tokens'; /** @@ -44,7 +44,7 @@ export function ɵɵpureFunction0(slotOffset: number, pureFn: () => T, thisAr // TODO(kara): use bindingRoot instead of bindingStartIndex when implementing host bindings const bindingIndex = getBindingRoot() + slotOffset; const lView = getLView(); - return isCreationMode(lView) ? + return lView[bindingIndex] === NO_CHANGE ? updateBinding(lView, bindingIndex, thisArg ? pureFn.call(thisArg) : pureFn()) : getBinding(lView, bindingIndex); } diff --git a/packages/core/src/render3/util/view_utils.ts b/packages/core/src/render3/util/view_utils.ts index c9250b25f5..e03fdd5910 100644 --- a/packages/core/src/render3/util/view_utils.ts +++ b/packages/core/src/render3/util/view_utils.ts @@ -176,8 +176,9 @@ export function viewAttachedToContainer(view: LView): boolean { } /** Returns a constant from `TConstants` instance. */ -export function getConstant(consts: TConstants | null, index: number | null | undefined) { - return consts === null || index == null ? null : consts[index]; +export function getConstant(consts: TConstants | null, index: number | null | undefined): T| + null { + return consts === null || index == null ? null : consts[index] as unknown as T; } /** diff --git a/packages/core/test/acceptance/pure_function_spec.ts b/packages/core/test/acceptance/pure_function_spec.ts index 01abb54b2e..517b34869b 100644 --- a/packages/core/test/acceptance/pure_function_spec.ts +++ b/packages/core/test/acceptance/pure_function_spec.ts @@ -6,9 +6,10 @@ * found in the LICENSE file at https://angular.io/license */ import {CommonModule} from '@angular/common'; -import {Component, Input} from '@angular/core'; +import {Component, Directive, Input, QueryList, ViewChild, ViewChildren} from '@angular/core'; import {TestBed} from '@angular/core/testing'; import {By} from '@angular/platform-browser'; +import {onlyInIvy} from '@angular/private/testing'; describe('components using pure function instructions internally', () => { describe('with array literals', () => { @@ -479,4 +480,84 @@ describe('components using pure function instructions internally', () => { expect(objectComps[1].config).toEqual({opacity: 1, duration: 600}); }); }); + + onlyInIvy('issue has only been fixed for Ivy').describe('identical literals', () => { + @Directive({selector: '[dir]'}) + class Dir { + @Input('dir') value: any; + } + + it('should not share object literals across elements', () => { + @Component({ + template: ` +
+
+ ` + }) + class App { + @ViewChildren(Dir) directives !: QueryList; + } + + TestBed.configureTestingModule({declarations: [Dir, App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + const directives = fixture.componentInstance.directives.toArray(); + expect(directives[0].value).not.toBe(directives[1].value); + }); + + it('should not share array literals across elements', () => { + @Component({ + template: ` +
+
+ ` + }) + class App { + @ViewChildren(Dir) directives !: QueryList; + } + + TestBed.configureTestingModule({declarations: [Dir, App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + const directives = fixture.componentInstance.directives.toArray(); + expect(directives[0].value).not.toBe(directives[1].value); + }); + + it('should not share object literals across component instances', () => { + @Component({template: `
`}) + class App { + @ViewChild(Dir) directive !: Dir; + } + + TestBed.configureTestingModule({declarations: [Dir, App]}); + const firstFixture = TestBed.createComponent(App); + firstFixture.detectChanges(); + + const secondFixture = TestBed.createComponent(App); + secondFixture.detectChanges(); + + expect(firstFixture.componentInstance.directive.value) + .not.toBe(secondFixture.componentInstance.directive.value); + }); + + it('should not share array literals across component instances', () => { + @Component({template: `
`}) + class App { + @ViewChild(Dir) directive !: Dir; + } + + TestBed.configureTestingModule({declarations: [Dir, App]}); + const firstFixture = TestBed.createComponent(App); + firstFixture.detectChanges(); + + const secondFixture = TestBed.createComponent(App); + secondFixture.detectChanges(); + + expect(firstFixture.componentInstance.directive.value) + .not.toBe(secondFixture.componentInstance.directive.value); + }); + + }); });