Skip to content

Commit 22786c8

Browse files
crisbetomhevery
crisbeto
authored and
mhevery
committed
fix(ivy): incorrectly generating shared pure function between null and object literal (angular#35481)
In angular#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 angular#35298. PR Close angular#35481
1 parent 9228d7f commit 22786c8

File tree

3 files changed

+226
-3
lines changed

3 files changed

+226
-3
lines changed

‎packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3128,6 +3128,159 @@ describe('compiler compliance', () => {
31283128
expectEmit(result.source, MyAppDeclaration, 'Invalid component definition');
31293129
});
31303130

3131+
it('should not share pure functions between null and object literals', () => {
3132+
const files = {
3133+
app: {
3134+
'spec.ts': `
3135+
import {Component, NgModule} from '@angular/core';
3136+
3137+
@Component({
3138+
template: \`
3139+
<div [dir]="{foo: null}"></div>
3140+
<div [dir]="{foo: {}}"></div>
3141+
\`
3142+
})
3143+
export class MyApp {}
3144+
3145+
@NgModule({declarations: [MyApp]})
3146+
export class MyModule {}
3147+
`
3148+
}
3149+
};
3150+
3151+
const MyAppDeclaration = `
3152+
const $c0$ = function () { return { foo: null }; };
3153+
const $c1$ = function () { return {}; };
3154+
const $c2$ = function (a0) { return { foo: a0 }; };
3155+
…
3156+
MyApp.ɵcmp = $r3$.ɵɵdefineComponent({
3157+
type: MyApp,
3158+
selectors: [["ng-component"]],
3159+
decls: 2,
3160+
vars: 6,
3161+
consts: [[${AttributeMarker.Bindings}, "dir"]],
3162+
template: function MyApp_Template(rf, ctx) {
3163+
if (rf & 1) {
3164+
$r3$.ɵɵelement(0, "div", 0);
3165+
$r3$.ɵɵelement(1, "div", 0);
3166+
}
3167+
if (rf & 2) {
3168+
$r3$.ɵɵproperty("dir", $r3$.ɵɵpureFunction0(2, $c0$));
3169+
$r3$.ɵɵadvance(1);
3170+
$r3$.ɵɵproperty("dir", $r3$.ɵɵpureFunction1(4, $c2$, $r3$.ɵɵpureFunction0(3, $c1$)));
3171+
}
3172+
},
3173+
encapsulation: 2
3174+
});
3175+
`;
3176+
3177+
const result = compile(files, angularFiles);
3178+
expectEmit(result.source, MyAppDeclaration, 'Invalid component definition');
3179+
});
3180+
3181+
it('should not share pure functions between null and array literals', () => {
3182+
const files = {
3183+
app: {
3184+
'spec.ts': `
3185+
import {Component, NgModule} from '@angular/core';
3186+
3187+
@Component({
3188+
template: \`
3189+
<div [dir]="{foo: null}"></div>
3190+
<div [dir]="{foo: []}"></div>
3191+
\`
3192+
})
3193+
export class MyApp {}
3194+
3195+
@NgModule({declarations: [MyApp]})
3196+
export class MyModule {}
3197+
`
3198+
}
3199+
};
3200+
3201+
const MyAppDeclaration = `
3202+
const $c0$ = function () { return { foo: null }; };
3203+
const $c1$ = function () { return []; };
3204+
const $c2$ = function (a0) { return { foo: a0 }; };
3205+
…
3206+
MyApp.ɵcmp = $r3$.ɵɵdefineComponent({
3207+
type: MyApp,
3208+
selectors: [["ng-component"]],
3209+
decls: 2,
3210+
vars: 6,
3211+
consts: [[${AttributeMarker.Bindings}, "dir"]],
3212+
template: function MyApp_Template(rf, ctx) {
3213+
if (rf & 1) {
3214+
$r3$.ɵɵelement(0, "div", 0);
3215+
$r3$.ɵɵelement(1, "div", 0);
3216+
}
3217+
if (rf & 2) {
3218+
$r3$.ɵɵproperty("dir", $r3$.ɵɵpureFunction0(2, $c0$));
3219+
$r3$.ɵɵadvance(1);
3220+
$r3$.ɵɵproperty("dir", $r3$.ɵɵpureFunction1(4, $c2$, $r3$.ɵɵpureFunction0(3, $c1$)));
3221+
}
3222+
},
3223+
encapsulation: 2
3224+
});
3225+
`;
3226+
3227+
const result = compile(files, angularFiles);
3228+
expectEmit(result.source, MyAppDeclaration, 'Invalid component definition');
3229+
});
3230+
3231+
it('should not share pure functions between null and function calls', () => {
3232+
const files = {
3233+
app: {
3234+
'spec.ts': `
3235+
import {Component, NgModule} from '@angular/core';
3236+
3237+
@Component({
3238+
template: \`
3239+
<div [dir]="{foo: null}"></div>
3240+
<div [dir]="{foo: getFoo()}"></div>
3241+
\`
3242+
})
3243+
export class MyApp {
3244+
getFoo() {
3245+
return 'foo!';
3246+
}
3247+
}
3248+
3249+
@NgModule({declarations: [MyApp]})
3250+
export class MyModule {}
3251+
`
3252+
}
3253+
};
3254+
3255+
const MyAppDeclaration = `
3256+
const $c0$ = function () { return { foo: null }; };
3257+
const $c1$ = function (a0) { return { foo: a0 }; };
3258+
…
3259+
MyApp.ɵcmp = $r3$.ɵɵdefineComponent({
3260+
type: MyApp,
3261+
selectors: [["ng-component"]],
3262+
decls: 2,
3263+
vars: 5,
3264+
consts: [[${AttributeMarker.Bindings}, "dir"]],
3265+
template: function MyApp_Template(rf, ctx) {
3266+
if (rf & 1) {
3267+
$r3$.ɵɵelement(0, "div", 0);
3268+
$r3$.ɵɵelement(1, "div", 0);
3269+
}
3270+
if (rf & 2) {
3271+
$r3$.ɵɵproperty("dir", $r3$.ɵɵpureFunction0(2, $c0$));
3272+
$r3$.ɵɵadvance(1);
3273+
$r3$.ɵɵproperty("dir", $r3$.ɵɵpureFunction1(3, $c1$, ctx.getFoo()));
3274+
}
3275+
},
3276+
encapsulation: 2
3277+
});
3278+
`;
3279+
3280+
const result = compile(files, angularFiles);
3281+
expectEmit(result.source, MyAppDeclaration, 'Invalid component definition');
3282+
});
3283+
31313284
});
31323285

