fix(undefined-object): track Liquid declarations inside HTML comments

.changeset/fix-undefined-object-html-comment.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Fix `UndefinedObject` false positive when an `assign`, `capture`, `increment`, or `decrement` is nested inside an HTML comment. Liquid is executed at runtime regardless of being inside `<!-- ... -->`, but the parser stores the comment body as opaque text, so the visitor never reached the inner tags. The check now re-parses comment bodies and pre-registers any file-scoped variable declarations they contain, scoped from the end of the comment onward.

packages/theme-check-common/src/checks/undefined-object/index.spec.ts

@@ -488,4 +488,100 @@ describe('Module: UndefinedObject', () => {
     assert(offenses.length == 0);
     expect(offenses).to.be.empty;
   });
+
+  describe('Liquid inside HTML comments (issue #1099)', () => {
+    it('should not report when assign is inside an HTML comment', async () => {
+      const sourceCode = `
+        <!-- {% assign foo = 10 %} -->
+        {{ foo }}
+      `;
+
+      const offenses = await runLiquidCheck(UndefinedObject, sourceCode);
+      expect(offenses).to.be.empty;
+    });
+
+    it('should not report when capture is inside an HTML comment', async () => {
+      const sourceCode = `
+        <!-- {% capture greeting %}hi{% endcapture %} -->
+        {{ greeting }}
+      `;
+
+      const offenses = await runLiquidCheck(UndefinedObject, sourceCode);
+      expect(offenses).to.be.empty;
+    });
+
+    it('should not report when increment/decrement is inside an HTML comment', async () => {
+      const sourceCode = `
+        <!-- {% increment counter %}{% decrement other %} -->
+        {{ counter }} {{ other }}
+      `;
+
+      const offenses = await runLiquidCheck(UndefinedObject, sourceCode);
+      expect(offenses).to.be.empty;
+    });
+
+    it('should track multiple assigns within a single HTML comment', async () => {
+      const sourceCode = `
+        <!-- {% assign a = 1 %} {% assign b = 2 %} -->
+        {{ a }} {{ b }}
+      `;
+
+      const offenses = await runLiquidCheck(UndefinedObject, sourceCode);
+      expect(offenses).to.be.empty;
+    });
+
+    it('should still flag references that appear before the HTML comment', async () => {
+      // Variables defined inside an HTML comment are only in scope from the
+      // end of the comment onward, matching the file-position semantics of
+      // the existing assign/capture handling.
+      const sourceCode = `
+        {{ before }}
+        <!-- {% assign before = 1 %} -->
+      `;
+
+      const offenses = await runLiquidCheck(UndefinedObject, sourceCode);
+      expect(offenses).toHaveLength(1);
+      expect(offenses[0].message).toEqual("Unknown object 'before' used.");
+    });
+
+    it('should still flag genuinely undefined variables when comment exists', async () => {
+      const sourceCode = `
+        <!-- {% assign foo = 10 %} -->
+        {{ unrelated }}
+      `;
+
+      const offenses = await runLiquidCheck(UndefinedObject, sourceCode);
+      expect(offenses).toHaveLength(1);
+      expect(offenses[0].message).toEqual("Unknown object 'unrelated' used.");
+    });
+
+    it('should track nested assigns inside a comment-wrapped if block', async () => {
+      const sourceCode = `
+        <!--
+          {% if some_condition %}
+            {% assign nested = 'a' %}
+          {% else %}
+            {% assign nested = 'b' %}
+          {% endif %}
+        -->
+        {{ nested }}
+      `;
+
+      const offenses = await runLiquidCheck(UndefinedObject, sourceCode);
+      expect(offenses.map((o) => o.message)).to.not.contain("Unknown object 'nested' used.");
+    });
+
+    it('should treat malformed Liquid in a comment body as opaque text', async () => {
+      // If the body cannot be parsed as Liquid, the check should silently
+      // skip it rather than failing the entire run.
+      const sourceCode = `
+        <!-- {% assign foo = %} -->
+        {{ unrelated }}
+      `;
+
+      const offenses = await runLiquidCheck(UndefinedObject, sourceCode);
+      // We still report the unrelated reference; we don't crash.
+      expect(offenses.map((o) => o.message)).to.contain("Unknown object 'unrelated' used.");
+    });
+  });
 });

