Refactors PrintStatement so that .values array is all expressions

[markwpearce]

; and , are converted to PrintSeparatorExpression

Addresses #1280

[TwitchBronBron]

The more I look at this, the more I think LiteralExpression is not the correct expression type for this. Perhaps we should make a new PrintSeparatorExpression instead, just to retain the semantic integrity of LiteralExpression.

@elsassph You've had a decent amount of experience with parsers. Any thoughts on how we'd approach this one?

[philippe-wm]

It does feel wrong to make ; into an expression, but considering the way print parameters is modelled as an array of expression it is coherent.

I guess the alternative would be to model it as binary-expressions where ; or , is the operator, but that would be quite a breaking change.

[TwitchBronBron]

Hmm. I'm not crazy about the idea of BinaryExpression to represent the print expression. Let's go with a new expression called PrintSeparatorExpression that looks similar to LiteralExpression but semantically means something else.

[TwitchBronBron]

The more I dug into this, the more I dislike the concept of this expression basically not actually representing code.

I just had another thought. What if we stored an array of separators on .tokens instead? Something like this:

 constructor(options: {
        print: Token;
        expressions: Array<Expression>;
        /**
          * This list must always contain 1 less item than `expressions`. For example, `print 1, 2 ; 3` would contain
          *  expressions: [1, 2, 3] and separators: [',', ';']. 
          */
        separators: Array<Token>
    }) {
}

This keeps the separators separate from the actual "code", while retaining their position. It makes things slightly for annoying Ast creation, but we have similar problems with TemplateStringExpression, and we're not even capturing the commas for function parameters at this point.

[TwitchBronBron]

That doesn't work quite right either. This silly print statement can have all sorts of crazy combinations.

[TwitchBronBron]

I refactored this to leverage a new expression called PrintSeparatorExpression. However, it's still not feeling quite right, because these things are not actual expressions. :/

What about instead storing arrays between each section? Similar to one of my previous examples, except add another level of array.

 constructor(options: {
        print: Token;
        expressions: Array<Expression>;
        separators: Array<Array<Token>>
    }) {
}

Here's an example:

print ,,,1;2 3;;

We'd produce the following ast:

new PrintStatement({
    print: token('print'),
    expressions: [
        literal('1'), 
        literal('2'),
        literal('3')
    ],
    //separators can come before the first expression, so we start with them.
    separators: [
        [token(','), token(','), token(',')],
        [token(';')],
        //no separator between 2 and 3
        [],
        [token(';'), token(';')]
    ]
})

This feels more "correct" in that we don't have these weird expressions floating around that aren't actual code. However, it gets a bit extra gross neeting to maintain these deep nested lists of things. From a plugin perspective, it's a lot easier to build the list of print items just by pushing to a flat expressions list.

@markwpearce @philippe-wm any final thoughts? If you are both okay with the current implementation of PrintSeparatorExpression, then I'll merge it as-is and discard this idea.

[markwpearce]

@TwitchBronBron I think this is ready for merging. I'm happy with this approach.