Fix UnusedAssign false positive inside inline {% liquid ... style ... endstyle %}#1182
Open
rushikeshmore wants to merge 1 commit intoShopify:mainfrom
Open
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
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
1 participant
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 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