packages/theme-check-common/src/checks/undefined-object/index.ts

@@ -1,4 +1,5 @@
 import {
+  HtmlComment,
   LiquidDocParamNode,
   LiquidHtmlNode,
   LiquidTag,
@@ -12,6 +13,7 @@ import {
   NamedTags,
   NodeTypes,
   Position,
+  toLiquidAST,
 } from '@shopify/liquid-html-parser';
 import { LiquidCheckDefinition, Mode, Severity, SourceCodeType, ThemeDocset } from '../../types';
 import { isError, last } from '../../utils';
@@ -73,6 +75,38 @@ export const UndefinedObject: LiquidCheckDefinition = {
         }
       },
 
+      async HtmlComment(node: HtmlComment) {
+        // Liquid inside HTML comments is still executed at runtime: the
+        // browser strips the `<!-- ... -->` after Liquid has already run.
+        // The HTML grammar treats `HtmlComment.body` as opaque text, so the
+        // visitor never reaches Liquid nodes nested inside one. We compensate
+        // by re-parsing the comment body and pre-registering any file-scoped
+        // variable declarations (`assign`, `capture`, `increment`,
+        // `decrement`) so references after the comment do not get flagged
+        // as undefined.
+        //
+        // We only register variables whose effect is visible *outside* the
+        // comment. Block-local definitions (`for`, `tablerow`, `form`,
+        // `paginate`, `layout none`) cannot leak past the closing `-->` and
+        // any references to them inside the comment are themselves
+        // unreachable to the visitor, so there is nothing to track.
+        if (typeof node.body !== 'string' || node.body.length === 0) return;
+
+        let commentAst;
+        try {
+          commentAst = toLiquidAST(node.body);
+        } catch {
+          // The body is malformed Liquid (or just plain text). Treat the
+          // comment as opaque rather than failing the entire check run.
+          return;
+        }
+
+        const scopeStart = node.position.end;
+        for (const child of collectFileScopeDefiningTags(commentAst.children)) {
+          indexVariableScope(child.name, { start: scopeStart });
+        }
+      },
+
       async LiquidTag(node, ancestors) {
         if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return;
 
@@ -292,3 +326,43 @@ function isLiquidTagIncrement(node: LiquidTag): node is LiquidTagIncrement {
 function isLiquidTagDecrement(node: LiquidTag): node is LiquidTagDecrement {
   return node.name === NamedTags.decrement && typeof node.markup !== 'string';
 }
+
+/**
+ * Walk a Liquid sub-AST (for example, the parsed body of an HTML comment)
+ * and yield the names of every `assign`, `capture`, `increment`, and
+ * `decrement` tag found. These are the four tag types whose variable
+ * declarations remain in scope after their enclosing block ends.
+ *
+ * Block-local declarations from `for`, `tablerow`, `form`, `paginate`, and
+ * `layout` are intentionally skipped: their scope cannot escape the block,
+ * so they cannot contribute to a reference outside the original HTML
+ * comment.
+ */
+function* collectFileScopeDefiningTags(nodes: LiquidHtmlNode[]): Generator<{ name: string }> {
+  for (const node of nodes) {
+    if (node.type === NodeTypes.LiquidTag) {
+      const tag = node as LiquidTag;
+
+      if (isLiquidTagAssign(tag) && tag.markup.name) {
+        yield { name: tag.markup.name };
+      } else if (isLiquidTagCapture(tag) && tag.markup.name) {
+        yield { name: tag.markup.name };
+      } else if (
+        (isLiquidTagIncrement(tag) || isLiquidTagDecrement(tag)) &&
+        tag.markup.name !== null
+      ) {
+        yield { name: tag.markup.name };
+      }
+
+      // `capture`, `if`, `unless`, `case`, `for`, `tablerow`, `form`,
+      // `paginate` etc. can contain nested children that themselves declare
+      // file-scope variables (e.g. an `assign` inside a `capture` body
+      // pattern, or `assign` branches inside an `if`).
+      if (Array.isArray(tag.children)) {
+        yield* collectFileScopeDefiningTags(tag.children);
+      }
+    } else if ('children' in node && Array.isArray((node as any).children)) {
+      yield* collectFileScopeDefiningTags((node as any).children);
+    }
+  }
+}