diff --git a/.changeset/liquiddoc-string-enums-editor.md b/.changeset/liquiddoc-string-enums-editor.md new file mode 100644 index 000000000..70489ed82 --- /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 `render`, `content_for` and `block` parameter completions insert the first one. The `default` filter widens enum values to `string`. diff --git a/.changeset/liquiddoc-string-enums.md b/.changeset/liquiddoc-string-enums.md new file mode 100644 index 000000000..07aee6572 --- /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'}`, and literal arguments to `render`, `content_for` and `block` are checked against them. diff --git a/.changeset/quiet-doc-delimiters.md b/.changeset/quiet-doc-delimiters.md new file mode 100644 index 000000000..307ab38dc --- /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 leave incomplete string annotations unparsed so they cannot consume 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..58be2c0b7 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,80 @@ 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'", + "'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(["'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..e068829e1 100644 --- a/packages/liquid-html-parser/src/liquid-doc/tokenizer.ts +++ b/packages/liquid-html-parser/src/liquid-doc/tokenizer.ts @@ -170,16 +170,15 @@ 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 = readTypeEnd(text, pos); + if (typeEnd !== undefined) { tokens.push({ type: ParamTokenType.Type, - value: text.slice(pos + 1, end - 1), + value: text.slice(pos + 1, typeEnd - 1), start: pos + startOffset, - end: end + startOffset, + end: typeEnd + startOffset, }); - pos = end; + pos = typeEnd; continue; } @@ -247,12 +246,34 @@ export function tokenizeParamContent(text: string, startOffset: number): ParamTo return tokens; } +/** Returns the exclusive end of the `{type}` annotation at `start`. */ +function readTypeEnd(text: string, start: number): number | undefined { + if (text[start] !== '{') return undefined; + + for (let pos = start + 1; pos < text.length; pos++) { + const ch = text[pos]; + + if (ch === "'" || ch === '"') { + const closeQuote = text.indexOf(ch, pos + 1); + if (closeQuote === -1) { + const typeEnd = text.indexOf('}', pos + 1); + return typeEnd === -1 ? undefined : typeEnd + 1; + } + pos = closeQuote; + continue; + } + + if (ch === '}') return pos + 1; + } + + return undefined; +} + 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 diff --git a/packages/theme-check-common/src/block-parameters.ts b/packages/theme-check-common/src/block-parameters.ts index 3eae1d9f9..0c789ad72 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 { normalizeNamedParamType, 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; /** @@ -97,8 +97,7 @@ export function resolveBlockParameters( * omitted or malformed type so callers do not report speculative mismatches. */ export function liquidDocType(type: string | null | undefined): string | undefined { - const normalizedType = type?.toLowerCase(); - return normalizedType && parseParamTypeSyntax(normalizedType) ? normalizedType : undefined; + return type ? normalizeNamedParamType(type) : undefined; } function withLiquidDoc( @@ -109,7 +108,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..2bb99b546 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,19 @@ 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([]); + expect(await run("{% block 'card', variant: 42 %}{% endblock %}", block)).toMatchObject([ + { message: "Type mismatch for argument 'variant': expected string, got number" }, + ]); + }); + 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..025d5d198 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, + checkArgumentType, + getArgumentTypeMismatchMessage, + inferArgumentType, + 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 (checkArgumentType(expectedType, argument.value).kind !== 'incompatible') 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-doc-param-types/index.spec.ts b/packages/theme-check-common/src/checks/valid-doc-param-types/index.spec.ts index 2e66619f2..3068ae8c8 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,56 @@ 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 %}', + ]); + }); + + describe('without a docset', () => { + const runWithoutDocset = (source: string) => + runLiquidCheck(ValidDocParamTypes, source, undefined, { themeDocset: undefined }); + + it('reports invalid string literal types', async () => { + const source = `{% doc %}\n @param {'heading' |} variant\n{% enddoc %}`; + const offenses = await runWithoutDocset(source); + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toBe("The parameter type ''heading' |' is not supported."); + expect(applySuggestions(source, offenses[0])).toEqual([ + '{% doc %}\n @param variant\n{% enddoc %}', + ]); + }); + + it.each(["'heading' | 'small'", '"heading"'])('accepts the string enum {%s}', async (type) => { + const source = `{% doc %}\n @param {${type}} variant\n{% enddoc %}`; + expect(await runWithoutDocset(source)).toHaveLength(0); + }); + + it.each(['product', 'product[]', 'invalidType', 'unknown[]'])( + 'does not check the named type {%s}, which requires the docset', + async (type) => { + const source = `{% doc %}\n @param {${type}} variant\n{% enddoc %}`; + expect(await runWithoutDocset(source)).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..e8367dc32 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,6 @@ +import { TextNode } from '@shopify/liquid-html-parser'; import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; -import { getValidParamTypes, parseParamType } from '../../liquid-doc/utils'; +import { getValidParamTypes, parseParamType, parseStringLiterals } from '../../liquid-doc/utils'; export const ValidDocParamTypes: LiquidCheckDefinition = { meta: { @@ -18,49 +19,34 @@ export const ValidDocParamTypes: LiquidCheckDefinition = { }, create(context) { - if (!context.themeDocset) { - 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 { - async LiquidDocParamNode(node) { - if (!node.paramType) { - return; - } + async function isSupportedParamType(type: string): Promise { + if (usesStringLiteralSyntax(type)) return parseStringLiterals(type) !== undefined; - const parsedParamType = parseParamType(await validParamTypesPromise, node.paramType.value); + // Named types come from the docset, so they can only be checked with one. + if (!validParamTypesPromise) return true; + return parseParamType(await validParamTypesPromise, type) !== undefined; + } - if (parsedParamType) { - return; - } + return { + async LiquidDocParamNode(node) { + const { paramType } = node; + if (!paramType || (await isSupportedParamType(paramType.value))) return; context.report({ - message: `The parameter type '${node.paramType.value}' is not supported.`, + message: `The parameter type '${paramType.value}' is not supported.`, // Index is offset to include the curly brackets around the param type - startIndex: node.paramType.position.start - 1, - endIndex: node.paramType.position.end + 1, + startIndex: paramType.position.start - 1, + endIndex: paramType.position.end + 1, suggest: [ { message: 'Remove invalid parameter type', 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*/, - ' ', - ), - ); + const [start, end] = paramTypeRemovalRange(node.source, paramType); + corrector.replace(start, end, ' '); }, }, ], @@ -69,3 +55,28 @@ export const ValidDocParamTypes: LiquidCheckDefinition = { }; }, }; + +/** + * Quotes and pipes only appear in string literal types, such as + * `'heading' | 'small'`. Named types, such as `product[]`, never contain them. + */ +function usesStringLiteralSyntax(type: string): boolean { + return /['"|]/.test(type); +} + +/** + * Returns the range that removing a parameter type replaces with one space: + * the type, its braces, and the spaces and tabs around them. For example, + * `@param { bad } [name]` becomes `@param [name]`. + */ +function paramTypeRemovalRange(source: string, paramType: TextNode): [start: number, end: number] { + let start = paramType.position.start - 1; + let end = paramType.position.end + 1; + while (isSpaceOrTab(source.charAt(start - 1))) start--; + while (isSpaceOrTab(source.charAt(end))) end++; + return [start, end]; +} + +function isSpaceOrTab(char: string): boolean { + return char === ' ' || char === '\t'; +} 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..b2bfd96f9 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,13 @@ import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; -import { NodeTypes, RenderMarkup } from '@shopify/liquid-html-parser'; +import { RenderMarkup } from '@shopify/liquid-html-parser'; import { LiquidDocParameter } from '../../liquid-doc/liquidDoc'; -import { inferArgumentType, isTypeCompatible } from '../../liquid-doc/utils'; +import { + ArgumentTypeCheck, + checkArgumentType, + getArgumentTypeMismatchMessage, + getValidParamTypes, + parseParamType, +} from '../../liquid-doc/utils'; import { findTypeMismatchParams, generateTypeMismatchSuggestions, @@ -28,41 +34,59 @@ 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; + if (!expectedType) return; + + const argument = node.variable.name; + if (!(await isAliasMismatch(expectedType, checkArgumentType(expectedType, argument)))) 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, - }); - } - } + 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, + ), + }); + } + + /** + * Aliases check literals against every declared type, including named types + * and arrays, which no literal matches. With a docset, types that it does not + * define are left to ValidDocParamTypes so each declaration is reported once. + * The docset lookup uses the declared spelling, as ValidDocParamTypes does, + * so `Product` is reported only as an unsupported type. + */ + async function isAliasMismatch( + expectedType: string, + check: ArgumentTypeCheck, + ): Promise { + switch (check.kind) { + case 'incompatible': + return true; + case 'named-type': + if (!context.themeDocset) return true; + validParamTypesPromise ??= context.themeDocset + .liquidDrops() + .then((entries) => new Set(getValidParamTypes(entries).keys())); + return parseParamType(await validParamTypesPromise, expectedType) !== undefined; + case 'compatible': + case 'unchecked': + return false; } } @@ -79,7 +103,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..c2dca5589 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, + checkArgumentType, + getArgumentTypeMismatchMessage, getDefaultValueForType, - inferArgumentType, - isTypeCompatible, + parseStringLiterals, } from './utils'; import { isLiquidString } from '../checks/utils'; @@ -109,7 +109,8 @@ export function reportDuplicateArguments( /** * Find type mismatch between the arguments provided for `content_for` tag and `render` tag - * and their associated file's LiquidDoc + * and their associated file's LiquidDoc. Arguments declared with named Liquid types, such as + * `product`, are skipped. */ export function findTypeMismatchParams( liquidDocParameters: Map, @@ -118,21 +119,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 && + checkArgumentType(liquidDocParamDef.type, arg.value).kind === 'incompatible' + ) { + typeMismatchParams.push(arg); } } @@ -151,17 +143,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 +166,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/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..8f231be21 --- /dev/null +++ b/packages/theme-check-common/src/liquid-doc/doc-param-type.ts @@ -0,0 +1,110 @@ +/** + * `liquid-html-parser` intentionally keeps the contents of a LiquidDoc `{type}` + * annotation as text. Theme Check parses the supported subset here because + * named types are validated against the active Liquid docset. If LiquidDoc gets + * a complete type grammar, syntax parsing should move to the parser while + * docset validation stays here. + */ +/** 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; +} + +/** + * Lowercases a named LiquidDoc type such as `Product[]`, which is how argument + * checks compare named types. Returns undefined when the value is not valid + * named type syntax. + */ +export function normalizeNamedParamType(value: string): string | undefined { + const normalizedType = value.toLowerCase(); + return parseParamTypeSyntax(normalizedType) ? normalizedType : undefined; +} + +/** + * 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/enum-arguments.spec.ts b/packages/theme-check-common/src/liquid-doc/enum-arguments.spec.ts new file mode 100644 index 000000000..fac1ff053 --- /dev/null +++ b/packages/theme-check-common/src/liquid-doc/enum-arguments.spec.ts @@ -0,0 +1,346 @@ +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 { ValidDocParamTypes } from '../checks/valid-doc-param-types'; +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); + }); + + it.each(['Product', 'Product[]'])( + `${caller.name} leaves uppercase declaration %s to ValidDocParamTypes`, + async (type) => { + const offenses = await runLiquidCheck( + caller.check, + caller.source('123'), + 'templates/index.liquid', + {}, + { [caller.file]: definition(type) }, + ); + expect(offenses).toHaveLength(0); + + const declarationOffenses = await runLiquidCheck( + ValidDocParamTypes, + definition(type), + caller.file, + ); + expect(declarationOffenses.map(({ message }) => message)).toEqual([ + `The parameter type '${type}' is not supported.`, + ]); + }, + ); + + it.each(['unknown', 'unknown[]'])( + `${caller.name} leaves named declaration %s to ValidDocParamTypes when the docset lacks it`, + async (type) => { + const offenses = await runLiquidCheck( + caller.check, + caller.source('123'), + 'templates/index.liquid', + {}, + { [caller.file]: definition(type) }, + ); + expect(offenses).toHaveLength(0); + }, + ); + + describe(`${caller.name} without a docset`, () => { + const runWithoutDocset = (value: string, type: string) => + runLiquidCheck( + caller.check, + caller.source(value), + 'templates/index.liquid', + { themeDocset: undefined }, + { [caller.file]: definition(type) }, + ); + + it.each(['product', 'Product', 'product[]', 'string[]', 'unknown'])( + 'checks literal values against named declaration %s', + async (type) => { + const offenses = await runWithoutDocset('123', type); + expect(offenses.map(({ message }) => message)).toEqual([ + `Type mismatch for argument 'variant': expected ${type.toLowerCase()}, got number`, + ]); + }, + ); + + it('checks literal values against enum declarations', async () => { + const offenses = await runWithoutDocset("'body'", enumType); + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toContain(enumType); + }); + + it.each(["'heading' |", "'heading' | number", 'product['])( + 'does not cascade errors from unsupported declaration %s', + async (type) => { + expect(await runWithoutDocset("'body'", type)).toHaveLength(0); + }, + ); + + it('leaves dynamic values unverified', async () => { + expect(await runWithoutDocset('product', 'product')).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/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.spec.ts b/packages/theme-check-common/src/liquid-doc/utils.spec.ts index 322e7c27b..28aaa3a33 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,63 @@ import { describe, expect, it } from 'vitest'; -import { BasicParamTypes, parseParamType } from './utils'; +import { LiquidTagRender, toLiquidHtmlAST } from '@shopify/liquid-html-parser'; +import { + BasicParamTypes, + ArgumentTypeCheck, + checkArgumentType, + getDefaultValueForType, + parseParamType, +} from './utils'; + +const compatible: ArgumentTypeCheck = { kind: 'compatible' }; +const incompatible: ArgumentTypeCheck = { kind: 'incompatible' }; +const unchecked: ArgumentTypeCheck = { kind: 'unchecked' }; +const namedType = (type: string): ArgumentTypeCheck => ({ kind: 'named-type', type }); 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('checkArgumentType', () => { + it.each([ + ["'heading' | 'small'", "'heading'", compatible], + ["'heading' | 'small'", '"small"', compatible], + ["'heading' | 'small'", "'Heading'", incompatible], + ["'heading' | 'small'", "'large'", incompatible], + ["'heading' | 'small'", '42', incompatible], + ["'heading' | 'small'", 'nil', incompatible], + ["'heading' | 'small'", 'true', incompatible], + ["'heading' | 'small'", '(1..3)', incompatible], + ["'heading' | 'small'", 'block.settings.variant', unchecked], + ["'heading' |", "'small'", unchecked], + ['product', '42', namedType('product')], + ['Product[]', '42', namedType('product[]')], + ['string[]', "'heading'", namedType('string[]')], + ['product', 'product', unchecked], + ['product[', '42', unchecked], + ['String', "'heading'", compatible], + ['NUMBER', '42', compatible], + ['boolean', "'heading'", compatible], + ['string', '42', incompatible], + ])('checks %s against %s', (type, value, expected) => { + const ast = toLiquidHtmlAST(`{% render 'text', variant: ${value} %}`); + const render = ast.children[0] as LiquidTagRender; + expect(checkArgumentType(type, render.markup.args[0].value)).toEqual(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 2285a32e1..0df73b3ba 100644 --- a/packages/theme-check-common/src/liquid-doc/utils.ts +++ b/packages/theme-check-common/src/liquid-doc/utils.ts @@ -1,8 +1,18 @@ -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 { normalizeNamedParamType, parseStringLiterals } from './doc-param-type'; + +export { + normalizeNamedParamType, + 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. @@ -29,6 +39,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 "''"; @@ -77,6 +90,68 @@ export function isTypeCompatible(expectedType: string, actualType: BasicParamTyp return normalizedExpectedType === actualType; } +/** + * The result of checking an argument value against its LiquidDoc type. + * + * - `compatible` and `incompatible`: the literal value was checked. + * - `unchecked`: the value is dynamic, the type is malformed, or a block array + * literal is compared with a type other than a string enum. + * - `named-type`: the type is a named Liquid type or array, such as `product` + * or `string[]`, and `type` is its lowercase spelling. No literal matches + * it, but only the docset can confirm that the type exists. + */ +export type ArgumentTypeCheck = + | { kind: 'compatible' } + | { kind: 'incompatible' } + | { kind: 'unchecked' } + | { kind: 'named-type'; type: string }; + +/** Checks a literal argument value against a LiquidDoc type. */ +export function checkArgumentType( + expectedType: string, + argument: LiquidExpression | BlockArrayLiteral, +): ArgumentTypeCheck { + if (argument.type === NodeTypes.VariableLookup) return { kind: 'unchecked' }; + + const literals = parseStringLiterals(expectedType); + if (literals) { + return literalCheck( + argument.type === NodeTypes.String && + literals.some((literal) => literal.value === argument.value), + ); + } + + if (argument.type === 'BlockArrayLiteral') return { kind: 'unchecked' }; + + const type = normalizeNamedParamType(expectedType); + if (!type) return { kind: 'unchecked' }; + if (!isBasicParamType(type)) return { kind: 'named-type', type }; + + return literalCheck(isTypeCompatible(type, inferArgumentType(argument))); +} + +function literalCheck(matches: boolean): ArgumentTypeCheck { + return matches ? { kind: 'compatible' } : { kind: 'incompatible' }; +} + +function isBasicParamType(type: string): type is BasicParamTypes { + return Object.values(BasicParamTypes).some((basicType) => basicType === type); +} + +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. */ @@ -107,28 +182,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]]; -} diff --git a/packages/theme-language-server-common/src/TypeSystem.spec.ts b/packages/theme-language-server-common/src/TypeSystem.spec.ts index bc5f7678a..044c954f5 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 () => ({}), }, @@ -733,7 +748,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 +782,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 +853,121 @@ 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.each([ + ["variant | default: 'small'", 'string'], + ['variant | default: 1', 'number'], + ["'other' | default: variant", 'string'], + ])('uses the widened fallback type for %s', async (expression, expected) => { + expect(await inferOutput(`{{ ${expression} }}`)).to.equal(expected); + }); + + it('widens a default assignment without changing the documented 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.equal('string'); + }); + + 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..d5428714b 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. @@ -634,31 +643,28 @@ 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': { const arrayType = inferType(typeRangeType.node, symbolsTable, objectMap, filtersMap); - if (typeof arrayType === 'string') { - return Untyped; - } else { - return arrayType.valueType; - } + return isArrayType(arrayType) ? arrayType.valueType : Untyped; } default: { @@ -672,7 +678,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 +723,9 @@ 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 fallbackType = inferType(lastFilter.args[0], symbolsTable, objectMap, filtersMap); + return getStringLiterals(fallbackType) ? 'string' : fallbackType; } } const filterEntry = filtersMap[lastFilter.name]; @@ -736,33 +741,35 @@ function inferType( } } -function inferLiquidDocParamType(node: LiquidDocParamNode, liquidDrops: ObjectEntry[]) { +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; - - let transformedParamType; + if (parsedParamType.kind === 'literal') return parsedParamType; - // 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 +777,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 +814,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 +1002,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}`; }