Repository: sql-formatter-org/sql-formatter
Pull request #429 (2022-09-05 13:05:38)

Rewrite inline block formatting logic
Instead of having an InlineBlock class, which deals with inline
block length calculation, the inline formatting is now handled
by subclass of Layout.

The old InlineBlock class was very error-prone, needing an update
each time a new AST node type was added, resulting in lots of bugs
over time.  It also duplicated the logic in ExpressionFormatter,
with the length calculation having to be constantly synced with
between the two.  On top of it, it also had bugs, like not taking
account denseMode and parameter value expansions.

Now the InlineLayout simply monitors all the chunks that get
passed through it:

- when encountering a newline an error is thrown,
- when things add up to too long of a string, an error is thrown,

...and we switch to formatting in non-inline mode.


Patch:
diff --git a/src/formatter/ExpressionFormatter.ts b/src/formatter/ExpressionFormatter.ts
index c05f479c..1cf823cc 100644
--- a/src/formatter/ExpressionFormatter.ts
+++ b/src/formatter/ExpressionFormatter.ts
@@ -26,9 +26,9 @@ import {
   PropertyAccessNode,
 } from 'src/parser/ast';
 
-import InlineBlock from './InlineBlock';
 import Layout, { WS } from './Layout';
 import toTabularFormat, { isTabularToken } from './tabularStyle';
