Merge pull request #1040 from Shopify/invalid-range-in-loops

Detect and report invalid ranges in loops

Author: aswamy

.changeset/new-tomatoes-sort.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Detect and report invalid ranges in loops

packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidLoopRange.spec.ts

@@ -0,0 +1,103 @@
+import { expect, describe, it, vi, beforeEach } from 'vitest';
+import { applyFix, runLiquidCheck } from '../../../test';
+import { LiquidHTMLSyntaxError } from '../index';
+import { toLiquidAST } from '@shopify/liquid-html-parser';
+import { INVALID_LOOP_RANGE_MESSAGE } from './InvalidLoopRange';
+
+vi.mock('@shopify/liquid-html-parser', async (importOriginal) => {
+  const original: any = await importOriginal();
+
+  return {
+    ...original,
+
+    // we will be mocked later on
+    toLiquidAST: vi.fn().mockImplementation((source, options) => {
+      return original.toLiquidAST(source, options);
+    }),
+  };
+});
+
+describe('detectInvalidLoopRange', async () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+
+  it('should not report when range is valid', async () => {
+    const testCases = [
+      `{% for i in (1..10) %}{% endfor %}`,
+      `{% tablerow x in (a..b) %}{% endtablerow %}`,
+    ];
+
+    for (const sourceCode of testCases) {
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).to.have.length(0);
+    }
+  });
+
+  it('should report when there is an extra `.` character in range', async () => {
+    const testCases = [
+      [`{% for i in (1...10) %}{% endfor %}`, '{% for i in (1..10) %}{% endfor %}'],
+      [
+        `{% tablerow x in (a...b) %}{% endtablerow %}`,
+        '{% tablerow x in (a..b) %}{% endtablerow %}',
+      ],
+      [
+        `{% tablerow x in (a .. b) %}{% endtablerow %}`,
+        '{% tablerow x in (a..b) %}{% endtablerow %}',
+      ],
+      [
+        `{% tablerow x in ( a..b ) %}{% endtablerow %}`,
+        '{% tablerow x in (a..b) %}{% endtablerow %}',
+      ],
+    ];
+
+    for (const [sourceCode, expected] of testCases) {
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).to.have.length(1);
+      expect(offenses[0].message).to.equal(INVALID_LOOP_RANGE_MESSAGE);
+
+      const fixed = applyFix(sourceCode, offenses[0]);
+      expect(fixed).to.equal(expected);
+    }
+  });
+
+  it('should report when there are multiple instances of the error', async () => {
+    const sourceCode = `{% for i in (1...10) %}{% endfor %} {% tablerow x in (a...b) %}{% endtablerow %}`;
+
+    const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+    expect(offenses).to.have.length(2);
+    expect(offenses[0].message).to.equal(INVALID_LOOP_RANGE_MESSAGE);
+    expect(offenses[1].message).to.equal(INVALID_LOOP_RANGE_MESSAGE);
+
+    expect(toLiquidAST).toHaveBeenCalledTimes(2);
+    expect(toLiquidAST).toHaveBeenCalledWith(`{% for i in (1..10) %}{% endfor %}`, {
+      allowUnclosedDocumentNode: false,
+      mode: 'strict',
+    });
+    expect(toLiquidAST).toHaveBeenCalledWith(`{% tablerow x in (a..b) %}{% endtablerow %}`, {
+      allowUnclosedDocumentNode: false,
+      mode: 'strict',
+    });
+
+    const fixed = applyFix(sourceCode, offenses[0]);
+    expect(fixed).to.equal(
+      `{% for i in (1..10) %}{% endfor %} {% tablerow x in (a...b) %}{% endtablerow %}`,
+    );
+
+    const fixed2 = applyFix(sourceCode, offenses[1]);
+    expect(fixed2).to.equal(
+      `{% for i in (1...10) %}{% endfor %} {% tablerow x in (a..b) %}{% endtablerow %}`,
+    );
+  });
+
+  it('should not report when the fixed code produces an invalid AST', async () => {
+    const sourceCode = `{% for i in (1..10) %}{% endfor %}`;
+
+    vi.mocked(toLiquidAST).mockImplementation((_source, _options) => {
+      throw new SyntaxError('Invalid AST');
+    });
+
+    const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+    expect(offenses).to.have.length(0);
+  });
+});

packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidLoopRange.ts

@@ -0,0 +1,81 @@
+import { LiquidTag, LiquidTagFor, LiquidTagTablerow, NodeTypes } from '@shopify/liquid-html-parser';
+import { Problem, SourceCodeType } from '../../..';
+import { ensureValidAst } from './utils';
+import { isLoopLiquidTag } from '../../utils';
+
+// Lax parser does NOT complain about leading/trailing spaces inside ranges (e.g. `(1 .. 10 )`) and
+// within the parenthesis (e.g. `( 1 .. 10 )`), but fails to render the liquid when using the gem.
+// Strict parser does NOT complain, but still renders it.
+// To avoid any issues, we will remove extra spaces.
+const RANGE_MARKUP_REGEX = /\(\s*(\w+)\s*(\.{2,})\s*(\w+)\s*\)/;
+
+export const INVALID_LOOP_RANGE_MESSAGE =
+  'Ranges must be in the following format: (<start>..<end>)';
+
+export function detectInvalidLoopRange(
+  node: LiquidTag,
+): Problem<SourceCodeType.LiquidHtml> | undefined {
+  if (node.type === NodeTypes.LiquidTag && !isLoopLiquidTag(node)) {
+    return;
+  }
+
+  const markup = (node as LiquidTagFor | LiquidTagTablerow).markup;
+
+  if (!markup) {
+    return;
+  }
+
+  if (typeof markup === 'string') {
+    return validateMarkup(node, markup);
+  }
+
+  if (markup.collection.type === NodeTypes.Range) {
+    return validateMarkup(
+      node,
+      node.source.slice(markup.collection.position.start, markup.collection.position.end),
+    );
+  }
+}
+
+function validateMarkup(
+  node: LiquidTag,
+  markup: string,
+): Problem<SourceCodeType.LiquidHtml> | undefined {
+  const match = markup.match(RANGE_MARKUP_REGEX);
+  if (!match || match.index === undefined) {
+    return;
+  }
+
+  const [, start, , end] = match;
+
+  const expectedRangeMarkup = `(${start}..${end})`;
+
+  if (markup.slice(match.index) === expectedRangeMarkup) {
+    return;
+  }
+
+  const markupIndex = node.source.indexOf(markup, node.position.start);
+
+  const startIndex = markupIndex + match.index;
+  const endIndex = markupIndex + markup.length;
+
+  if (
+    !ensureValidAst(
+      node.source.slice(node.position.start, startIndex) +
+        expectedRangeMarkup +
+        node.source.slice(endIndex, node.position.end),
+    )
+  ) {
+    // If the new AST is invalid, we don't want to auto-fix it
+    return;
+  }
+
+  return {
+    message: INVALID_LOOP_RANGE_MESSAGE,
+    startIndex,
+    endIndex,
+    fix: (corrector) => {
+      corrector.replace(startIndex, endIndex, expectedRangeMarkup);
+    },
+  };
+}

packages/theme-check-common/src/checks/liquid-html-syntax-error/index.ts

@@ -4,6 +4,7 @@ import { detectMultipleAssignValues } from './checks/MultipleAssignValues';
 import { detectInvalidBooleanExpressions } from './checks/InvalidBooleanExpressions';
 import { detectInvalidEchoValue } from './checks/InvalidEchoValue';
 import { detectInvalidConditionalNode } from './checks/InvalidConditionalNode';
+import { detectInvalidLoopRange } from './checks/InvalidLoopRange';
 
 type LineColPosition = {
   line: number;
@@ -54,7 +55,8 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
           const problem =
             detectMultipleAssignValues(node) ||
             detectInvalidEchoValue(node) ||
-            detectInvalidConditionalNode(node);
+            detectInvalidConditionalNode(node) ||
+            detectInvalidLoopRange(node);
 
           if (!problem) {
             return;