Repository: sql-formatter-org/sql-formatter
Pull request #262 (2022-06-28 14:17:54)

8.0: Refactor WhitespaceBuilder to avoid string-manipulation
Instead of using `WhitespaceBuilder.toString()` method to get the result of formatting a sub-expression (e.g. a Parenthesis block) and then doing string-manipulation to append it to current formatting state, the `WhitespaceBuilder` now exposes two new methods:

- `toLayout()` returns the internal representation of formatted SQL,
- `addLayout()` takes the output from the above method and appends it to current formatting context, adding indentation as needed to each line.

Additionally the `Statement` node no more contains semicolon token inside its children, but instead has a `hasSemicolon` field.


Patch:
diff --git a/src/core/ExpressionFormatter.ts b/src/core/ExpressionFormatter.ts
index 3e94067d..f330c760 100644
--- a/src/core/ExpressionFormatter.ts
+++ b/src/core/ExpressionFormatter.ts
@@ -17,7 +17,7 @@ import {
   Parenthesis,
 } from './ast';
 import { indentString } from './config';
-import WhitespaceBuilder, { WS } from './WhitespaceBuilder';
+import WhitespaceBuilder, { LayoutItem, WS } from './WhitespaceBuilder';
 
 /** Formats a generic SQL expression */
 export default class ExpressionFormatter {
@@ -40,7 +40,7 @@ export default class ExpressionFormatter {
     this.query = new WhitespaceBuilder(this.indentation);
   }
 
-  public format(nodes: AstNode[]): string {
+  public format(nodes: AstNode[]): WhitespaceBuilder {
     this.nodes = nodes;
 
     for (this.index = 0; this.index < this.nodes.length; this.index++) {
@@ -75,7 +75,7 @@ export default class ExpressionFormatter {
           break;
       }
     }
-    return this.query.toString();
+    return this.query;
   }
 
   private formatFunctionCall(node: FunctionCall) {
@@ -91,20 +91,16 @@ export default class ExpressionFormatter {
   private formatParenthesis(node: Parenthesis) {
     const inline = this.inlineBlock.isInlineBlock(node);
 
-    const formattedSql = new ExpressionFormatter(this.cfg, this.params, {
-      inline,
-    })
-      .format(node.children)
-      .trimEnd();
+    const subLayout = this.formatSubExpression(node.children, inline);
 
     if (inline) {
-      this.query.add(node.openParen, formattedSql, node.closeParen, WS.SPACE);
+      this.query.add(node.openParen, ...subLayout, WS.NO_SPACE, node.closeParen, WS.SPACE);
     } else {
-      this.query.add(node.openParen);
+      this.query.add(node.openParen, WS.NEWLINE);
 
-      formattedSql.split(/\n/).forEach(line => {
-        this.query.add(WS.NEWLINE, WS.INDENT, WS.SINGLE_INDENT, line);
-      });
+      this.indentation.increaseBlockLevel();
+      this.query.addLayout(subLayout);
+      this.indentation.decreaseBlockLevel();
 
       this.query.add(WS.NEWLINE, WS.INDENT, node.closeParen, WS.SPACE);
     }
@@ -124,38 +120,22 @@ export default class ExpressionFormatter {
   }
 
   private formatClause(node: Clause) {
-    const formattedSql = new ExpressionFormatter(this.cfg, this.params, { inline: this.inline })
-      .format(node.children)
-      .trimEnd();
+    const subLayout = this.formatSubExpression(node.children);
 
     this.indentation.decreaseTopLevel();
-    this.query.add(WS.NEWLINE, WS.INDENT, this.show(node.nameToken));
+    this.query.add(WS.NEWLINE, WS.INDENT, this.show(node.nameToken), WS.NEWLINE);
     this.indentation.increaseTopLevel();
 
-    if (formattedSql.length > 0) {
-      formattedSql.split(/\n/).forEach(line => {
-        this.query.add(WS.NEWLINE, WS.INDENT, line);
-      });
-    }
-
-    this.query.add(WS.SPACE);
+    this.query.addLayout(subLayout);
   }
 
   private formatBinaryClause(node: BinaryClause) {
-    const formattedSql = new ExpressionFormatter(this.cfg, this.params, { inline: this.inline })
-      .format(node.children)
-      .trimEnd();
+    const subLayout = this.formatSubExpression(node.children);
 
     this.indentation.decreaseTopLevel();
-    this.query.add(WS.NEWLINE, WS.INDENT, this.show(node.nameToken));
-
-    if (formattedSql.length > 0) {
-      formattedSql.split(/\n/).forEach(line => {
-        this.query.add(WS.NEWLINE, WS.INDENT, line);
-      });
-    }
+    this.query.add(WS.NEWLINE, WS.INDENT, this.show(node.nameToken), WS.NEWLINE);
 
-    this.query.add(WS.NEWLINE, WS.INDENT);
+    this.query.addLayout(subLayout);
   }
 
   private formatLimitClause(node: LimitClause) {
@@ -183,6 +163,10 @@ export default class ExpressionFormatter {
     this.query.add('*', WS.SPACE);
   }
 
+  private formatSubExpression(nodes: AstNode[], inline = this.inline): LayoutItem[] {
+    return new ExpressionFormatter(this.cfg, this.params, { inline }).format(nodes).toLayout();
+  }
+
   private formatToken(token: Token): void {
     switch (token.type) {
       case TokenType.LINE_COMMENT:
@@ -231,12 +215,20 @@ export default class ExpressionFormatter {
 
   /** Formats a block comment onto query */
   private formatBlockComment(token: Token) {
-    this.query.add(WS.NEWLINE, WS.INDENT, this.indentComment(token.value), WS.NEWLINE, WS.INDENT);
+    this.splitBlockComment(token.value).forEach(line => {
+      this.query.add(WS.NEWLINE, WS.INDENT, line);
+    });
+    this.query.add(WS.NEWLINE, WS.INDENT);
   }
 
-  /** Aligns comment to current indentation level */
-  private indentComment(comment: string): string {
-    return comment.replace(/\n[ \t]*/gu, '\n' + this.indentation.getIndent() + ' ');
+  private splitBlockComment(comment: string): string[] {
+    return comment.split(/\n/).map(line => {
+      if (/^\s*\*/.test(line)) {
+        return ' ' + line.replace(/^\s*/, '');
+      } else {
+        return line.replace(/^\s*/, '');
+      }
+    });
   }
 
   private formatJoin(token: Token) {
@@ -270,9 +262,6 @@ export default class ExpressionFormatter {
     if (token.value === ',') {
       this.formatComma(token);
       return;
-    } else if (token.value === ';') {
-      this.formatQuerySeparator(token);
-      return;
     } else if (token.value === ':') {
       this.query.add(WS.NO_SPACE, this.show(token), WS.SPACE);
       return;
@@ -333,14 +322,6 @@ export default class ExpressionFormatter {
     }
   }
 
-  private formatQuerySeparator(token: Token) {
-    if (this.cfg.newlineBeforeSemicolon) {
-      this.query.add(WS.NEWLINE, this.show(token));
-    } else {
-      this.query.add(WS.NO_SPACE, this.show(token));
-    }
-  }
-
   private show(token: Token): string {
     return this.showToken(token);
   }
diff --git a/src/core/Formatter.ts b/src/core/Formatter.ts
index 438d16de..e24daaf3 100644
--- a/src/core/Formatter.ts
+++ b/src/core/Formatter.ts
@@ -8,6 +8,7 @@ import ExpressionFormatter from './ExpressionFormatter';
 import { indentString } from './config';
 import AliasAs from './AliasAs';
 import { Statement } from './ast';
+import { WS } from './WhitespaceBuilder';
 
 /** Main formatter class that produces a final output string from list of tokens */
 export default class Formatter {
@@ -54,10 +55,22 @@ export default class Formatter {
 
   private formatAst(statements: Statement[]): string {
     return statements
-      .map(stat => new ExpressionFormatter(this.cfg, this.params).format(stat.children))
+      .map(stat => this.formatStatement(stat))
       .join('\n'.repeat(this.cfg.linesBetweenQueries + 1));
   }
 
+  private formatStatement(statement: Statement): string {
+    const wsBuilder = new ExpressionFormatter(this.cfg, this.params).format(statement.children);
+    if (!statement.hasSemicolon) {
+      // do nothing
+    } else if (this.cfg.newlineBeforeSemicolon) {
+      wsBuilder.add(WS.NEWLINE, ';');
+    } else {
+      wsBuilder.add(WS.NO_SPACE, ';');
+    }
+    return wsBuilder.toString();
+  }
+
   private postFormat(query: string): string {
     if (this.cfg.tabulateAlias) {
       query = formatAliasPositions(query);
diff --git a/src/core/Parser.ts b/src/core/Parser.ts
index aa351204..55017f3f 100644
--- a/src/core/Parser.ts
+++ b/src/core/Parser.ts
@@ -15,8 +15,7 @@ import {
 import { EOF_TOKEN, type Token, TokenType, isToken } from './token';
 
 /**
- * A rudimentary parser that slices token stream into list of SQL statements
- * and groups of parenthesis.
+ * A simple parser that creates a very rudimentary syntax tree.
  */
 export default class Parser {
   private index = 0;
@@ -36,11 +35,11 @@ export default class Parser {
     const children: AstNode[] = [];
     while (true) {
       if (this.look().value === ';') {
-        children.push(this.nextTokenNode());
-        return { type: 'statement', children };
+        this.next();
+        return { type: 'statement', children, hasSemicolon: true };
       } else if (this.look().type === TokenType.EOF) {
         if (children.length > 0) {
-          return { type: 'statement', children };
+          return { type: 'statement', children, hasSemicolon: false };
         } else {
           return undefined;
         }
diff --git a/src/core/WhitespaceBuilder.ts b/src/core/WhitespaceBuilder.ts
index c19bf120..19f18698 100644
--- a/src/core/WhitespaceBuilder.ts
+++ b/src/core/WhitespaceBuilder.ts
@@ -12,6 +12,8 @@ export enum WS {
   SINGLE_INDENT = 6, // Adds whitespace for single indentation step
 }
 
+export type LayoutItem = WS.SPACE | WS.SINGLE_INDENT | WS.NEWLINE | string;
+
 /**
  * API for constructing SQL string (especially the whitespace part).
  *
@@ -20,10 +22,24 @@ export enum WS {
  * Now it's storing items to array and builds the string only in the end.
  */
 export default class WhitespaceBuilder {
-  private query: (WS.SPACE | WS.SINGLE_INDENT | WS.NEWLINE | string)[] = [];
+  private layout: LayoutItem[] = [];
 
   constructor(private indentation: Indentation) {}
 
+  /**
+   * Appands an already constructed sub-layout
+   * and indents each line in it by current level of indentation
+   */
+  public addLayout(layout: LayoutItem[]) {
+    this.addIndentation();
+    layout.forEach((item, i) => {
+      this.layout.push(item);
+      if (item === WS.NEWLINE && i < layout.length - 1) {
+        this.addIndentation();
+      }
+    });
+  }
+
   /**
    * Appends token strings and whitespace modifications to SQL string.
    */
@@ -31,7 +47,7 @@ export default class WhitespaceBuilder {
     for (const item of items) {
       switch (item) {
         case WS.SPACE:
-          this.query.push(WS.SPACE);
+          this.layout.push(WS.SPACE);
           break;
         case WS.NO_SPACE:
           this.trimHorizontalWhitespace();
@@ -44,34 +60,38 @@ export default class WhitespaceBuilder {
           this.trimAllWhitespace();
           break;
         case WS.INDENT:
-          for (let i = 0; i < this.indentation.getLevel(); i++) {
-            this.query.push(WS.SINGLE_INDENT);
-          }
+          this.addIndentation();
           break;
         case WS.SINGLE_INDENT:
-          this.query.push(WS.SINGLE_INDENT);
+          this.layout.push(WS.SINGLE_INDENT);
           break;
         default:
-          this.query.push(item);
+          this.layout.push(item);
       }
     }
   }
 
   private trimHorizontalWhitespace() {
-    while (isHorizontalWhitespace(last(this.query))) {
-      this.query.pop();
+    while (isHorizontalWhitespace(last(this.layout))) {
+      this.layout.pop();
     }
   }
 
   private trimAllWhitespace() {
-    while (isWhitespace(last(this.query))) {
-      this.query.pop();
+    while (isWhitespace(last(this.layout))) {
+      this.layout.pop();
     }
   }
 
   private addNewline() {
-    if (this.query.length > 0 && last(this.query) !== WS.NEWLINE) {
-      this.query.push(WS.NEWLINE);
+    if (this.layout.length > 0 && last(this.layout) !== WS.NEWLINE) {
+      this.layout.push(WS.NEWLINE);
+    }
+  }
+
+  private addIndentation() {
+    for (let i = 0; i < this.indentation.getLevel(); i++) {
+      this.layout.push(WS.SINGLE_INDENT);
     }
   }
 
@@ -79,10 +99,17 @@ export default class WhitespaceBuilder {
    * Returns the final SQL string.
    */
   public toString(): string {
-    return this.query.map(item => this.itemToString(item)).join('');
+    return this.layout.map(item => this.itemToString(item)).join('');
+  }
+
+  /**
+   * Returns the constructed whitespace layout structure.
+   */
+  public toLayout(): LayoutItem[] {
+    return this.layout;
   }
 
-  private itemToString(item: WS | string) {
+  private itemToString(item: LayoutItem): string {
     switch (item) {
       case WS.SPACE:
         return ' ';
diff --git a/src/core/ast.ts b/src/core/ast.ts
index e2bfa276..965b1b36 100644
--- a/src/core/ast.ts
+++ b/src/core/ast.ts
@@ -3,6 +3,7 @@ import { Token } from './token';
 export type Statement = {
   type: 'statement';
   children: AstNode[];
+  hasSemicolon: boolean;
 };
 
 export type Clause = {


Test patch:
diff --git a/test/features/comments.ts b/test/features/comments.ts
index 6c46bc2e..68f6dcd5 100644
--- a/test/features/comments.ts
+++ b/test/features/comments.ts
@@ -56,8 +56,7 @@ export default function supportsComments(format: FormatFn, opts: CommentsConfig
     `);
   });
 
-  // XXX: Temporarily disabled. Fix this!
-  it.skip('formats line comments followed by semicolon', () => {
+  it('formats line comments followed by semicolon', () => {
     expect(
       format(`
       SELECT a FROM b
diff --git a/test/unit/Parser.test.ts b/test/unit/Parser.test.ts
index 456fe7f7..140558d5 100644
--- a/test/unit/Parser.test.ts
+++ b/test/unit/Parser.test.ts
@@ -36,16 +36,8 @@ describe('Parser', () => {
               },
               "type": "token",
             },
-            Object {
-              "token": Object {
-                "text": ";",
-                "type": "OPERATOR",
-                "value": ";",
-                "whitespaceBefore": "",
-              },
-              "type": "token",
-            },
           ],
+          "hasSemicolon": true,
           "type": "statement",
         },
         Object {
@@ -60,6 +52,7 @@ describe('Parser', () => {
               "type": "token",
             },
           ],
+          "hasSemicolon": false,
           "type": "statement",
         },
       ]
@@ -108,6 +101,7 @@ describe('Parser', () => {
               "type": "clause",
             },
           ],
+          "hasSemicolon": false,
           "type": "statement",
         },
       ]
@@ -172,6 +166,7 @@ describe('Parser', () => {
               "type": "clause",
             },
           ],
+          "hasSemicolon": false,
           "type": "statement",
         },
       ]
@@ -254,6 +249,7 @@ describe('Parser', () => {
               "type": "clause",
             },
           ],
+          "hasSemicolon": false,
           "type": "statement",
         },
       ]
@@ -312,16 +308,8 @@ describe('Parser', () => {
               },
               "type": "clause",
             },
-            Object {
-              "token": Object {
-                "text": ";",
-                "type": "OPERATOR",
-                "value": ";",
-                "whitespaceBefore": "",
-              },
-              "type": "token",
-            },
           ],
+          "hasSemicolon": true,
           "type": "statement",
         },
       ]
@@ -349,6 +337,7 @@ describe('Parser', () => {
               "type": "limit_clause",
             },
           ],
+          "hasSemicolon": false,
           "type": "statement",
         },
       ]
@@ -382,6 +371,7 @@ describe('Parser', () => {
               "type": "limit_clause",
             },
           ],
+          "hasSemicolon": false,
           "type": "statement",
         },
       ]
@@ -408,6 +398,7 @@ describe('Parser', () => {
               "type": "clause",
             },
           ],
+          "hasSemicolon": false,
           "type": "statement",
         },
       ]