31333286
describe('inherited base classes', () => {

‎packages/compiler/src/constant_pool.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,16 @@ import {OutputContext, error} from './util';
1111

1212
const CONSTANT_PREFIX = '_c';
1313

14+
/**
15+
* `ConstantPool` tries to reuse literal factories when two or more literals are identical.
16+
* We determine whether literals are identical by creating a key out of their AST using the
17+
* `KeyVisitor`. This constant is used to replace dynamic expressions which can't be safely
18+
* converted into a key. E.g. given an expression `{foo: bar()}`, since we don't know what
19+
* the result of `bar` will be, we create a key that looks like `{foo: <unknown>}`. Note
20+
* that we use a variable, rather than something like `null` in order to avoid collisions.
21+
*/
22+
const UNKNOWN_VALUE_KEY = o.variable('<unknown>');
23+
1424
export const enum DefinitionKind {Injector, Directive, Component, Pipe}
1525

1626
/**
@@ -127,16 +137,16 @@ export class ConstantPool {
127137

128138
getLiteralFactory(literal: o.LiteralArrayExpr|o.LiteralMapExpr):
129139
{literalFactory: o.Expression, literalFactoryArguments: o.Expression[]} {
130-
// Create a pure function that builds an array of a mix of constant and variable expressions
140+
// Create a pure function that builds an array of a mix of constant and variable expressions
131141
if (literal instanceof o.LiteralArrayExpr) {
132-
const argumentsForKey = literal.entries.map(e => e.isConstant() ? e : o.literal(null));
142+
const argumentsForKey = literal.entries.map(e => e.isConstant() ? e : UNKNOWN_VALUE_KEY);
133143
const key = this.keyOf(o.literalArr(argumentsForKey));
134144
return this._getLiteralFactory(key, literal.entries, entries => o.literalArr(entries));
135145
} else {
136146
const expressionForKey = o.literalMap(
137147
literal.entries.map(e => ({
138148
key: e.key,
139-
value: e.value.isConstant() ? e.value : o.literal(null),
149+
value: e.value.isConstant() ? e.value : UNKNOWN_VALUE_KEY,
140150
quoted: e.quoted
141151
})));
142152
const key = this.keyOf(expressionForKey);

‎packages/core/test/acceptance/pure_function_spec.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -559,5 +559,65 @@ describe('components using pure function instructions internally', () => {
559559
.not.toBe(secondFixture.componentInstance.directive.value);
560560
});
561561

562+
it('should not confuse object literals and null inside of a literal', () => {
563+
@Component({
564+
template: `
565+
<div [dir]="{foo: null}"></div>
566+
<div [dir]="{foo: {}}"></div>
567+
`
568+
})
569+
class App {
570+
@ViewChildren(Dir) directives !: QueryList<Dir>;
571+
}
572+
573+
TestBed.configureTestingModule({declarations: [Dir, App]});
574+
const fixture = TestBed.createComponent(App);
575+
fixture.detectChanges();
576+
const values = fixture.componentInstance.directives.map(directive => directive.value);
577+
578+
expect(values).toEqual([{foo: null}, {foo: {}}]);
579+
});
580+
581+
it('should not confuse array literals and null inside of a literal', () => {
582+
@Component({
583+
template: `
584+
<div [dir]="{foo: null}"></div>
585+
<div [dir]="{foo: []}"></div>
586+
`
587+
})
588+
class App {
589+
@ViewChildren(Dir) directives !: QueryList<Dir>;
590+
}
591+
592+
TestBed.configureTestingModule({declarations: [Dir, App]});
593+
const fixture = TestBed.createComponent(App);
594+
fixture.detectChanges();
595+
const values = fixture.componentInstance.directives.map(directive => directive.value);
596+
597+
expect(values).toEqual([{foo: null}, {foo: []}]);
598+
});
599+
600+
it('should not confuse function calls and null inside of a literal', () => {
601+
@Component({
602+
template: `
603+
<div [dir]="{foo: null}"></div>
604+
<div [dir]="{foo: getFoo()}"></div>
605+
`
606+
})
607+
class App {
608+
@ViewChildren(Dir) directives !: QueryList<Dir>;
609+
getFoo() { return 'foo!'; }
610+
}
611+
612+
TestBed.configureTestingModule({declarations: [Dir, App]});
613+
const fixture = TestBed.createComponent(App);
614+
fixture.detectChanges();
615+
const values = fixture.componentInstance.directives.map(directive => directive.value);
616+
617+
expect(values).toEqual([{foo: null}, {foo: 'foo!'}]);
618+
});
619+
620+
621+
562622
});
563623
});

0 commit comments

Comments
 (0)