Update liquid-html-syntax-error check

Author: karreiro

.changeset/blue-comics-return.md

@@ -0,0 +1,5 @@
+---
+"@shopify/theme-check-common": patch
+---
+
+Improve LiquidHTMLSyntaxError parity with Ruby Liquid syntax errors

packages/theme-check-common/package.json

@@ -42,6 +42,7 @@
     "@types/line-column": "^1.0.0",
     "@types/lodash": "^4.17.20",
     "@types/postcss-safe-parser": "^5.0.4",
-    "@vitest/expect": "2.1.9"
+    "@vitest/expect": "2.1.9",
+    "yaml": "^2.8.3"
   }
 }

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

@@ -31,7 +31,6 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
       '{% if 1 %}hello{% endif %}',
       '{% if 0 %}hello{% endif %}',
       "{% if 'string' %}hello{% endif %}",
-      '{% if contains %}hello{% endif %}',
     ];
 
     for (const testCase of testCases) {
@@ -370,6 +369,39 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
     }
   });
 
+  it('should report an offense for invalid operators in unless expressions', async () => {
+    const testCases = [
+      {
+        source: '{% unless x && y %}{% endunless %}',
+        expectedMessage:
+          "Conditional is invalid. Anything after 'x' will be ignored. Use 'and'/'or' instead of '&&'/'||' for multiple conditions",
+      },
+      {
+        source: '{% unless x || y %}{% endunless %}',
+        expectedMessage:
+          "Conditional is invalid. Anything after 'x' will be ignored. Use 'and'/'or' instead of '&&'/'||' for multiple conditions",
+      },
+      {
+        source: '{% unless x startswith "foo" %}{% endunless %}',
+        expectedMessage:
+          "Conditional is invalid. Anything after 'x' will be ignored: 'startswith \"foo\"'",
+      },
+      {
+        source: '{% unless x === y %}{% endunless %}',
+        expectedMessage: "Conditional is invalid. Anything after 'x' will be ignored: '=== y'",
+      },
+    ];
+
+    for (const testCase of testCases) {
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, testCase.source);
+      expect(offenses).to.have.length(1);
+      expect(offenses[0].message).to.equal(`Syntax is not supported: ${testCase.expectedMessage}`);
+
+      const fixed = applyFix(testCase.source, offenses[0]);
+      expect(fixed).to.equal('{% unless x %}{% endunless %}');
+    }
+  });
+
   it('should NOT report an offense for valid logical operators', async () => {
     const validCases = [
       '{% if price > 100 and discount < 50 %}hello{% endif %}',

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

@@ -17,6 +17,7 @@ interface ExpressionIssue {
 const TOKEN_PATTERNS = {
   logical: /^(and|or)$/,
   comparison: /^(==|!=|>=|<=|>|<|contains)$/,
+  invalidOperator: /^(&&|\|\||===|startswith)$/,
   invalid: /^[@#$&]$/,
   literal: /^(['"][^'"]*['"]|\d+(?:\.\d+)?|true|false|nil|empty|blank)$/,
 } as const;
@@ -68,6 +69,10 @@ function isOperatorToken(token: Token): boolean {
   return token.type === 'logical' || token.type === 'comparison';
 }
 
+function isJavaScriptLogicalOperator(token: Token): boolean {
+  return token.value === '&&' || token.value === '||';
+}
+
 function checkInvalidStartingToken(tokens: Token[]): ExpressionIssue | null {
   const firstToken = tokens[0];
   if (firstToken.type === 'invalid' || firstToken.type === 'comparison') {
@@ -146,6 +151,38 @@ function checkLaxParsingIssues(tokens: Token[]): ExpressionIssue | null {
   return null;
 }
 
+function checkInvalidOperatorsAfterValue(tokens: Token[]): ExpressionIssue | null {
+  for (let i = 1; i < tokens.length; i++) {
+    const current = tokens[i];
+    const previous = tokens[i - 1];
+
+    if (!isValueToken(previous) || current.type !== 'invalidOperator') continue;
+
+    const validExpr = tokens
+      .slice(0, i)
+      .map((t) => t.value)
+      .join(' ');
+    const ignored = tokens
+      .slice(i)
+      .map((t) => t.value)
+      .join(' ');
+
+    if (isJavaScriptLogicalOperator(current)) {
+      return {
+        message: `Conditional is invalid. Anything after '${validExpr}' will be ignored. Use 'and'/'or' instead of '&&'/'||' for multiple conditions`,
+        fix: validExpr,
+      };
+    }
+
+    return {
+      message: `Conditional is invalid. Anything after '${validExpr}' will be ignored: '${ignored}'`,
+      fix: validExpr,
+    };
+  }
+
+  return null;
+}
+
 function analyzeConditionalExpression(markup: string): ExpressionIssue | null {
   const trimmed = markup.trim();
   if (!trimmed) return null;
@@ -160,6 +197,7 @@ function analyzeConditionalExpression(markup: string): ExpressionIssue | null {
   return (
     checkInvalidStartingToken(tokens) ||
     checkTrailingTokensAfterComparison(tokens) ||
-    checkLaxParsingIssues(tokens)
+    checkLaxParsingIssues(tokens) ||
+    checkInvalidOperatorsAfterValue(tokens)
   );
 }

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

@@ -0,0 +1,73 @@
+import type { LiquidTag, AssignMarkup } from './parser-compat';
+import type { Context } from './context';
+import {
+  variableHasBareArrayAccess,
+  hasSkippedCharacters,
+  hasSkippedPrefixCharacters,
+  hasRubyAcceptedEmptyFirstFilterArgument,
+  hasRubyAcceptedEmptyAssignRhs,
+  hasRubyAcceptedAssignLhsExtraIdentifier,
+  hasRubyAcceptedAssignLhsUnclosedQuote,
+  variableHasBareContainsValueExpression,
+  rawMarkup,
+} from './utils';
+
+export function checkAssignTag(node: LiquidTag, context: Context): void {
+  if (typeof node.markup === 'string') {
+    if (hasRubyAcceptedEmptyAssignRhs(node.markup)) return;
+    if (hasRubyAcceptedEmptyFirstFilterArgument(node.markup)) return;
+    if (hasRubyAcceptedAssignLhsExtraIdentifier(node.markup)) return;
+    if (hasRubyAcceptedAssignLhsUnclosedQuote(node.markup)) return;
+
+    context.report({
+      message: `Syntax error in 'assign' tag`,
+      startIndex: node.position.start,
+      endIndex: node.position.end,
+    });
+    return;
+  }
+
+  const markup = node.markup as AssignMarkup;
+
+  if (variableHasBareArrayAccess(markup.value)) {
+    context.report({
+      message: 'Bare bracket access is not allowed',
+      startIndex: node.position.start,
+      endIndex: node.position.end,
+    });
+    return;
+  }
+
+  if (variableHasBareContainsValueExpression(markup.value)) {
+    context.report({
+      message: `Syntax error in 'assign' tag`,
+      startIndex: node.position.start,
+      endIndex: node.position.end,
+    });
+    return;
+  }
+
+  if (hasSkippedPrefixCharacters(node.source, node.markupPosition.start, markup.position.start)) {
+    context.report({
+      message: `Syntax error in 'assign' tag`,
+      startIndex: node.position.start,
+      endIndex: node.position.end,
+    });
+    return;
+  }
+
+  const raw = rawMarkup(node);
+  if (hasRubyAcceptedEmptyAssignRhs(raw)) return;
+  if (hasRubyAcceptedEmptyFirstFilterArgument(raw)) return;
+  if (hasRubyAcceptedAssignLhsExtraIdentifier(raw)) return;
+  if (hasRubyAcceptedAssignLhsUnclosedQuote(raw)) return;
+
+  const parsedRaw = node.source.slice(markup.position.start, node.markupPosition.end);
+  if (hasSkippedCharacters(parsedRaw)) {
+    context.report({
+      message: `Syntax error in 'assign' tag`,
+      startIndex: node.position.start,
+      endIndex: node.position.end,
+    });
+  }
+}

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

@@ -0,0 +1,29 @@
+import type { LiquidTag } from './parser-compat';
+import { builtinTags } from './builtin-tags';
+import type { Context } from './context';
+
+const KNOWN_LIQUID_TAGS = new Set(['#', ...Object.keys(builtinTags)]);
+
+export function checkBaseTag(node: LiquidTag, context: Context): void {
+  if ('reason' in node && typeof node.reason === 'string') {
+    context.report({
+      message: node.reason,
+      startIndex: node.position.start,
+      endIndex: node.position.end,
+    });
+    return;
+  }
+
+  if (isUnknownTagInsideLiquidBlock(node)) {
+    context.report({
+      message: `Unknown tag '${node.name}'`,
+      startIndex: node.position.start,
+      endIndex: node.position.end,
+    });
+  }
+}
+
+function isUnknownTagInsideLiquidBlock(node: LiquidTag): boolean {
+  // Statements inside `{% liquid %}` do not have their own `{%` delimiter.
+  return !node.source.startsWith('{%', node.position.start) && !KNOWN_LIQUID_TAGS.has(node.name);
+}