mavoweb / mavo

Create web applications entirely by writing HTML and CSS!
https://mavo.io
MIT License
2.82k stars 178 forks source link

Make if(), or(), and() short-circuit #587

Open LeaVerou opened 4 years ago

LeaVerou commented 4 years ago

Testcase for if(): https://codepen.io/leaverou/pen/GRpOyqZ?editors=1101

This is arguably a bug (since if the clauses have side effects there is no conditional execution) and will likely improve performance significantly in certain cases.

It should be fairly easy to implement: All we need to do is wrap the code in a function, and then in Mavo.Functions.iff() test if argument is a function and if so, execute the correct one. Same for or() and and().

LeaVerou commented 4 years ago

The main complexity with this is what to do in the case the operands are arrays. Currently, in if(a, b, c), if a is an array, we apply the conditional element-wise, same as our other operators. Perhaps we should short circuit only in the case when a is not an array? Would that be inconsistent?

karger commented 3 years ago

I think if the variables are arrays we should short circuit on an element by element basis.

LeaVerou commented 3 years ago

How would we do that? I don’t think that’s possible but maybe I don’t understand what you’re proposing

karger commented 3 years ago

Perhaps it is unclear because there are different ways to imagine handling vector expressions. Let me be concrete; suppose A is an array and have an expr like if(isNumber(A), A+1, 0). If I start by evaluating the different elements, I see the expr A+1 and immediately apply an increment to each element of A, generating errors. But if I first evaluate isNumber(A), I get an array of results. I can now go through that array, and evaluate the (vector) expr A+1 only on positions corresponding true entries in the first array.

Now what if instead I say if(isNumber(A), 1). In that case, every entry corrsponding to a number in A should become 1, while other entries should become undefined.

joyously commented 3 years ago

Wouldn't it make more sense that isNumber on an array returns false?

LeaVerou commented 3 years ago

@karger Oh interesting. FWIW in your particular example, short-circuiting makes no difference, since A + 1 doesn't actually increment anything, it just adds 1 to each element of the array and returns it. But I think I see your point. I wonder if this could be implemented by rewriting properties in if() to filter(condition, propertyName), though it seems quite intensive for something that will rarely make any difference :/

@joyously In general, MavoScript operators and functions operate on arrays element-wise.

karger commented 3 years ago

It doesn't make a difference in the result, but it could make a difference in whether errors get thrown trying to evaluate A+1 when (an entry of) A is not defined for example

karger commented 3 years ago

I think the decision about what to do here is orthogonal to efficiency (or maybe as you say short circuiting will help) but instead about (implementation) complexity. It's nice to be able to say "to evaluate an expr, I'll evaluate the arguments, then apply the outer function to them". We've just seen how you can't do that if you want to apply short circuiting---you instead need to evaluate the first argument, then work entry by entry deciding what to do with the second argument. Which means certain functions need an unusual code path.

LeaVerou commented 3 years ago

It doesn't make a difference in the result, but it could make a difference in whether errors get thrown trying to evaluate A+1 when (an entry of) A is not defined for example

No such errors are thrown, at least by native MS functions and operators

Which means certain functions need an unusual code path.

The problem is that at the point of rewriting we don't know which properties are arrays and which ones aren't. At least currently, there are plans to change this.

LeaVerou commented 6 months ago

How about this plan:

This gives us the best of both worlds, and involves no breaking changes!