fix(ivy): incorrectly generating shared pure function between null and object literal (#35481)

In #33705 we made it so that we generate pure functions for object/array literals in order to avoid having them be shared across elements/views. The problem this introduced is that further down the line the `ContantPool` uses the generated literal in order to figure out whether to share an existing factory or to create a new one. `ConstantPool` determines whether to share a factory by creating a key from the AST node and using it to look it up in the factory cache, however the key generation function didn't handle function invocations and replaced them with `null`. This means that the key for `{foo: pureFunction0(...)}` and `{foo: null}` are the same.

These changes rework the logic so that instead of generating a `null` key
for function invocations, we generate a variable called `<unknown>` which
shouldn't be able to collide with anything.

Fixes #35298.

PR Close #35481
This commit is contained in:
crisbeto 2020-02-16 12:49:23 +01:00 committed by Miško Hevery
parent 9228d7f15d
commit 22786c8e88
3 changed files with 226 additions and 3 deletions

View File

@ -3128,6 +3128,159 @@ describe('compiler compliance', () => {
expectEmit(result.source, MyAppDeclaration, 'Invalid component definition'); expectEmit(result.source, MyAppDeclaration, 'Invalid component definition');
}); });
it('should not share pure functions between null and object literals', () => {
const files = {
app: {
'spec.ts': `
import {Component, NgModule} from '@angular/core';
@Component({
template: \`
<div [dir]="{foo: null}"></div>
<div [dir]="{foo: {}}"></div>
\`
})
export class MyApp {}
@NgModule({declarations: [MyApp]})
export class MyModule {}
`
}
};
const MyAppDeclaration = `
const $c0$ = function () { return { foo: null }; };
const $c1$ = function () { return {}; };
const $c2$ = function (a0) { return { foo: a0 }; };
MyApp.ɵcmp = $r3$.ɵɵdefineComponent({
type: MyApp,
selectors: [["ng-component"]],
decls: 2,
vars: 6,
consts: [[${AttributeMarker.Bindings}, "dir"]],
template: function MyApp_Template(rf, ctx) {
if (rf & 1) {
$r3$.ɵɵelement(0, "div", 0);
$r3$.ɵɵelement(1, "div", 0);
}
if (rf & 2) {
$r3$.ɵɵproperty("dir", $r3$.ɵɵpureFunction0(2, $c0$));
$r3$.ɵɵadvance(1);
$r3$.ɵɵproperty("dir", $r3$.ɵɵpureFunction1(4, $c2$, $r3$.ɵɵpureFunction0(3, $c1$)));
}
},
encapsulation: 2
});
`;
const result = compile(files, angularFiles);
expectEmit(result.source, MyAppDeclaration, 'Invalid component definition');
});
it('should not share pure functions between null and array literals', () => {
const files = {
app: {
'spec.ts': `
import {Component, NgModule} from '@angular/core';
@Component({
template: \`
<div [dir]="{foo: null}"></div>
<div [dir]="{foo: []}"></div>
\`
})
export class MyApp {}
@NgModule({declarations: [MyApp]})
export class MyModule {}
`
}
};
const MyAppDeclaration = `
const $c0$ = function () { return { foo: null }; };
const $c1$ = function () { return []; };
const $c2$ = function (a0) { return { foo: a0 }; };
MyApp.ɵcmp = $r3$.ɵɵdefineComponent({
type: MyApp,
selectors: [["ng-component"]],
decls: 2,
vars: 6,
consts: [[${AttributeMarker.Bindings}, "dir"]],
template: function MyApp_Template(rf, ctx) {
if (rf & 1) {
$r3$.ɵɵelement(0, "div", 0);
$r3$.ɵɵelement(1, "div", 0);
}
if (rf & 2) {
$r3$.ɵɵproperty("dir", $r3$.ɵɵpureFunction0(2, $c0$));
$r3$.ɵɵadvance(1);
$r3$.ɵɵproperty("dir", $r3$.ɵɵpureFunction1(4, $c2$, $r3$.ɵɵpureFunction0(3, $c1$)));
}
},
encapsulation: 2
});
`;
const result = compile(files, angularFiles);
expectEmit(result.source, MyAppDeclaration, 'Invalid component definition');
});
it('should not share pure functions between null and function calls', () => {
const files = {
app: {
'spec.ts': `
import {Component, NgModule} from '@angular/core';
@Component({
template: \`
<div [dir]="{foo: null}"></div>
<div [dir]="{foo: getFoo()}"></div>
\`
})
export class MyApp {
getFoo() {
return 'foo!';
}
}
@NgModule({declarations: [MyApp]})
export class MyModule {}
`
}
};
const MyAppDeclaration = `
const $c0$ = function () { return { foo: null }; };
const $c1$ = function (a0) { return { foo: a0 }; };
MyApp.ɵcmp = $r3$.ɵɵdefineComponent({
type: MyApp,
selectors: [["ng-component"]],
decls: 2,
vars: 5,
consts: [[${AttributeMarker.Bindings}, "dir"]],
template: function MyApp_Template(rf, ctx) {
if (rf & 1) {
$r3$.ɵɵelement(0, "div", 0);
$r3$.ɵɵelement(1, "div", 0);
}
if (rf & 2) {
$r3$.ɵɵproperty("dir", $r3$.ɵɵpureFunction0(2, $c0$));
$r3$.ɵɵadvance(1);
$r3$.ɵɵproperty("dir", $r3$.ɵɵpureFunction1(3, $c1$, ctx.getFoo()));
}
},
encapsulation: 2
});
`;
const result = compile(files, angularFiles);
expectEmit(result.source, MyAppDeclaration, 'Invalid component definition');
});
}); });
describe('inherited base classes', () => { describe('inherited base classes', () => {

View File

@ -11,6 +11,16 @@ import {OutputContext, error} from './util';
const CONSTANT_PREFIX = '_c'; const CONSTANT_PREFIX = '_c';
/**
* `ConstantPool` tries to reuse literal factories when two or more literals are identical.
* We determine whether literals are identical by creating a key out of their AST using the
* `KeyVisitor`. This constant is used to replace dynamic expressions which can't be safely
* converted into a key. E.g. given an expression `{foo: bar()}`, since we don't know what
* the result of `bar` will be, we create a key that looks like `{foo: <unknown>}`. Note
* that we use a variable, rather than something like `null` in order to avoid collisions.
*/
const UNKNOWN_VALUE_KEY = o.variable('<unknown>');
export const enum DefinitionKind {Injector, Directive, Component, Pipe} export const enum DefinitionKind {Injector, Directive, Component, Pipe}
/** /**
@ -129,14 +139,14 @@ export class ConstantPool {
{literalFactory: o.Expression, literalFactoryArguments: o.Expression[]} { {literalFactory: o.Expression, literalFactoryArguments: o.Expression[]} {
// Create a pure function that builds an array of a mix of constant and variable expressions // Create a pure function that builds an array of a mix of constant and variable expressions
if (literal instanceof o.LiteralArrayExpr) { if (literal instanceof o.LiteralArrayExpr) {
const argumentsForKey = literal.entries.map(e => e.isConstant() ? e : o.literal(null)); const argumentsForKey = literal.entries.map(e => e.isConstant() ? e : UNKNOWN_VALUE_KEY);
const key = this.keyOf(o.literalArr(argumentsForKey)); const key = this.keyOf(o.literalArr(argumentsForKey));
return this._getLiteralFactory(key, literal.entries, entries => o.literalArr(entries)); return this._getLiteralFactory(key, literal.entries, entries => o.literalArr(entries));
} else { } else {
const expressionForKey = o.literalMap( const expressionForKey = o.literalMap(
literal.entries.map(e => ({ literal.entries.map(e => ({
key: e.key, key: e.key,
value: e.value.isConstant() ? e.value : o.literal(null), value: e.value.isConstant() ? e.value : UNKNOWN_VALUE_KEY,
quoted: e.quoted quoted: e.quoted
}))); })));
const key = this.keyOf(expressionForKey); const key = this.keyOf(expressionForKey);

View File

@ -559,5 +559,65 @@ describe('components using pure function instructions internally', () => {
.not.toBe(secondFixture.componentInstance.directive.value); .not.toBe(secondFixture.componentInstance.directive.value);
}); });
it('should not confuse object literals and null inside of a literal', () => {
@Component({
template: `
<div [dir]="{foo: null}"></div>
<div [dir]="{foo: {}}"></div>
`
})
class App {
@ViewChildren(Dir) directives !: QueryList<Dir>;
}
TestBed.configureTestingModule({declarations: [Dir, App]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
const values = fixture.componentInstance.directives.map(directive => directive.value);
expect(values).toEqual([{foo: null}, {foo: {}}]);
});
it('should not confuse array literals and null inside of a literal', () => {
@Component({
template: `
<div [dir]="{foo: null}"></div>
<div [dir]="{foo: []}"></div>
`
})
class App {
@ViewChildren(Dir) directives !: QueryList<Dir>;
}
TestBed.configureTestingModule({declarations: [Dir, App]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
const values = fixture.componentInstance.directives.map(directive => directive.value);
expect(values).toEqual([{foo: null}, {foo: []}]);
});
it('should not confuse function calls and null inside of a literal', () => {
@Component({
template: `
<div [dir]="{foo: null}"></div>
<div [dir]="{foo: getFoo()}"></div>
`
})
class App {
@ViewChildren(Dir) directives !: QueryList<Dir>;
getFoo() { return 'foo!'; }
}
TestBed.configureTestingModule({declarations: [Dir, App]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
const values = fixture.componentInstance.directives.map(directive => directive.value);
expect(values).toEqual([{foo: null}, {foo: 'foo!'}]);
});
}); });
}); });