Merge pull request #1054 from Shopify/invalid-pipe-check

Invalid pipe check

Author: EvilGenius13

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

@@ -25,7 +25,7 @@ describe('detectInvalidEchoValue', async () => {
   });
 
   it('should not report when there are no filters provided', async () => {
-    const sourceCode = `{% echo '123' | %}`;
+    const sourceCode = `{% echo '123' %}`;
     const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
     expect(offenses).to.have.length(0);
   });

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

@@ -57,8 +57,13 @@ async function detectInvalidFilterNameInMarkup(
     const filterEndIndex = match.index! + match[0].length;
     const afterFilter = trimmedMarkup.slice(filterEndIndex);
 
+    // This regex finds invalid trailing characters after a filter name using lookaheads:
+    // 1. Skip valid syntax like ": parameter" or "| nextfilter"
+    // 2. Capture any junk characters that shouldn't be there
+    // 3. Stop before valid boundaries like colons or pipes
+    // e.g. "upcase xyz" finds "xyz", but "upcase | downcase" is ignored as valid
     const invalidSegment = afterFilter.match(
-      /^(?!\s*(?::|$|\|\s*[a-zA-Z]))([^:|]+?)(?=\s*(?::|$|\|))/,
+      /^(?!\s*(?::|$|\|\s*[a-zA-Z]|\|\s*\||\s*\|\s*(?:[}%]|$)))([^:|]+?)(?=\s*(?::|$|\|))/,
     )?.[1];
     if (!invalidSegment) {
       continue;

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

@@ -0,0 +1,184 @@
+import { describe, it, expect } from 'vitest';
+import { runLiquidCheck, applyFix } from '../../../test';
+import { LiquidHTMLSyntaxError } from '../index';
+
+describe('Module: InvalidPipeSyntax', () => {
+  describe('Double pipe patterns', () => {
+    it('should detect and fix double pipes in variable output', async () => {
+      const sourceCode = `{{ 'hello' | upcase | | downcase }}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(1);
+      expect(offenses[0].message).toContain('Remove extra `|` character(s)');
+      expect(offenses[0].fix).toBeDefined();
+
+      const fixedCode = applyFix(sourceCode, offenses[0]);
+      expect(fixedCode).toBe(`{{ 'hello' | upcase | downcase }}`);
+    });
+
+    it('should detect and fix double pipes with extra whitespace', async () => {
+      const sourceCode = `{{ 'hello' | upcase |   | downcase }}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(1);
+      expect(offenses[0].message).toContain('Remove extra `|` character(s)');
+      expect(offenses[0].fix).toBeDefined();
+
+      const fixedCode = applyFix(sourceCode, offenses[0]);
+      expect(fixedCode).toBe(`{{ 'hello' | upcase | downcase }}`);
+    });
+
+    it('should detect and fix double pipes in assign tag', async () => {
+      const sourceCode = `{% assign result = 'hello' | upcase | | downcase %}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(1);
+      expect(offenses[0].message).toContain('Remove extra `|` character(s)');
+      expect(offenses[0].fix).toBeDefined();
+
+      const fixedCode = applyFix(sourceCode, offenses[0]);
+      expect(fixedCode).toBe(`{% assign result = 'hello' | upcase | downcase %}`);
+    });
+
+    it('should detect and fix double pipes in echo tag', async () => {
+      const sourceCode = `{% echo 'hello' | upcase | | downcase %}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(1);
+      expect(offenses[0].message).toContain('Remove extra `|` character(s)');
+      expect(offenses[0].fix).toBeDefined();
+
+      const fixedCode = applyFix(sourceCode, offenses[0]);
+      expect(fixedCode).toBe(`{% echo 'hello' | upcase | downcase %}`);
+    });
+
+    it('should handle multiple double pipes in one expression', async () => {
+      const sourceCode = `{{ 'hello' | upcase | | downcase | | reverse }}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(2);
+
+      expect(offenses[0].message).toContain('Remove extra `|` character(s)');
+      expect(offenses[1].message).toContain('Remove extra `|` character(s)');
+
+      expect(offenses[0].fix).toBeDefined();
+      expect(offenses[1].fix).toBeDefined();
+    });
+  });
+
+  describe('Trailing pipe patterns', () => {
+    it('should detect and fix trailing pipe in variable output', async () => {
+      const sourceCode = `{{ 'hello' | upcase | }}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(1);
+      expect(offenses[0].message).toContain('Remove the trailing `|` character');
+      expect(offenses[0].fix).toBeDefined();
+
+      const fixedCode = applyFix(sourceCode, offenses[0]);
+      expect(fixedCode).toBe(`{{ 'hello' | upcase }}`);
+    });
+
+    it('should detect and fix trailing pipe in assign tag', async () => {
+      const sourceCode = `{% assign result = 'hello' | upcase | %}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(1);
+      expect(offenses[0].message).toContain('Remove the trailing `|` character');
+      expect(offenses[0].fix).toBeDefined();
+
+      const fixedCode = applyFix(sourceCode, offenses[0]);
+      expect(fixedCode).toBe(`{% assign result = 'hello' | upcase %}`);
+    });
+
+    it('should detect and fix trailing pipe in echo tag', async () => {
+      const sourceCode = `{% echo 'hello' | upcase | %}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(1);
+      expect(offenses[0].message).toContain('Remove the trailing `|` character');
+      expect(offenses[0].fix).toBeDefined();
+
+      const fixedCode = applyFix(sourceCode, offenses[0]);
+      expect(fixedCode).toBe(`{% echo 'hello' | upcase %}`);
+    });
+
+    it('should detect and fix multiple trailing pipes', async () => {
+      const sourceCode = `{{ 'hello' | upcase | | }}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(2); // Double pipe AND trailing pipe
+      expect(offenses[0].message).toContain('Remove extra `|` character(s)');
+      expect(offenses[1].message).toContain('Remove the trailing `|` character');
+      expect(offenses[0].fix).toBeDefined();
+      expect(offenses[1].fix).toBeDefined();
+    });
+  });
+
+  describe('Complex pipe scenarios', () => {
+    it('should handle mixed valid and invalid pipes', async () => {
+      const sourceCode = `{{ 'hello' | upcase | | downcase | reverse | }}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(2);
+
+      // First should be double pipe
+      expect(offenses[0].message).toContain('Remove extra `|` character(s)');
+      // Second should be trailing pipe
+      expect(offenses[1].message).toContain('Remove the trailing `|` character');
+    });
+
+    it('should not interfere with valid filter parameter syntax', async () => {
+      const sourceCode = `{{ 'hello' | append: 'world' | upcase }}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(0);
+    });
+
+    it('should handle liquid tag with mixed pipe issues', async () => {
+      const sourceCode = `{% liquid
+        assign foo = 'test' | upcase | | downcase |
+        echo bar | reverse
+      %}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(2);
+      expect(offenses[0].message).toContain('Remove extra `|` character(s)');
+      expect(offenses[1].message).toContain('Remove the trailing `|` character');
+    });
+  });
+
+  describe('Valid syntax should not be flagged', () => {
+    it('should not report on valid filter chains', async () => {
+      const sourceCode = `{{ 'hello' | upcase | append: 'world' | downcase }}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(0);
+    });
+
+    it('should not report on simple filters', async () => {
+      const sourceCode = `{{ 'hello' | upcase }}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(0);
+    });
+
+    it('should not report on filters with arguments', async () => {
+      const sourceCode = `{{ product.title | append: ' - ' | append: shop.name }}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(0);
+    });
+
+    it('should not report on valid assign with filters', async () => {
+      const sourceCode = `{% assign title = product.title | upcase | truncate: 50 %}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(0);
+    });
+
+    it('should not report on valid echo with filters', async () => {
+      const sourceCode = `{% echo product.title | upcase | truncate: 50 %}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(0);
+    });
+  });
+
+  describe('Edge cases', () => {
+    it('should handle pipes in string literals correctly', async () => {
+      const sourceCode = `{{ 'hello | world' | upcase }}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(0);
+    });
+
+    it('should not report on conditional expressions with pipes', async () => {
+      const sourceCode = `{{ product.title | default: 'No title' }}`;
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).toHaveLength(0);
+    });
+  });
+});

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

