Don't include namespaces when transpiling ternary / null coalescing expressions

[TwitchBronBron]

ternary and null coalescing expressions are incorrectly "capturing" namespace names for the non-simple statements. We should do a better job filtering those out. (Math should not be sent as a parameter)

[TwitchBronBron]

Closing as duplicate of #678

[przemekborowski]

@TwitchBronBron
Is there a reason to capture consequent / alternate variables at all? IMO solution with filtering namespace parameters is not consistent.

Check this example

namespace aaaa
  function foo()
    return invalid
  end function
end namespace

function bar()
  return invalid
end function

test = true ? aaaa.foo() : bar()

It will be transpiled to IIFE where bar is captured but aaaa_foo not, but they are both functions. Fix for not capturing anything but condition is trivial and it will remove this annoying warning during compilation (or boilerplate code used to get rid of this warning). There is similar problem when foo and bar are const variables defined in namespaces, or classes defined in namespace. Alternative solution could be to only pass VariableExpression as params to IIFE

null coalescing operator could be fixed same way.

[TwitchBronBron]

@przemekborowski we definitely have some cleanup to do around some of these captures. Our transpiler is a bit "dumb" and doesn't really know a lot about the variables it's capturing. With the upcoming type tracking work in #815 we should hopefully have more detailed type information about the variables we're capturing so we can properly handle passing in aaaa_foo properly instead of aaaa (which doesn't exist at runtime).

Alternative solution could be to only pass VariableExpression as params to IIFE
We intentionally avoided this approach, because it required us to evaluate all of the expressions in both the consequent and the alternate. One of the requirements for ternary was to completely skip evaluating the alternate if the condition evaluates to true. Consider this:

value = true ? value : m.throwsExceptionIfCalled()

throwsExceptionIfCalled should only be evaluated if the condition evalutates to false. This is how many other ternary and null coalescing operators work in other languages.

[przemekborowski]

m.throwsExceptionIfCalled() is CallExpression so with proposal of passing only VariableExpressions as parameters to ternary function will not change moment of execution, it would be transpiled to:

 34     test = (function(__bsCondition, value)                                                                                                                                                                                                              
 35             if __bsCondition then                                                                                                                                                                                                                
 36                 return value                                                                                                                                                                                                        
 37             else                                                                                                                                                                                                                                 
 38                 return m.throwsExceptionIfCalled()                                                                                                                                                                                                                
 39             end if                                                                                                                                                                                                                               
 40         end function)(true, value)  

[TwitchBronBron]

You're right. :) I was just trying to explain that we need to avoid executing the alternate in the true case.

As of right now, we do only pass VariableExpression instances to the IIFE.

bar() turns into (roughly) new CallExpression([/*args*/], new VariableExpression("bar")). aaaa.foo turns into (roughly) new GetExpression("foo", new VariableExpression("aaaa")). So then we pass bar and aaaa to the IIFE.

The problem is, aaaa turns out to be a namespace, but we haven't properly detected that so we treat it like any other dotted get access by only passing the leftmost item (the VariableExpression).

The ternary code is here:

brighterscript/src/parser/Expression.ts

Line 1341 in 995a564

transpile(state: BrsTranspileState) {

And the core logic for collecting the expression info is here:

brighterscript/src/util.ts

Line 1137 in 995a564

public getExpressionInfo(expression: Expression): ExpressionInfo {

Once #815 lands, we should be able to update that function to leverage the SymbolTable to do a better job determining which items are actually namespaces and thus actually pass in their transpiled/resolved states to the IIFE.

[przemekborowski]

Exactly brighterscript/src/util.ts is a place where it could be fixed

IMO even now, following change could fix it (I haven't done extensive testing though):
Inside getExpressionInfo remove part:

        // Collect all expressions. Most of these expressions are fairly small so this should be quick!
        // This should only be called during transpile time and only when we actually need it.
        expression?.walk(expressionWalker, {
            walkMode: WalkMode.visitExpressions
        });

and add condition to wrap expressionWalker:

  if (isVariableExpression(expression)) {
        //handle the expression itself (for situations when expression is a VariableExpression)
        expressionWalker(expression);
 }

plus some cleanup inside expressionWalker to not perform checks already done on the caller side.

I can prepare PR with that if you want to follow that path.

[TwitchBronBron]

You're welcome to start working on this to see if you can come up with a solution, sure! However, I don't think this specific suggestion will work.

The BrighterScript AST stores expressions backwards. So for:

alpha.beta.charlie() 'assume `alpha.beta` are namespaces

This is the AST structure:

new CallExpression(
    [/*args*/], 
    new DottedGetExpression(
        "charlie", 
        new DottedGetExpression(
            "beta",
            new VariableExpression("alpha")
        )
    )
)          

getExpressionInfo( will be called with a reference to the CallExpression. The .walk actually walks backwards through:

CallExpression -> DottedGetExpression('charlie') -> DottedGetExpression('beta') -> VariableExpression('alpha')

So you can't just skip the walk, because you'll rarely start with VariableExpression_ except for very simple expressions.

But please feel free to play around with the codebase to see how it's working.