From 26cdb29efd89eef964525edcd6dcad17b62c46e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20S=C3=B8gaard?= <9662430+andershagbard@users.noreply.github.com> Date: Sun, 14 Jun 2026 15:00:10 +0200 Subject: [PATCH 1/2] fix(liquid-html-parser): support filter expressions in render named arguments When a render tag uses a filter expression as a named argument value (e.g. `render 'snippet', for: block.id | append: '-list'`), the parser was falling back to the base-case `LiquidTag` with a string markup instead of producing a structured `RenderMarkup` node. This happened because `renderArguments` used `tagArguments` which only allowed `liquidExpression` (no filters) as argument values. The leftover `| append: '-list'` caused `liquidTagRender` to fail, triggering base-case fallback. Add `renderTagArguments`, `renderTagNamedArgument`, and `renderTagArgumentValue` rules that consume filter chains in named argument values. `renderTagArgumentValue` is like `liquidVariable` but without the `&delim` lookahead at the end, allowing it to work for both last and non-last named arguments in a list. This fixes a false positive in the `OrphanedSnippet` check: snippets rendered via a tag with filter expressions in named arguments were incorrectly reported as orphaned because the theme graph traversal (which relies on `RenderMarkup` nodes) never saw the render reference. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- packages/liquid-html-parser/grammar/liquid-html.ohm | 7 +++++-- packages/liquid-html-parser/src/stage-1-cst.ts | 10 ++++++++++ packages/liquid-html-parser/src/stage-2-ast.spec.ts | 7 +++++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/packages/liquid-html-parser/grammar/liquid-html.ohm b/packages/liquid-html-parser/grammar/liquid-html.ohm index c60759c29..8eea5aa59 100644 --- a/packages/liquid-html-parser/grammar/liquid-html.ohm +++ b/packages/liquid-html-parser/grammar/liquid-html.ohm @@ -143,8 +143,11 @@ Liquid <: Helpers { liquidTagRenderMarkup = snippetExpression renderVariableExpression? renderAliasExpression? renderArguments - renderArguments = (argumentSeparatorOptionalComma tagArguments) (space* ",")? space* - completionModeRenderArguments = (argumentSeparatorOptionalComma tagArguments) (space* ",")? space* (argumentSeparator? liquidVariableLookup space*)? + renderArguments = (argumentSeparatorOptionalComma renderTagArguments) (space* ",")? space* + completionModeRenderArguments = (argumentSeparatorOptionalComma renderTagArguments) (space* ",")? space* (argumentSeparator? liquidVariableLookup space*)? + renderTagArguments = listOf + renderTagNamedArgument = variableSegment space* ":" space* renderTagArgumentValue + renderTagArgumentValue = liquidComplexExpression liquidFilter* space* snippetExpression = liquidString | variableSegmentAsLookup renderVariableExpression = space+ ("for" | "with") space+ liquidExpression renderAliasExpression = space+ "as" space+ variableSegment diff --git a/packages/liquid-html-parser/src/stage-1-cst.ts b/packages/liquid-html-parser/src/stage-1-cst.ts index 35f3e9a9b..c77e59bba 100644 --- a/packages/liquid-html-parser/src/stage-1-cst.ts +++ b/packages/liquid-html-parser/src/stage-1-cst.ts @@ -919,6 +919,16 @@ function toCST( source, }, renderArguments: 1, + renderTagArguments: 0, + renderTagNamedArgument: { + type: ConcreteNodeTypes.NamedArgument, + name: 0, + value: 4, + locStart, + locEnd, + source, + }, + renderTagArgumentValue: 0, completionModeRenderArguments: function ( _0, namedArguments, diff --git a/packages/liquid-html-parser/src/stage-2-ast.spec.ts b/packages/liquid-html-parser/src/stage-2-ast.spec.ts index 91b2188a9..b37e9c38a 100644 --- a/packages/liquid-html-parser/src/stage-2-ast.spec.ts +++ b/packages/liquid-html-parser/src/stage-2-ast.spec.ts @@ -509,6 +509,13 @@ describe('Unit: Stage 2 (AST)', () => { { name: 'key2', valueType: 'String' }, ], }, + { + expression: `'snippet', for: block.id | append: '-list'`, + snippetType: 'String', + alias: null, + renderVariableExpression: null, + namedArguments: [{ name: 'for', valueType: 'VariableLookup' }], + }, ].forEach( ({ expression, snippetType, renderVariableExpression, alias, namedArguments }) => { for (const { toAST, expectPath, expectPosition } of testCases) { From 167ba597cd4bc763b76dc437df6e1181cdf9f6e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20S=C3=B8gaard?= <9662430+andershagbard@users.noreply.github.com> Date: Sat, 20 Jun 2026 12:10:59 +0200 Subject: [PATCH 2/2] feat(theme-check-common): support union and string literal types in LiquidDoc @param Adds support for union types (e.g. {string|number}) and string literal types (e.g. {'banner'|'label'}) in {% doc %} @param annotations. - parseParamType: accepts | separated members, validates each as a named type or quoted string literal - isTypeCompatible: returns true if actual type matches any union member; literals are compatible with string args - getDefaultValueForType: uses first literal as default for literal unions - findTypeMismatchParams: removes skip guard that silently ignored non-basic types, so union/literal types now get type-checked - LiquidDocParamTypeCompletionProvider: trigger regex extended to fire after | and inside quoted literals Co-Authored-By: Claude Sonnet 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01T7YveNnZpURm8j81wUDK7D --- .changeset/doc-union-literal-types.md | 12 ++ .../valid-doc-param-types/index.spec.ts | 53 +++++++++ .../src/liquid-doc/arguments.ts | 13 +-- .../src/liquid-doc/utils.spec.ts | 103 +++++++++++++++++- .../src/liquid-doc/utils.ts | 48 ++++++-- .../LiquidDocParamTypeCompletionProvider.ts | 2 +- 6 files changed, 204 insertions(+), 27 deletions(-) create mode 100644 .changeset/doc-union-literal-types.md diff --git a/.changeset/doc-union-literal-types.md b/.changeset/doc-union-literal-types.md new file mode 100644 index 000000000..60c10b589 --- /dev/null +++ b/.changeset/doc-union-literal-types.md @@ -0,0 +1,12 @@ +--- +'@shopify/theme-check-common': minor +'@shopify/theme-language-server-common': minor +--- + +Add support for union types and string literal types in `{% doc %}` `@param` annotations. + +Named types can now be combined with `|`: `{string|number}` + +String literals are now valid param types: `{'banner'|'label'}` + +Unions of named types and literals are also supported: `{string|'banner'|'label'}` diff --git a/packages/theme-check-common/src/checks/valid-doc-param-types/index.spec.ts b/packages/theme-check-common/src/checks/valid-doc-param-types/index.spec.ts index 2e66619f2..18a69bc15 100644 --- a/packages/theme-check-common/src/checks/valid-doc-param-types/index.spec.ts +++ b/packages/theme-check-common/src/checks/valid-doc-param-types/index.spec.ts @@ -72,4 +72,57 @@ describe('Module: ValidDocParamTypes', () => { expect(suggestions).to.include(`{% doc %} @param param1 - Example param {% enddoc %}`); } }); + + it('should not report an error for a union of named types', async () => { + const sourceCode = ` + {% doc %} + @param {string|number} param1 - A string or number + {% enddoc %} + `; + const offenses = await runLiquidCheck(ValidDocParamTypes, sourceCode); + expect(offenses).to.be.empty; + }); + + it('should not report an error for a string literal type', async () => { + const sourceCode = ` + {% doc %} + @param {'banner'} param1 - Must be banner + {% enddoc %} + `; + const offenses = await runLiquidCheck(ValidDocParamTypes, sourceCode); + expect(offenses).to.be.empty; + }); + + it('should not report an error for a union of string literals', async () => { + const sourceCode = ` + {% doc %} + @param {'banner'|'label'} param1 - Either banner or label + {% enddoc %} + `; + const offenses = await runLiquidCheck(ValidDocParamTypes, sourceCode); + expect(offenses).to.be.empty; + }); + + it('should not report an error for a mixed union of named types and string literals', async () => { + const sourceCode = ` + {% doc %} + @param {string|'banner'|'label'} param1 - A string or specific value + {% enddoc %} + `; + const offenses = await runLiquidCheck(ValidDocParamTypes, sourceCode); + expect(offenses).to.be.empty; + }); + + it('should report an error when a union contains an invalid named type', async () => { + const sourceCode = ` + {% doc %} + @param {string|invalidType} param1 - Example param + {% enddoc %} + `; + const offenses = await runLiquidCheck(ValidDocParamTypes, sourceCode); + expect(offenses).to.have.length(1); + expect(offenses[0].message).to.equal( + "The parameter type 'string|invalidType' is not supported.", + ); + }); }); diff --git a/packages/theme-check-common/src/liquid-doc/arguments.ts b/packages/theme-check-common/src/liquid-doc/arguments.ts index 5aa749e40..578d8586d 100644 --- a/packages/theme-check-common/src/liquid-doc/arguments.ts +++ b/packages/theme-check-common/src/liquid-doc/arguments.ts @@ -9,12 +9,7 @@ import { NodeTypes, } from '@shopify/liquid-html-parser'; import { Context, LiquidDocParameter, SourceCodeType, StringCorrector } from '..'; -import { - BasicParamTypes, - getDefaultValueForType, - inferArgumentType, - isTypeCompatible, -} from './utils'; +import { getDefaultValueForType, inferArgumentType, isTypeCompatible } from './utils'; import { isLiquidString } from '../checks/utils'; /** @@ -124,11 +119,7 @@ export function findTypeMismatchParams( const liquidDocParamDef = liquidDocParameters.get(arg.name); if (liquidDocParamDef && liquidDocParamDef.type) { - const paramType = liquidDocParamDef.type.toLowerCase(); - const supportedTypes = Object.keys(BasicParamTypes).map((type) => type.toLowerCase()); - if (!supportedTypes.includes(paramType)) { - continue; - } + const paramType = liquidDocParamDef.type; if (!isTypeCompatible(paramType, inferArgumentType(arg.value))) { typeMismatchParams.push(arg); diff --git a/packages/theme-check-common/src/liquid-doc/utils.spec.ts b/packages/theme-check-common/src/liquid-doc/utils.spec.ts index 322e7c27b..5f163f72f 100644 --- a/packages/theme-check-common/src/liquid-doc/utils.spec.ts +++ b/packages/theme-check-common/src/liquid-doc/utils.spec.ts @@ -1,10 +1,10 @@ import { describe, expect, it } from 'vitest'; -import { BasicParamTypes, parseParamType } from './utils'; +import { BasicParamTypes, getDefaultValueForType, isTypeCompatible, parseParamType } from './utils'; describe('liquid-doc/utils', () => { - describe('parseParamType', () => { - const validParamTypes = new Set([...Object.values(BasicParamTypes), 'product']); + const validParamTypes = new Set([...Object.values(BasicParamTypes), 'product']); + describe('parseParamType', () => { it('should parse all values provided in the `validParamTypes` set', () => { const tests = { string: ['string', false], @@ -19,5 +19,102 @@ describe('liquid-doc/utils', () => { expect(result).toEqual(expected); }); }); + + it('should accept union of named types', () => { + expect(parseParamType(validParamTypes, 'string|number')).toBeDefined(); + expect(parseParamType(validParamTypes, 'string|number|boolean')).toBeDefined(); + expect(parseParamType(validParamTypes, 'string|product')).toBeDefined(); + }); + + it('should accept string literal types', () => { + expect(parseParamType(validParamTypes, "'banner'")).toBeDefined(); + expect(parseParamType(validParamTypes, '"banner"')).toBeDefined(); + expect(parseParamType(validParamTypes, "'banner'|'label'")).toBeDefined(); + }); + + it('should accept union of named types and string literals', () => { + expect(parseParamType(validParamTypes, "string|'banner'")).toBeDefined(); + expect(parseParamType(validParamTypes, "'banner'|number")).toBeDefined(); + }); + + it('should return string as pseudoType for literal-only unions', () => { + expect(parseParamType(validParamTypes, "'banner'|'label'")).toEqual(['string', false]); + }); + + it('should return the first named type as pseudoType for mixed unions', () => { + expect(parseParamType(validParamTypes, 'number|string')).toEqual(['number', false]); + }); + + it('should reject unions containing invalid named types', () => { + expect(parseParamType(validParamTypes, 'string|invalidType')).toBeUndefined(); + expect(parseParamType(validParamTypes, "'banner'|invalidType")).toBeUndefined(); + }); + + it('should reject bare unquoted values that are not valid named types', () => { + expect(parseParamType(validParamTypes, 'banner')).toBeUndefined(); + }); + }); + + describe('isTypeCompatible', () => { + it('should return true when types match exactly', () => { + expect(isTypeCompatible('string', BasicParamTypes.String)).toBe(true); + expect(isTypeCompatible('number', BasicParamTypes.Number)).toBe(true); + }); + + it('should return false when types do not match', () => { + expect(isTypeCompatible('string', BasicParamTypes.Number)).toBe(false); + expect(isTypeCompatible('number', BasicParamTypes.String)).toBe(false); + }); + + it('should return true for boolean regardless of actual type', () => { + expect(isTypeCompatible('boolean', BasicParamTypes.String)).toBe(true); + expect(isTypeCompatible('boolean', BasicParamTypes.Number)).toBe(true); + }); + + it('should return true when actual type matches any member of a union', () => { + expect(isTypeCompatible('string|number', BasicParamTypes.String)).toBe(true); + expect(isTypeCompatible('string|number', BasicParamTypes.Number)).toBe(true); + }); + + it('should return false when actual type matches no member of a union', () => { + expect(isTypeCompatible('string|number', BasicParamTypes.Boolean)).toBe(false); + }); + + it('should return true for string args against a string literal union', () => { + expect(isTypeCompatible("'banner'|'label'", BasicParamTypes.String)).toBe(true); + }); + + it('should return false for non-string args against a string literal union', () => { + expect(isTypeCompatible("'banner'|'label'", BasicParamTypes.Number)).toBe(false); + }); + + it('should return true for string args against a mixed literal and named union', () => { + expect(isTypeCompatible("'banner'|number", BasicParamTypes.String)).toBe(true); + expect(isTypeCompatible("'banner'|number", BasicParamTypes.Number)).toBe(true); + }); + }); + + describe('getDefaultValueForType', () => { + it('should return defaults for basic types', () => { + expect(getDefaultValueForType('string')).toBe("''"); + expect(getDefaultValueForType('number')).toBe('0'); + expect(getDefaultValueForType('boolean')).toBe('false'); + expect(getDefaultValueForType('object')).toBe(''); + expect(getDefaultValueForType(null)).toBe(''); + }); + + it('should use the first member default for union of named types', () => { + expect(getDefaultValueForType('string|number')).toBe("''"); + expect(getDefaultValueForType('number|string')).toBe('0'); + }); + + it('should use the first string literal as default for literal unions', () => { + expect(getDefaultValueForType("'banner'|'label'")).toBe("'banner'"); + expect(getDefaultValueForType('"banner"|"label"')).toBe('"banner"'); + }); + + it('should use the first literal as default for mixed unions', () => { + expect(getDefaultValueForType("'banner'|string")).toBe("'banner'"); + }); }); }); diff --git a/packages/theme-check-common/src/liquid-doc/utils.ts b/packages/theme-check-common/src/liquid-doc/utils.ts index bd47d15ef..fb803d528 100644 --- a/packages/theme-check-common/src/liquid-doc/utils.ts +++ b/packages/theme-check-common/src/liquid-doc/utils.ts @@ -19,6 +19,9 @@ export enum BasicParamTypes { Object = 'object', } +/** Matches single- or double-quoted string literals, e.g. 'banner' or "label" */ +const LITERAL_TYPE_RE = /^'[^']*'$|^"[^"]*"$/; + export enum SupportedDocTagTypes { Param = 'param', Example = 'example', @@ -27,9 +30,16 @@ export enum SupportedDocTagTypes { /** * Provides a default completion value for an argument / parameter of a given type. + * For union types, uses the first member. For string literal types, uses the literal itself. */ export function getDefaultValueForType(type: string | null) { - switch (type?.toLowerCase()) { + const firstMember = type?.split('|')[0] ?? null; + + if (firstMember && LITERAL_TYPE_RE.test(firstMember)) { + return firstMember; + } + + switch (firstMember?.toLowerCase()) { case BasicParamTypes.String: return "''"; case BasicParamTypes.Number: @@ -64,17 +74,23 @@ export function inferArgumentType(arg: LiquidExpression): BasicParamTypes { /** * Checks if the provided argument type is compatible with the expected type. + * Supports union types (e.g. string|number) and string literal types (e.g. 'banner'|'label'). * Makes certain types more permissive: * - Boolean accepts any value, since everything is truthy / falsy in Liquid + * - String literal types (e.g. 'banner') accept any string value */ export function isTypeCompatible(expectedType: string, actualType: BasicParamTypes): boolean { - const normalizedExpectedType = expectedType.toLowerCase(); + return expectedType.split('|').some((member) => { + if (LITERAL_TYPE_RE.test(member)) { + return actualType === BasicParamTypes.String; + } - if (normalizedExpectedType === BasicParamTypes.Boolean) { - return true; - } + const normalized = member.toLowerCase(); + + if (normalized === BasicParamTypes.Boolean) return true; - return normalizedExpectedType === actualType; + return normalized === actualType; + }); } /** @@ -112,14 +128,22 @@ export function parseParamType( validParamTypes: Set, value: string, ): [pseudoType: string, isArray: boolean] | undefined { - const paramTypeMatch = value.match(/^([a-z_]+)(\[\])?$/); + const members = value.split('|'); - if (!paramTypeMatch) return undefined; + for (const member of members) { + if (LITERAL_TYPE_RE.test(member)) continue; - const extractedParamType = paramTypeMatch[1]; - const isArrayType = !!paramTypeMatch[2]; + const namedTypeMatch = member.match(/^([a-z_]+)(\[\])?$/); + if (!namedTypeMatch) return undefined; + if (!validParamTypes.has(namedTypeMatch[1])) return undefined; + } - if (!validParamTypes.has(extractedParamType)) return undefined; + // Return the first named type member for backward-compat callers; fall back to string for literal-only unions + const firstNamed = members.find((m) => !LITERAL_TYPE_RE.test(m)); + if (firstNamed) { + const match = firstNamed.match(/^([a-z_]+)(\[\])?$/)!; + return [match[1], !!match[2]]; + } - return [extractedParamType, isArrayType]; + return [BasicParamTypes.String, false]; } diff --git a/packages/theme-language-server-common/src/completions/providers/LiquidDocParamTypeCompletionProvider.ts b/packages/theme-language-server-common/src/completions/providers/LiquidDocParamTypeCompletionProvider.ts index b04da71eb..a9a84bd9d 100644 --- a/packages/theme-language-server-common/src/completions/providers/LiquidDocParamTypeCompletionProvider.ts +++ b/packages/theme-language-server-common/src/completions/providers/LiquidDocParamTypeCompletionProvider.ts @@ -35,7 +35,7 @@ export class LiquidDocParamTypeCompletionProvider implements Provider { if ( fragments.length > 2 || fragments[0] !== `@${SupportedDocTagTypes.Param}` || - !/^\{[a-zA-Z]*$/.test(fragments[1]) + !/^\{[a-zA-Z'"|]*$/.test(fragments[1]) ) { return []; }