@@ -0,0 +1,86 @@
+import { LiquidVariableOutput, LiquidTag, NodeTypes, NamedTags } from '@shopify/liquid-html-parser';
+import { Problem, SourceCodeType } from '../../..';
+import { INVALID_SYNTAX_MESSAGE } from './utils';
+
+export async function detectInvalidPipeSyntax(
+  node: LiquidVariableOutput | LiquidTag,
+): Promise<Problem<SourceCodeType.LiquidHtml>[]> {
+  if (node.type === NodeTypes.LiquidVariableOutput) {
+    if (typeof node.markup !== 'string') {
+      return [];
+    }
+    return detectPipeSyntaxInMarkup(node, node.markup);
+  }
+
+  if (node.type === NodeTypes.LiquidTag) {
+    if (node.name === NamedTags.echo && typeof node.markup !== 'string') {
+      return [];
+    }
+    if (node.name === NamedTags.assign && typeof node.markup !== 'string') {
+      return [];
+    }
+    if (
+      typeof node.markup === 'string' &&
+      (node.name === NamedTags.echo || node.name === NamedTags.assign)
+    ) {
+      return detectPipeSyntaxInMarkup(node, node.markup);
+    }
+  }
+
+  return [];
+}
+
+async function detectPipeSyntaxInMarkup(
+  node: LiquidVariableOutput | LiquidTag,
+  markup: string,
+): Promise<Problem<SourceCodeType.LiquidHtml>[]> {
+  const problems: Problem<SourceCodeType.LiquidHtml>[] = [];
+  const trimmedMarkup = markup.trim();
+
+  // Check for multiple consecutive pipes
+  const extraPipesPattern = /(\|\s*){2,}/g;
+  const extraPipesMatches = Array.from(trimmedMarkup.matchAll(extraPipesPattern));
+
+  for (const match of extraPipesMatches) {
+    const markupStartInSource = node.source.indexOf(markup, node.position.start);
+    if (markupStartInSource === -1) {
+      continue;
+    }
+    const problemStartInSource = markupStartInSource + match.index!;
+    const problemEndInSource = problemStartInSource + match[0].length;
+
+    problems.push({
+      message: `${INVALID_SYNTAX_MESSAGE}. Remove extra \`|\` character(s).`,
+      startIndex: problemStartInSource,
+      endIndex: problemEndInSource,
+      fix: (corrector) => {
+        corrector.replace(problemStartInSource, problemEndInSource, '| ');
+      },
+    });
+  }
+
+  // Check for trailing pipes
+  const trailingPipePattern = /\s*\|\s*$/;
+  const trailingPipeMatch = trimmedMarkup.match(trailingPipePattern);
+
+  if (trailingPipeMatch) {
+    const markupStartInSource = node.source.indexOf(markup, node.position.start);
+    if (markupStartInSource === -1) {
+      return problems;
+    }
+    const trailingPipeStartInMarkup = trailingPipeMatch.index!;
+    const problemStartInSource = markupStartInSource + trailingPipeStartInMarkup;
+    const problemEndInSource = problemStartInSource + trailingPipeMatch[0].length;
+
+    problems.push({
+      message: `${INVALID_SYNTAX_MESSAGE}. Remove the trailing \`|\` character.`,
+      startIndex: problemStartInSource,
+      endIndex: problemEndInSource,
+      fix: (corrector) => {
+        corrector.replace(problemStartInSource, problemEndInSource, '');
+      },
+    });
+  }
+
+  return problems;
+}

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

