From 11fa47fe526381ed1b66c18347cd130bd45a593a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Claud=C3=A9ric=20Demers?= Date: Fri, 25 Sep 2026 16:02:40 -0400 Subject: [PATCH 1/4] Fix quoted delimiters in LiquidDoc parameter types --- .changeset/quiet-doc-delimiters.md | 5 + packages/liquid-html-parser/src/ast.test.ts | 17 +++ .../src/liquid-doc/parser.test.ts | 107 ++++++++++++++++++ .../src/liquid-doc/tokenizer.ts | 53 ++++++++- .../src/test/liquid-doc/fixed.liquid | 11 ++ .../src/test/liquid-doc/index.liquid | 11 ++ 6 files changed, 200 insertions(+), 4 deletions(-) create mode 100644 .changeset/quiet-doc-delimiters.md diff --git a/.changeset/quiet-doc-delimiters.md b/.changeset/quiet-doc-delimiters.md new file mode 100644 index 000000000..c5702d7b4 --- /dev/null +++ b/.changeset/quiet-doc-delimiters.md @@ -0,0 +1,5 @@ +--- +'@shopify/liquid-html-parser': patch +--- + +Preserve quoted braces in LiquidDoc parameter types and recover incomplete string annotations without consuming subsequent parameters. diff --git a/packages/liquid-html-parser/src/ast.test.ts b/packages/liquid-html-parser/src/ast.test.ts index 996d97a6d..f0bdeb7e1 100644 --- a/packages/liquid-html-parser/src/ast.test.ts +++ b/packages/liquid-html-parser/src/ast.test.ts @@ -1825,6 +1825,23 @@ describe('Unit: Stage 2 (AST)', () => { expectPath(ast, 'children.0.markup.1.children.0.children.1.markup.name').to.eql('var3'); }); + it('should preserve string enum doc types in Liquid and HTML documents', () => { + const paramType = "'Heading' | 'a|b' | '}' | ''"; + const source = `{% doc %}\n @param {${paramType}} [variant] - Shared text style\n{% enddoc %}`; + for (const toAST of [toLiquidAST, toLiquidHtmlAST]) { + const expectPosition = makeExpectPosition(toAST.name); + ast = toAST(source); + expectPath(ast, 'children.0.body.nodes.0.paramType.type').toEqual('TextNode'); + expectPath(ast, 'children.0.body.nodes.0.paramType.value').toEqual(paramType); + expectPosition(ast, 'children.0.body.nodes.0.paramType').toEqual(paramType); + expectPosition(ast, 'children.0.body.nodes.0.paramName').toEqual('variant'); + expectPath(ast, 'children.0.body.nodes.0.required').toEqual(false); + expectPath(ast, 'children.0.body.nodes.0.paramDescription.value').toEqual( + 'Shared text style', + ); + } + }); + it(`should parse doc tags`, () => { ast = toLiquidAST(`{% doc %}{% enddoc %}`); expectPath(ast, 'children.0.type').to.eql('LiquidRawTag'); diff --git a/packages/liquid-html-parser/src/liquid-doc/parser.test.ts b/packages/liquid-html-parser/src/liquid-doc/parser.test.ts index f7339b0c4..1fb9267e1 100644 --- a/packages/liquid-html-parser/src/liquid-doc/parser.test.ts +++ b/packages/liquid-html-parser/src/liquid-doc/parser.test.ts @@ -104,6 +104,113 @@ describe('Unit: liquid-doc parser', () => { expect(param.required).toBe(false); }); + it.each([ + "'heading' | 'small'", + '"heading"|"small"', + '\'Heading\' | "small"', + "'' | ' ' | 'two spaces'", + "'a|b' | 'small'", + "'}' | '{' | '{}'", + "'a}b' | 'small'", + "'a} [variant] - b'", + "'a} [variant] Users'", + "'a} variant - Users'", + '"it\'s" | \'"quoted"\'', + String.raw`'backslash\' | 'small'`, + " 'heading' \t|\t 'small' ", + ])('preserves string enum type %s and its source positions', (paramType) => { + const nodes = parseDoc(`\n @param {${paramType}} [variant] - Shared text style\n`); + expect(nodes).toHaveLength(1); + const param = asParam(nodes[0]); + expect(param.paramType!.value).toBe(paramType); + expect(param.paramName.value).toBe('variant'); + expect(param.required).toBe(false); + expect(param.paramDescription!.value).toBe('Shared text style'); + for (const text of [param.paramType!, param.paramName, param.paramDescription!]) { + expect(param.source.slice(text.position.start, text.position.end)).toBe(text.value); + } + expect(param.source[param.paramType!.position.start - 1]).toBe('{'); + expect(param.source[param.paramType!.position.end]).toBe('}'); + }); + + it('parses required enum parameters with multiline descriptions and CRLF', () => { + const nodes = parseDoc( + "\r\n@param {'heading' | 'small'} variant - Shared style\r\n for text.\r\n@param {string} next\r\n", + ); + expect(nodes).toHaveLength(2); + const param = asParam(nodes[0]); + expect(param.paramType!.value).toBe("'heading' | 'small'"); + expect(param.paramName.value).toBe('variant'); + expect(param.required).toBe(true); + expect(param.paramDescription!.value).toContain('for text.'); + expect(asParam(nodes[1]).paramName.value).toBe('next'); + }); + + it.each([ + "'heading' |", + "| 'heading'", + "'heading' || 'small'", + "'heading' 'small'", + "'heading", + '"heading', + "'a}b' | 'small", + "'a}b' trailing", + ])('preserves malformed enum type %s for semantic validation', (paramType) => { + const description = "It's {the shared style}."; + const nodes = parseDoc( + `\n@param {${paramType}} [variant] - ${description}\n@param {number} next\n`, + ); + expect(nodes).toHaveLength(2); + const param = asParam(nodes[0]); + expect(param.paramType!.value).toBe(paramType); + expect(param.paramName.value).toBe('variant'); + expect(param.required).toBe(false); + expect(param.paramDescription!.value).toBe(description); + expect(asParam(nodes[1]).paramName.value).toBe('next'); + }); + + it.each(['', '- '])( + 'recovers a missing type quote before a parameter description with separator %j', + (separator) => { + for (const name of ['variant', '[variant]']) { + for (const description of ["It's {the shared style}.", "Users'"]) { + const nodes = parseDoc( + `\n@param {'heading} ${name} ${separator}${description}\n@param {number} next\n`, + ); + expect(nodes).toHaveLength(2); + const param = asParam(nodes[0]); + expect(param.paramType!.value).toBe("'heading"); + expect(param.paramName.value).toBe('variant'); + expect(param.required).toBe(name === 'variant'); + expect(param.paramDescription!.value).toBe(description); + expect(asParam(nodes[1]).paramName.value).toBe('next'); + } + } + }, + ); + + it('prefers a parameter boundary when the missing delimiter is ambiguous', () => { + // This could be a closed string missing its outer brace, or an unclosed + // string followed by a parameter and an apostrophe in its description. + const param = asParam(parseDoc("\n@param {'a} [variant] - b'\n")[0]); + expect(param.paramType!.value).toBe("'a"); + expect(param.paramName.value).toBe('variant'); + expect(param.required).toBe(false); + expect(param.paramDescription!.value).toBe("b'"); + }); + + it.each(["'heading", "'a}b'", "'a}b' | 'small'"])( + 'keeps an unclosed type %s from consuming the next parameter', + (paramType) => { + const nodes = parseDoc(`\n@param {${paramType}\n@param {number} next\n`); + expect(nodes).toHaveLength(2); + const param = asParam(nodes[0]); + expect(param.paramType).toBeNull(); + expect(param.paramName.value).toBe(''); + expect(asParam(nodes[1]).paramName.value).toBe('next'); + }, + ); + it('parses param with description after dash', () => { const nodes = parseDoc('\n@param product - The product to display\n'); expect(nodes.length).toBe(1); diff --git a/packages/liquid-html-parser/src/liquid-doc/tokenizer.ts b/packages/liquid-html-parser/src/liquid-doc/tokenizer.ts index 7ef9d8e5a..bb7740ae8 100644 --- a/packages/liquid-html-parser/src/liquid-doc/tokenizer.ts +++ b/packages/liquid-html-parser/src/liquid-doc/tokenizer.ts @@ -170,9 +170,9 @@ export function tokenizeParamContent(text: string, startOffset: number): ParamTo } // Type: {type} - TYPE_RE.lastIndex = pos; - if (TYPE_RE.test(text)) { - const end = TYPE_RE.lastIndex; + const typeEnd = findTypeEnd(text, pos); + if (typeEnd !== -1) { + const end = typeEnd; tokens.push({ type: ParamTokenType.Type, value: text.slice(pos + 1, end - 1), @@ -247,12 +247,57 @@ export function tokenizeParamContent(text: string, startOffset: number): ParamTo return tokens; } +/** Find the closing brace without treating braces inside string literals as delimiters. */ +function findTypeEnd(text: string, start: number): number { + if (text[start] !== '{') return -1; + + let quote: string | undefined; + let recoveryBrace = -1; + + for (let pos = start + 1; pos < text.length; pos++) { + const ch = text[pos]; + if (ch === '\n' || ch === '\r') break; + + if (quote) { + if (ch === quote) { + if ( + recoveryBrace !== -1 && + /^[ \t]*(?:\[[^\]]*\]|[\w][\w-]*)[ \t]+/.test(text.slice(recoveryBrace + 1, pos)) + ) { + let next = pos + 1; + while (text[next] === ' ' || text[next] === '\t') next++; + // An unmatched quote can close at an apostrophe in the parameter's + // description, which need not have a dash. A real type delimiter wins; + // otherwise prefer a recognizable parameter boundary, even at EOL. + // This also recovers ambiguous input with a missing outer brace and a + // parameter-looking name inside its last string literal. + if (text[next] !== '|' && text[next] !== '}') { + return recoveryBrace + 1; + } + } + quote = undefined; + recoveryBrace = -1; + } else if (ch === '}' && recoveryBrace === -1) { + recoveryBrace = pos; + } + continue; + } + + if (ch === '}') return pos + 1; + // Liquid strings do not interpret backslash escapes. + if (ch === "'" || ch === '"') quote = ch; + } + + // Keep a malformed, closed annotation recognizable to semantic checks, and + // preserve its parameter name even when a string quote is missing. + return recoveryBrace === -1 ? -1 : recoveryBrace + 1; +} + const ANNOTATION_RE = /@(\w+)/y; const WHITESPACE_RE = /[ \t]+/y; const NEWLINE_RE = /\r?\n/y; /** Matches `@word` appearing after at least one character within a line (mid-line). */ const MIDLINE_ANNOTATION_RE = /.@\w+/g; -const TYPE_RE = /\{([^}]*)\}/y; const OPTIONAL_NAME_RE = /\[([^\]]*)\]/y; const WORD_RE = /[\w][\w-]*/y; const PARAM_WHITESPACE_RE = /[ \t]+/y; diff --git a/packages/prettier-plugin-liquid/src/test/liquid-doc/fixed.liquid b/packages/prettier-plugin-liquid/src/test/liquid-doc/fixed.liquid index 2adb04aa5..99521e1e7 100644 --- a/packages/prettier-plugin-liquid/src/test/liquid-doc/fixed.liquid +++ b/packages/prettier-plugin-liquid/src/test/liquid-doc/fixed.liquid @@ -18,6 +18,17 @@ It should format the param description with a dash separator @param paramName - param with description {% enddoc %} +It should preserve string enum types and literal contents +{% doc %} + @param {'Heading' | "small" | '' | 'two spaces' | 'a|b' | '}'} [variant] - Shared text style +{% enddoc %} + +It should preserve malformed enum types for linting +{% doc %} + @param {'heading} [variant] - It's {the shared style}. + @param {'heading' |} variant - Shared text style +{% enddoc %} + It should respect the liquidDocParamDash option liquidDocParamDash: false liquidDocParamDash: false {% doc %} diff --git a/packages/prettier-plugin-liquid/src/test/liquid-doc/index.liquid b/packages/prettier-plugin-liquid/src/test/liquid-doc/index.liquid index 094749c28..9842c6f24 100644 --- a/packages/prettier-plugin-liquid/src/test/liquid-doc/index.liquid +++ b/packages/prettier-plugin-liquid/src/test/liquid-doc/index.liquid @@ -18,6 +18,17 @@ It should format the param description with a dash separator @param paramName - param with description {% enddoc %} +It should preserve string enum types and literal contents +{% doc %} +@param { 'Heading' | "small" | '' | 'two spaces' | 'a|b' | '}' } [variant] - Shared text style +{% enddoc %} + +It should preserve malformed enum types for linting +{% doc %} +@param {'heading} [variant] - It's {the shared style}. +@param {'heading' |} variant - Shared text style +{% enddoc %} + It should respect the liquidDocParamDash option liquidDocParamDash: false {% doc %} @param paramName param with description From 295bed869455fb929e27e086c3d6f33f21f1005b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Claud=C3=A9ric=20Demers?= Date: Tue, 29 Sep 2026 15:06:53 -0400 Subject: [PATCH 2/4] Accept string enums in LiquidDoc parameter types Co-Authored-By: Claude Opus 5.5 (1M context) --- .changeset/liquiddoc-string-enums.md | 7 ++ .../valid-doc-param-types/index.spec.ts | 71 ++++++++++++ .../src/checks/valid-doc-param-types/index.ts | 28 ++--- .../src/liquid-doc/doc-param-type.spec.ts | 108 ++++++++++++++++++ .../src/liquid-doc/doc-param-type.ts | 93 +++++++++++++++ .../src/liquid-doc/liquidDoc.spec.ts | 16 +++ .../src/liquid-doc/utils.ts | 33 ++---- 7 files changed, 315 insertions(+), 41 deletions(-) create mode 100644 .changeset/liquiddoc-string-enums.md create mode 100644 packages/theme-check-common/src/liquid-doc/doc-param-type.spec.ts create mode 100644 packages/theme-check-common/src/liquid-doc/doc-param-type.ts diff --git a/.changeset/liquiddoc-string-enums.md b/.changeset/liquiddoc-string-enums.md new file mode 100644 index 000000000..fc0b2bad4 --- /dev/null +++ b/.changeset/liquiddoc-string-enums.md @@ -0,0 +1,7 @@ +--- +'@shopify/theme-check-common': minor +--- + +Support string enums in LiquidDoc parameter types + +`@param` accepts unions of string literals, such as `{'heading' | 'small'}`. 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..8fa1b490f 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 @@ -42,6 +42,45 @@ describe('Module: ValidDocParamTypes', () => { expect(offenses).to.be.empty; }); + it.each([ + "'heading' | 'small'", + '"Heading"|"small"', + "'heading'", + "'' | ' small '", + "'a|b' | 'a}b'", + `"it's" | 'say "hi"'`, + ])('accepts the string enum {%s}', async (type) => { + const source = `{% doc %}\n @param {${type}} [variant] - Text style\n{% enddoc %}`; + expect(await runLiquidCheck(ValidDocParamTypes, source)).toHaveLength(0); + }); + + it.each([ + "'heading' |", + "| 'heading'", + "'heading' || 'small'", + "'heading' 'small'", + "'heading' | small", + "'heading' | 'small", + "'heading' | number", + 'heading | small', + '1 | 2', + 'true | false', + "('heading' | 'small')", + "'heading'[]", + ])( + 'reports the complete invalid enum {%s} and preserves the optional parameter in its fix', + async (type) => { + const source = `{% doc %}\n @param { ${type} } [variant] - Text style\n{% enddoc %}`; + const offenses = await runLiquidCheck(ValidDocParamTypes, source); + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toBe(`The parameter type ' ${type} ' is not supported.`); + expect(source.slice(offenses[0].start.index, offenses[0].end.index)).toBe(`{ ${type} }`); + expect(applySuggestions(source, offenses[0])).toEqual([ + '{% doc %}\n @param [variant] - Text style\n{% enddoc %}', + ]); + }, + ); + it('should report an error with suggestions when an invalid parameter type is used', async () => { const sourceCode = ` {% doc %} @@ -72,4 +111,36 @@ describe('Module: ValidDocParamTypes', () => { expect(suggestions).to.include(`{% doc %} @param param1 - Example param {% enddoc %}`); } }); + + it('validates named types, arrays, and enums together', async () => { + const source = `{% doc %} + @param {product} product + @param {string[]} labels + @param {'heading' | 'small'} [variant] + @param {unknown[]} unknown + @param {'heading' |} invalid_variant + {% enddoc %}`; + const offenses = await runLiquidCheck(ValidDocParamTypes, source); + expect(offenses.map(({ message }) => message)).toEqual([ + "The parameter type 'unknown[]' is not supported.", + "The parameter type ''heading' |' is not supported.", + ]); + }); + + it('removes the invalid type without changing braces in its description', async () => { + const source = `{% doc %}\n @param {'heading' |} [variant] - example {foo}\n{% enddoc %}`; + const offenses = await runLiquidCheck(ValidDocParamTypes, source); + expect(offenses).toHaveLength(1); + expect(applySuggestions(source, offenses[0])).toEqual([ + '{% doc %}\n @param [variant] - example {foo}\n{% enddoc %}', + ]); + }); + + it('preserves the existing behavior when no docset is supplied', async () => { + const source = `{% doc %}\n @param {'heading' |} variant\n{% enddoc %}`; + const offenses = await runLiquidCheck(ValidDocParamTypes, source, undefined, { + themeDocset: undefined, + }); + expect(offenses).toHaveLength(0); + }); }); diff --git a/packages/theme-check-common/src/checks/valid-doc-param-types/index.ts b/packages/theme-check-common/src/checks/valid-doc-param-types/index.ts index 584fe82d4..3549db469 100644 --- a/packages/theme-check-common/src/checks/valid-doc-param-types/index.ts +++ b/packages/theme-check-common/src/checks/valid-doc-param-types/index.ts @@ -1,5 +1,5 @@ import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; -import { getValidParamTypes, parseParamType } from '../../liquid-doc/utils'; +import { getValidParamTypes, parseDocParamType } from '../../liquid-doc/utils'; export const ValidDocParamTypes: LiquidCheckDefinition = { meta: { @@ -22,10 +22,8 @@ export const ValidDocParamTypes: LiquidCheckDefinition = { return {}; } - // To avoid recalculating valid param types during theme-check, constructing - // the promise beforehand. - const validParamTypesPromise = context - .themeDocset!.liquidDrops() + const validParamTypesPromise = context.themeDocset + .liquidDrops() .then((entries) => new Set(getValidParamTypes(entries).keys())); return { @@ -34,7 +32,10 @@ export const ValidDocParamTypes: LiquidCheckDefinition = { return; } - const parsedParamType = parseParamType(await validParamTypesPromise, node.paramType.value); + const parsedParamType = parseDocParamType( + await validParamTypesPromise, + node.paramType.value, + ); if (parsedParamType) { return; @@ -51,16 +52,11 @@ export const ValidDocParamTypes: LiquidCheckDefinition = { fix: (corrector) => { if (!node.paramType) return; - corrector.replace( - node.position.start, - node.position.end, - node.source.slice(node.position.start, node.position.end).replace( - // We could have padded spaces around + inside the param type - // e.g. `{ string }`, `{string}`, or ` { string } ` - /\s*\{\s*[^\s]+\s*\}\s*/, - ' ', - ), - ); + let start = node.paramType.position.start - 1; + let end = node.paramType.position.end + 1; + while (/[ \t]/.test(node.source.charAt(start - 1))) start--; + while (/[ \t]/.test(node.source.charAt(end))) end++; + corrector.replace(start, end, ' '); }, }, ], diff --git a/packages/theme-check-common/src/liquid-doc/doc-param-type.spec.ts b/packages/theme-check-common/src/liquid-doc/doc-param-type.spec.ts new file mode 100644 index 000000000..3ae3f9cdd --- /dev/null +++ b/packages/theme-check-common/src/liquid-doc/doc-param-type.spec.ts @@ -0,0 +1,108 @@ +import { describe, expect, it } from 'vitest'; +import { BasicParamTypes, parseDocParamType, parseParamType, parseStringLiterals } from './utils'; + +describe('parseDocParamType', () => { + const validParamTypes = new Set([...Object.values(BasicParamTypes), 'product']); + + it.each([...Object.values(BasicParamTypes), 'product'])( + 'represents the named type %s', + (name) => { + expect(parseDocParamType(validParamTypes, name)).toEqual({ kind: 'named', name }); + }, + ); + + it.each(['string', 'product'])('represents arrays of %s', (valueType) => { + expect(parseDocParamType(validParamTypes, `${valueType}[]`)).toEqual({ + kind: 'array', + valueType, + }); + }); + + it('represents enums as unions of string literals with their original spelling', () => { + expect(parseDocParamType(validParamTypes, `'Heading' | "small" | '' | 'a|b' | 'a}b'`)).toEqual({ + kind: 'union', + types: [ + { kind: 'literal', value: 'Heading', raw: "'Heading'" }, + { kind: 'literal', value: 'small', raw: '"small"' }, + { kind: 'literal', value: '', raw: "''" }, + { kind: 'literal', value: 'a|b', raw: "'a|b'" }, + { kind: 'literal', value: 'a}b', raw: "'a}b'" }, + ], + }); + }); + + it('parses string literals independently of the named-type catalog', () => { + expect(parseDocParamType(new Set(), "'heading'")).toEqual({ + kind: 'literal', + value: 'heading', + raw: "'heading'", + }); + expect(parseDocParamType(new Set(), 'product')).toBeUndefined(); + }); + + it.each([ + '', + 'unknown', + 'unknown[]', + 'String', + ' string ', + 'string[][]', + 'string | number', + "'heading' |", + "'heading' | number", + "'heading'[]", + "'heading' | 'small", + '1 | 2', + ])('rejects unsupported or malformed type %s', (value) => { + expect(parseDocParamType(validParamTypes, value)).toBeUndefined(); + }); + + it('preserves the legacy named-type tuple API', () => { + expect(parseParamType(validParamTypes, 'product')).toEqual(['product', false]); + expect(parseParamType(validParamTypes, 'product[]')).toEqual(['product', true]); + expect(parseParamType(validParamTypes, "'heading' | 'small'")).toBeUndefined(); + }); +}); + +describe('parseStringLiterals', () => { + it.each([ + ["'heading' | 'small'", ['heading', 'small'], ["'heading'", "'small'"]], + [` "Heading"|'small' `, ['Heading', 'small'], ['"Heading"', "'small'"]], + ["'heading'", ['heading'], ["'heading'"]], + ["'' | ' small '", ['', ' small '], ["''", "' small '"]], + ["'a|b'\t|\t'a}b'", ['a|b', 'a}b'], ["'a|b'", "'a}b'"]], + [`"it's" | 'say "hi"'`, ["it's", 'say "hi"'], [`"it's"`, `'say "hi"'`]], + [String.raw`'a\nb' | 'a\'`, [String.raw`a\nb`, 'a\\'], [String.raw`'a\nb'`, "'a\\'"]], + ["'heading' | 'heading'", ['heading', 'heading'], ["'heading'", "'heading'"]], + ])('parses %s without changing literal values', (type, values, raw) => { + const literals = parseStringLiterals(type as string); + expect(literals?.map((literal) => literal.value)).toEqual(values); + expect(literals?.map((literal) => literal.raw)).toEqual(raw); + }); + + it.each([ + '', + ' ', + 'string', + 'product[]', + 'heading | small', + "'heading' |", + "'heading' | ", + "| 'heading'", + "'heading' || 'small'", + "'heading' 'small'", + "'heading' | small", + "'heading' | 'small", + "'heading' trailing", + "'heading' | number", + '1 | 2', + 'true | false', + "('heading' | 'small')", + "'heading'[]", + "'heading' |\n'small'", + "'heading\rsmall'", + String.raw`'it\'s'`, + ])('rejects the entire invalid annotation %s', (type) => { + expect(parseStringLiterals(type)).toBeUndefined(); + }); +}); diff --git a/packages/theme-check-common/src/liquid-doc/doc-param-type.ts b/packages/theme-check-common/src/liquid-doc/doc-param-type.ts new file mode 100644 index 000000000..47710abc8 --- /dev/null +++ b/packages/theme-check-common/src/liquid-doc/doc-param-type.ts @@ -0,0 +1,93 @@ +/** A string literal type, such as `'heading'`. `raw` keeps the quotes it was written with. */ +export interface StringLiteralType { + kind: 'literal'; + value: string; + raw: string; +} + +export type DocParamType = + | { kind: 'named'; name: string } + | { kind: 'array'; valueType: string } + | StringLiteralType + | { kind: 'union'; types: DocParamType[] }; + +/** Parses a supported LiquidDoc type while preserving its full type information. */ +export function parseDocParamType( + validParamTypes: Set, + value: string, +): DocParamType | undefined { + const literals = parseStringLiterals(value); + if (literals) return literals.length === 1 ? literals[0] : { kind: 'union', types: literals }; + + const namedType = parseParamType(validParamTypes, value); + if (!namedType) return undefined; + + const [name, isArray] = namedType; + return isArray ? { kind: 'array', valueType: name } : { kind: 'named', name }; +} + +/** + * Parses a type that only allows string literals, such as `'heading' | 'small'`, + * preserving their spelling for fixes. Liquid strings do not decode backslash + * escapes. Only an unquoted pipe separates literals, and the entire annotation + * must be valid. + */ +export function parseStringLiterals(type: string): StringLiteralType[] | undefined { + if (/[\r\n]/.test(type)) return undefined; + + const literals: StringLiteralType[] = []; + let position = 0; + + function skipWhitespace() { + while (type[position] === ' ' || type[position] === '\t') position++; + } + + while (position < type.length) { + skipWhitespace(); + const quote = type[position]; + if (quote !== "'" && quote !== '"') return undefined; + + const end = type.indexOf(quote, position + 1); + if (end === -1) return undefined; + + literals.push({ + kind: 'literal', + value: type.slice(position + 1, end), + raw: type.slice(position, end + 1), + }); + position = end + 1; + skipWhitespace(); + + if (position === type.length) return literals; + if (type[position] !== '|') return undefined; + position++; + } + + return undefined; +} + +/** Legacy tuple API for named types and arrays; string literals require parseDocParamType. */ +export function parseParamType( + validParamTypes: Set, + value: string, +): [pseudoType: string, isArray: boolean] | undefined { + const parsedParamType = parseParamTypeSyntax(value); + + if (!parsedParamType || !validParamTypes.has(parsedParamType[0])) return undefined; + + return parsedParamType; +} + +/** + * Splits a lowercase LiquidDoc type such as `product[]` into its base type + * and array flag. Returns undefined when the value is not valid type syntax. + */ +export function parseParamTypeSyntax( + value: string, +): [pseudoType: string, isArray: boolean] | undefined { + const paramTypeMatch = value.match(/^([a-z_]+)(\[\])?$/); + + if (!paramTypeMatch) return undefined; + + return [paramTypeMatch[1], !!paramTypeMatch[2]]; +} diff --git a/packages/theme-check-common/src/liquid-doc/liquidDoc.spec.ts b/packages/theme-check-common/src/liquid-doc/liquidDoc.spec.ts index 24a941efe..59e686d33 100644 --- a/packages/theme-check-common/src/liquid-doc/liquidDoc.spec.ts +++ b/packages/theme-check-common/src/liquid-doc/liquidDoc.spec.ts @@ -10,6 +10,22 @@ describe('Unit: extractDocDefinition', () => { return toSourceCode(uri, code).ast as LiquidHtmlNode; } + it('preserves string enum spelling and optionality when extracting parameters', () => { + const source = `{% doc %} + Renders the block's content as text. + @param {'Heading' | "small"} [variant] - Shared text style, added as a text-- class; plain text when omitted +{% enddoc %}`; + expect(extractDocDefinition(uri, toAST(source)).liquidDoc?.parameters).toEqual([ + { + name: 'variant', + description: 'Shared text style, added as a text-- class; plain text when omitted', + type: `'Heading' | "small"`, + required: false, + nodeType: 'param', + }, + ]); + }); + it('should return default doc definition if no renderable content is present', async () => { const ast = toAST(` {% doc %} diff --git a/packages/theme-check-common/src/liquid-doc/utils.ts b/packages/theme-check-common/src/liquid-doc/utils.ts index 2285a32e1..1bbf04688 100644 --- a/packages/theme-check-common/src/liquid-doc/utils.ts +++ b/packages/theme-check-common/src/liquid-doc/utils.ts @@ -4,6 +4,14 @@ import { isSnippet } from '../to-schema'; import { isBlock } from '../to-schema'; import { ObjectEntry, UriString } from '../types'; +export { + parseDocParamType, + parseParamType, + parseParamTypeSyntax, + parseStringLiterals, +} from './doc-param-type'; +export type { DocParamType, StringLiteralType } from './doc-param-type'; + /** * The base set of supported param types for LiquidDoc. * @@ -107,28 +115,3 @@ export function getValidParamTypes(objectEntries: ObjectEntry[]): Map, - value: string, -): [pseudoType: string, isArray: boolean] | undefined { - const parsedParamType = parseParamTypeSyntax(value); - - if (!parsedParamType || !validParamTypes.has(parsedParamType[0])) return undefined; - - return parsedParamType; -} - -/** - * Splits a lowercase LiquidDoc type such as `product[]` into its base type - * and array flag. Returns undefined when the value is not valid type syntax. - */ -export function parseParamTypeSyntax( - value: string, -): [pseudoType: string, isArray: boolean] | undefined { - const paramTypeMatch = value.match(/^([a-z_]+)(\[\])?$/); - - if (!paramTypeMatch) return undefined; - - return [paramTypeMatch[1], !!paramTypeMatch[2]]; -} From 7c8b9d418f31d3c47db4427b40cf079eb686a285 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Claud=C3=A9ric=20Demers?= Date: Tue, 29 Sep 2026 15:07:48 -0400 Subject: [PATCH 3/4] Validate arguments against LiquidDoc string enums Co-Authored-By: Claude Opus 5.5 (1M context) --- .changeset/liquiddoc-string-enums.md | 2 +- .../src/block-parameters.ts | 12 +- .../valid-block-argument-types/index.spec.ts | 10 + .../valid-block-argument-types/index.ts | 32 ++- .../index.ts | 66 +++-- .../src/liquid-doc/arguments.ts | 43 ++- .../src/liquid-doc/enum-arguments.spec.ts | 270 ++++++++++++++++++ .../src/liquid-doc/utils.spec.ts | 49 +++- .../src/liquid-doc/utils.ts | 46 ++- 9 files changed, 471 insertions(+), 59 deletions(-) create mode 100644 packages/theme-check-common/src/liquid-doc/enum-arguments.spec.ts diff --git a/.changeset/liquiddoc-string-enums.md b/.changeset/liquiddoc-string-enums.md index fc0b2bad4..07aee6572 100644 --- a/.changeset/liquiddoc-string-enums.md +++ b/.changeset/liquiddoc-string-enums.md @@ -4,4 +4,4 @@ Support string enums in LiquidDoc parameter types -`@param` accepts unions of string literals, such as `{'heading' | 'small'}`. +`@param` accepts unions of string literals, such as `{'heading' | 'small'}`, and literal arguments to `render`, `content_for` and `block` are checked against them. diff --git a/packages/theme-check-common/src/block-parameters.ts b/packages/theme-check-common/src/block-parameters.ts index 3eae1d9f9..4a78fc7ab 100644 --- a/packages/theme-check-common/src/block-parameters.ts +++ b/packages/theme-check-common/src/block-parameters.ts @@ -1,5 +1,5 @@ import type { DocDefinition, LiquidDocParameter } from './liquid-doc/liquidDoc'; -import { parseParamTypeSyntax } from './liquid-doc/utils'; +import { parseParamTypeSyntax, parseStringLiterals } from './liquid-doc/utils'; import { schemaSettingLiquidType } from './schema-settings'; import { hasNoSchemaTag } from './to-schema'; import type { Dependencies, Setting, ThemeBlock } from './types'; @@ -13,8 +13,8 @@ export const BLOCK_CONTENT_PARAMETER = 'content'; export interface BlockParameter { name: string; /** - * The LiquidDoc-syntax type the caller passes, such as `string` or - * `product[]`. Undefined when the declaration is untyped or unmapped. + * The LiquidDoc-syntax type the caller passes, such as `string`, `product[]` + * or `'heading' | 'small'`. Undefined when the declaration is untyped or unmapped. */ type?: string; /** @@ -109,7 +109,11 @@ function withLiquidDoc( return { name: liquidDoc.name, - type: liquidDocType(liquidDoc.type), + // String literal types keep their spelling, since arguments must match them exactly. + type: + liquidDoc.type && parseStringLiterals(liquidDoc.type) + ? liquidDoc.type.trim() + : liquidDocType(liquidDoc.type), required: liquidDoc.required, liquidDoc, }; diff --git a/packages/theme-check-common/src/checks/valid-block-argument-types/index.spec.ts b/packages/theme-check-common/src/checks/valid-block-argument-types/index.spec.ts index b28cdb258..d3e71bbd8 100644 --- a/packages/theme-check-common/src/checks/valid-block-argument-types/index.spec.ts +++ b/packages/theme-check-common/src/checks/valid-block-argument-types/index.spec.ts @@ -245,6 +245,16 @@ describe('ValidBlockArgumentTypes', () => { expect(offenses).toEqual([]); }); + it('keeps the schema type of a setting that LiquidDoc declares as a string enum', async () => { + const block = blockSource( + [{ id: 'variant', type: 'text' }], + ["@param {'heading' | 'small'} [variant] - Variant"], + ); + + expect(await definitions(block)).toEqual([]); + expect(await run("{% block 'card', variant: 'body' %}{% endblock %}", block)).toEqual([]); + }); + it('does not speculate about omitted LiquidDoc or unmapped schema types', async () => { const offenses = await definitions( blockSource([{ id: 'item', type: 'metaobject' }], ['@param [item] - Item']), diff --git a/packages/theme-check-common/src/checks/valid-block-argument-types/index.ts b/packages/theme-check-common/src/checks/valid-block-argument-types/index.ts index 83e11af8c..fdc1871bf 100644 --- a/packages/theme-check-common/src/checks/valid-block-argument-types/index.ts +++ b/packages/theme-check-common/src/checks/valid-block-argument-types/index.ts @@ -4,7 +4,15 @@ import { type BlockParameters, liquidDocType, } from '../../block-parameters'; -import { BasicParamTypes, inferArgumentType, isTypeCompatible } from '../../liquid-doc/utils'; +import { generateTypeMismatchSuggestions } from '../../liquid-doc/arguments'; +import { + BasicParamTypes, + getArgumentTypeMismatchMessage, + inferArgumentType, + isArgumentTypeCompatible, + isTypeCompatible, + parseStringLiterals, +} from '../../liquid-doc/utils'; import * as path from '../../path'; import { isBlock } from '../../to-schema'; import { Severity, SourceCodeType, type Context, type LiquidCheckDefinition } from '../../types'; @@ -50,6 +58,11 @@ export const ValidBlockArgumentTypes: LiquidCheckDefinition = { for (const argument of markup.args) { const expectedType = parameters.get(argument.name)?.type; + if (expectedType && parseStringLiterals(expectedType)) { + reportStringLiteralMismatch(context, argument, expectedType); + continue; + } + const actualType = literalType(argument.value); if (!expectedType || !actualType || isCallTypeCompatible(expectedType, actualType)) { continue; @@ -103,6 +116,23 @@ function isCallTypeCompatible(expectedType: string, actualType: string): boolean return isTypeCompatible(expectedType, actualType as BasicParamTypes); } +/** String literal types accept only their own values, so literals are compared by value. */ +function reportStringLiteralMismatch( + context: Context, + argument: BlockMarkup['args'][number], + expectedType: string, +): void { + if (isArgumentTypeCompatible(expectedType, argument.value) !== false) return; + + const { start, end } = argument.value.position; + context.report({ + message: getArgumentTypeMismatchMessage(argument.name, expectedType, argument.value), + startIndex: start, + endIndex: end, + suggest: generateTypeMismatchSuggestions(expectedType, start, end), + }); +} + function reportLiquidDocTypeMismatch( context: Context, node: LiquidDocParamNode, diff --git a/packages/theme-check-common/src/checks/valid-render-snippet-argument-types/index.ts b/packages/theme-check-common/src/checks/valid-render-snippet-argument-types/index.ts index 5fae39b1d..79e0be724 100644 --- a/packages/theme-check-common/src/checks/valid-render-snippet-argument-types/index.ts +++ b/packages/theme-check-common/src/checks/valid-render-snippet-argument-types/index.ts @@ -1,7 +1,12 @@ import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; import { NodeTypes, RenderMarkup } from '@shopify/liquid-html-parser'; import { LiquidDocParameter } from '../../liquid-doc/liquidDoc'; -import { inferArgumentType, isTypeCompatible } from '../../liquid-doc/utils'; +import { + getArgumentTypeMismatchMessage, + getValidParamTypes, + isArgumentTypeCompatible, + parseParamType, +} from '../../liquid-doc/utils'; import { findTypeMismatchParams, generateTypeMismatchSuggestions, @@ -28,42 +33,45 @@ export const ValidRenderSnippetArgumentTypes: LiquidCheckDefinition = { }, create(context) { + let validParamTypesPromise: Promise> | undefined; + /** * Checks for type mismatches when alias is used with `for` or `with` syntax. - * This can be refactored at a later date to share more code with regular named arguments as they are both backed by LiquidExpression nodes. - * * E.g. {% render 'card' with 123 as title %} */ - function findAndReportAliasType( + async function findAndReportAliasType( node: RenderMarkup, liquidDocParameters: Map, ) { - if ( - node.alias && - node.variable?.name && - node.variable.name.type !== NodeTypes.VariableLookup - ) { - const paramIsDefinedWithType = liquidDocParameters - .get(node.alias.value) - ?.type?.toLowerCase(); - if (paramIsDefinedWithType) { - const providedParamType = inferArgumentType(node.variable.name); - if (!isTypeCompatible(paramIsDefinedWithType, providedParamType)) { - const suggestions = generateTypeMismatchSuggestions( - paramIsDefinedWithType, - node.variable.name.position.start, - node.variable.name.position.end, - ); + if (!node.alias || !node.variable?.name) return; + + const expectedType = liquidDocParameters.get(node.alias.value)?.type; + const argument = node.variable.name; + if (!expectedType || argument.type === NodeTypes.VariableLookup) return; - context.report({ - message: `Type mismatch for argument '${node.alias.value}': expected ${paramIsDefinedWithType}, got ${providedParamType}`, - startIndex: node.variable.name.position.start, - endIndex: node.variable.name.position.end, - suggest: suggestions, - }); - } - } + const compatibility = isArgumentTypeCompatible(expectedType, argument); + if (compatibility === true) return; + + if (compatibility === undefined) { + // Aliases also check named Liquid types and arrays. Validate the + // declaration first so malformed types do not cause a second error. + if (!context.themeDocset) return; + validParamTypesPromise ??= context.themeDocset + .liquidDrops() + .then((entries) => new Set(getValidParamTypes(entries).keys())); + if (!parseParamType(await validParamTypesPromise, expectedType)) return; } + + context.report({ + message: getArgumentTypeMismatchMessage(node.alias.value, expectedType, argument), + startIndex: argument.position.start, + endIndex: argument.position.end, + suggest: generateTypeMismatchSuggestions( + expectedType, + argument.position.start, + argument.position.end, + ), + }); } return { @@ -79,7 +87,7 @@ export const ValidRenderSnippetArgumentTypes: LiquidCheckDefinition = { if (!liquidDocParameters) return; - findAndReportAliasType(node, liquidDocParameters); + await findAndReportAliasType(node, liquidDocParameters); const typeMismatchParams = findTypeMismatchParams(liquidDocParameters, node.args); reportTypeMismatches(context, typeMismatchParams, liquidDocParameters); diff --git a/packages/theme-check-common/src/liquid-doc/arguments.ts b/packages/theme-check-common/src/liquid-doc/arguments.ts index 02bb65256..3f3be6ca5 100644 --- a/packages/theme-check-common/src/liquid-doc/arguments.ts +++ b/packages/theme-check-common/src/liquid-doc/arguments.ts @@ -10,10 +10,10 @@ import { } from '@shopify/liquid-html-parser'; import { Context, LiquidDocParameter, SourceCodeType, StringCorrector } from '..'; import { - BasicParamTypes, + getArgumentTypeMismatchMessage, getDefaultValueForType, - inferArgumentType, - isTypeCompatible, + isArgumentTypeCompatible, + parseStringLiterals, } from './utils'; import { isLiquidString } from '../checks/utils'; @@ -118,21 +118,12 @@ export function findTypeMismatchParams( const typeMismatchParams: LiquidNamedArgument[] = []; for (const arg of providedParams) { - if (arg.value.type === NodeTypes.VariableLookup) { - continue; - } - 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; - } - - if (!isTypeCompatible(paramType, inferArgumentType(arg.value))) { - typeMismatchParams.push(arg); - } + if ( + liquidDocParamDef?.type && + isArgumentTypeCompatible(liquidDocParamDef.type, arg.value) === false + ) { + typeMismatchParams.push(arg); } } @@ -151,17 +142,14 @@ export function reportTypeMismatches( const paramDef = liquidDocParameters.get(arg.name); if (!paramDef || !paramDef.type) continue; - const expectedType = paramDef.type.toLowerCase(); - const actualType = inferArgumentType(arg.value); - const suggestions = generateTypeMismatchSuggestions( - expectedType, + paramDef.type, arg.value.position.start, arg.value.position.end, ); context.report({ - message: `Type mismatch for argument '${arg.name}': expected ${expectedType}, got ${actualType}`, + message: getArgumentTypeMismatchMessage(arg.name, paramDef.type, arg.value), startIndex: arg.value.position.start, endIndex: arg.value.position.end, suggest: suggestions, @@ -177,6 +165,17 @@ export function generateTypeMismatchSuggestions( startPosition: number, endPosition: number, ) { + const literals = parseStringLiterals(expectedType); + if (literals) { + return literals.map((literal) => ({ + message: `Replace with ${literal.raw}`, + fix: (fixer: StringCorrector) => { + return fixer.replace(startPosition, endPosition, literal.raw); + }, + })); + } + + expectedType = expectedType.toLowerCase(); const defaultValue = getDefaultValueForType(expectedType); const suggestions = []; diff --git a/packages/theme-check-common/src/liquid-doc/enum-arguments.spec.ts b/packages/theme-check-common/src/liquid-doc/enum-arguments.spec.ts new file mode 100644 index 000000000..6a8f3c485 --- /dev/null +++ b/packages/theme-check-common/src/liquid-doc/enum-arguments.spec.ts @@ -0,0 +1,270 @@ +import { describe, expect, it } from 'vitest'; +import { toLiquidHtmlAST } from '@shopify/liquid-html-parser'; +import { ValidRenderSnippetArgumentTypes } from '../checks/valid-render-snippet-argument-types'; +import { ValidContentForArgumentTypes } from '../checks/valid-content-for-argument-types'; +import { ValidBlockArgumentTypes } from '../checks/valid-block-argument-types'; +import { MissingRenderSnippetArguments } from '../checks/missing-render-snippet-arguments'; +import { MissingContentForArguments } from '../checks/missing-content-for-arguments'; +import { MissingBlockArguments } from '../checks/missing-block-arguments'; +import { applySuggestions, runLiquidCheck } from '../test'; + +const enumType = `'heading' | "small"`; + +function definition(type = enumType, optional = true) { + return `{% doc %} + @param {${type}} ${optional ? '[variant]' : 'variant'} - Text style + {% enddoc %} + {{ variant }}`; +} + +const namedCallers = [ + { + name: 'render', + check: ValidRenderSnippetArgumentTypes, + missingCheck: MissingRenderSnippetArguments, + file: 'snippets/text.liquid', + source: (value?: string) => `{% render 'text'${value ? `, variant: ${value}` : ''} %}`, + }, + { + name: 'content_for', + check: ValidContentForArgumentTypes, + missingCheck: MissingContentForArguments, + file: 'blocks/text.liquid', + source: (value?: string) => + `{% content_for 'block', type: 'text', id: 'text'${value ? `, variant: ${value}` : ''} %}`, + }, + { + name: 'block', + check: ValidBlockArgumentTypes, + missingCheck: MissingBlockArguments, + file: 'blocks/text.liquid', + source: (value?: string) => + `{% block 'text'${value ? `, variant: ${value}` : ''} %}Text{% endblock %}`, + }, +]; + +const aliasCallers = ['with', 'for'].map((keyword) => ({ + name: `render ${keyword} alias`, + check: ValidRenderSnippetArgumentTypes, + file: 'snippets/text.liquid', + source: (value: string) => `{% render 'text' ${keyword} ${value} as variant %}`, +})); + +describe('LiquidDoc enum arguments', () => { + for (const caller of [...namedCallers, ...aliasCallers]) { + describe(caller.name, () => { + it.each(["'heading'", '"heading"', "'small'", '"small"'])( + 'accepts enum member %s', + async (value) => { + const offenses = await runLiquidCheck( + caller.check, + caller.source(value), + 'templates/index.liquid', + {}, + { [caller.file]: definition() }, + ); + expect(offenses).toHaveLength(0); + }, + ); + + it.each(["'Heading'", "'body'", '123', 'false', 'nil', 'null', 'empty', 'blank', '(1..3)'])( + 'rejects literal %s and highlights only the value', + async (value) => { + const source = caller.source(value); + const offenses = await runLiquidCheck( + caller.check, + source, + 'templates/index.liquid', + {}, + { [caller.file]: definition() }, + ); + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toContain(enumType); + expect(offenses[0].message).toContain(`got ${value}`); + expect(source.slice(offenses[0].start.index, offenses[0].end.index)).toBe(value); + }, + ); + + it.each(['style', 'block.settings.style', "settings['style']"])( + 'leaves dynamic value %s unverified', + async (value) => { + const offenses = await runLiquidCheck( + caller.check, + caller.source(value), + 'templates/index.liquid', + {}, + { [caller.file]: definition() }, + ); + expect(offenses).toHaveLength(0); + }, + ); + + it.each(["'heading' |", "'heading' | number"])( + 'does not cascade errors from unsupported declaration %s', + async (type) => { + const offenses = await runLiquidCheck( + caller.check, + caller.source("'body'"), + 'templates/index.liquid', + {}, + { [caller.file]: definition(type) }, + ); + expect(offenses).toHaveLength(0); + }, + ); + + it('offers each allowed member as a replacement and preserves its quoting', async () => { + const source = caller.source("'body'"); + const offenses = await runLiquidCheck( + caller.check, + source, + 'templates/index.liquid', + {}, + { [caller.file]: definition() }, + ); + + expect(offenses).toHaveLength(1); + expect(offenses[0].suggest?.map((suggestion) => suggestion.message)).toEqual([ + "Replace with 'heading'", + 'Replace with "small"', + ]); + const suggestions = applySuggestions(source, offenses[0]); + expect(suggestions).toEqual([caller.source("'heading'"), caller.source('"small"')]); + for (const suggestion of suggestions ?? []) { + expect(() => toLiquidHtmlAST(suggestion)).not.toThrow(); + expect( + await runLiquidCheck( + caller.check, + suggestion, + 'templates/index.liquid', + {}, + { + [caller.file]: definition(), + }, + ), + ).toHaveLength(0); + } + }); + }); + } + + for (const caller of namedCallers) { + it(`${caller.name} permits omission of an optional enum`, async () => { + const offenses = await runLiquidCheck( + caller.missingCheck, + caller.source(), + 'templates/index.liquid', + {}, + { [caller.file]: definition() }, + ); + expect(offenses).toHaveLength(0); + }); + + it(`${caller.name} reports omission of a required enum`, async () => { + const offenses = await runLiquidCheck( + caller.missingCheck, + caller.source(), + 'templates/index.liquid', + {}, + { [caller.file]: definition(enumType, false) }, + ); + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toContain("Missing required argument 'variant'"); + }); + } + + for (const caller of aliasCallers) { + it.each(['product', 'product[]', 'string[]'])( + `${caller.name} preserves checking of literal values against named declaration %s`, + async (type) => { + const offenses = await runLiquidCheck( + caller.check, + caller.source('123'), + 'templates/index.liquid', + {}, + { [caller.file]: definition(type) }, + ); + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toBe( + `Type mismatch for argument 'variant': expected ${type}, got number`, + ); + }, + ); + + it('leaves dynamic alias values unverified for named declarations', async () => { + const offenses = await runLiquidCheck( + caller.check, + caller.source('product'), + 'templates/index.liquid', + {}, + { [caller.file]: definition('product') }, + ); + expect(offenses).toHaveLength(0); + }); + } + + for (const caller of namedCallers.filter((caller) => caller.name !== 'block')) { + it.each(['product', 'product[]', 'string[]', 'unknown'])( + `${caller.name} preserves existing skips for named declaration %s`, + async (type) => { + const offenses = await runLiquidCheck( + caller.check, + caller.source("'heading'"), + 'templates/index.liquid', + {}, + { [caller.file]: definition(type) }, + ); + expect(offenses).toHaveLength(0); + }, + ); + + it.each([`"Heading" | 'small'`, `'' | 'heading'`, `"a'b" | 'small'`])( + `${caller.name} inserts a legal enum member for a missing required argument (%s)`, + async (type) => { + const source = caller.source(); + const firstMember = type.split(' | ')[0]; + const offenses = await runLiquidCheck( + caller.missingCheck, + source, + 'templates/index.liquid', + {}, + { [caller.file]: definition(type, false) }, + ); + + expect(offenses).toHaveLength(1); + const suggestions = applySuggestions(source, offenses[0]); + expect(suggestions).toEqual([caller.source(firstMember)]); + for (const suggestion of suggestions ?? []) { + expect(() => toLiquidHtmlAST(suggestion)).not.toThrow(); + expect( + await runLiquidCheck( + caller.check, + suggestion, + 'templates/index.liquid', + {}, + { + [caller.file]: definition(type, false), + }, + ), + ).toHaveLength(0); + } + }, + ); + } + + it('rejects a block array literal for a string enum without trying to infer its primitive type', async () => { + const source = `{% block 'text', variant: ['heading', 'small'] %}Text{% endblock %}`; + const offenses = await runLiquidCheck( + ValidBlockArgumentTypes, + source, + 'templates/index.liquid', + {}, + { 'blocks/text.liquid': definition() }, + ); + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toContain("got ['heading', 'small']"); + expect(source.slice(offenses[0].start.index, offenses[0].end.index)).toBe( + "['heading', 'small']", + ); + }); +}); 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..5fe43621b 100644 --- a/packages/theme-check-common/src/liquid-doc/utils.spec.ts +++ b/packages/theme-check-common/src/liquid-doc/utils.spec.ts @@ -1,7 +1,54 @@ import { describe, expect, it } from 'vitest'; -import { BasicParamTypes, parseParamType } from './utils'; +import { LiquidTagRender, toLiquidHtmlAST } from '@shopify/liquid-html-parser'; +import { + BasicParamTypes, + getDefaultValueForType, + isArgumentTypeCompatible, + parseParamType, +} from './utils'; describe('liquid-doc/utils', () => { + describe('getDefaultValueForType', () => { + it.each([ + ["'Heading' | 'small'", "'Heading'"], + [` "it's" | 'small' `, `"it's"`], + ["'' | 'small'", "''"], + ["'heading' |", ''], + ['string', "''"], + ['NUMBER', '0'], + ['boolean', 'false'], + ['object', ''], + [null, ''], + ])('suggests a valid literal for %s', (type, expected) => { + expect(getDefaultValueForType(type)).toBe(expected); + }); + }); + + describe('isArgumentTypeCompatible', () => { + it.each([ + ["'heading' | 'small'", "'heading'", true], + ["'heading' | 'small'", '"small"', true], + ["'heading' | 'small'", "'Heading'", false], + ["'heading' | 'small'", "'large'", false], + ["'heading' | 'small'", '42', false], + ["'heading' | 'small'", 'nil', false], + ["'heading' | 'small'", 'true', false], + ["'heading' | 'small'", '(1..3)', false], + ["'heading' | 'small'", 'block.settings.variant', undefined], + ["'heading' |", "'small'", undefined], + ['product', '42', undefined], + ['string[]', "'heading'", undefined], + ['String', "'heading'", true], + ['NUMBER', '42', true], + ['boolean', "'heading'", true], + ['string', '42', false], + ] as const)('checks %s against %s', (type, value, expected) => { + const ast = toLiquidHtmlAST(`{% render 'text', variant: ${value} %}`); + const render = ast.children[0] as LiquidTagRender; + expect(isArgumentTypeCompatible(type, render.markup.args[0].value)).toBe(expected); + }); + }); + describe('parseParamType', () => { const validParamTypes = new Set([...Object.values(BasicParamTypes), 'product']); diff --git a/packages/theme-check-common/src/liquid-doc/utils.ts b/packages/theme-check-common/src/liquid-doc/utils.ts index 1bbf04688..eaf9d1632 100644 --- a/packages/theme-check-common/src/liquid-doc/utils.ts +++ b/packages/theme-check-common/src/liquid-doc/utils.ts @@ -1,8 +1,9 @@ -import { LiquidExpression, NodeTypes } from '@shopify/liquid-html-parser'; +import { BlockArrayLiteral, LiquidExpression, NodeTypes } from '@shopify/liquid-html-parser'; import { assertNever } from '../utils'; import { isSnippet } from '../to-schema'; import { isBlock } from '../to-schema'; import { ObjectEntry, UriString } from '../types'; +import { parseStringLiterals } from './doc-param-type'; export { parseDocParamType, @@ -37,6 +38,9 @@ export enum SupportedDocTagTypes { * Provides a default completion value for an argument / parameter of a given type. */ export function getDefaultValueForType(type: string | null) { + const literals = type ? parseStringLiterals(type) : undefined; + if (literals) return literals[0].raw; + switch (type?.toLowerCase()) { case BasicParamTypes.String: return "''"; @@ -85,6 +89,46 @@ export function isTypeCompatible(expectedType: string, actualType: BasicParamTyp return normalizedExpectedType === actualType; } +/** + * Checks literal values against a documented type. Undefined means that the + * value is dynamic or the declaration is outside the types we can check. + */ +export function isArgumentTypeCompatible( + expectedType: string, + argument: LiquidExpression | BlockArrayLiteral, +): boolean | undefined { + if (argument.type === NodeTypes.VariableLookup) return undefined; + + const literals = parseStringLiterals(expectedType); + if (literals) { + return ( + argument.type === NodeTypes.String && + literals.some((literal) => literal.value === argument.value) + ); + } + + if (argument.type === 'BlockArrayLiteral') return undefined; + + const normalizedType = expectedType.toLowerCase(); + if (!Object.values(BasicParamTypes).some((type) => type === normalizedType)) return undefined; + + return isTypeCompatible(normalizedType, inferArgumentType(argument)); +} + +export function getArgumentTypeMismatchMessage( + name: string, + expectedType: string, + argument: LiquidExpression | BlockArrayLiteral, +): string { + if (parseStringLiterals(expectedType)) { + const actualValue = argument.source.slice(argument.position.start, argument.position.end); + return `Invalid value for argument '${name}': expected ${expectedType.trim()}, got ${actualValue}`; + } + + const actualType = argument.type === 'BlockArrayLiteral' ? 'array' : inferArgumentType(argument); + return `Type mismatch for argument '${name}': expected ${expectedType.toLowerCase()}, got ${actualType}`; +} + /** * Checks if the provided file path supports the LiquidDoc tag. */ From 78895cde49ab6bb8e5113ca116e3a7c34ee6612b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Claud=C3=A9ric=20Demers?= Date: Tue, 29 Sep 2026 15:08:55 -0400 Subject: [PATCH 4/4] Support LiquidDoc string enums in the language server Co-Authored-By: Claude Opus 5.5 (1M context) --- .changeset/liquiddoc-string-enums-editor.md | 7 + .../src/TypeSystem.spec.ts | 225 +++++++++++++++++- .../src/TypeSystem.ts | 152 +++++++++--- .../BlockParameterCompletionProvider.spec.ts | 20 ++ .../FilterCompletionProvider.spec.ts | 13 + .../providers/FilterCompletionProvider.ts | 4 +- .../ObjectAttributeCompletionProvider.spec.ts | 13 + .../ObjectAttributeCompletionProvider.ts | 6 +- .../ObjectCompletionProvider.spec.ts | 18 ++ .../common/CompletionItemProperties.ts | 6 +- .../src/diagnostics/runChecks.spec.ts | 67 ++++++ .../src/docset/MarkdownRenderer.spec.ts | 47 ++++ .../src/docset/MarkdownRenderer.ts | 26 +- ...LiquidObjectAttributeHoverProvider.spec.ts | 13 + .../LiquidObjectAttributeHoverProvider.ts | 11 +- .../LiquidObjectHoverProvider.spec.ts | 13 + .../providers/LiquidObjectHoverProvider.ts | 4 +- .../src/utils/liquidDoc.spec.ts | 65 ++++- .../src/utils/liquidDoc.ts | 22 +- 19 files changed, 667 insertions(+), 65 deletions(-) create mode 100644 .changeset/liquiddoc-string-enums-editor.md diff --git a/.changeset/liquiddoc-string-enums-editor.md b/.changeset/liquiddoc-string-enums-editor.md new file mode 100644 index 000000000..b83f8e6ac --- /dev/null +++ b/.changeset/liquiddoc-string-enums-editor.md @@ -0,0 +1,7 @@ +--- +'@shopify/theme-language-server-common': minor +--- + +Support LiquidDoc string enums in hovers and completions + +Allowed values are kept through assignments and `default`, and `render`, `content_for` and `block` parameter completions insert the first one. diff --git a/packages/theme-language-server-common/src/TypeSystem.spec.ts b/packages/theme-language-server-common/src/TypeSystem.spec.ts index bc5f7678a..222f1a0df 100644 --- a/packages/theme-language-server-common/src/TypeSystem.spec.ts +++ b/packages/theme-language-server-common/src/TypeSystem.spec.ts @@ -14,14 +14,21 @@ import { BasicParamTypes, ObjectEntry, SourceCodeType, + StringLiteralType, visit, } from '@shopify/theme-check-common'; import { assert, beforeEach, describe, expect, it, vi } from 'vitest'; import { URI } from 'vscode-uri'; import { SettingsSchemaJSONFile } from './settings'; -import { ArrayType, TypeSystem } from './TypeSystem'; +import { ArrayType, InferredType, TypeSystem } from './TypeSystem'; import { isLiquidVariableOutput, isNamedLiquidTag } from './utils'; +const literal = (raw: string): StringLiteralType => ({ + kind: 'literal', + value: raw.slice(1, -1), + raw, +}); + describe('Module: TypeSystem', () => { let typeSystem: TypeSystem; let settingsProvider: any; @@ -151,6 +158,14 @@ describe('Module: TypeSystem', () => { name: 'size', return_type: [{ type: 'number', name: '' }], }, + { + name: 'upcase', + return_type: [{ type: 'string', name: '' }], + }, + { + name: 'split', + return_type: [{ type: 'array', array_value: 'string' }], + }, ], systemTranslations: async () => ({}), }, @@ -322,6 +337,49 @@ describe('Module: TypeSystem', () => { const inferredType = await typeSystem.inferType(xVariable, ast, 'file:///file.liquid'); expect(inferredType).to.equal('image'); }); + + it.each(["'heading'", 'variant'])( + 'resolves repeated default references with linear work starting from %s', + async (initialValue) => { + const assignmentCount = 12; + const ast = toLiquidHtmlAST(` + {% doc %} + @param {'heading' | 'small'} variant + {% enddoc %} + {% assign x = ${initialValue} %} + ${'{% assign x = x | default: x %}'.repeat(assignmentCount)} + {{ x }} + `); + let filterReads = 0; + for (const child of ast.children) { + if (!isNamedLiquidTag(child, NamedTags.assign)) continue; + const value = child.markup.value; + const filters = value.filters; + Object.defineProperty(value, 'filters', { + enumerable: true, + get() { + filterReads++; + return filters; + }, + }); + } + const output = ast.children.at(-1)!; + assert(isLiquidVariableOutput(output)); + + const inferred = await typeSystem.inferType( + output.markup, + ast, + 'file:///snippets/example.liquid', + ); + + expect(inferred).to.eql( + initialValue === 'variant' + ? { kind: 'union', types: [literal("'heading'"), literal("'small'")] } + : 'string', + ); + expect(filterReads).toBeLessThan(assignmentCount * 10); + }, + ); }); it('should return the type of variables in for loop', async () => { @@ -733,7 +791,7 @@ describe('Module: TypeSystem', () => { }); it('should support path-contextual variable types', async () => { - let inferredType: string | ArrayType; + let inferredType: InferredType; const contexts: [string, string][] = [ ['section', 'sections/my-section.liquid'], ['comment', 'sections/main-article.liquid'], @@ -767,11 +825,16 @@ describe('Module: TypeSystem', () => { }); describe('LiquidDoc inferred type', () => { - const liquidDocParamTypeToTypeMap = { + const liquidDocParamTypeToTypeMap: Record = { [BasicParamTypes.String]: 'string', [BasicParamTypes.Number]: 'number', [BasicParamTypes.Boolean]: 'boolean', [BasicParamTypes.Object]: 'untyped', + "'heading'": literal("'heading'"), + "'heading' | 'small'": { kind: 'union', types: [literal("'heading'"), literal("'small'")] }, + [`'Heading' | "Small"`]: { kind: 'union', types: [literal("'Heading'"), literal('"Small"')] }, + "'heading' |": 'untyped', + "'heading' | number": 'untyped', invalid: 'untyped', }; @@ -833,6 +896,162 @@ describe('Module: TypeSystem', () => { valueType: 'product', }); }); + + describe('string enums', () => { + const enumType: InferredType = { + kind: 'union', + types: [literal("'heading'"), literal('"small"')], + }; + + async function inferOutput(source: string): Promise { + const ast = toLiquidHtmlAST(` + {% doc %} + @param {'heading' | "small"} [variant] + {% enddoc %} + ${source} + `); + const output = ast.children.at(-1)!; + assert(isLiquidVariableOutput(output)); + return typeSystem.inferType(output.markup, ast, 'file:///snippets/example.liquid'); + } + + it('preserves members through chained assignments', async () => { + expect( + await inferOutput(` + {% assign style = variant %} + {% assign copy = style %} + {{ copy }} + `), + ).to.eql(enumType); + }); + + it('replaces the enum when the variable is reassigned', async () => { + expect( + await inferOutput(` + {% assign variant = 1 %} + {{ variant }} + `), + ).to.equal('number'); + }); + + it('keeps the earlier enum type of a copy after reassigning the original', async () => { + expect( + await inferOutput(` + {% assign copy = variant %} + {% assign variant = 'other' %} + {{ copy }} + `), + ).to.eql(enumType); + }); + + it.each([ + ['size', 'number'], + ['first', 'string'], + ['last', 'string'], + ['missing', 'unknown'], + ])('uses string semantics for the %s property', async (property, expected) => { + expect(await inferOutput(`{{ variant.${property} }}`)).to.equal(expected); + }); + + it.each([ + ['size', 'number'], + ['upcase', 'string'], + ['split: ","', { kind: 'array', valueType: 'string' }], + ['unknown_filter', 'untyped'], + ])('uses the return type of the %s filter', async (filter, expected) => { + expect(await inferOutput(`{{ variant | ${filter} }}`)).to.eql(expected); + }); + + it('preserves the enum with an existing member as the default', async () => { + expect(await inferOutput(`{{ variant | default: 'small' }}`)).to.eql(enumType); + }); + + it('includes a new literal default with its original spelling', async () => { + expect(await inferOutput(`{{ variant | default: "Heading" }}`)).to.eql({ + kind: 'union', + types: [...enumType.types, literal('"Heading"')], + }); + }); + + it('merges enum defaults without duplicate values', async () => { + expect( + await inferOutput(` + {% doc %} + @param {'small' | 'other'} fallback + {% enddoc %} + {{ variant | default: fallback }} + `), + ).to.eql({ + kind: 'union', + types: [...enumType.types, literal("'other'")], + }); + }); + + it.each([ + ['variant | default: product.title', 'string'], + ['product.title | default: variant', 'string'], + ['variant | default: 1', 'untyped'], + ['1 | default: variant', 'untyped'], + ['variant | default: unknown', 'untyped'], + ['unknown | default: variant', 'untyped'], + ['variant | upcase | default: variant', 'string'], + ['variant | size | default: variant', 'untyped'], + ['variant | default: "small" | upcase', 'string'], + ])('widens the enum as needed for %s', async (expression, expected) => { + expect(await inferOutput(`{{ ${expression} }}`)).to.equal(expected); + }); + + it('includes a literal input when the default is an enum', async () => { + expect(await inferOutput(`{{ 'other' | default: variant }}`)).to.eql({ + kind: 'union', + types: [literal("'other'"), ...enumType.types], + }); + }); + + it('does not add default values to the original enum', async () => { + const ast = toLiquidHtmlAST(` + {% doc %} + @param {'heading' | "small"} variant + {% enddoc %} + {% assign copy = variant | default: 'other' %} + {{ copy }} + `); + const output = ast.children.at(-1)!; + assert(isLiquidVariableOutput(output)); + assert(typeof output.markup !== 'string'); + const lookup = output.markup.expression; + assert(lookup.type === NodeTypes.VariableLookup); + const variables = await typeSystem.availableVariables( + ast, + '', + lookup, + 'file:///snippets/example.liquid', + ); + expect(variables.find(({ entry }) => entry.name === 'variant')?.type).to.eql(enumType); + expect(variables.find(({ entry }) => entry.name === 'copy')?.type).to.eql({ + kind: 'union', + types: [...enumType.types, literal("'other'")], + }); + }); + + it('does not treat an enum as an array when resolving a loop variable', async () => { + const ast = toLiquidHtmlAST(` + {% doc %} + @param {'heading' | 'small'} variant + {% enddoc %} + {% for item in variant %}{{ item }}{% endfor %} + `); + const loop = ast.children[1]; + assert(isNamedLiquidTag(loop, NamedTags.for)); + const branch = loop.children![0]; + assert(branch.type === NodeTypes.LiquidBranch); + const output = branch.children[0]; + assert(isLiquidVariableOutput(output)); + expect( + await typeSystem.inferType(output.markup, ast, 'file:///snippets/example.liquid'), + ).to.equal('untyped'); + }); + }); }); describe('metafieldDefinitionsObjectMap', async () => { diff --git a/packages/theme-language-server-common/src/TypeSystem.ts b/packages/theme-language-server-common/src/TypeSystem.ts index e5bfc2c8a..641a2748a 100644 --- a/packages/theme-language-server-common/src/TypeSystem.ts +++ b/packages/theme-language-server-common/src/TypeSystem.ts @@ -30,8 +30,9 @@ import { FETCHED_METAFIELD_CATEGORIES, BasicParamTypes, getValidParamTypes, - parseParamType, + parseDocParamType, schemaSettingLiquidType, + StringLiteralType, } from '@shopify/theme-check-common'; import { GetThemeSettingsSchemaForURI, @@ -56,7 +57,7 @@ export class TypeSystem { thing: Identifier | ComplexLiquidExpression | LiquidVariable | AssignMarkup, partialAst: LiquidHtmlNode, uri: string, - ): Promise { + ): Promise { const [objectMap, filtersMap, symbolsTable] = await Promise.all([ this.objectMap(uri, partialAst), this.filtersMap(), @@ -71,7 +72,7 @@ export class TypeSystem { partial: string, node: LiquidVariableLookup, uri: string, - ): Promise<{ entry: DocsetEntry; type: PseudoType | ArrayType }[]> { + ): Promise<{ entry: DocsetEntry; type: InferredType }[]> { const [objectMap, filtersMap, symbolsTable] = await Promise.all([ this.objectMap(uri, partialAst), this.filtersMap(), @@ -87,7 +88,7 @@ export class TypeSystem { .map(([identifier, typeRanges]) => { const typeRange = findLast(typeRanges, (typeRange) => isCorrectTypeRange(typeRange, node))!; const type = resolveTypeRangeType(typeRange.type, symbolsTable, objectMap, filtersMap); - const entry = objectMap[isArrayType(type) ? type.valueType : type] ?? {}; + const entry = objectMap[isArrayType(type) ? type.valueType : getBaseType(type)] ?? {}; return { entry: { ...entry, name: identifier }, type, @@ -420,6 +421,8 @@ type String = typeof String; /** A pseudo-type is the possible values of an ObjectEntry's return_type.type */ export type PseudoType = ObjectEntryName | String | Untyped | Unknown | 'number' | 'boolean'; +export type InferredType = PseudoType | ArrayType | StringLiteralType | UnionType; + /** * A variable can have many types in the same file * @@ -438,7 +441,7 @@ interface TypeRange { identifier: Identifier; /** The type of the variable */ - type: PseudoType | ArrayType | LazyVariableType | LazyDeconstructedExpression; + type: InferredType | LazyVariableType | LazyDeconstructedExpression; /** * The range may be one of two things: @@ -458,6 +461,12 @@ const arrayType = (valueType: PseudoType): ArrayType => ({ valueType, }); +/** A union type (e.g. 'heading' | 'small'). Only string literals can be members for now. */ +export type UnionType = { + kind: 'union'; + types: StringLiteralType[]; +}; + /** * Because a type may depend on another, this represents the type of * something as the type of a LiquidVariable chain. @@ -467,6 +476,7 @@ type LazyVariableType = { kind: NodeTypes.LiquidVariable; node: LiquidVariable; offset: number; + resolvedType?: InferredType; }; const lazyVariable = (node: LiquidVariable, offset: number): LazyVariableType => ({ kind: NodeTypes.LiquidVariable, @@ -487,6 +497,7 @@ type LazyDeconstructedExpression = { kind: 'deconstructed'; node: LiquidExpression; offset: number; + resolvedType?: InferredType; }; const LazyDeconstructedExpression = ( node: LiquidExpression, @@ -634,35 +645,40 @@ function seedBlockVariables( /** * Given a TypeRange['type'] (which may be lazy), resolve its type recursively. * - * The output is a fully resolved PseudoType | ArrayType. Which means we - * could use it to power completions. + * The output is a fully resolved type that can power completions. */ function resolveTypeRangeType( typeRangeType: TypeRange['type'], symbolsTable: SymbolsTable, objectMap: ObjectMap, filtersMap: FiltersMap, -): PseudoType | ArrayType { +): InferredType { if (typeof typeRangeType === 'string') { return typeRangeType; } switch (typeRangeType.kind) { - case 'array': { + case 'array': + case 'literal': + case 'union': { return typeRangeType; } case 'deconstructed': { + if (typeRangeType.resolvedType !== undefined) return typeRangeType.resolvedType; const arrayType = inferType(typeRangeType.node, symbolsTable, objectMap, filtersMap); - if (typeof arrayType === 'string') { - return Untyped; - } else { - return arrayType.valueType; - } + return (typeRangeType.resolvedType = isArrayType(arrayType) ? arrayType.valueType : Untyped); } default: { - return inferType(typeRangeType.node, symbolsTable, objectMap, filtersMap); + // The symbols table belongs to this inference request. Reuse resolved assignments + // when expressions such as `x | default: x` look up the same binding twice. + return (typeRangeType.resolvedType ??= inferType( + typeRangeType.node, + symbolsTable, + objectMap, + filtersMap, + )); } } } @@ -672,7 +688,7 @@ function inferType( symbolsTable: SymbolsTable, objectMap: ObjectMap, filtersMap: FiltersMap, -): PseudoType | ArrayType { +): InferredType { if (typeof thing === 'string') { return objectMap[thing as PseudoType]?.name ?? Untyped; } @@ -717,10 +733,18 @@ function inferType( if (thing.filters.length > 0) { const lastFilter = thing.filters.at(-1)!; if (lastFilter.name === 'default') { - // default filter is a special case, we need to return the type of the expression - // instead of the filter. if (lastFilter.args.length > 0 && lastFilter.args[0].type !== NodeTypes.NamedArgument) { - return inferType(lastFilter.args[0], symbolsTable, objectMap, filtersMap); + const fallback = lastFilter.args[0]; + const fallbackType = inferType(fallback, symbolsTable, objectMap, filtersMap); + const input = { ...thing, filters: thing.filters.slice(0, -1) }; + const inputType = inferType(input, symbolsTable, objectMap, filtersMap); + + if (getStringLiterals(inputType) || getStringLiterals(fallbackType)) { + return inferStringLiteralDefaultType(input, inputType, fallback, fallbackType); + } + + // Preserve the existing fallback-based inference for other types. + return fallbackType; } } const filterEntry = filtersMap[lastFilter.name]; @@ -736,33 +760,74 @@ function inferType( } } -function inferLiquidDocParamType(node: LiquidDocParamNode, liquidDrops: ObjectEntry[]) { +/** Keep finite string choices through default without narrowing a general string. */ +function inferStringLiteralDefaultType( + input: LiquidVariable, + inputType: InferredType, + fallback: LiquidExpression, + fallbackType: InferredType, +): InferredType { + const inputLiterals = + getStringLiterals(inputType) ?? + (input.filters.length === 0 && input.expression.type === NodeTypes.String + ? [stringLiteral(input.expression)] + : undefined); + const fallbackLiterals = + getStringLiterals(fallbackType) ?? + (fallback.type === NodeTypes.String ? [stringLiteral(fallback)] : undefined); + + if (inputLiterals && fallbackLiterals) { + const literals = [...inputLiterals, ...fallbackLiterals]; + const types = literals.filter( + (literal, index) => literals.findIndex((other) => other.value === literal.value) === index, + ); + return types.length === 1 ? types[0] : { kind: 'union', types }; + } + + return getBaseType(inputType) === 'string' && getBaseType(fallbackType) === 'string' + ? 'string' + : Untyped; +} + +function stringLiteral( + node: Extract, +): StringLiteralType { + return { + kind: 'literal', + value: node.value, + raw: node.source.slice(node.position.start, node.position.end), + }; +} + +function inferLiquidDocParamType( + node: LiquidDocParamNode, + liquidDrops: ObjectEntry[], +): InferredType { const paramTypeValue = node.paramType?.value; if (!paramTypeValue) return Untyped; const validParamTypes = getValidParamTypes(liquidDrops); - const parsedParamType = parseParamType(new Set(validParamTypes.keys()), paramTypeValue); + const parsedParamType = parseDocParamType(new Set(validParamTypes.keys()), paramTypeValue); if (!parsedParamType) return Untyped; - const [type, isArray] = parsedParamType; + if (parsedParamType.kind === 'literal') return parsedParamType; - let transformedParamType; - - // BasicParamTypes.Object does not map to any specific type in the type system. - if (type === BasicParamTypes.Object) { - transformedParamType = Untyped; - } else { - transformedParamType = type; + if (parsedParamType.kind === 'union') { + const { types } = parsedParamType; + // The type system only has unions of string literals for now. + return types.every((type): type is StringLiteralType => type.kind === 'literal') + ? { kind: 'union', types } + : Untyped; } - if (isArray) { - return arrayType(transformedParamType); - } + const type = parsedParamType.kind === 'array' ? parsedParamType.valueType : parsedParamType.name; + // BasicParamTypes.Object does not map to any specific type in the type system. + const transformedParamType = type === BasicParamTypes.Object ? Untyped : type; - return transformedParamType; + return parsedParamType.kind === 'array' ? arrayType(transformedParamType) : transformedParamType; } function inferLookupType( @@ -770,7 +835,7 @@ function inferLookupType( symbolsTable: SymbolsTable, objectMap: ObjectMap, filtersMap: FiltersMap, -): PseudoType | ArrayType { +): InferredType { // we return the type of the drop, so a.b.c const node = thing; @@ -807,7 +872,7 @@ function inferLookupType( // e.g. product.images -> ArrayType // e.g. product.name -> string else { - curr = inferPseudoTypePropertyType(curr, lookup, objectMap); + curr = inferPseudoTypePropertyType(getBaseType(curr), lookup, objectMap); } // Early return @@ -995,8 +1060,21 @@ function isArrayReturnType(rt: ReturnType): rt is ArrayReturnType { return rt.type === 'array'; } -export function isArrayType(thing: PseudoType | ArrayType): thing is ArrayType { - return typeof thing !== 'string'; +export function isArrayType(thing: InferredType): thing is ArrayType { + return typeof thing !== 'string' && thing.kind === 'array'; +} + +/** The string values a type allows, when it allows nothing else. */ +export function getStringLiterals(type: InferredType | undefined): StringLiteralType[] | undefined { + if (typeof type !== 'object' || type.kind === 'array') return undefined; + return type.kind === 'literal' ? [type] : type.types; +} + +/** The primitive or object type used for property and filter lookup. */ +export function getBaseType(type: InferredType): PseudoType { + if (typeof type === 'string') return type; + // String literals and their unions are looked up as strings. + return isArrayType(type) ? 'array' : 'string'; } /** Assumes findLast */ diff --git a/packages/theme-language-server-common/src/completions/providers/BlockParameterCompletionProvider.spec.ts b/packages/theme-language-server-common/src/completions/providers/BlockParameterCompletionProvider.spec.ts index 469810260..d954ad70c 100644 --- a/packages/theme-language-server-common/src/completions/providers/BlockParameterCompletionProvider.spec.ts +++ b/packages/theme-language-server-common/src/completions/providers/BlockParameterCompletionProvider.spec.ts @@ -487,6 +487,26 @@ describe('Module: BlockParameterCompletionProvider', () => { }); }); + describe('LiquidDoc string enums', () => { + it('shows the allowed values and inserts the first one', async () => { + openBlock( + documentManager, + 'card', + blockSource([], ["@param {'heading' | 'small'} [variant] - Text style"]), + ); + + await expect(provider).to.complete(template(`{% block 'card', var█ %}{% endblock %}`), [ + expect.objectContaining({ + label: 'variant', + documentation: expect.objectContaining({ + value: "### variant (Optional): `'heading' | 'small'`\n\nText style", + }), + textEdit: expect.objectContaining({ newText: "variant: ${1:'heading'}$0" }), + }), + ]); + }); + }); + describe('when the tag cannot be resolved', () => { it('offers nothing for a missing target block', async () => { await expect(provider).to.complete(template(`{% block 'missing', █ %}{% endblock %}`), []); diff --git a/packages/theme-language-server-common/src/completions/providers/FilterCompletionProvider.spec.ts b/packages/theme-language-server-common/src/completions/providers/FilterCompletionProvider.spec.ts index 27df1b3a4..68e6b307e 100644 --- a/packages/theme-language-server-common/src/completions/providers/FilterCompletionProvider.spec.ts +++ b/packages/theme-language-server-common/src/completions/providers/FilterCompletionProvider.spec.ts @@ -149,6 +149,19 @@ describe('Module: FilterCompletionProvider', async () => { await expect(provider).to.complete('{{ string | █ }}', stringFilters.concat(anyFilters)); }); + it.each(['{{ variant | █ }}', '{% assign style = variant %}{{ style | █ }}'])( + 'offers string filters for enum variables: %s', + async (source) => { + await expect(provider).to.complete( + { + relativePath: 'snippets/text.liquid', + source: `{% doc %}\n@param {'heading' | 'small'} variant\n{% enddoc %}\n${source}`, + }, + stringFilters.concat(anyFilters), + ); + }, + ); + it('should complete array types with array filters', async () => { await expect(provider).to.complete('{{ array | █ }}', arrayFilters.concat(anyFilters)); }); diff --git a/packages/theme-language-server-common/src/completions/providers/FilterCompletionProvider.ts b/packages/theme-language-server-common/src/completions/providers/FilterCompletionProvider.ts index 530cfff94..18c7bab40 100644 --- a/packages/theme-language-server-common/src/completions/providers/FilterCompletionProvider.ts +++ b/packages/theme-language-server-common/src/completions/providers/FilterCompletionProvider.ts @@ -6,7 +6,7 @@ import { InsertTextFormat, TextEdit, } from 'vscode-languageserver'; -import { PseudoType, TypeSystem, isArrayType } from '../../TypeSystem'; +import { PseudoType, TypeSystem, getBaseType } from '../../TypeSystem'; import { memoize } from '../../utils'; import { AugmentedLiquidSourceCode } from '../../documents'; import { LiquidCompletionParams } from '../params'; @@ -46,7 +46,7 @@ export class FilterCompletionProvider implements Provider { partialAst, params.textDocument.uri, ); - const options = await this.options(isArrayType(inputType) ? 'array' : inputType); + const options = await this.options(getBaseType(inputType)); return options .filter(({ name }) => name.startsWith(partial)) diff --git a/packages/theme-language-server-common/src/completions/providers/ObjectAttributeCompletionProvider.spec.ts b/packages/theme-language-server-common/src/completions/providers/ObjectAttributeCompletionProvider.spec.ts index c01df2717..ff566070a 100644 --- a/packages/theme-language-server-common/src/completions/providers/ObjectAttributeCompletionProvider.spec.ts +++ b/packages/theme-language-server-common/src/completions/providers/ObjectAttributeCompletionProvider.spec.ts @@ -80,6 +80,19 @@ describe('Module: ObjectAttributeCompletionProvider', async () => { }); }); + it.each(['{{ variant.█ }}', '{% assign style = variant %}{{ style.█ }}'])( + 'offers string properties for enum variables: %s', + async (source) => { + await expect(provider).to.complete( + { + relativePath: 'snippets/text.liquid', + source: `{% doc %}\n@param {'heading' | 'small'} variant\n{% enddoc %}\n${source}`, + }, + ['size'], + ); + }, + ); + it('does not complete number lookups', async () => { await expect(provider).to.complete('{{ product[01█ }}', []); }); diff --git a/packages/theme-language-server-common/src/completions/providers/ObjectAttributeCompletionProvider.ts b/packages/theme-language-server-common/src/completions/providers/ObjectAttributeCompletionProvider.ts index 95f062f0a..9dfd3a8b5 100644 --- a/packages/theme-language-server-common/src/completions/providers/ObjectAttributeCompletionProvider.ts +++ b/packages/theme-language-server-common/src/completions/providers/ObjectAttributeCompletionProvider.ts @@ -1,7 +1,7 @@ import { NodeTypes } from '@shopify/liquid-html-parser'; import { ObjectEntry } from '@shopify/theme-check-common'; import { CompletionItem, CompletionItemKind } from 'vscode-languageserver'; -import { TypeSystem, isArrayType } from '../../TypeSystem'; +import { TypeSystem, getBaseType, isArrayType } from '../../TypeSystem'; import { LiquidCompletionParams } from '../params'; import { Provider, createCompletionItem, sortByName } from './common'; import { GetThemeSettingsSchemaForURI } from '../../settings'; @@ -50,7 +50,7 @@ export class ObjectAttributeCompletionProvider implements Provider { ArrayCoreProperties.map((name) => ({ name })), partial, ); - } else if (parentType === 'string') { + } else if (getBaseType(parentType) === 'string') { return completionItems( StringCoreProperties.map((name) => ({ name })), partial, @@ -58,7 +58,7 @@ export class ObjectAttributeCompletionProvider implements Provider { } const objectMap = await this.typeSystem.objectMap(params.textDocument.uri, partialAst); - const parentTypeProperties = objectMap[parentType]?.properties || []; + const parentTypeProperties = objectMap[getBaseType(parentType)]?.properties || []; return completionItems(parentTypeProperties, partial); } } diff --git a/packages/theme-language-server-common/src/completions/providers/ObjectCompletionProvider.spec.ts b/packages/theme-language-server-common/src/completions/providers/ObjectCompletionProvider.spec.ts index a967fe1bd..f5581f043 100644 --- a/packages/theme-language-server-common/src/completions/providers/ObjectCompletionProvider.spec.ts +++ b/packages/theme-language-server-common/src/completions/providers/ObjectCompletionProvider.spec.ts @@ -119,6 +119,24 @@ describe('Module: ObjectCompletionProvider', async () => { }); }); + it.each([ + ['variant', '{{ vari█ }}'], + ['style', '{% assign style = variant %}{{ sty█ }}'], + ])('retains enum values in the completion documentation for %s', async (name, source) => { + await expect(provider).to.complete( + { + relativePath: 'snippets/text.liquid', + source: `{% doc %}\n@param {'Heading' | "Small"} variant\n{% enddoc %}\n${source}`, + }, + [ + expect.objectContaining({ + label: name, + documentation: { kind: 'markdown', value: `### ${name}: \`'Heading' | "Small"\`` }, + }), + ], + ); + }); + it('should complete variable lookups', async () => { const contexts = [ `{{ a█`, diff --git a/packages/theme-language-server-common/src/completions/providers/common/CompletionItemProperties.ts b/packages/theme-language-server-common/src/completions/providers/common/CompletionItemProperties.ts index 083d38346..f7eca0f34 100644 --- a/packages/theme-language-server-common/src/completions/providers/common/CompletionItemProperties.ts +++ b/packages/theme-language-server-common/src/completions/providers/common/CompletionItemProperties.ts @@ -1,7 +1,7 @@ import { DocsetEntry } from '@shopify/theme-check-common'; import { CompletionItem, CompletionItemTag, MarkupContent } from 'vscode-languageserver'; import { DocsetEntryType, render } from '../../../docset'; -import { ArrayType, PseudoType } from '../../../TypeSystem'; +import { InferredType } from '../../../TypeSystem'; // ASCII tokens that make a string appear lower in the list. // @@ -19,7 +19,7 @@ export function createCompletionItem( entry: DocsetEntry & { deprioritized?: boolean }, extraProperties: Partial = {}, docsetEntryType?: DocsetEntryType, - entryType?: PseudoType | ArrayType, + entryType?: InferredType, ): CompletionItem { // prettier-ignore const sortToken = entry.deprecated @@ -41,7 +41,7 @@ export function createCompletionItem( function documentationProperties( entry: DocsetEntry, docsetEntryType?: DocsetEntryType, - entryType?: PseudoType | ArrayType, + entryType?: InferredType, ): { documentation: MarkupContent; } { diff --git a/packages/theme-language-server-common/src/diagnostics/runChecks.spec.ts b/packages/theme-language-server-common/src/diagnostics/runChecks.spec.ts index 386ec6d22..148f1e6a8 100644 --- a/packages/theme-language-server-common/src/diagnostics/runChecks.spec.ts +++ b/packages/theme-language-server-common/src/diagnostics/runChecks.spec.ts @@ -276,4 +276,71 @@ describe('Module: runChecks', () => { diagnostics: [], }); }); + + it.each([ + { + docPath: 'snippets/text.liquid', + callerSource: "{% render 'text', variant: 'large' %}", + checkCode: 'ValidRenderSnippetArgumentTypes', + }, + { + docPath: 'blocks/text.liquid', + callerSource: "{% content_for 'block', type: 'text', id: 'text', variant: 'large' %}", + checkCode: 'ValidContentForArgumentTypes', + }, + ])( + 'refreshes $checkCode diagnostics when an enum changes', + async ({ docPath, callerSource, checkCode }) => { + const docURI = path.join(rootUri, docPath); + const callerURI = path.join(rootUri, 'sections', 'main.liquid'); + const docSource = "{% doc %}\n@param {'heading' | 'small'} [variant]\n{% enddoc %}"; + const checks = allChecks.filter((check) => check.meta.code === checkCode); + expect(checks).toHaveLength(1); + + runChecks = makeRunChecks(documentManager, diagnosticsManager, { + fs, + loadConfig: async () => ({ + context: 'theme', + settings: {}, + checks, + rootUri, + }), + themeDocset: { + filters: async () => [], + objects: async () => [], + liquidDrops: async () => [], + tags: async () => [], + systemTranslations: async () => ({}), + }, + jsonValidationSet: { schemas: async () => [] }, + }); + documentManager.open(docURI, docSource, 0); + documentManager.open(callerURI, callerSource, 0); + + await runChecks([docURI]); + const valueStart = callerSource.indexOf("'large'"); + expect(connection.sendDiagnostics).toHaveBeenCalledWith({ + uri: callerURI, + version: 0, + diagnostics: [ + expect.objectContaining({ + code: checkCode, + range: { + start: { line: 0, character: valueStart }, + end: { line: 0, character: valueStart + "'large'".length }, + }, + }), + ], + }); + + documentManager.change(docURI, docSource.replace("'small'", "'large'"), 1); + connection.sendDiagnostics.mockClear(); + await runChecks([docURI]); + expect(connection.sendDiagnostics).toHaveBeenCalledWith({ + uri: callerURI, + version: 0, + diagnostics: [], + }); + }, + ); }); diff --git a/packages/theme-language-server-common/src/docset/MarkdownRenderer.spec.ts b/packages/theme-language-server-common/src/docset/MarkdownRenderer.spec.ts index 8c3f0650d..cab1a3f10 100644 --- a/packages/theme-language-server-common/src/docset/MarkdownRenderer.spec.ts +++ b/packages/theme-language-server-common/src/docset/MarkdownRenderer.spec.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from 'vitest'; import { render, renderHtmlEntry, type HtmlEntry } from './MarkdownRenderer'; import type { DocsetEntry } from '@shopify/theme-check-common'; +import type { UnionType } from '../TypeSystem'; const DOC_ENTRY: DocsetEntry = { name: 'entry', @@ -17,6 +18,52 @@ const HTML_ENTRY: HtmlEntry = { describe('MarkdownRenderer', () => { describe('render()', () => { + it('renders enum members without interpreting Markdown or constructing an object reference', () => { + const type: UnionType = { + kind: 'union', + types: [ + { kind: 'literal', value: '**bold**', raw: "'**bold**'" }, + { kind: 'literal', value: '', raw: '""' }, + ], + }; + const entry = { + ...DOC_ENTRY, + access: { global: false, parents: [], template: [] }, + }; + + expect(render(entry, type, 'object')).toEqual( + '### entry: `\'**bold**\' | ""`\nsummary\n\n---\n\ndescription', + ); + }); + + it('uses a longer code delimiter when enum values contain backticks', () => { + const type: UnionType = { + kind: 'union', + types: [ + { kind: 'literal', value: '``Heading``', raw: "'``Heading``'" }, + { kind: 'literal', value: '`Small`', raw: '"`Small`"' }, + ], + }; + + expect(render({ name: 'variant' }, type, 'object')).toEqual( + '### variant: ```\'``Heading``\' | "`Small`"```', + ); + }); + + it('displays line breaks in inferred enum values without breaking Markdown', () => { + const type: UnionType = { + kind: 'union', + types: [ + { kind: 'literal', value: 'Heading', raw: "'Heading'" }, + { kind: 'literal', value: '\r\n`small`\nnext', raw: "'\r\n`small`\nnext'" }, + ], + }; + + expect(render({ name: 'variant' }, type, 'object')).toEqual( + "### variant: ``'Heading' | '\\r\\n`small`\\nnext'``", + ); + }); + it('converts a docset entry to markdown', async () => { expect(render(DOC_ENTRY)).toEqual(`### entry\nsummary\n\n---\n\ndescription`); }); diff --git a/packages/theme-language-server-common/src/docset/MarkdownRenderer.ts b/packages/theme-language-server-common/src/docset/MarkdownRenderer.ts index be0ba2c30..4e105800f 100644 --- a/packages/theme-language-server-common/src/docset/MarkdownRenderer.ts +++ b/packages/theme-language-server-common/src/docset/MarkdownRenderer.ts @@ -1,5 +1,12 @@ import { DocsetEntry, FilterEntry, ObjectEntry, TagEntry } from '@shopify/theme-check-common'; -import { ArrayType, PseudoType, Unknown, docsetEntryReturnType, isArrayType } from '../TypeSystem'; +import { + InferredType, + Unknown, + docsetEntryReturnType, + getStringLiterals, + isArrayType, +} from '../TypeSystem'; +import { formatLiquidDocParamType } from '../utils/liquidDoc'; import { Attribute, Tag, Value } from './HtmlDocset'; const HORIZONTAL_SEPARATOR = '\n\n---\n\n'; @@ -9,7 +16,7 @@ export type DocsetEntryType = 'filter' | 'tag' | 'object'; export function render( entry: DocsetEntry | FilterEntry | TagEntry, - returnType?: PseudoType | ArrayType, + returnType?: InferredType, docsetEntryType?: DocsetEntryType, ) { return [title(entry, returnType), docsetEntryBody(entry, returnType, docsetEntryType)] @@ -23,11 +30,14 @@ export function renderHtmlEntry(entry: HtmlEntry, parentEntry?: HtmlEntry) { function title( entry: DocsetEntry | ObjectEntry | FilterEntry | HtmlEntry, - returnType?: PseudoType | ArrayType, + returnType?: InferredType, ) { returnType = returnType ?? docsetEntryReturnType(entry as ObjectEntry, Unknown); + const literals = getStringLiterals(returnType); - if (isArrayType(returnType)) { + if (literals) { + return `### ${entry.name}: ${formatLiquidDocParamType(literals)}`; + } else if (isArrayType(returnType)) { return `### ${entry.name}: \`${returnType.valueType}[]\``; } else if (returnType !== Unknown) { return `### ${entry.name}: \`${returnType}\``; @@ -46,7 +56,7 @@ function sanitize(s: string | undefined) { function docsetEntryBody( entry: DocsetEntry, - returnType?: PseudoType | ArrayType, + returnType?: InferredType, docsetEntryType?: DocsetEntryType, ) { return [ @@ -95,7 +105,7 @@ const shopifyDevRoot = `https://shopify.dev/docs/api/liquid`; function shopifyDevReference( entry: DocsetEntry, - returnType?: PseudoType | ArrayType, + returnType?: InferredType, docsetEntryType?: DocsetEntryType, ) { switch (docsetEntryType) { @@ -110,7 +120,9 @@ function shopifyDevReference( } case 'object': { - if (!returnType) { + if (getStringLiterals(returnType)) { + return undefined; + } else if (!returnType) { return `[Shopify Reference](${shopifyDevRoot}/objects/${entry.name})`; } else if (isArrayType(returnType)) { return `[Shopify Reference](${shopifyDevRoot}/objects/${returnType.valueType})`; diff --git a/packages/theme-language-server-common/src/hover/providers/LiquidObjectAttributeHoverProvider.spec.ts b/packages/theme-language-server-common/src/hover/providers/LiquidObjectAttributeHoverProvider.spec.ts index e04bf7c69..ad1d63b74 100644 --- a/packages/theme-language-server-common/src/hover/providers/LiquidObjectAttributeHoverProvider.spec.ts +++ b/packages/theme-language-server-common/src/hover/providers/LiquidObjectAttributeHoverProvider.spec.ts @@ -79,6 +79,19 @@ describe('Module: LiquidObjectAttributeHoverProvider', async () => { } }); + it.each(['{{ variant.size█ }}', '{% assign style = variant %}{{ style.size█ }}'])( + 'uses string properties for enum variables: %s', + async (source) => { + await expect(provider).to.hover( + { + relativePath: 'snippets/text.liquid', + source: `{% doc %}\n@param {'heading' | 'small'} variant\n{% enddoc %}\n${source}`, + }, + '### size: `number`', + ); + }, + ); + describe('when hovering over an array built-in method', () => { it('should return the hover description of the object property', async () => { const contexts = [ diff --git a/packages/theme-language-server-common/src/hover/providers/LiquidObjectAttributeHoverProvider.ts b/packages/theme-language-server-common/src/hover/providers/LiquidObjectAttributeHoverProvider.ts index 3f0bc59fd..8db92a5f2 100644 --- a/packages/theme-language-server-common/src/hover/providers/LiquidObjectAttributeHoverProvider.ts +++ b/packages/theme-language-server-common/src/hover/providers/LiquidObjectAttributeHoverProvider.ts @@ -1,7 +1,7 @@ import { NodeTypes } from '@shopify/liquid-html-parser'; import { LiquidHtmlNode } from '@shopify/theme-check-common'; import { Hover, HoverParams } from 'vscode-languageserver'; -import { TypeSystem, Unknown, Untyped, isArrayType } from '../../TypeSystem'; +import { TypeSystem, Unknown, Untyped, getBaseType, isArrayType } from '../../TypeSystem'; import { render } from '../../docset'; import { BaseHoverProvider } from '../BaseHoverProvider'; @@ -32,8 +32,9 @@ export class LiquidObjectAttributeHoverProvider implements BaseHoverProvider { const objectMap = await this.typeSystem.objectMap(uri, ancestors[0]); const parentType = await this.typeSystem.inferType(node, ancestors[0], uri); + const parentBaseType = getBaseType(parentType); - if (isArrayType(parentType) || parentType === 'string' || parentType === Untyped) { + if (isArrayType(parentType) || parentBaseType === 'string' || parentType === Untyped) { const nodeType = await this.typeSystem.inferType( { ...parentNode, lookups: parentNode.lookups.slice(0, lookupIndex + 1) }, ancestors[0], @@ -44,7 +45,7 @@ export class LiquidObjectAttributeHoverProvider implements BaseHoverProvider { if (isArrayType(nodeType) || nodeType === Unknown) return null; // We want want `## first: `nodeType` with the docs of the nodeType - const entry = { ...(objectMap[nodeType] ?? {}), name: currentNode.value }; + const entry = { ...(objectMap[getBaseType(nodeType)] ?? {}), name: currentNode.value }; return { contents: { @@ -54,12 +55,12 @@ export class LiquidObjectAttributeHoverProvider implements BaseHoverProvider { }; } - const parentEntry = objectMap[parentType]; + const parentEntry = objectMap[parentBaseType]; if (!parentEntry) { return null; } - const parentTypeProperties = objectMap[parentType]?.properties || []; + const parentTypeProperties = parentEntry.properties || []; const entry = parentTypeProperties.find((p) => p.name === currentNode.value); if (!entry) { return null; diff --git a/packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.spec.ts b/packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.spec.ts index f98985427..64712f777 100644 --- a/packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.spec.ts +++ b/packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.spec.ts @@ -147,6 +147,19 @@ describe('Module: LiquidObjectHoverProvider', async () => { } }); + it.each([ + ['variant', '{{ variant█ }}'], + ['style', '{% assign style = variant %}{{ style█ }}'], + ])('retains enum values when hovering %s', async (name, source) => { + await expect(provider).to.hover( + { + relativePath: 'snippets/text.liquid', + source: `{% doc %}\n@param {'Heading' | "Small"} variant\n{% enddoc %}\n${source}`, + }, + `### ${name}: \`'Heading' | "Small"\``, + ); + }); + it('should support paginate inside paginate tags', async () => { const context = ` {% paginate all_products by 5 %} diff --git a/packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.ts b/packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.ts index b14aa22e2..211aea9d5 100644 --- a/packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.ts +++ b/packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.ts @@ -1,6 +1,6 @@ import { LiquidHtmlNode, LiquidVariableLookup, NodeTypes } from '@shopify/liquid-html-parser'; import { Hover, HoverParams } from 'vscode-languageserver'; -import { TypeSystem, Unknown, isArrayType } from '../../TypeSystem'; +import { TypeSystem, Unknown, getBaseType, isArrayType } from '../../TypeSystem'; import { render } from '../../docset'; import { BaseHoverProvider } from '../BaseHoverProvider'; @@ -33,7 +33,7 @@ export class LiquidObjectHoverProvider implements BaseHoverProvider { const type = await this.typeSystem.inferType(node, ancestors[0], params.textDocument.uri); const objectMap = await this.typeSystem.objectMap(params.textDocument.uri, ancestors[0]); - const entry = objectMap[isArrayType(type) ? type.valueType : type]; + const entry = objectMap[isArrayType(type) ? type.valueType : getBaseType(type)]; if (type === Unknown) { return null; diff --git a/packages/theme-language-server-common/src/utils/liquidDoc.spec.ts b/packages/theme-language-server-common/src/utils/liquidDoc.spec.ts index 9dd43e6a9..0ec5c3246 100644 --- a/packages/theme-language-server-common/src/utils/liquidDoc.spec.ts +++ b/packages/theme-language-server-common/src/utils/liquidDoc.spec.ts @@ -1,6 +1,10 @@ import { DocDefinition } from '@shopify/theme-check-common'; import { describe, expect, it } from 'vitest'; -import { formatLiquidDocContentMarkdown, formatLiquidDocParameter } from './liquidDoc'; +import { + formatLiquidDocContentMarkdown, + formatLiquidDocParameter, + getParameterCompletionTemplate, +} from './liquidDoc'; describe('Module: liquidDoc', async () => { describe('formatLiquidDocContentMarkdown', async () => { @@ -101,6 +105,41 @@ This is a description }); describe('formatLiquidDocParameter', async () => { + it('preserves enum values, quotes, and case', () => { + const parameter = { + name: 'variant', + description: 'The text style', + type: `'Heading' | "Small"`, + required: false, + nodeType: 'param', + } as const; + + expect(formatLiquidDocParameter(parameter)).toEqual( + '- `variant` (Optional): `\'Heading\' | "Small"` - The text style', + ); + expect(formatLiquidDocParameter(parameter, true)).toEqual( + '### `variant` (Optional): `\'Heading\' | "Small"`\n\nThe text style', + ); + }); + + it.each([ + ["'**bold**' | ''", "`'**bold**' | ''`"], + ["'`heading`' | 'small'", "``'`heading`' | 'small'``"], + ["'``heading``' | '`small`'", "```'``heading``' | '`small`'```"], + [" \t' heading ' | ' small '\t ", "`' heading ' | ' small '`"], + ])('renders enum annotation %s as literal Markdown code', (type, expected) => { + const parameter = { + name: 'variant', + description: null, + type, + required: true, + nodeType: 'param', + } as const; + + expect(formatLiquidDocParameter(parameter)).toEqual(`- \`variant\`: ${expected}`); + expect(formatLiquidDocParameter(parameter, true)).toEqual(`### \`variant\`: ${expected}`); + }); + it('should format a required parameter correctly', async () => { expect( formatLiquidDocParameter({ @@ -164,4 +203,28 @@ This is a description ).toEqual('### `title`: string\n\nThe title of the product'); }); }); + + describe('getParameterCompletionTemplate', () => { + it.each([ + ['string', "value: '$1'$0"], + ['number', 'value: ${1:0}$0'], + ['boolean', 'value: ${1:false}$0'], + ['object', 'value: ${1:}$0'], + ['product[]', 'value: ${1:}$0'], + [null, 'value: ${1:}$0'], + ])('preserves the completion for %s parameters', (type, expected) => { + expect(getParameterCompletionTemplate('value', type)).toEqual(expected); + }); + + it.each([ + ["'Heading' | 'small'", "variant: ${1:'Heading'}$0"], + ['"Heading" | \'small\'', 'variant: ${1:"Heading"}$0'], + ["'$1' | 'small'", "variant: ${1:'\\$1'}$0"], + ["'}' | 'small'", "variant: ${1:'\\}'}$0"], + ["'a\\b' | 'small'", "variant: ${1:'a\\\\b'}$0"], + ["'${1}\\path' | 'small'", "variant: ${1:'\\${1\\}\\\\path'}$0"], + ])('uses a snippet-safe first enum value for %s', (type, expected) => { + expect(getParameterCompletionTemplate('variant', type)).toEqual(expected); + }); + }); }); diff --git a/packages/theme-language-server-common/src/utils/liquidDoc.ts b/packages/theme-language-server-common/src/utils/liquidDoc.ts index 66c62b235..b0a1cf3e9 100644 --- a/packages/theme-language-server-common/src/utils/liquidDoc.ts +++ b/packages/theme-language-server-common/src/utils/liquidDoc.ts @@ -3,6 +3,8 @@ import { DocDefinition, getDefaultValueForType, LiquidDocParameter, + parseStringLiterals, + StringLiteralType, SupportedDocTagTypes, } from '@shopify/theme-check-common'; @@ -11,7 +13,7 @@ export function formatLiquidDocParameter( heading: boolean = false, ) { const nameStr = required ? `\`${name}\`` : `\`${name}\` (Optional)`; - const typeStr = type ? `: ${type}` : ''; + const typeStr = type ? `: ${formatLiquidDocParamType(type)}` : ''; if (heading) { const descStr = description ? `\n\n${description}` : ''; @@ -22,6 +24,19 @@ export function formatLiquidDocParameter( return `- ${nameStr}${typeStr}${descStr}`; } +export function formatLiquidDocParamType(type: string | StringLiteralType[]): string { + if (typeof type === 'string' && !parseStringLiterals(type)) return type; + + const annotation = + typeof type === 'string' ? type.trim() : type.map((literal) => literal.raw).join(' | '); + // Inferred literal values can span lines; display those line breaks explicitly. + const display = annotation.replace(/\r/g, '\\r').replace(/\n/g, '\\n'); + const backticks = display.match(/`+/g) ?? []; + const fenceLength = backticks.reduce((length, run) => Math.max(length, run.length + 1), 1); + const fence = '`'.repeat(fenceLength); + return `${fence}${display}${fence}`; +} + export function formatLiquidDocTagHandle(label: string, description: string, example: string) { return `### @${label}\n\n${description}\n\n` + `**Example**\n\n\`\`\`liquid\n${example}\n\`\`\``; } @@ -34,6 +49,7 @@ export const SUPPORTED_LIQUID_DOC_TAG_HANDLES = { .map((type) => `\`${type}\``) .join(', ')}\n` + ` or liquid object that isn't exclusively a global object in our [API Docs](https://shopify.dev/docs/api/liquid/objects)\n` + + "- String values can be restricted to an enum, such as `{'heading' | 'small'}`\n" + '- An optional parameter is denoted by square brackets around the parameter name\n' + '- The description is optional Markdown text', example: @@ -41,6 +57,7 @@ export const SUPPORTED_LIQUID_DOC_TAG_HANDLES = { " @param {string} name - The person's name\n" + " @param {number} [fav_num] - The person's favorite number\n" + " @param {product} prod - The person's chosen product\n" + + " @param {'heading' | 'small'} [variant] - The text style\n" + '{% enddoc %}\n', template: `param {$2} $1$0`, }, @@ -60,8 +77,9 @@ export const SUPPORTED_LIQUID_DOC_TAG_HANDLES = { export function getParameterCompletionTemplate(name: string, type: string | null) { const paramDefaultValue = getDefaultValueForType(type); + const escapedDefaultValue = paramDefaultValue.replace(/[\\$}]/g, '\\$&'); - const valueTemplate = paramDefaultValue === "''" ? `'$1'$0` : `\${1:${paramDefaultValue}}$0`; + const valueTemplate = paramDefaultValue === "''" ? `'$1'$0` : `\${1:${escapedDefaultValue}}$0`; return `${name}: ${valueTemplate}`; }