fixes various issues with translating assignment operators... by tomblind · Pull Request #241 · TypeScriptToLua/TypeScriptToLua

tomblind · 2018-10-14T16:28:16Z

fixes #221
fixes #225

There's a lot of cases here but here's the gist:

Simple statements remain largely the same. The exception being statements where the left-side is a "complex" property access:

arr[getIndex()] += 1;
getObj().prop += 1;

do local __TS_obj, __TS_index = arr, (getIndex())+1; __TS_obj[__TS_index] = (__TS_obj[__TS_index]+(1)); end;
do local __TS_obj, __TS_index = getObj(), "prop"; __TS_obj[__TS_index] = (__TS_obj[__TS_index]+(1)); end;

"Complex" meaning the object or property is not a simple literal or variable.

Expressions are similar but will cache the right-side in a temp if it is complex:

z = x = obj.prop;
z = obj.prop += 1;

z = ((function() local __TS_tmp = obj.prop; x = __TS_tmp; return __TS_tmp end)());
z = (function() local __TS_tmp = obj.prop+1; obj.prop = __TS_tmp; return __TS_tmp end)();

And if the left-side is a complex property access everything gets cached (using function args):

z = getObj().prop += 1;

z = (function(o, i, v) v = (o[i]+v); o[i] = v; return v end)(getObj(), "prop", 1);

The plan was to use lib polyfills for these but I have opted to skip that for now. It only affects edge-cases with complex property accesses. Also, bitwise operators would require separate implementations for each lua target.

There are also special implementations for destructuring assignments:

[x, y] = xTup;
xTup = [x, y] = yTup;

do local __TS_tmp0,__TS_tmp1 = table.unpack(xTup); x,y = __TS_tmp0,__TS_tmp1 end;
xTup = ((function(...) x,y = ...; return {...} end)(table.unpack(yTup)));

I discovered that in JS (or node.js at least), desturcturing assignments evaluate the right side first and then the left. I assume this is to make it simple for older ES polyfills to use temp variables. This is why I implemented the statement version with temps - to ensure execution order matches the equivalent JS.

There's an extensive set of tests added (both functional and translation). Feel free to drop questions on me as this is fairly complex and has a lot of what-if scenarios.

…ssions

Perryvw

Nice changes, looks like you put a lot of thought into this. I have a few remarks/questions:
First of all the implementation of transpileAssignmentExpression looks very low level and I would prefer if it could be generalized a little. I am afraid it is too focussed on the current issues and will not deal well with cases we missed at this current moment.

Secondly, did you consider deeper nested access expressions, for example:

const a = [[1], [99]];
let i = 0;
function index() { return i++; }
a[index()][0] += 1;
// a should be 2 here

Similarly for arrays of objects etc. This would also make a nice test case.

Perryvw · 2018-10-17T17:48:01Z

src/TSHelper.ts

    }