@@ -35,7 +35,7 @@ describe('detectMultipleAssignValues', async () => {
   });
 
   it('should not report when there are no filters provided', async () => {
-    const sourceCode = `{% assign foo = '123' | %}`;
+    const sourceCode = `{% assign foo = '123' %}`;
     const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
     expect(offenses).to.have.length(0);
   });

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

@@ -71,6 +71,10 @@ export function detectMultipleAssignValues(
     return;
   }
 
+  if (endIndex <= startIndex) {
+    return;
+  }
+
   return {
     message: INVALID_SYNTAX_MESSAGE,
     startIndex,

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

@@ -8,6 +8,7 @@ import { detectInvalidLoopRange } from './checks/InvalidLoopRange';
 import { detectInvalidLoopArguments } from './checks/InvalidLoopArguments';
 import { detectConditionalNodeUnsupportedParenthesis } from './checks/InvalidConditionalNodeParenthesis';
 import { detectInvalidFilterName } from './checks/InvalidFilterName';
+import { detectInvalidPipeSyntax } from './checks/InvalidPipeSyntax';
 
 type LineColPosition = {
   line: number;
@@ -80,6 +81,11 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
           if (filterProblems.length > 0) {
             filterProblems.forEach((filterProblem) => context.report(filterProblem));
           }
+
+          const pipeProblems = await detectInvalidPipeSyntax(node);
+          if (pipeProblems.length > 0) {
+            pipeProblems.forEach((pipeProblem) => context.report(pipeProblem));
+          }
         },
         async LiquidBranch(node) {
           const problem = detectInvalidConditionalNode(node);
@@ -96,6 +102,11 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
             filterProblems.forEach((problem) => context.report(problem));
           }
 
+          const pipeProblems = await detectInvalidPipeSyntax(node);
+          if (pipeProblems.length > 0) {
+            pipeProblems.forEach((pipeProblem) => context.report(pipeProblem));
+          }
+
           const problem = detectInvalidEchoValue(node);
           if (problem) {
             context.report(problem);