From 194f44c56982d8c4bc818b5221b7b22d379265be Mon Sep 17 00:00:00 2001 From: SinhSinh An Date: Sun, 26 Apr 2026 23:44:56 -0500 Subject: [PATCH 1/2] fix(undefined-object): track Liquid declarations inside HTML comments `` followed by `{{ foo }}` triggered an "Unknown object 'foo' used" warning even though Liquid is executed inside HTML comments at runtime (the browser strips `` after Liquid has already run). The HTML grammar stores `HtmlComment.body` as opaque text, so the visitor that powers UndefinedObject never reaches Liquid nodes nested inside a comment. Rather than refactor the grammar/AST/printer (which would be a breaking change for downstream consumers), the check now re-parses each `HtmlComment.body` with `toLiquidAST` and walks the resulting sub-AST for the four tag types that declare file-scoped variables: `assign`, `capture`, `increment`, `decrement`. Each declaration is registered with `scope.start = comment.position.end` so the variable becomes visible from the end of the comment onward, matching the existing position-based scoping rules. References that appear before the comment continue to be flagged. Block-local declarations (`for`, `tablerow`, `form`, `paginate`, `layout none`) are intentionally skipped because their scope cannot escape the block, and any references to them inside the comment are themselves unreachable to the visitor. Closes #1099 --- .../fix-undefined-object-html-comment.md | 5 + .../src/checks/undefined-object/index.spec.ts | 96 +++++++++++++++++++ .../src/checks/undefined-object/index.ts | 76 +++++++++++++++ 3 files changed, 177 insertions(+) create mode 100644 .changeset/fix-undefined-object-html-comment.md diff --git a/.changeset/fix-undefined-object-html-comment.md b/.changeset/fix-undefined-object-html-comment.md new file mode 100644 index 000000000..b0ee0476a --- /dev/null +++ b/.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. diff --git a/packages/theme-check-common/src/checks/undefined-object/index.spec.ts b/packages/theme-check-common/src/checks/undefined-object/index.spec.ts index 8c16228e4..76c321d0d 100644 --- a/packages/theme-check-common/src/checks/undefined-object/index.spec.ts +++ b/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 = ` + + {{ 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 = ` + + {{ 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 = ` + + {{ 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 = ` + + {{ 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 }} + + `; + + 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 = ` + + {{ 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 = ` + + {{ 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 = ` + + {{ 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."); + }); + }); }); diff --git a/packages/theme-check-common/src/checks/undefined-object/index.ts b/packages/theme-check-common/src/checks/undefined-object/index.ts index 9057bbb94..004ba4e53 100644 --- a/packages/theme-check-common/src/checks/undefined-object/index.ts +++ b/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,45 @@ 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); + } + } +} From e3519db60c5739ea20cf2a07971fe0484c1a2e94 Mon Sep 17 00:00:00 2001 From: SinhSinh An Date: Sun, 26 Apr 2026 23:51:10 -0500 Subject: [PATCH 2/2] style: apply prettier formatting --- .../theme-check-common/src/checks/undefined-object/index.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/theme-check-common/src/checks/undefined-object/index.ts b/packages/theme-check-common/src/checks/undefined-object/index.ts index 004ba4e53..e28684751 100644 --- a/packages/theme-check-common/src/checks/undefined-object/index.ts +++ b/packages/theme-check-common/src/checks/undefined-object/index.ts @@ -338,9 +338,7 @@ function isLiquidTagDecrement(node: LiquidTag): node is LiquidTagDecrement { * so they cannot contribute to a reference outside the original HTML * comment. */ -function* collectFileScopeDefiningTags( - nodes: LiquidHtmlNode[], -): Generator<{ name: string }> { +function* collectFileScopeDefiningTags(nodes: LiquidHtmlNode[]): Generator<{ name: string }> { for (const node of nodes) { if (node.type === NodeTypes.LiquidTag) { const tag = node as LiquidTag;