+
+    // Returns true for expressions that may have effects when evaluated
+    public static isExpressionWithEffect(node: ts.Expression): boolean {


I think the name of this (and the next) function is a little unclear, maybe something like isComplexAccessExpression or isExpressionWithEvaluationEffect would be clearer?

Perryvw · 2018-10-17T17:49:05Z

src/TSHelper.ts

+    // If expression is property/element access with possible effects from being evaluated, returns true along with the
+    // separated object and index expressions.
+    public static isAccessExpressionWithEffects(node: ts.Expression, checker: ts.TypeChecker):
+        [true, ts.Expression, ts.Expression] | [false, null, null] {


Why was this type chosen over something like [boolean, ts.Expression, ts.Expression] which seems like it encompasses both cases?

I did this out of habit because I usually use strict null checks and this allows TS to deduce the subsequent types in the tuple by evaluating the first. But you're right, without strict null checks they are equivalent and the later is more succinct.

I see. I agree in that case that is actually a nice way of defining the type.

Perryvw · 2018-10-17T17:53:45Z

src/Transpiler.ts

+                        result = `(function() local __TS_tmp = ${rhs}; ${lhs} = __TS_tmp; return __TS_tmp end)()`;
+                    } else {
+                        result = `(function() ${lhs} = ${rhs}; return ${rhs} end)()`;
+                    }


Very large switch case, would refactor to its own method.

tomblind · 2018-10-17T18:48:39Z

the implementation of transpileAssignmentExpression looks very low level and I would prefer if it could be generalized a little. I am afraid it is too focussed on the current issues and will not deal well with cases we missed at this current moment.

Did you have something particular in mind? I can break it down into smaller manageable methods and generalize a tiny bit between a couple of the cases, but most of them are fairly distinct and tough to generalize because of subtle differences (which values get cached in temps, etc...).

tomblind · 2018-10-18T00:05:45Z

Alrighty, I've made changes and added tests based on your feedback. I refactored transpileAssignmentExpression in a way that hopefully makes it a bit more maintainable. It now generates an assignment statement and decides to wrap it as an expression at the end instead of having separate code paths for expressions/statements. This changes the output lua a bit, but makes it more consistent which is probably a good thing. The important thing is that it provides a single place to add new exceptions/rules as situations we haven't thought of yet come up. let me know what you think.

Thanks for taking the time to comb through this!

Perryvw

Nice changes!

Perryvw · 2018-10-18T07:38:50Z

test/unit/math.spec.ts

+    @TestCase("of().p.d += af()[i()][i()]", "o=9;a=6")
+    @TestCase("of().p.d -= af()[i()][i()]", "o=-3;a=6")
+    @TestCase("of().p.d *= af()[i()][i()]", "o=18;a=6")
+    @TestCase("af()[i()][i()] /= of().p.d", "o=3;a=2.0")


This seems to be exactly what I would expect, maybe it would be nice to also check the other elements in a have not changed though, maybe.

Perryvw · 2018-10-18T07:43:03Z

test/translation/lua/assignments.lua

+do local __TS_obj, __TS_index = getObj(), "prop"; local __TS_tmp = (__TS_obj[__TS_index] << (getArr()[(getIndex())+1])); __TS_obj[__TS_index] = __TS_tmp; end;
+do local __TS_obj, __TS_index = getObj(), "prop"; local __TS_tmp = (__TS_obj[__TS_index] >> (getArr()[(getIndex())+1])); __TS_obj[__TS_index] = __TS_tmp; end;
 z = ((function() x = y; return y end)());
 z = ((function() obj.prop = x; return x end)());


Just a general remark regarding translation tests: We don't really value them that much (and are considering maybe just getting rid of them in favor of functional tests). This under the reasoning that we don't really care that much about what our resulting lua looks like, as long as it works. For now I'm fine with these though since this does ensure the stability of our translation.

I agree the translation tests should not be taken as seriously as the functional, but I wouldn't remove them as they force you to pay attention when something changes in the output lua that may have unintended consequences. Perhaps something the functional tests miss, or maybe the result is still correct but the new generated lua is terribly inefficient. But yes, I think they should only be a back-up to the functional tests, not a replacement.

fixes for issues with assignments with side-effects, especially expre…

e4ef49b

…ssions

Perryvw reviewed Oct 17, 2018

View reviewed changes

refactoring based on feedback

50ff3ca

Perryvw approved these changes Oct 18, 2018

View reviewed changes

checking all array values in deep assignment tests

d991754

Perryvw merged commit 3237c20 into TypeScriptToLua:master Oct 18, 2018

tomblind deleted the assignment_fixes branch October 18, 2018 19:54

tomblind mentioned this pull request Nov 11, 2018

Post-incrementing properties generates invalid identifiers #276

Closed

Uh oh!

Conversation

tomblind commented Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Perryvw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomblind commented Oct 17, 2018

Uh oh!

tomblind commented Oct 18, 2018

Uh oh!

Perryvw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tomblind commented Oct 14, 2018 •

edited

Loading