fix(undefined-object): track Liquid declarations inside HTML comments#1197
Open
SinhSinhAn wants to merge 2 commits intoShopify:mainfrom
Open
fix(undefined-object): track Liquid declarations inside HTML comments#1197SinhSinhAn wants to merge 2 commits intoShopify:mainfrom
SinhSinhAn wants to merge 2 commits intoShopify:mainfrom
Conversation
`<!-- {% assign foo = 10 %} -->` 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 Shopify#1099
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR?
Fix #1099 (filed by @charlespwd). The reproduction:
was producing
Unknown object 'foo' usedeven though Liquid runs inside HTML comments at runtime — the browser only strips<!-- ... -->after Liquid has already executed.Why was this broken?
The HTML grammar stores
HtmlComment.bodyas opaque text:The check infrastructure walks the AST via
visitLiquid, so anyLiquidTagnested inside anHtmlComment.bodyis invisible to every visitor.UndefinedObjectnever sees the{% assign foo %}, never recordsfooas defined, and the later{{ foo }}is flagged.This is technically a parser-layer asymmetry (Liquid is parsed inside attribute values and inside
liquidRawTagbodies, but not inside HTML comment bodies), but fixing it at the grammar level is invasive: it would changeHtmlComment.bodyfromstringto a structured array, break theprettier-plugin-liquidHtmlCommentprinter atprinter-liquid-html.ts:301-325, and ripple through every downstream consumer that readsnode.bodyas a string. That feels like a design call for the maintainers, not a drive-by.How this PR fixes it
Scope the fix to the
UndefinedObjectcheck itself. When the visitor encounters anHtmlComment, the check re-parsesnode.bodywith the existingtoLiquidASThelper and walks the sub-AST for the four tag types whose declarations are visible outside the enclosing block:assigncaptureincrementdecrementEach declaration is registered with
scope.start = comment.position.endso the variable becomes visible from the end of the comment onward, matching the existing position-based scoping rules used elsewhere in the check. References that appear before the comment continue to be flagged correctly.Block-local declarations (
for,tablerow,form,paginate,layout none) are intentionally skipped:-->If the comment body is malformed Liquid (or just plain HTML text with no tags), parsing throws and the check silently skips the comment rather than failing the entire run.
Trade-offs vs. fixing the grammar
The deep fix (parse Liquid inside the HTML comment grammar rule, change
HtmlComment.bodyto a structured array) would also benefit other checks likeUnknownFilter,DeprecatedFilter, andUnusedAssign. I'd be happy to tackle that as a follow-up if you'd prefer the broader fix; the current PR is shaped to land the user-visible win quickly without touching the AST shape or the prettier printer.Test plan
Added 8 regression tests under a
Liquid inside HTML comments (issue #1099)describe block inindex.spec.ts:{% if %}block are tracked through both branchesFull regression run: 954 / 954 tests pass across
theme-check-common. TypeScript build clean.Closes #1099