+import InlineLayout, { InlineLayoutError } from './InlineLayout';
 
 interface ExpressionFormatterParams {
   cfg: FormatOptions;
@@ -40,7 +40,6 @@ interface ExpressionFormatterParams {
 /** Formats a generic SQL expression */
 export default class ExpressionFormatter {
   private cfg: FormatOptions;
-  private inlineBlock: InlineBlock;
   private params: Params;
   private layout: Layout;
 
@@ -51,7 +50,6 @@ export default class ExpressionFormatter {
   constructor({ cfg, params, layout, inline = false }: ExpressionFormatterParams) {
     this.cfg = cfg;
     this.inline = inline;
-    this.inlineBlock = new InlineBlock(this.cfg.expressionWidth);
     this.params = params;
     this.layout = layout;
   }
@@ -121,22 +119,22 @@ export default class ExpressionFormatter {
   }
 
   private formatParenthesis(node: ParenthesisNode) {
-    const inline = this.inlineBlock.isInlineBlock(node);
+    const inlineLayout = this.formatInlineExpression(node.children);
 
-    if (inline) {
+    if (inlineLayout) {
       this.layout.add(node.openParen);
-      this.layout = this.formatSubExpression(node.children, inline);
+      this.layout.add(...inlineLayout.getLayoutItems());
       this.layout.add(WS.NO_SPACE, node.closeParen, WS.SPACE);
     } else {
       this.layout.add(node.openParen, WS.NEWLINE);
 
       if (isTabularStyle(this.cfg)) {
         this.layout.add(WS.INDENT);
-        this.layout = this.formatSubExpression(node.children, inline);
+        this.layout = this.formatSubExpression(node.children);
       } else {
         this.layout.indentation.increaseBlockLevel();
         this.layout.add(WS.INDENT);
-        this.layout = this.formatSubExpression(node.children, inline);
+        this.layout = this.formatSubExpression(node.children);
         this.layout.indentation.decreaseBlockLevel();
       }
 
@@ -170,7 +168,6 @@ export default class ExpressionFormatter {
 
   private formatSetOperation(node: SetOperationNode) {
     this.layout.add(WS.NEWLINE, WS.INDENT, this.showKw(node.name), WS.NEWLINE);
-
     this.layout.add(WS.INDENT);
     this.layout = this.formatSubExpression(node.children);
   }
@@ -282,15 +279,39 @@ export default class ExpressionFormatter {
     });
   }
 
-  private formatSubExpression(nodes: AstNode[], inline = this.inline): Layout {
+  private formatSubExpression(nodes: AstNode[]): Layout {
     return new ExpressionFormatter({
       cfg: this.cfg,
       params: this.params,
       layout: this.layout,
-      inline,
+      inline: this.inline,
     }).format(nodes);
   }
 
+  private formatInlineExpression(nodes: AstNode[]): Layout | undefined {
+    const oldParamIndex = this.params.getPositionalParameterIndex();
+    try {
+      return new ExpressionFormatter({
+        cfg: this.cfg,
+        params: this.params,
+        layout: new InlineLayout(this.cfg.expressionWidth),
+        inline: true,
+      }).format(nodes);
+    } catch (e) {
+      if (e instanceof InlineLayoutError) {
+        // While formatting, some of the positional parameters might have
+        // been consumed, which increased the current parameter index.
+        // We reset the index to an earlier state, so we can run the
+        // formatting again and re-consume these parameters in non-inline mode.
+        this.params.setPositionalParameterIndex(oldParamIndex);
+        return undefined;
+      } else {
+        // forward all unexpected errors
+        throw e;
+      }
+    }
+  }
+
   private formatKeywordNode(node: KeywordNode): void {
     switch (node.tokenType) {
       case TokenType.RESERVED_JOIN:
diff --git a/src/formatter/InlineBlock.ts b/src/formatter/InlineBlock.ts
deleted file mode 100644
index a551630a..00000000
--- a/src/formatter/InlineBlock.ts
+++ /dev/null
@@ -1,99 +0,0 @@
-import { TokenType, isLogicalOperator } from 'src/lexer/token';
-import {
-  AstNode,
-  BetweenPredicateNode,
-  KeywordNode,
-  NodeType,
-  ParenthesisNode,
-} from 'src/parser/ast';
-
-/**
- * Bookkeeper for inline blocks.
- *
- * Inline blocks are parenthesised expressions that are shorter than INLINE_MAX_LENGTH.
- * These blocks are formatted on a single line, unlike longer parenthesised
- * expressions where open-parenthesis causes newline and increase of indentation.
- */
-export default class InlineBlock {
-  constructor(private expressionWidth: number) {}
-
-  /**
-   * Check if this should be an inline parentheses block
-   * Examples are "NOW()", "COUNT(*)", "int(10)", key(`somecolumn`), DECIMAL(7,2)
-   */
-  public isInlineBlock(parenthesis: ParenthesisNode): boolean {
-    return this.inlineParenthesisWidth(parenthesis) <= this.expressionWidth;
-  }
-
-  private inlineParenthesisWidth(parenthesis: ParenthesisNode): number {
-    // +2 for the two parenthesis
-    return this.inlineWidth(parenthesis.children) + 2;
-  }
-
-  private inlineWidth(nodes: AstNode[]): number {
-    let length = 0;
-
-    for (const node of nodes) {
-      switch (node.type) {
-        case NodeType.function_call:
-          length += node.name.text.length + this.inlineParenthesisWidth(node.parenthesis);
-          break;
-        case NodeType.array_subscript:
-          length += node.array.text.length + this.inlineParenthesisWidth(node.parenthesis);
-          break;
-        case NodeType.property_access:
-          // +1 for the separating dot
-          length += this.inlineWidth([node.object]) + 1 + this.inlineWidth([node.property]);
-          break;
-        case NodeType.parenthesis:
-          length += this.inlineParenthesisWidth(node);
-          break;
-        case NodeType.between_predicate:
-          length += this.betweenWidth(node);
-          break;
-        case NodeType.clause:
-        case NodeType.limit_clause:
-        case NodeType.set_operation:
-        case NodeType.line_comment:
-        case NodeType.block_comment:
-          return Infinity;
-        case NodeType.all_columns_asterisk:
-        case NodeType.comma:
-          length += 1;
-          break;
-        case NodeType.literal:
-        case NodeType.identifier:
-        case NodeType.parameter:
-        case NodeType.operator:
-          length += node.text.length;
-          break;
-        case NodeType.keyword:
-          length += node.text.length;
-          if (this.isForbiddenKeyword(node)) {
-            return Infinity;
-          }
-          break;
-      }
-
-      // Overran max length
-      if (length > this.expressionWidth) {
-        return length;
-      }
-    }
-    return length;
-  }
-
-  private betweenWidth(node: BetweenPredicateNode): number {
-    return (
-      node.between.text.length +
-      this.inlineWidth(node.expr1) +
-      node.and.text.length +
-      this.inlineWidth(node.expr2)
-    );
-  }
-
-  // Reserved words that cause newlines are not allowed inside inline parentheses block
-  private isForbiddenKeyword(node: KeywordNode) {
-    return isLogicalOperator(node.tokenType) || node.tokenType === TokenType.CASE;
-  }
-}
diff --git a/src/formatter/InlineLayout.ts b/src/formatter/InlineLayout.ts
new file mode 100644
index 00000000..fcb1b4a0
--- /dev/null
+++ b/src/formatter/InlineLayout.ts
@@ -0,0 +1,56 @@
+// eslint-disable-next-line max-classes-per-file
+import Indentation from './Indentation';
+import Layout, { WS } from './Layout';
+
+/**
+ * Like Layout, but only formats single-line expressions.
+ *
+ * Throws InlineLayoutError:
+ * - when encountering a newline
+ * - when exceeding configured expressionWidth
+ */
+export default class InlineLayout extends Layout {
+  private length = 0;
+  // Keeps track of the trailing whitespace,
+  // so that we can decrease length when encountering WS.NO_SPACE,
+  // but only when there actually is a space to remove.
+  private trailingSpace = false;
+
+  constructor(private expressionWidth: number) {
+    super(new Indentation('')); // no indentation in inline layout
+  }
+
+  public add(...items: (WS | string)[]) {
+    items.forEach(item => this.addToLength(item));
+    if (this.length > this.expressionWidth) {
+      // We have exceeded the allowable width
+      throw new InlineLayoutError();
+    }
+    super.add(...items);
+  }
+
+  private addToLength(item: WS | string) {
+    if (typeof item === 'string') {
+      this.length += item.length;
+      this.trailingSpace = false;
+    } else if (item === WS.MANDATORY_NEWLINE || item === WS.NEWLINE) {
+      // newlines not allowed within inline block
+      throw new InlineLayoutError();
+    } else if (item === WS.INDENT || item === WS.SINGLE_INDENT || item === WS.SPACE) {
+      if (!this.trailingSpace) {
+        this.length++;
+        this.trailingSpace = true;
+      }
+    } else if (item === WS.NO_NEWLINE || item === WS.NO_SPACE) {
+      if (this.trailingSpace) {
+        this.trailingSpace = false;
+        this.length--;
+      }
+    }
+  }
+}
+
+/**
+ * Thrown when block of SQL can't be formatted as a single line.
+ */
+export class InlineLayoutError extends Error {}
diff --git a/src/formatter/Layout.ts b/src/formatter/Layout.ts
index 5173f7ce..f7a4d0d7 100644
--- a/src/formatter/Layout.ts
+++ b/src/formatter/Layout.ts
@@ -104,6 +104,13 @@ export default class Layout {
     return this.items.map(item => this.itemToString(item)).join('');
   }
 
+  /**
+   * Returns the internal layout data
+   */
+  public getLayoutItems(): LayoutItem[] {
+    return this.items;
+  }
+
   private itemToString(item: LayoutItem): string {
     switch (item) {
       case WS.SPACE:
diff --git a/src/formatter/Params.ts b/src/formatter/Params.ts
index f1bf1653..cae1aa95 100644
--- a/src/formatter/Params.ts
+++ b/src/formatter/Params.ts
@@ -4,11 +4,8 @@ export type ParamItems = { [k: string]: string };
  * Handles placeholder replacement with given params.
  */
 export default class Params {
-  /**
-   * @param {ParamItems} params
-   */
-  params: ParamItems | string[] | undefined;
-  index: number;
+  private params: ParamItems | string[] | undefined;
+  private index: number;
 
   constructor(params: ParamItems | string[] | undefined) {
     this.params = params;
@@ -18,7 +15,7 @@ export default class Params {
   /**
    * Returns param value that matches given placeholder with param key.
    */
-  get({ key, text }: { key?: string; text: string }): string {
+  public get({ key, text }: { key?: string; text: string }): string {
     if (!this.params) {
       return text;
     }
@@ -28,4 +25,18 @@ export default class Params {
     }
     return (this.params as string[])[this.index++];
   }
+
+  /**
+   * Returns index of current positional parameter.
+   */
+  public getPositionalParameterIndex(): number {
+    return this.index;
+  }
+
+  /**
+   * Sets index of current positional parameter.
+   */
+  public setPositionalParameterIndex(i: number) {
+    this.index = i;
+  }
 }


Test patch:
diff --git a/test/bigquery.test.ts b/test/bigquery.test.ts
index faf25232..cd8638f5 100644
--- a/test/bigquery.test.ts
+++ b/test/bigquery.test.ts
@@ -119,12 +119,10 @@ describe('BigQueryFormatter', () => {
   });
 
   it('supports STRUCT types', () => {
-    const result = format(
-      'SELECT STRUCT("Alpha" as name, [23.4, 26.3, 26.4, 26.1] as splits) FROM beta'
-    );
+    const result = format('SELECT STRUCT("Alpha" as name, [23.4, 26.3, 26.4] as splits) FROM beta');
     expect(result).toBe(dedent`
       SELECT
-        STRUCT ("Alpha" as name, [23.4, 26.3, 26.4, 26.1] as splits)
+        STRUCT ("Alpha" as name, [23.4, 26.3, 26.4] as splits)
       FROM
         beta
     `);
diff --git a/test/options/expressionWidth.ts b/test/options/expressionWidth.ts
index f58a0812..c3496afe 100644
--- a/test/options/expressionWidth.ts
+++ b/test/options/expressionWidth.ts
@@ -51,23 +51,46 @@ export default function supportsExpressionWidth(format: FormatFn) {
     `);
   });
 
-  // BUG: Spaces should be considered when computing expression length
-  it('calculates parenthesized expression length without considering spaces', () => {
+  it('calculates parenthesized expression length (also considering spaces)', () => {
     const result = format('SELECT (price * tax) AS total FROM table_name WHERE (amount > 25);', {
       expressionWidth: 10,
       denseOperators: true,
     });
     expect(result).toBe(dedent`
     SELECT
-      (
-        price*tax
-      ) AS total
+      (price*tax) AS total
     FROM
       table_name
     WHERE
-      (
-        amount>25
-      );
+      (amount>25);
+    `);
+  });
+
+  it('formats inline when length of substituted parameters < expressionWidth', () => {
+    const result = format('SELECT (?, ?, ?) AS total;', {
+      expressionWidth: 11,
+      paramTypes: { positional: true },
+      params: ['10', '20', '30'],
+    });
+    expect(result).toBe(dedent`
+      SELECT
+        (10, 20, 30) AS total;
+    `);
+  });
+
+  it('formats NOT-inline when length of substituted parameters > expressionWidth', () => {
+    const result = format('SELECT (?, ?, ?) AS total;', {
+      expressionWidth: 11,
+      paramTypes: { positional: true },
+      params: ['100', '200', '300'],
+    });
+    expect(result).toBe(dedent`
+      SELECT
+        (
+          100,
+          200,
+          300
+        ) AS total;
     `);
   });
 }