Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($parse): always re-evaluate filters within literals when an input is an object #15990

Merged
merged 2 commits into from
May 25, 2017
Merged

fix($parse): always re-evaluate filters within literals when an input is an object #15990

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
2f2fb2d
fix($parse): always re-evaluate filters within literals when an input…
jbedard May 12, 2017
ad9b0d2
test($parse): reorganize $parse tests
jbedard May 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 59 additions & 26 deletions src/ng/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,15 +622,44 @@ function isStateless($filter, filterName) {
return !fn.$stateful;
}

function findConstantAndWatchExpressions(ast, $filter) {
// Detect nodes which could depend on non-shallow state of objects
function isPure(node, parentIsPure) {
switch (node.type) {
// Computed members might invoke a stateful toString()
case AST.MemberExpression:
if (node.computed) {
return false;
}
break;

// Unary always convert to primative
case AST.UnaryExpression:
return true;
Copy link
Member

Choose a reason for hiding this comment

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

Can't unary expressions invoke a stateful valueOf()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think you're right.

Seems like any numeric operator (everything except !, && and ||?) will use valueOf(). However this will be handled so I don't think it will lead to bugs, but this method is still wrong.

Now that I type that... && and || also don't convert to primitives and might return an object so those are also incorrect, but we watch the full logical expression so it won't lead to bugs, but this method is still wrong.

So I don't think there's a bug but it might be nice to make this function more correct?

Once again, always wait for @gkalpak to review your PRs...

Copy link
Member

Choose a reason for hiding this comment

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

First of all, I don't know what && and || have to do with this (since they are not unary operators) 😕

Regarding getValueOf(), I don't think it is enough to "save" us (in cases where valueOf() returns non-primitive values). I admit it is very unlikely that this will affect any real app, but from a theoretical point of view, I think expressions such as '+obj', where obj has a valueOf() method that returns an object will not be handled correctly (e.g. if the returned object stays the same by reference, but one of its properties changes).

Copy link
Member

Choose a reason for hiding this comment

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

Hm...OK maybe that is not true, because unary operators will always evaluate to the same thing for objects whose valueOf returns a non-primitve value (! will return false, +/- will return NaN), so this might actually work correctly 😁

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I forgot that && and || are not BinaryExpressions (the next case statement) so never mind about that one!

I do still think that numeric unary/binary operators are non-pure because valueOf might be stateful. Which is a bug in isPure, but the use of getValueOf later on prevents a bug in $parse. I'll probably still correct the isPure function...

Copy link
Member

Choose a reason for hiding this comment

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

I do still think that numeric unary/binary operators are non-pure because valueOf might be stateful. Which is a bug in isPure, but the use of getValueOf later on prevents a bug in $parse. I'll probably still correct the isPure function...

That's also my conclusion 😃


// The binary + operator can invoke a stateful toString().
case AST.BinaryExpression:
return node.operator !== '+';

// Functions / filters probably read state from within objects
case AST.CallExpression:
return false;
}

return (undefined === parentIsPure) || parentIsPure;
}

function findConstantAndWatchExpressions(ast, $filter, parentIsPure) {
var allConstants;
var argsToWatch;
var isStatelessFilter;

var astIsPure = ast.isPure = isPure(ast, parentIsPure);

switch (ast.type) {
case AST.Program:
allConstants = true;
forEach(ast.body, function(expr) {
findConstantAndWatchExpressions(expr.expression, $filter);
findConstantAndWatchExpressions(expr.expression, $filter, astIsPure);
allConstants = allConstants && expr.expression.constant;
});
ast.constant = allConstants;
Expand All @@ -640,26 +669,26 @@ function findConstantAndWatchExpressions(ast, $filter) {
ast.toWatch = [];
break;
case AST.UnaryExpression:
findConstantAndWatchExpressions(ast.argument, $filter);
findConstantAndWatchExpressions(ast.argument, $filter, astIsPure);
ast.constant = ast.argument.constant;
ast.toWatch = ast.argument.toWatch;
break;
case AST.BinaryExpression:
findConstantAndWatchExpressions(ast.left, $filter);
findConstantAndWatchExpressions(ast.right, $filter);
findConstantAndWatchExpressions(ast.left, $filter, astIsPure);
findConstantAndWatchExpressions(ast.right, $filter, astIsPure);
ast.constant = ast.left.constant && ast.right.constant;
ast.toWatch = ast.left.toWatch.concat(ast.right.toWatch);
break;
case AST.LogicalExpression:
findConstantAndWatchExpressions(ast.left, $filter);
findConstantAndWatchExpressions(ast.right, $filter);
findConstantAndWatchExpressions(ast.left, $filter, astIsPure);
findConstantAndWatchExpressions(ast.right, $filter, astIsPure);
ast.constant = ast.left.constant && ast.right.constant;
ast.toWatch = ast.constant ? [] : [ast];
break;
case AST.ConditionalExpression:
findConstantAndWatchExpressions(ast.test, $filter);
findConstantAndWatchExpressions(ast.alternate, $filter);
findConstantAndWatchExpressions(ast.consequent, $filter);
findConstantAndWatchExpressions(ast.test, $filter, astIsPure);
findConstantAndWatchExpressions(ast.alternate, $filter, astIsPure);
findConstantAndWatchExpressions(ast.consequent, $filter, astIsPure);
ast.constant = ast.test.constant && ast.alternate.constant && ast.consequent.constant;
ast.toWatch = ast.constant ? [] : [ast];
break;
Expand All @@ -668,9 +697,9 @@ function findConstantAndWatchExpressions(ast, $filter) {
ast.toWatch = [ast];
break;
case AST.MemberExpression:
findConstantAndWatchExpressions(ast.object, $filter);
findConstantAndWatchExpressions(ast.object, $filter, astIsPure);
if (ast.computed) {
findConstantAndWatchExpressions(ast.property, $filter);
findConstantAndWatchExpressions(ast.property, $filter, astIsPure);
}
ast.constant = ast.object.constant && (!ast.computed || ast.property.constant);
ast.toWatch = [ast];
Expand All @@ -680,7 +709,7 @@ function findConstantAndWatchExpressions(ast, $filter) {
allConstants = isStatelessFilter;
argsToWatch = [];
forEach(ast.arguments, function(expr) {
findConstantAndWatchExpressions(expr, $filter);
findConstantAndWatchExpressions(expr, $filter, astIsPure);
allConstants = allConstants && expr.constant;
if (!expr.constant) {
argsToWatch.push.apply(argsToWatch, expr.toWatch);
Expand All @@ -690,16 +719,16 @@ function findConstantAndWatchExpressions(ast, $filter) {
ast.toWatch = isStatelessFilter ? argsToWatch : [ast];
break;
case AST.AssignmentExpression:
findConstantAndWatchExpressions(ast.left, $filter);
findConstantAndWatchExpressions(ast.right, $filter);
findConstantAndWatchExpressions(ast.left, $filter, astIsPure);
findConstantAndWatchExpressions(ast.right, $filter, astIsPure);
ast.constant = ast.left.constant && ast.right.constant;
ast.toWatch = [ast];
break;
case AST.ArrayExpression:
allConstants = true;
argsToWatch = [];
forEach(ast.elements, function(expr) {
findConstantAndWatchExpressions(expr, $filter);
findConstantAndWatchExpressions(expr, $filter, astIsPure);
allConstants = allConstants && expr.constant;
if (!expr.constant) {
argsToWatch.push.apply(argsToWatch, expr.toWatch);
Expand All @@ -712,13 +741,13 @@ function findConstantAndWatchExpressions(ast, $filter) {
allConstants = true;
argsToWatch = [];
forEach(ast.properties, function(property) {
findConstantAndWatchExpressions(property.value, $filter);
findConstantAndWatchExpressions(property.value, $filter, astIsPure);
allConstants = allConstants && property.value.constant && !property.computed;
if (!property.value.constant) {
argsToWatch.push.apply(argsToWatch, property.value.toWatch);
}
if (property.computed) {
findConstantAndWatchExpressions(property.key, $filter);
findConstantAndWatchExpressions(property.key, $filter, astIsPure);
if (!property.key.constant) {
argsToWatch.push.apply(argsToWatch, property.key.toWatch);
}
Expand Down Expand Up @@ -803,7 +832,7 @@ ASTCompiler.prototype = {
var intoId = self.nextId();
self.recurse(watch, intoId);
self.return_(intoId);
self.state.inputs.push(fnKey);
self.state.inputs.push({name: fnKey, isPure: watch.isPure});
watch.watchId = key;
});
this.state.computing = 'fn';
Expand Down Expand Up @@ -839,13 +868,16 @@ ASTCompiler.prototype = {

watchFns: function() {
var result = [];
var fns = this.state.inputs;
var inputs = this.state.inputs;
var self = this;
forEach(fns, function(name) {
result.push('var ' + name + '=' + self.generateFunction(name, 's'));
forEach(inputs, function(input) {
result.push('var ' + input.name + '=' + self.generateFunction(input.name, 's'));
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any performance/code-size gain in setting and using var name = input.name; here?
Or does the minification process deal with that automatically?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think performance/code-size has no (noticeable) gain or loss either way, so I'd vote for whatever is more readable/prettier. My preference is how it is (no local var) but I can change it if others would prefer that.

if (input.isPure) {
result.push(input.name, '.isPure=true;');
}
});
if (fns.length) {
result.push('fn.inputs=[' + fns.join(',') + '];');
if (inputs.length) {
result.push('fn.inputs=[' + inputs.map(function(i) { return i.name; }).join(',') + '];');
}
return result.join('');
},
Expand Down Expand Up @@ -1251,6 +1283,7 @@ ASTInterpreter.prototype = {
inputs = [];
forEach(toWatch, function(watch, key) {
var input = self.recurse(watch);
input.isPure = watch.isPure;
watch.input = input;
inputs.push(input);
watch.watchId = key;
Expand Down Expand Up @@ -1817,7 +1850,7 @@ function $ParseProvider() {
inputExpressions = inputExpressions[0];
return scope.$watch(function expressionInputWatch(scope) {
var newInputValue = inputExpressions(scope);
if (!expressionInputDirtyCheck(newInputValue, oldInputValueOf, parsedExpression.literal)) {
if (!expressionInputDirtyCheck(newInputValue, oldInputValueOf, inputExpressions.isPure)) {
lastResult = parsedExpression(scope, undefined, undefined, [newInputValue]);
oldInputValueOf = newInputValue && getValueOf(newInputValue);
}
Expand All @@ -1837,7 +1870,7 @@ function $ParseProvider() {

for (var i = 0, ii = inputExpressions.length; i < ii; i++) {
var newInputValue = inputExpressions[i](scope);
if (changed || (changed = !expressionInputDirtyCheck(newInputValue, oldInputValueOfValues[i], parsedExpression.literal))) {
if (changed || (changed = !expressionInputDirtyCheck(newInputValue, oldInputValueOfValues[i], inputExpressions[i].isPure))) {
oldInputValues[i] = newInputValue;
oldInputValueOfValues[i] = newInputValue && getValueOf(newInputValue);
}
Expand Down
20 changes: 20 additions & 0 deletions test/ng/directive/ngClassSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,26 @@ describe('ngClass', function() {
})
);

//https://github.com/angular/angular.js/issues/15960#issuecomment-299109412
it('should always reevaluate filters with non-primitive inputs within literals', function() {
module(function($filterProvider) {
$filterProvider.register('foo', valueFn(function(o) {
return o.a || o.b;
}));
});

inject(function($rootScope, $compile) {
$rootScope.testObj = {};
element = $compile('<div ng-class="{x: (testObj | foo)}">')($rootScope);

$rootScope.$apply();
expect(element).not.toHaveClass('x');

$rootScope.$apply('testObj.a = true');
expect(element).toHaveClass('x');
});
});

describe('large objects', function() {
var getProp;
var veryLargeObj;
Expand Down
Loading