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

jbedard · 2017-05-15T18:47:36Z

This changes the compareObjectIdentity flag from being per-expression to per-input. This allows inputs to filters to not compare object entity, but other inputs such as - x or {x: x} to only compare object entity. This also expands this compareObjectIdentity = true beyond just object literals.

jbedard · 2017-05-15T18:48:04Z

src/ng/parse.js

@@ -622,15 +622,51 @@ function isStateless($filter, filterName) {
  return !fn.$stateful;
 }

-function findConstantAndWatchExpressions(ast, $filter) {
+// Detect nodes which could depend on non-shallow state of objects
+function isShallowState(stack) {


This could probably use a better name/terminology...

isStackPure?

jbedard · 2017-05-15T18:48:31Z

src/ng/parse.js

@@ -781,7 +819,7 @@ ASTCompiler.prototype = {
      filters: {},
      fn: {vars: [], body: [], own: {}},
      assign: {vars: [], body: [], own: {}},
-      inputs: []
+      inputs: {}


Probably a nicer way of doing this...

By doing this (and later Object.keys) you are abusing the fact that Object.keys returns the keys in the order that were added to assert that expressions as foo.bar();foo.baz() will be evaluated in the right order. Even when this is somehow true, this is not part of the spec and we should not depend on this out-of-spec fact.

I forgot the order even mattered, good point 👍

jbedard · 2017-05-15T19:11:49Z

test/ng/parseSpec.js

+          expect(watcherCalls).toBe(2);
+        }));
+
+        it('should always reevaluate filters with literal input containing non-primitives',


One way to test this better (which I'll add) would be something like [(obj | filterThatConvertsToPrimitive)]

petebacondarwin · 2017-05-15T19:12:46Z

src/ng/parse.js

-function findConstantAndWatchExpressions(ast, $filter) {
+// Detect nodes which could depend on non-shallow state of objects
+function isShallowState(stack) {
+  for (var i = 0; i < stack.length; i++) {


Do we need to walk the stack every time? It seems that we are either pushing or popping items on and off this stack and then re-evaluating the shallowStateness of the stack. Can we not cache the state of each stack and then we only need to evaluate the pushed item?

Yeah that's probably true. Probably store the result of the switch statement in the stack? I'll try it out...

lgalfaso · 2017-05-15T20:16:52Z

src/ng/parse.js

+  return true;
+}
+
+function findConstantAndWatchExpressions(ast, $filter, stack) {


I am not following completely the logic here, but by just looking at the code it looks like stack can be replace with a simple boolean (that the first called call with true and the rest do ast.isShallowState = parentShallowState && isShallowState([ast]); (and of course this can be simplified).

Yeah I think that's possible. I guess the idea of the stack was that it could also potentially be used for other purposes, but I'll try your suggestion.

Yeah, I think this is what I was getting at with #15990 (comment)

jbedard · 2017-05-20T00:10:46Z

I think I've addressed all the comments. Also added a ng-class test for the exact issue someone reported.

I'm also open to other ideas around this and similar issues such as #15905 if anyone has alternative ideas.

Note that if the $filter solution in this PR is extended to interceptors it will also fix #15905 except one-time literals containing objects. That one failure is caused by this condition which I really wish I could find an alternative solution for, especially since that condition forces workarounds in multiple places where interceports are used. The other thing that would fix that one ng-class test is dropping nested object support in ng-class 😒

petebacondarwin

This looks pretty good; a few minor comments.
My main question is about the purity of UnaryExpressions.
Otherwise happy to land this.

petebacondarwin · 2017-05-20T10:18:14Z

src/ng/parse.js

  var allConstants;
  var argsToWatch;
  var isStatelessFilter;
+
+  ast.isPure = parentIsPure = isPure(ast, (undefined === parentIsPure) || parentIsPure);


(undefined === parentIsPure) || parentIsPure - I would move this logic into the isPure() function.

petebacondarwin · 2017-05-20T10:19:09Z

src/ng/parse.js

+      break;
+
+    case AST.UnaryExpression:
+      return true;


In this case would you not defer to the parentPureness also?

The idea is to allow shallow-watching the input in things like fn(!input) where !parentIsPure, but the child is.

I've tried to exclude case which can invoke toString like "" + input because those could be non-pure (when toString reads inner state).

petebacondarwin · 2017-05-20T10:21:48Z

src/ng/parse.js

-    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'));


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?

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.

petebacondarwin · 2017-05-20T10:34:49Z

test/ng/parseSpec.js

@@ -2973,6 +2973,172 @@ describe('parser', function() {
          expect(watcherCalls).toBe(1);
        }));

+        it('should not reevaluate filters in literals with non-primitive input that does support valueOf()',


can we use some describe blocks to simplify the test descriptions here:

describe('containing literals', function() { describe('with non-primitive inputs', function() { describe('that support valueOf()', function() { it('should not reevaluate filters, inputs do not change', ...); it('should reevaluate filters, when an input valueOf() result changes', ...); it('should not reevaluate filters, when the input changes but valueOf() does not ', ...); }); }); describe('with non-primitive inputs', function() { }); });

you get the idea...

jbedard · 2017-05-24T07:27:17Z

Updated based on PR feedback. Also added more tests and added a separate commit to reorganize the tests. How those tests are organized is debatable but I think what I did is similar to the suggestion in the comment.

petebacondarwin

LGTM!
Thanks @jbedard - please squash and merge (assuming Travis is happy).

jbedard · 2017-05-24T18:05:58Z

@lgalfaso have a chance to look at the latest update?

lgalfaso

LGTM!

Just one minor comment.

lgalfaso · 2017-05-24T19:52:08Z

src/ng/parse.js

  var allConstants;
  var argsToWatch;
  var isStatelessFilter;
+
+  ast.isPure = parentIsPure = isPure(ast, parentIsPure);


[nit] I find it slightly odd to change the value of the argument. I would just use ast.isPure below and keep the argument as is.

I think I agree with that. Could also change the parentIsPure param to parentAst, but I'll go with your suggestion unless someone says otherwise.

… is an object Fixes angular#15964 Closes angular#15990

… is an object Fixes #15964 Closes #15990

jbedard · 2017-05-25T05:02:38Z

Made the change and merged into master and v1.6.x

gkalpak · 2017-05-25T13:56:19Z

src/ng/parse.js

+
+    // Unary always convert to primative
+    case AST.UnaryExpression:
+      return true;


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

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...

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).

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 😁

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...

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 😃

googlebot added the cla: yes label May 15, 2017

jbedard commented May 15, 2017

View reviewed changes

petebacondarwin reviewed May 15, 2017

View reviewed changes

lgalfaso reviewed May 15, 2017

View reviewed changes

Narretz added this to the 1.6.5 milestone May 16, 2017

Narretz added the component: $parse label May 16, 2017

jbedard force-pushed the 15964 branch 2 times, most recently from 437c9af to 3d07e75 Compare May 18, 2017 05:32

petebacondarwin reviewed May 20, 2017

View reviewed changes

jbedard force-pushed the 15964 branch 2 times, most recently from 0e2296e to fdf7a2a Compare May 24, 2017 07:27

petebacondarwin approved these changes May 24, 2017

View reviewed changes

lgalfaso approved these changes May 24, 2017

View reviewed changes

fix($parse): always re-evaluate filters within literals when an input…

2f2fb2d

… is an object Fixes angular#15964 Closes angular#15990

jbedard force-pushed the 15964 branch 2 times, most recently from e056dd9 to f0df97d Compare May 25, 2017 03:56

test($parse): reorganize $parse tests

ad9b0d2

jbedard force-pushed the 15964 branch from f0df97d to ad9b0d2 Compare May 25, 2017 04:11

jbedard merged commit aef3ef7 into angular:master May 25, 2017

jbedard added a commit that referenced this pull request May 25, 2017

fix($parse): always re-evaluate filters within literals when an input…

b5118ac

… is an object Fixes #15964 Closes #15990

jbedard added a commit that referenced this pull request May 25, 2017

fix($parse): always re-evaluate filters within literals when an input…

ec97686

… is an object Fixes #15964 Closes #15990

gkalpak reviewed May 25, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jbedard commented May 15, 2017 •

edited

Loading

jbedard May 15, 2017

petebacondarwin May 15, 2017

jbedard May 15, 2017

lgalfaso May 15, 2017

jbedard May 15, 2017

jbedard May 15, 2017

petebacondarwin May 15, 2017

jbedard May 15, 2017

lgalfaso May 15, 2017

jbedard May 15, 2017

petebacondarwin May 15, 2017

jbedard commented May 20, 2017

petebacondarwin left a comment

petebacondarwin May 20, 2017

petebacondarwin May 20, 2017

jbedard May 20, 2017

petebacondarwin May 20, 2017

jbedard May 24, 2017

petebacondarwin May 20, 2017

jbedard commented May 24, 2017

petebacondarwin left a comment •

edited

Loading

jbedard commented May 24, 2017

lgalfaso left a comment •

edited

Loading

lgalfaso May 24, 2017

jbedard May 24, 2017

jbedard commented May 25, 2017

gkalpak May 25, 2017

jbedard May 25, 2017

gkalpak May 25, 2017

gkalpak May 25, 2017

jbedard May 25, 2017

gkalpak May 26, 2017

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

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

Conversation

jbedard commented May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented May 20, 2017

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented May 24, 2017

petebacondarwin left a comment • edited Loading

Choose a reason for hiding this comment

jbedard commented May 24, 2017

lgalfaso left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented May 15, 2017 •

edited

Loading

petebacondarwin left a comment •

edited

Loading

lgalfaso left a comment •

edited

Loading