When calling a function with preceding statement arguments, the function itself and the self parameter also need to be stored to temp variables.

I've made a PR to the source branch with (currently failing) tests for this:
tomblind#1

Wow, great work! First of all, sorry that I probably won't have the time to review this.
I just have one quick question regarding the "caching" of tables like self and _G (as described in the PR description).
Since assigning a table only copies the reference, not the table, does that even work/make sense?

@lolleko
Thanks! Regarding tables, what is assigned to the variable name is what might be changed by preceding statements, so we need to cache what was originally assigned to prevent incorrect behavior.

Consider this:

function changeG(this: void) { _G = {}; return 1; };
foo(i += changeG());

function changeG()
    _G = {}
    return 1
end
local ____foo_1 = foo
local ____G_0 = _G
i = i + changeG()
____foo_1(____G_0, i)

changeG() should be executed after _G was already evaluated as the first parameter here, but it's bumped back because of preceding statements, so foo would now be called with the new value of _G instead of the old one. So we have to cache the old one and pass that instead.

Nullish coalescing is not implemented with preceding statements.

This led me to see a larger problem: transformBinaryOperation (in visitors/binary-expression/index.ts) takes left and right lua.expressions rather than ts expressions. This means there is no way to obtain the preceding statements, and every usage of transformBinaryExpression that uses a short-circuit binary operation is potentially wrong.

E.g. see this failing test:

test("nullish-coalescing assignment expression with side effect rhs", () => {
    util.testFunction`
        let i = 0;
        let t = { foo: 0 }
        const bar = (t.foo ??= i++)
        return [i, t.foo, bar]
    `
        .expectToMatchJsResult();
});

Assignment expressions with a table index lhs uses transformBinaryOperation.

If I can comment further, in hindsight I think an issue with this preceding statements implementation is that it makes the code much more stateful; every transform now has the possibility of adding preceding statements, and so can easily lead bugs like above.

An idea for a less stateful implementation would be to introduce some interface ExpressionAndPrecedingStatements (with a shorter name) which contains a lua.Expression and preceding statements needed when evaluated. (Most) parts of the code base that currently return lua.Expression would return that interface instead. This way preceding statements would need to be handled explicitly, and can be treated together with an expression, e.g. the logic oftransformBinaryOperation doesn't need to be modified too much to work with preceding statements using this.

Utilities such as transformExpressionAndPutStatements(ts.Expression, precedingStatements: lua.Statement[] could help with simple cases.

This would touch more of the existing code, but might be worth it. Preceding statements already has an impact on almost all parts of transformation.

I'm open to discuss this/alternatives

@tomblind

changeG() should be executed after _G was already evaluated as the first parameter here, but it's bumped back because of preceding statements, so foo would now be called with the new value of _G instead of the old one. So we have to cache the old one and pass that instead.

Thanks for clarifying, I understand now.

@GlassBricks

every usage of transformBinaryExpression that uses a short-circuit binary operation is potentially wrong.

Good catch on this one. It's going to take some refactoring to get right. 😞

An idea for a less stateful implementation would be to introduce some interface
ExpressionAndPrecedingStatements (with a shorter name) which contains a lua.Expression and preceding statements needed when evaluated.

This was my original plan for this, but it's a fundamental change that would require refactoring pretty much all transformations, so I avoided it in the hopes of keeping the scope of this PR down.

This was my original plan for this, but it's a fundamental change that would require refactoring pretty much all transformations, so I avoided it in the hopes of keeping the scope of this PR down.

The scope of this PR is already pretty big... Even without refactoring everything, using this interface still may be beneficial.

Double-checking every function that takes a lua.Expression but not preceding statements, transformNullishCoalescingOperator in binaryExpression/index.ts is also potentially wrong (and still uses an IIFE).

This was my original plan for this, but it's a fundamental change that would require refactoring pretty much all transformations, so I avoided it in the hopes of keeping the scope of this PR down.

The scope of this PR is already pretty big... Even without refactoring everything, using this interface still may be beneficial.

Double-checking every function that takes a lua.Expression but not preceding statements, transformNullishCoalescingOperator in binaryExpression/index.ts is also potentially wrong (and still uses an IIFE).

Right now the changes to expressions are very limited, all logic for combining and forwarding preceding statements is now spread over the statement transformers, which is already more than I would ideally like. Doing this change would have a very large impact because all expression transformers would need to be touched, and all of them would require to have some logic about how to forward preceding statements from its child expressions.

If we can make this PR change without doing this refactor, I would be in favor of that. I am interested in this alternative, but would prefer to judge it as an independent change.

The issue with transformBinaryOperator is fixed and tests have been added for it.

transformNullishCoalescingOperator in binaryExpression/index.ts is also potentially wrong (and still uses an IIFE).

Thanks for reminding me on this. I had meant to put in a bug for it but forgot. Since it's not directly related to preceding statements, I think it would be best to fix this in a follow up PR.

To comment on the above, I believe you can use ts.setOriginalNode on a generated TS node to retain type information and source map position. This might be useful to reuse functionality of other complex transforms.