Fix UnusedAssign false positive inside inline {% liquid ... style ... endstyle %}#1182
Conversation
The inline-liquid raw-tag CST mapping parsed every raw-tag body as
plain text. As a result, variable references inside
{% liquid ... style ... echo foo ... endstyle %} never produced
AST nodes, so checks that walk the tree saw no usage and flagged
assigns as unused.
The top-level liquidRawTagImpl already distinguishes schema/raw
(kept as text) from other raw tags like style (re-parsed as Liquid
so children are part of the AST). The inline-liquid mapping now
mirrors this: schema and raw stay text; everything else is
re-parsed with the LiquidStatement grammar so references inside
style and friends are preserved.
Added a regression test in UnusedAssign that reproduces the
exact snippet from the original report. Full vitest suite
(294 files, 1860 tests) remains green.
Closes Shopify#465
|
Friendly check-in — this PR has been open for two weeks. CLA + mergeability checks are green. Happy to rebase, split commits, or adjust anything that would make review easier. |
|
@charlespwd @graygilmore — could one of you take a look when you have a moment? PR's been open three weeks, CLA + mergeability checks are green, and it's a small contained fix for #465 (UnusedAssign false positive in inline |
|
@rushikeshmore Thank you for the submission. Unfortunately, changing the stage-1-cst also opens behavioural changes to If you do re-attempt this problem, please follow the PR template for the description https://github.com/Shopify/theme-tools/blob/main/.github/PULL_REQUEST_TEMPLATE.md |
What are you adding in this PR?
Fixes a false-positive
UnusedAssignwarning for variables that are only referenced inside astyleblock within an inline{% liquid %}tag.Repro from the issue:
{%- liquid assign shopName = shop.name capture example echo 'Shopify' endcapture style echo example echo shopName endstyle -%}Before the fix, both
exampleandshopNameget reported as "assigned but not used". After the fix, no offense is reported.Root cause
The CST mapping in
packages/liquid-html-parser/src/stage-1-cst.tshas twoliquidRawTagImplentries — one for the mainLiquidgrammar (top-level{% style %}...{% endstyle %}) and one for theLiquidStatementgrammar (inline{% liquid ... style ... endstyle %}).The top-level mapping already distinguishes:
schema/raw→TextNodeGrammar(raw string, not visited)style,stylesheet,javascript) → re-parsed withgrammars.Liquidso children end up in the ASTThe inline mapping forced every raw tag body through
TextNodeGrammar. That meant theecho exampleandecho shopNameinside the inlinestyleblock never producedVariableLookupnodes, andUnusedAssign(which walksVariableLookupto decide "used") never registered them as used.Fix
Mirror the top-level switch in the inline mapping.
schemaandrawstill stay as raw text; every other raw tag is re-parsed withgrammars.LiquidStatement(the inline-liquid grammar) so references insidestyleand friends become first-class AST nodes.Adds a regression test in
unused-assign/index.spec.tsusing the exact snippet from #465.Related
This supersedes the grammar-level attempt in #683 — the real bug was not in the grammar (style is a valid raw tag name in both the top-level and inline-liquid grammars) but in how the CST mapping re-parsed the raw body.
What's next? Any followup issues?
The inline form of
{% liquid ... javascript %}/{% liquid ... stylesheet %}/{% liquid ... schema %}is exotic but now produces the same AST shape as their top-level counterparts. No follow-ups I can see.Before you deploy
changesetfor@shopify/liquid-html-parservitest rungreen — 294 files, 1860 tests, 1 pre-existing skipyarn format:checkcleanyarn workspace ... type-checkclean for@shopify/liquid-html-parser,@shopify/theme-check-common,@shopify/theme-check-node,@shopify/theme-language-server-common,@shopify/prettier-plugin-liquidCloses #465