Merge pull request #1037 from Shopify/find-and-fix-multiple-echo-values

Find/fix multiple `echo` values

Author: aswamy

.changeset/yellow-pears-fetch.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Find/fix multiple `echo` values

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

@@ -16,7 +16,7 @@ vi.mock('@shopify/liquid-html-parser', async (importOriginal) => {
   };
 });
 
-describe('detectTrailingAssignValue', async () => {
+describe('detectMultipleAssignValues', async () => {
   it('should not report when there are no trailing values', async () => {
     const testCases = [`{% assign foo = '123' %}`, `{% assign foo = '123' | upcase %}`];
 

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

@@ -1,6 +1,6 @@
 import { LiquidTag } from '@shopify/liquid-html-parser';
 import { Problem, SourceCodeType } from '../../..';
-import { ensureValidAst, INVALID_SYNTAX_MESSAGE } from './utils';
+import { ensureValidAst, getFirstValueInMarkup, INVALID_SYNTAX_MESSAGE } from './utils';
 
 export function detectMultipleAssignValues(
   node: LiquidTag,
@@ -42,9 +42,7 @@ export function detectMultipleAssignValues(
     assignmentValue,
   ] = match;
 
-  // Only capture the first item in the value section
-  const firstValueMatch = assignmentValue.match(/"[^"]*"|'[^']*'|\S+/);
-  const firstAssignmentValue = firstValueMatch ? firstValueMatch[0] : null;
+  const firstAssignmentValue = getFirstValueInMarkup(assignmentValue);
 
   if (!firstAssignmentValue) {
     return;

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

@@ -0,0 +1,88 @@
+import { expect, describe, it, vi } from 'vitest';
+import { applyFix, runLiquidCheck } from '../../../test';
+import { LiquidHTMLSyntaxError } from '../index';
+import { toLiquidAST } from '@shopify/liquid-html-parser';
+
+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('detectMultipleEchoValues', async () => {
+  it('should not report when there are no trailing values', async () => {
+    const testCases = [
+      `{% echo '123' %}`,
+      `{% echo '123' | upcase %}`,
+      `{{ '123' }}`,
+      `{{ '123' | upcase }}`,
+    ];
+
+    for (const sourceCode of testCases) {
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).to.have.length(0);
+    }
+  });
+
+  it('should report when there are multiple values (no filters)', async () => {
+    const testCases = [
+      [`{% echo '123' 555 text %}`, "{% echo '123' %}"],
+      [`{% echo "123" 555 text %}`, '{% echo "123" %}'],
+      [`{% echo 123 555 text %}`, '{% echo 123 %}'],
+      [`{% echo true 555 text %}`, '{% echo true %}'],
+      [`{{ '123' 555 text }}`, `{{ '123' }}`],
+      [`{{ "123" 555 text }}`, `{{ "123" }}`],
+      [`{{ 123 555 text }}`, `{{ 123 }}`],
+      [`{{ true 555 text }}`, `{{ true }}`],
+    ];
+
+    for (const [sourceCode, expected] of testCases) {
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).to.have.length(1);
+      expect(offenses[0].message).to.equal('Syntax is not supported');
+
+      const fixed = applyFix(sourceCode, offenses[0]);
+      expect(fixed).to.equal(expected);
+    }
+  });
+
+  it('should report when there are multiple values (with filters)', async () => {
+    const testCases = [
+      [`{% echo '123' 555 text | upcase %}`, "{% echo '123' | upcase %}"],
+      [`{% echo "123" 555 text | upcase %}`, '{% echo "123" | upcase %}'],
+      [`{% echo 123 555 text | default: 0 %}`, '{% echo 123 | default: 0 %}'],
+      [`{% echo true 555 text | fake-filter: 'yes' %}`, "{% echo true | fake-filter: 'yes' %}"],
+      [`{{ '123' 555 text | upcase }}`, `{{ '123' | upcase }}`],
+      [`{{ "123" 555 text | default: 0 }}`, `{{ "123" | default: 0 }}`],
+      [`{{ 123 555 text | fake-filter: 'yes' }}`, `{{ 123 | fake-filter: 'yes' }}`],
+      [`{{ true 555 text | fake-filter: 'yes' }}`, `{{ true | fake-filter: 'yes' }}`],
+    ];
+
+    for (const [sourceCode, expected] of testCases) {
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).to.have.length(1);
+      expect(offenses[0].message).to.equal('Syntax is not supported');
+
+      const fixed = applyFix(sourceCode, offenses[0]);
+      expect(fixed).to.equal(expected);
+    }
+  });
+
+  it('should not report when the fixed code produces an invalid AST', async () => {
+    const sourceCode = `{% echo '123' 555 text %}`;
+
+    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/MultipleEchoValues.ts

@@ -0,0 +1,68 @@
+import { LiquidTag, LiquidVariableOutput, NodeTypes } from '@shopify/liquid-html-parser';
+import { Problem, SourceCodeType } from '../../..';
+import { ensureValidAst, getFirstValueInMarkup, INVALID_SYNTAX_MESSAGE } from './utils';
+
+export function detectMultipleEchoValues(
+  node: LiquidTag | LiquidVariableOutput,
+): Problem<SourceCodeType.LiquidHtml> | undefined {
+  // We've broken it up into two groups:
+  // 1. The variable(s)
+  // 2. The filter section (non-captured)
+  const ECHO_MARKUP_REGEX = /([^|]*)(?:\s*\|\s*.*)?$/m;
+
+  if (node.type === NodeTypes.LiquidTag && node.name !== 'echo') {
+    return;
+  }
+
+  const markup = node.markup;
+
+  if (typeof markup !== 'string') {
+    return;
+  }
+
+  const match = markup.match(ECHO_MARKUP_REGEX);
+  if (!match) {
+    return;
+  }
+
+  const [, echoValue] = match;
+
+  const firstEchoValue = getFirstValueInMarkup(echoValue);
+
+  if (!firstEchoValue) {
+    return;
+  }
+
+  const removalIndices = (source: string) => {
+    const offset = source.indexOf(markup);
+
+    return {
+      startIndex: offset + firstEchoValue.length,
+      endIndex: offset + echoValue.trimEnd().length,
+    };
+  };
+
+  const tagSource = node.source.slice(node.position.start, node.position.end);
+  const { startIndex: tagSourceRemovalStartIndex, endIndex: tagSourceRemovalEndIndex } =
+    removalIndices(tagSource);
+
+  if (
+    !ensureValidAst(
+      tagSource.slice(0, tagSourceRemovalStartIndex) + tagSource.slice(tagSourceRemovalEndIndex),
+    )
+  ) {
+    // If the new AST is invalid, we don't want to auto-fix it
+    return;
+  }
+
+  const { startIndex, endIndex } = removalIndices(node.source);
+
+  return {
+    message: INVALID_SYNTAX_MESSAGE,
+    startIndex,
+    endIndex,
+    fix: (corrector) => {
+      corrector.replace(startIndex, endIndex, '');
+    },
+  };
+}

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

@@ -11,3 +11,8 @@ export function ensureValidAst(source: string) {
     return false;
   }
 }
+
+export function getFirstValueInMarkup(markup: string) {
+  const match = markup.match(/"[^"]*"|'[^']*'|\S+/);
+  return match?.at(0);
+}

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

@@ -2,6 +2,7 @@ import { Severity, SourceCodeType, LiquidCheckDefinition } from '../../types';
 import { getOffset, isError } from '../../utils';
 import { detectMultipleAssignValues } from './checks/MultipleAssignValues';
 import { detectInvalidBooleanExpressions } from './checks/InvalidBooleanExpressions';
+import { detectMultipleEchoValues as detectMultipleEchoValues } from './checks/MultipleEchoValues';
 
 type LineColPosition = {
   line: number;
@@ -49,7 +50,16 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
           context.report(problem);
         },
         async LiquidTag(node) {
-          const problem = detectMultipleAssignValues(node);
+          const problem = detectMultipleAssignValues(node) || detectMultipleEchoValues(node);
+
+          if (!problem) {
+            return;
+          }
+
+          context.report(problem);
+        },
+        async LiquidVariableOutput(node) {
+          const problem = detectMultipleEchoValues(node);
 
           if (!problem) {
             return;