Merge pull request #1049 from Shopify/remove-unsupported-parenthesis

Add check to find/fix unsupported parenthesis

Author: aswamy

.changeset/cuddly-yaks-run.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Add check to find/fix unsupported parenthesis

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

@@ -13,7 +13,6 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
       '{% if true or false %}hello{% endif %}',
       '{% if 10 > 5 and user.active %}hello{% endif %}',
       '{% if price >= 100 or discount %}hello{% endif %}',
-      '{% if (1 > 0) and (2 < 3) %}hello{% endif %}',
       "{% if user.name contains 'admin' or user.role == 'owner' %}hello{% endif %}",
     ];
 

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

@@ -0,0 +1,52 @@
+import { expect, describe, it, vi, beforeEach } from 'vitest';
+import { applyFix, runLiquidCheck } from '../../../test';
+import { LiquidHTMLSyntaxError } from '../index';
+
+describe('detectConditionalNodeUnsupportedParenthesis', async () => {
+  it('should not report no unsupported parenthesis in the markup', async () => {
+    const testCases = [`{% if condition %}{% endif %}`, `{% if range == (1..2) %}{% endif %}`];
+
+    for (const sourceCode of testCases) {
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).to.have.length(0);
+    }
+  });
+
+  it('should report when there are unsupported parenthesis in the markup', async () => {
+    const testCases = [
+      [
+        `{% if (this and that) or other %}{% endif %}`,
+        '{% if this and that or other %}{% endif %}',
+      ],
+      [`{% if (a > b) and (c == d) %}{% endif %}`, '{% if a > b and c == d %}{% endif %}'],
+    ];
+
+    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 instances of the error', async () => {
+    const sourceCode = `{% if (a or c) %}{% endif %} {% if (a == b) %}{% endif %} {% if condition %}{% endif %}`;
+
+    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');
+
+    const fixed = applyFix(sourceCode, offenses[0]);
+    expect(fixed).to.equal(
+      `{% if a or c %}{% endif %} {% if (a == b) %}{% endif %} {% if condition %}{% endif %}`,
+    );
+
+    const fixed2 = applyFix(sourceCode, offenses[1]);
+    expect(fixed2).to.equal(
+      `{% if (a or c) %}{% endif %} {% if a == b %}{% endif %} {% if condition %}{% endif %}`,
+    );
+  });
+});

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

@@ -0,0 +1,39 @@
+import { LiquidTag } from '@shopify/liquid-html-parser';
+import { SourceCodeType, Problem } from '../../..';
+import {
+  doesFragmentContainUnsupportedParentheses,
+  getFragmentsInMarkup,
+  INVALID_SYNTAX_MESSAGE,
+} from './utils';
+
+export function detectConditionalNodeUnsupportedParenthesis(
+  node: LiquidTag,
+): Problem<SourceCodeType.LiquidHtml> | undefined {
+  if (!['if', 'elsif', 'unless'].includes(node.name)) return;
+  if (typeof node.markup !== 'string' || !node.markup.trim()) return;
+
+  const markupIndex = node.source.indexOf(node.markup, node.position.start);
+
+  const fragments = getFragmentsInMarkup(node.markup);
+
+  const badFragments = fragments.filter((fragment) =>
+    doesFragmentContainUnsupportedParentheses(fragment.value),
+  );
+
+  if (badFragments.length) {
+    return {
+      message: INVALID_SYNTAX_MESSAGE,
+      startIndex: markupIndex,
+      endIndex: markupIndex + node.markup.length,
+      fix: (corrector) => {
+        for (const fragment of badFragments) {
+          corrector.replace(
+            markupIndex + fragment.index!,
+            markupIndex + fragment.index! + fragment.value.length,
+            fragment.value.replace(/[\(\)]/g, ''),
+          );
+        }
+      },
+    };
+  }
+}

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

@@ -6,6 +6,7 @@ import { detectInvalidEchoValue } from './checks/InvalidEchoValue';
 import { detectInvalidConditionalNode } from './checks/InvalidConditionalNode';
 import { detectInvalidLoopRange } from './checks/InvalidLoopRange';
 import { detectInvalidLoopArguments } from './checks/InvalidLoopArguments';
+import { detectConditionalNodeUnsupportedParenthesis } from './checks/InvalidConditionalNodeParenthesis';
 
 type LineColPosition = {
   line: number;
@@ -58,13 +59,17 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
           const problems = [
             detectMultipleAssignValues(node),
             detectInvalidEchoValue(node),
-            detectInvalidConditionalNode(node),
             detectInvalidLoopRange(node),
             detectInvalidLoopArguments(node, await tagsPromise),
           ].filter(Boolean) as Problem<SourceCodeType.LiquidHtml>[];
 
-          if (!problems.length) {
-            return;
+          // Fixers for `detectConditionalNodeUnsupportedParenthesis` and `detectInvalidConditionalNode` consume
+          // the whole node markup, so we MUST not run both.
+          const conditionalNodeProblem =
+            detectConditionalNodeUnsupportedParenthesis(node) || detectInvalidConditionalNode(node);
+
+          if (conditionalNodeProblem) {
+            problems.push(conditionalNodeProblem);
           }
 
           problems.forEach(context.report);