Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a56a76
fix(ivy): incorrectly generating shared pure function between null an…
crisbeto Feb 16, 2020
b87b90f
fixup! fix(ivy): incorrectly generating shared pure function between …
crisbeto Feb 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
153 changes: 153 additions & 0 deletions packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts
View file
Open in desktop
Expand Up @@ -3128,6 +3128,159 @@ describe('compiler compliance', () => {
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', () => {
Expand Down
16 changes: 13 additions & 3 deletions packages/compiler/src/constant_pool.ts
View file
Open in desktop
Expand Up @@ -11,6 +11,16 @@ import {OutputContext, error} from './util';

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}

/**
Expand Down Expand Up @@ -127,16 +137,16 @@ export class ConstantPool {

getLiteralFactory(literal: o.LiteralArrayExpr|o.LiteralMapExpr):
{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) {
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));
return this._getLiteralFactory(key, literal.entries, entries => o.literalArr(entries));
} else {
const expressionForKey = o.literalMap(
literal.entries.map(e => ({
key: e.key,
value: e.value.isConstant() ? e.value : o.literal(null),
value: e.value.isConstant() ? e.value : UNKNOWN_VALUE_KEY,
quoted: e.quoted
})));
const key = this.keyOf(expressionForKey);
Expand Down
60 changes: 60 additions & 0 deletions packages/core/test/acceptance/pure_function_spec.ts
View file
Open in desktop
Expand Up @@ -559,5 +559,65 @@ describe('components using pure function instructions internally', () => {
.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>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one has been an issue even before #33705, but we just never hit it.

`
})
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!'}]);
});



});
});