Merge pull request #1039 from Shopify/bug-fix-multiple-instance-syntax-errors

Fix syntax check for tags where multiple instances of same error occur

Author: aswamy

.changeset/weak-rules-notice.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Fix syntax check for tags where multiple instances of same error occur

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

@@ -1,4 +1,4 @@
-import { expect, describe, it, vi } from 'vitest';
+import { expect, describe, it, vi, beforeEach } from 'vitest';
 import { applyFix, runLiquidCheck } from '../../../test';
 import { LiquidHTMLSyntaxError } from '../index';
 import { toLiquidAST } from '@shopify/liquid-html-parser';
@@ -17,6 +17,10 @@ vi.mock('@shopify/liquid-html-parser', async (importOriginal) => {
 });
 
 describe('detectInvalidEchoValue', async () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+
   it('should not report when echo value is valid', async () => {
     const testCases = [
       `{% echo '123' %}`,
@@ -78,6 +82,27 @@ describe('detectInvalidEchoValue', async () => {
     }
   });
 
+  it('should report when there are multiple instances of the error', async () => {
+    const sourceCode = `{% echo zero %} {% echo one two %} {% echo one two %}`;
+
+    const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+    expect(offenses).to.have.length(2);
+    expect(offenses[0].message).to.equal('Syntax is not supported');
+    expect(offenses[1].message).to.equal('Syntax is not supported');
+
+    expect(toLiquidAST).toHaveBeenCalledTimes(2);
+    expect(toLiquidAST).toHaveBeenCalledWith(`{% echo one %}`, {
+      allowUnclosedDocumentNode: false,
+      mode: 'strict',
+    });
+
+    const fixed = applyFix(sourceCode, offenses[0]);
+    expect(fixed).to.equal(`{% echo zero %} {% echo one %} {% echo one two %}`);
+
+    const fixed2 = applyFix(sourceCode, offenses[1]);
+    expect(fixed2).to.equal(`{% echo zero %} {% echo one two %} {% echo one %}`);
+  });
+
   it('should report when there is no value', async () => {
     const testCases = [
       [`{% echo | upcase %}`, '{% echo blank %}'],

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

@@ -48,30 +48,27 @@ export function detectInvalidEchoValue(
     };
   }
 
-  const removalIndices = (source: string) => {
-    const offset = source.indexOf(markup);
+  const removalIndices = (source: string, startingIndex: number) => {
+    const offset = source.indexOf(markup, startingIndex);
 
     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);
+  const { startIndex, endIndex } = removalIndices(node.source, node.position.start);
 
   if (
     !ensureValidAst(
-      tagSource.slice(0, tagSourceRemovalStartIndex) + tagSource.slice(tagSourceRemovalEndIndex),
+      node.source.slice(node.position.start, startIndex) +
+        node.source.slice(endIndex, node.position.end),
     )
   ) {
     // 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,

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

@@ -1,4 +1,4 @@
-import { expect, describe, it, vi } from 'vitest';
+import { expect, describe, it, vi, beforeEach } from 'vitest';
 import { applyFix, runLiquidCheck } from '../../../test';
 import { LiquidHTMLSyntaxError } from '../index';
 import { toLiquidAST } from '@shopify/liquid-html-parser';
@@ -17,6 +17,10 @@ vi.mock('@shopify/liquid-html-parser', async (importOriginal) => {
 });
 
 describe('detectMultipleAssignValues', async () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+
   it('should not report when there are no trailing values', async () => {
     const testCases = [`{% assign foo = '123' %}`, `{% assign foo = '123' | upcase %}`];
 
@@ -44,6 +48,31 @@ describe('detectMultipleAssignValues', async () => {
     }
   });
 
+  it('should report when there are multiple instances of the error', async () => {
+    const sourceCode = `{% assign foo = blank %} {% assign foo = '123' 555 text %} {% assign foo = '123' 555 text %}`;
+
+    const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+    expect(offenses).to.have.length(2);
+    expect(offenses[0].message).to.equal('Syntax is not supported');
+    expect(offenses[1].message).to.equal('Syntax is not supported');
+
+    expect(toLiquidAST).toHaveBeenCalledTimes(2);
+    expect(toLiquidAST).toHaveBeenCalledWith(`{% assign foo = '123' %}`, {
+      allowUnclosedDocumentNode: false,
+      mode: 'strict',
+    });
+
+    const fixed = applyFix(sourceCode, offenses[0]);
+    expect(fixed).to.equal(
+      `{% assign foo = blank %} {% assign foo = '123' %} {% assign foo = '123' 555 text %}`,
+    );
+
+    const fixed2 = applyFix(sourceCode, offenses[1]);
+    expect(fixed2).to.equal(
+      `{% assign foo = blank %} {% assign foo = '123' 555 text %} {% assign foo = '123' %}`,
+    );
+  });
+
   it('should report when there are multiple values (with filters)', async () => {
     const testCases = [
       [`{% assign foo = '123' 555 text | upcase %}`, "{% assign foo = '123' | upcase %}"],

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

@@ -48,8 +48,8 @@ export function detectMultipleAssignValues(
     return;
   }
 
-  const removalIndices = (source: string) => {
-    const offset = source.indexOf(markup);
+  const removalIndices = (source: string, startingIndex: number) => {
+    const offset = source.indexOf(markup, startingIndex);
 
     return {
       startIndex:
@@ -65,21 +65,18 @@ export function detectMultipleAssignValues(
     };
   };
 
-  const tagSource = node.source.slice(node.position.start, node.position.end);
-  const { startIndex: tagSourceRemovalStartIndex, endIndex: tagSourceRemovalEndIndex } =
-    removalIndices(tagSource);
+  const { startIndex, endIndex } = removalIndices(node.source, node.position.start);
 
   if (
     !ensureValidAst(
-      tagSource.slice(0, tagSourceRemovalStartIndex) + tagSource.slice(tagSourceRemovalEndIndex),
+      node.source.slice(node.position.start, startIndex) +
+        node.source.slice(endIndex, node.position.end),
     )
   ) {
     // 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,