Repository: rokucommunity/brighterscript
Pull request #1296 (2024-09-09 12:49:17)

Refactors PrintStatement so that `.values` array is all expressions
`;` and `,` are converted to `PrintSeparatorExpression`

Addresses #1280 

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: Hmm. 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: Hmm. That doesn't work quite right either. This silly print statement can have all sorts of crazy combinations. 

![image](https://github.com/user-attachments/assets/c334dfd1-1b7f-418a-816a-cb39f662e98a)

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:

```typescript
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.



Patch:
diff --git a/src/astUtils/reflection.ts b/src/astUtils/reflection.ts
index 5ec4fc49..360121ce 100644
--- a/src/astUtils/reflection.ts
+++ b/src/astUtils/reflection.ts
@@ -1,5 +1,5 @@
 import type { Body, AssignmentStatement, Block, ExpressionStatement, ExitStatement, FunctionStatement, IfStatement, IncrementStatement, PrintStatement, GotoStatement, LabelStatement, ReturnStatement, EndStatement, StopStatement, ForStatement, ForEachStatement, WhileStatement, DottedSetStatement, IndexedSetStatement, LibraryStatement, NamespaceStatement, ImportStatement, ClassStatement, InterfaceFieldStatement, InterfaceMethodStatement, InterfaceStatement, EnumStatement, EnumMemberStatement, TryCatchStatement, CatchStatement, ThrowStatement, MethodStatement, FieldStatement, ConstStatement, ContinueStatement, TypecastStatement, AliasStatement, ConditionalCompileStatement, ConditionalCompileConstStatement, ConditionalCompileErrorStatement, AugmentedAssignmentStatement, DimStatement } from '../parser/Statement';
-import type { LiteralExpression, BinaryExpression, CallExpression, FunctionExpression, DottedGetExpression, XmlAttributeGetExpression, IndexedGetExpression, GroupingExpression, EscapedCharCodeLiteralExpression, ArrayLiteralExpression, AALiteralExpression, UnaryExpression, VariableExpression, SourceLiteralExpression, NewExpression, CallfuncExpression, TemplateStringQuasiExpression, TemplateStringExpression, TaggedTemplateStringExpression, AnnotationExpression, FunctionParameterExpression, AAMemberExpression, TypecastExpression, TypeExpression, TypedArrayExpression, TernaryExpression, NullCoalescingExpression } from '../parser/Expression';
+import type { LiteralExpression, BinaryExpression, CallExpression, FunctionExpression, DottedGetExpression, XmlAttributeGetExpression, IndexedGetExpression, GroupingExpression, EscapedCharCodeLiteralExpression, ArrayLiteralExpression, AALiteralExpression, UnaryExpression, VariableExpression, SourceLiteralExpression, NewExpression, CallfuncExpression, TemplateStringQuasiExpression, TemplateStringExpression, TaggedTemplateStringExpression, AnnotationExpression, FunctionParameterExpression, AAMemberExpression, TypecastExpression, TypeExpression, TypedArrayExpression, TernaryExpression, NullCoalescingExpression, PrintSeparatorExpression } from '../parser/Expression';
 import type { BrsFile } from '../files/BrsFile';
 import type { XmlFile } from '../files/XmlFile';
 import type { BsDiagnostic, TypedefProvider } from '../interfaces';
@@ -304,6 +304,9 @@ export function isTypecastExpression(element: any): element is TypecastExpressio
 export function isTypedArrayExpression(element: any): element is TypedArrayExpression {
     return element?.kind === AstNodeKind.TypedArrayExpression;
 }
+export function isPrintSeparatorExpression(element: any): element is PrintSeparatorExpression {
+    return element?.kind === AstNodeKind.PrintSeparatorExpression;
+}
 
 // BscType reflection
 export function isStringType(value: any): value is StringType {
diff --git a/src/astUtils/visitors.spec.ts b/src/astUtils/visitors.spec.ts
index 049a8868..316313b9 100644
--- a/src/astUtils/visitors.spec.ts
+++ b/src/astUtils/visitors.spec.ts
@@ -341,6 +341,7 @@ describe('astUtils visitors', () => {
             program.setFile('source/main.brs', EXPRESSIONS_SRC);
             expect(actual).to.deep.equal([
                 'PrintStatement:1:LiteralExpression',             // print <"msg">; 3
+                'PrintStatement:1:PrintSeparatorExpression',      // print "msg"<;> 3
                 'PrintStatement:1:LiteralExpression',             // print "msg"; <3>
                 'PrintStatement:1:TemplateStringExpression',      // print <`expand ${var}`>
                 'PrintStatement:1:TemplateStringQuasiExpression', // print `<expand >${var}`
diff --git a/src/lexer/TokenKind.ts b/src/lexer/TokenKind.ts
index 3988723d..e7089bd2 100644
--- a/src/lexer/TokenKind.ts
+++ b/src/lexer/TokenKind.ts
@@ -1,3 +1,5 @@
+import type { Token } from './Token';
+
 export enum TokenKind {
     // parens (and friends)
     LeftParen = 'LeftParen', // (
@@ -711,6 +713,13 @@ export const AllowedTriviaTokens: ReadonlyArray<TokenKind> = [
 ];
 
 
+/**
+ * Tokens that can be used between expressions in a print statement
+ */
+export type PrintSeparatorToken = Token & {
+    kind: TokenKind.Comma | TokenKind.Semicolon;
+};
+
 /**
  * The tokens that may be in a binary expression
  */
diff --git a/src/parser/AstNode.ts b/src/parser/AstNode.ts
index 78c76acf..cc14d439 100644
--- a/src/parser/AstNode.ts
+++ b/src/parser/AstNode.ts
@@ -324,5 +324,6 @@ export enum AstNodeKind {
     ConditionalCompileStatement = 'ConditionalCompileStatement',
     ConditionalCompileConstStatement = 'ConditionalCompileConstStatement',
     ConditionalCompileErrorStatement = 'ConditionalCompileErrorStatement',
-    AugmentedAssignmentStatement = 'AugmentedAssignmentStatement'
+    AugmentedAssignmentStatement = 'AugmentedAssignmentStatement',
+    PrintSeparatorExpression = 'PrintSeparatorExpression'
 }
diff --git a/src/parser/Expression.ts b/src/parser/Expression.ts
index 9236b2f1..4b41036f 100644
--- a/src/parser/Expression.ts
+++ b/src/parser/Expression.ts
@@ -1,5 +1,6 @@
 /* eslint-disable no-bitwise */
 import type { Token, Identifier } from '../lexer/Token';
+import type { PrintSeparatorToken } from '../lexer/TokenKind';
 import { TokenKind } from '../lexer/TokenKind';
 import type { Block, NamespaceStatement } from './Statement';
 import type { Location } from 'vscode-languageserver';
@@ -966,6 +967,52 @@ export class LiteralExpression extends Expression {
     }
 }
 
+/**
+ * The print statement can have a mix of expressions and separators. These separators represent actual output to the screen,
+ * so this AstNode represents those separators (comma, semicolon, and whitespace)
+ */
+export class PrintSeparatorExpression extends Expression {
+    constructor(options: {
+        separator: PrintSeparatorToken;
+    }) {
+        super();
+        this.tokens = {
+            separator: options.separator
+        };
+        this.location = this.tokens.separator.location;
+    }
+
+    public readonly tokens: {
+        readonly separator: PrintSeparatorToken;
+    };
+
+    public readonly kind = AstNodeKind.PrintSeparatorExpression;
+
+    public location: Location;
+
+    transpile(state: BrsTranspileState) {
+        return [
+            ...this.tokens.separator.leadingWhitespace ?? [],
+            ...state.transpileToken(this.tokens.separator)
+        ];
+    }
+
+    walk(visitor: WalkVisitor, options: WalkOptions) {
+        //nothing to walk
+    }
+
+    get leadingTrivia(): Token[] {
+        return this.tokens.separator.leadingTrivia;
+    }
+
+    public clone() {
+        return new PrintSeparatorExpression({
+            separator: util.cloneToken(this.tokens?.separator)
+        });
+    }
+}
+
+
 /**
  * This is a special expression only used within template strings. It exists so we can prevent producing lots of empty strings
  * during template string transpile by identifying these expressions explicitly and skipping the bslib_toString around them
diff --git a/src/parser/Parser.ts b/src/parser/Parser.ts
index 85304bee..1bb748d8 100644
--- a/src/parser/Parser.ts
+++ b/src/parser/Parser.ts
@@ -1,6 +1,6 @@
 import type { Token, Identifier } from '../lexer/Token';
 import { isToken } from '../lexer/Token';
-import type { BlockTerminator } from '../lexer/TokenKind';
+import type { BlockTerminator, PrintSeparatorToken } from '../lexer/TokenKind';
 import { Lexer } from '../lexer/Lexer';
 import {
     AllowedLocalIdentifiers,
@@ -17,10 +17,6 @@ import {
     CompoundAssignmentOperators,
     BinaryExpressionOperatorTokens
 } from '../lexer/TokenKind';
-import type {
-    PrintSeparatorSpace,
-    PrintSeparatorTab
-} from './Statement';
 import {
     AliasStatement,
     AssignmentStatement,
@@ -96,7 +92,8 @@ import {
     TypedArrayExpression,
     UnaryExpression,
     VariableExpression,
-    XmlAttributeGetExpression
+    XmlAttributeGetExpression,
+    PrintSeparatorExpression
 } from './Expression';
 import type { Range } from 'vscode-languageserver';
 import type { Logger } from '../logging';
@@ -2441,16 +2438,11 @@ export class Parser {
     private printStatement(): PrintStatement {
         let printKeyword = this.advance();
 
-        let values: (
-            | Expression
-            | PrintSeparatorTab
-            | PrintSeparatorSpace)[] = [];
+        let values: Expression[] = [];
 
         while (!this.checkEndOfStatement()) {
-            if (this.check(TokenKind.Semicolon)) {
-                values.push(this.advance() as PrintSeparatorSpace);
-            } else if (this.check(TokenKind.Comma)) {
-                values.push(this.advance() as PrintSeparatorTab);
+            if (this.checkAny(TokenKind.Semicolon, TokenKind.Comma)) {
+                values.push(new PrintSeparatorExpression({ separator: this.advance() as PrintSeparatorToken }));
             } else if (this.check(TokenKind.Else)) {
                 break; // inline branch
             } else {
diff --git a/src/parser/Statement.ts b/src/parser/Statement.ts
index b6f279e3..667bd7de 100644
--- a/src/parser/Statement.ts
+++ b/src/parser/Statement.ts
@@ -9,13 +9,13 @@ import type { BrsTranspileState } from './BrsTranspileState';
 import { ParseMode } from './Parser';
 import type { WalkVisitor, WalkOptions } from '../astUtils/visitors';
 import { InternalWalkMode, walk, createVisitor, WalkMode, walkArray } from '../astUtils/visitors';
-import { isCallExpression, isCatchStatement, isConditionalCompileStatement, isEnumMemberStatement, isExpression, isExpressionStatement, isFieldStatement, isForEachStatement, isForStatement, isFunctionExpression, isFunctionStatement, isIfStatement, isInterfaceFieldStatement, isInterfaceMethodStatement, isInvalidType, isLiteralExpression, isMethodStatement, isNamespaceStatement, isTryCatchStatement, isTypedefProvider, isUnaryExpression, isVoidType, isWhileStatement } from '../astUtils/reflection';
+import { isCallExpression, isCatchStatement, isConditionalCompileStatement, isEnumMemberStatement, isExpressionStatement, isFieldStatement, isForEachStatement, isForStatement, isFunctionExpression, isFunctionStatement, isIfStatement, isInterfaceFieldStatement, isInterfaceMethodStatement, isInvalidType, isLiteralExpression, isMethodStatement, isNamespaceStatement, isPrintSeparatorExpression, isTryCatchStatement, isTypedefProvider, isUnaryExpression, isVoidType, isWhileStatement } from '../astUtils/reflection';
 import { TypeChainEntry, type GetTypeOptions, type TranspileResult, type TypedefProvider } from '../interfaces';
 import { createInvalidLiteral, createMethodStatement, createToken } from '../astUtils/creators';
 import { DynamicType } from '../types/DynamicType';
 import type { BscType } from '../types/BscType';
 import { SymbolTable } from '../SymbolTable';
-import type { AstNode, Expression } from './AstNode';
+import type { Expression } from './AstNode';
 import { AstNodeKind, Statement } from './AstNode';
 import { ClassType } from '../types/ClassType';
 import { EnumMemberType, EnumType } from '../types/EnumType';
@@ -878,16 +878,6 @@ export class IncrementStatement extends Statement {
     }
 }
 
-/** Used to indent the current `print` position to the next 16-character-width output zone. */
-export interface PrintSeparatorTab extends Token {
-    kind: TokenKind.Comma;
-}
-
-/** Used to insert a single whitespace character at the current `print` position. */
-export interface PrintSeparatorSpace extends Token {
-    kind: TokenKind.Semicolon;
-}
-
 /**
  * Represents a `print` statement within BrightScript.
  */
@@ -896,11 +886,11 @@ export class PrintStatement extends Statement {
      * Creates a new internal representation of a BrightScript `print` statement.
      * @param options the options for this statement
      * @param options.print a print token
-     * @param options.expressions an array of expressions or `PrintSeparator`s to be evaluated and printed.
+     * @param options.expressions an array of expressions to be evaluated and printed. Wrap PrintSeparator tokens (`;` or `,`) in `PrintSeparatorExpression`
      */
     constructor(options: {
         print?: Token;
-        expressions: Array<Expression | PrintSeparatorTab | PrintSeparatorSpace>;
+        expressions: Array<Expression>;
     }) {
         super();
         this.tokens = {
@@ -912,40 +902,48 @@ export class PrintStatement extends Statement {
             ...(this.expressions ?? [])
         );
     }
+
     public readonly tokens: {
         readonly print?: Token;
     };
-    public readonly expressions: Array<Expression | PrintSeparatorTab | PrintSeparatorSpace>;
+
+    public readonly expressions: Array<Expression>;
+
     public readonly kind = AstNodeKind.PrintStatement;
 
     public readonly location: Location | undefined;
 
     transpile(state: BrsTranspileState) {
         let result = [
-            state.transpileToken(this.tokens.print, 'print'),
-            ' '
+            state.transpileToken(this.tokens.print, 'print')
         ] as TranspileResult;
+
+        //if the first expression has no leading whitespace, add a single space between the `print` and the expression
+        if (this.expressions.length > 0 && !this.expressions[0].leadingTrivia.find(t => t.kind === TokenKind.Whitespace)) {
+            result.push(' ');
+        }
+
+        // eslint-disable-next-line @typescript-eslint/prefer-for-of
         for (let i = 0; i < this.expressions.length; i++) {
-            const expressionOrSeparator: any = this.expressions[i];
-            if (expressionOrSeparator.transpile) {
-                result.push(...(expressionOrSeparator as ExpressionStatement).transpile(state));
-            } else {
-                result.push(
-                    state.tokenToSourceNode(expressionOrSeparator)
-                );
-            }
-            //if there's an expression after us, add a space
-            if ((this.expressions[i + 1] as any)?.transpile) {
+            const expression = this.expressions[i];
+            let leadingWhitespace = expression.leadingTrivia.find(t => t.kind === TokenKind.Whitespace)?.text;
+            if (leadingWhitespace) {
+                result.push(leadingWhitespace);
+                //if the previous expression was NOT a separator, and this one is not also, add a space between them
+            } else if (i > 0 && !isPrintSeparatorExpression(this.expressions[i - 1]) && !isPrintSeparatorExpression(expression) && !leadingWhitespace) {
                 result.push(' ');
             }
+
+            result.push(
+                ...expression.transpile(state)
+            );
         }
         return result;
     }
 
     walk(visitor: WalkVisitor, options: WalkOptions) {
         if (options.walkMode & InternalWalkMode.walkExpressions) {
-            //sometimes we have semicolon Tokens in the expressions list (should probably fix that...), so only walk the actual expressions
-            walkArray(this.expressions as AstNode[], visitor, options, this, (item) => isExpression(item as any));
+            walkArray(this.expressions, visitor, options, this);
         }
     }
 
@@ -957,13 +955,7 @@ export class PrintStatement extends Statement {
         return this.finalizeClone(
             new PrintStatement({
                 print: util.cloneToken(this.tokens.print),
-                expressions: this.expressions?.map(e => {
-                    if (isExpression(e as any)) {
-                        return (e as Expression).clone();
-                    } else {
-                        return util.cloneToken(e as Token);
-                    }
-                })
+                expressions: this.expressions?.map(e => e?.clone())
             }),
             ['expressions' as any]
         );


Test patch:
diff --git a/src/parser/tests/statement/PrintStatement.spec.ts b/src/parser/tests/statement/PrintStatement.spec.ts
index bca529b2..803464de 100644
--- a/src/parser/tests/statement/PrintStatement.spec.ts
+++ b/src/parser/tests/statement/PrintStatement.spec.ts
@@ -189,8 +189,8 @@ describe('parser print statements', () => {
                     x = 5
                     print 25; " is equal to"; x ^ 2
                     a$ = "string"
-                    print a$; a$, a$; " "; a$
-                    print "zone 1", "zone 2", "zone 3", "zone 4"
+                    print a$;a$,a$;" ";a$
+                    print "zone 1","zone 2","zone 3","zone 4"
                     print "print statement #1 "
                     print "print statement #2"
                     print "this is a five " 5 "!!"
@@ -202,7 +202,7 @@ describe('parser print statements', () => {
                     print [
                         5
                     ]
-                    print tab(5) "tabbed 5"; tab(25) "tabbed 25"
+                    print tab(5) "tabbed 5";tab(25) "tabbed 25"
                     print tab(40) pos(0) 'prints 40 at position 40
                     print "these" tab(pos(0) + 5) "words" tab(pos(0) + 5) "are"
                     print tab(pos(0) + 5) "evenly" tab(pos(0) + 5) "spaced"