Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/liquiddoc-string-enums-editor.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@shopify/theme-language-server-common': minor
---

Support LiquidDoc string enums in hovers and completions

Allowed values are kept through assignments and `default`, and `render`, `content_for` and `block` parameter completions insert the first one.
7 changes: 7 additions & 0 deletions .changeset/liquiddoc-string-enums.md
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 5 additions & 0 deletions .changeset/quiet-doc-delimiters.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/liquid-html-parser': patch
---

Preserve quoted braces in LiquidDoc parameter types and recover incomplete string annotations without consuming subsequent parameters.
17 changes: 17 additions & 0 deletions packages/liquid-html-parser/src/ast.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
107 changes: 107 additions & 0 deletions packages/liquid-html-parser/src/liquid-doc/parser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,113 @@ describe('Unit: liquid-doc parser', () => {
expect(param.required).toBe(false);
});

it.each([
"'heading' | 'small'",
'"heading"|"small"',
'\'Heading\' | "small"',
"'' | ' ' | 'two spaces'",
"'a|b' | 'small'",
"'}' | '{' | '{}'",
"'a}b' | 'small'",
"'a} [variant] - b'",
"'a} [variant] Users'",
"'a} variant - Users'",
'"it\'s" | \'"quoted"\'',
String.raw`'backslash\' | 'small'`,
" 'heading' \t|\t 'small' ",
])('preserves string enum type %s and its source positions', (paramType) => {
const nodes = parseDoc(`\n @param {${paramType}} [variant] - Shared text style\n`);
expect(nodes).toHaveLength(1);
const param = asParam(nodes[0]);
expect(param.paramType!.value).toBe(paramType);
expect(param.paramName.value).toBe('variant');
expect(param.required).toBe(false);
expect(param.paramDescription!.value).toBe('Shared text style');
for (const text of [param.paramType!, param.paramName, param.paramDescription!]) {
expect(param.source.slice(text.position.start, text.position.end)).toBe(text.value);
}
expect(param.source[param.paramType!.position.start - 1]).toBe('{');
expect(param.source[param.paramType!.position.end]).toBe('}');
});

it('parses required enum parameters with multiline descriptions and CRLF', () => {
const nodes = parseDoc(
"\r\n@param {'heading' | 'small'} variant - Shared style\r\n for text.\r\n@param {string} next\r\n",
);
expect(nodes).toHaveLength(2);
const param = asParam(nodes[0]);
expect(param.paramType!.value).toBe("'heading' | 'small'");
expect(param.paramName.value).toBe('variant');
expect(param.required).toBe(true);
expect(param.paramDescription!.value).toContain('for text.');
expect(asParam(nodes[1]).paramName.value).toBe('next');
});

it.each([
"'heading' |",
"| 'heading'",
"'heading' || 'small'",
"'heading' 'small'",
"'heading",
'"heading',
"'a}b' | 'small",
"'a}b' trailing",
])('preserves malformed enum type %s for semantic validation', (paramType) => {
const description = "It's {the shared style}.";
const nodes = parseDoc(
`\n@param {${paramType}} [variant] - ${description}\n@param {number} next\n`,
);
expect(nodes).toHaveLength(2);
const param = asParam(nodes[0]);
expect(param.paramType!.value).toBe(paramType);
expect(param.paramName.value).toBe('variant');
expect(param.required).toBe(false);
expect(param.paramDescription!.value).toBe(description);
expect(asParam(nodes[1]).paramName.value).toBe('next');
});

it.each(['', '- '])(
'recovers a missing type quote before a parameter description with separator %j',
(separator) => {
for (const name of ['variant', '[variant]']) {
for (const description of ["It's {the shared style}.", "Users'"]) {
const nodes = parseDoc(
`\n@param {'heading} ${name} ${separator}${description}\n@param {number} next\n`,
);
expect(nodes).toHaveLength(2);
const param = asParam(nodes[0]);
expect(param.paramType!.value).toBe("'heading");
expect(param.paramName.value).toBe('variant');
expect(param.required).toBe(name === 'variant');
expect(param.paramDescription!.value).toBe(description);
expect(asParam(nodes[1]).paramName.value).toBe('next');
}
}
},
);

it('prefers a parameter boundary when the missing delimiter is ambiguous', () => {
// This could be a closed string missing its outer brace, or an unclosed
// string followed by a parameter and an apostrophe in its description.
const param = asParam(parseDoc("\n@param {'a} [variant] - b'\n")[0]);
expect(param.paramType!.value).toBe("'a");
expect(param.paramName.value).toBe('variant');
expect(param.required).toBe(false);
expect(param.paramDescription!.value).toBe("b'");
});

it.each(["'heading", "'a}b'", "'a}b' | 'small'"])(
'keeps an unclosed type %s from consuming the next parameter',
(paramType) => {
const nodes = parseDoc(`\n@param {${paramType}\n@param {number} next\n`);
expect(nodes).toHaveLength(2);
const param = asParam(nodes[0]);
expect(param.paramType).toBeNull();
expect(param.paramName.value).toBe('');
expect(asParam(nodes[1]).paramName.value).toBe('next');
},
);

it('parses param with description after dash', () => {
const nodes = parseDoc('\n@param product - The product to display\n');
expect(nodes.length).toBe(1);
Expand Down
53 changes: 49 additions & 4 deletions packages/liquid-html-parser/src/liquid-doc/tokenizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,9 @@ export function tokenizeParamContent(text: string, startOffset: number): ParamTo
}

// Type: {type}
TYPE_RE.lastIndex = pos;
if (TYPE_RE.test(text)) {
const end = TYPE_RE.lastIndex;
const typeEnd = findTypeEnd(text, pos);
if (typeEnd !== -1) {
const end = typeEnd;
tokens.push({
type: ParamTokenType.Type,
value: text.slice(pos + 1, end - 1),
Expand Down Expand Up @@ -247,12 +247,57 @@ export function tokenizeParamContent(text: string, startOffset: number): ParamTo
return tokens;
}

/** Find the closing brace without treating braces inside string literals as delimiters. */
function findTypeEnd(text: string, start: number): number {
if (text[start] !== '{') return -1;

let quote: string | undefined;
let recoveryBrace = -1;

for (let pos = start + 1; pos < text.length; pos++) {
const ch = text[pos];
if (ch === '\n' || ch === '\r') break;

if (quote) {
if (ch === quote) {
if (
recoveryBrace !== -1 &&
/^[ \t]*(?:\[[^\]]*\]|[\w][\w-]*)[ \t]+/.test(text.slice(recoveryBrace + 1, pos))
) {
let next = pos + 1;
while (text[next] === ' ' || text[next] === '\t') next++;
// An unmatched quote can close at an apostrophe in the parameter's
// description, which need not have a dash. A real type delimiter wins;
// otherwise prefer a recognizable parameter boundary, even at EOL.
// This also recovers ambiguous input with a missing outer brace and a
// parameter-looking name inside its last string literal.
if (text[next] !== '|' && text[next] !== '}') {
return recoveryBrace + 1;
}
}
quote = undefined;
recoveryBrace = -1;
} else if (ch === '}' && recoveryBrace === -1) {
recoveryBrace = pos;
}
continue;
}

if (ch === '}') return pos + 1;
// Liquid strings do not interpret backslash escapes.
if (ch === "'" || ch === '"') quote = ch;
}

// Keep a malformed, closed annotation recognizable to semantic checks, and
// preserve its parameter name even when a string quote is missing.
return recoveryBrace === -1 ? -1 : recoveryBrace + 1;
}
Comment on lines +251 to +294

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuk. Can we not tokenize and consume tokens here instead? Why are we going ch by ch? This feels fast but also gross.

I feel like a stack based parser here would make this a bit easier to follow. I don鈥檛 like the lookback.


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;
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 8 additions & 4 deletions packages/theme-check-common/src/block-parameters.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { DocDefinition, LiquidDocParameter } from './liquid-doc/liquidDoc';
import { parseParamTypeSyntax } from './liquid-doc/utils';
import { parseParamTypeSyntax, parseStringLiterals } from './liquid-doc/utils';
import { schemaSettingLiquidType } from './schema-settings';
import { hasNoSchemaTag } from './to-schema';
import type { Dependencies, Setting, ThemeBlock } from './types';
Expand All @@ -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;
/**
Expand Down Expand Up @@ -109,7 +109,11 @@ function withLiquidDoc(

return {
name: liquidDoc.name,
type: liquidDocType(liquidDoc.type),
// String literal types keep their spelling, since arguments must match them exactly.
type:
liquidDoc.type && parseStringLiterals(liquidDoc.type)
? liquidDoc.type.trim()
: liquidDocType(liquidDoc.type),
required: liquidDoc.required,
liquidDoc,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,16 @@ describe('ValidBlockArgumentTypes', () => {
expect(offenses).toEqual([]);
});

it('keeps the schema type of a setting that LiquidDoc declares as a string enum', async () => {
const block = blockSource(
[{ id: 'variant', type: 'text' }],
["@param {'heading' | 'small'} [variant] - Variant"],
);

expect(await definitions(block)).toEqual([]);
expect(await run("{% block 'card', variant: 'body' %}{% endblock %}", block)).toEqual([]);
});
Comment on lines +248 to +256

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We鈥檙e missing a failure case?


it('does not speculate about omitted LiquidDoc or unmapped schema types', async () => {
const offenses = await definitions(
blockSource([{ id: 'item', type: 'metaobject' }], ['@param [item] - Item']),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,15 @@ import {
type BlockParameters,
liquidDocType,
} from '../../block-parameters';
import { BasicParamTypes, inferArgumentType, isTypeCompatible } from '../../liquid-doc/utils';
import { generateTypeMismatchSuggestions } from '../../liquid-doc/arguments';
import {
BasicParamTypes,
getArgumentTypeMismatchMessage,
inferArgumentType,
isArgumentTypeCompatible,
isTypeCompatible,
parseStringLiterals,
} from '../../liquid-doc/utils';
import * as path from '../../path';
import { isBlock } from '../../to-schema';
import { Severity, SourceCodeType, type Context, type LiquidCheckDefinition } from '../../types';
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<SourceCodeType.LiquidHtml>,
argument: BlockMarkup['args'][number],
expectedType: string,
): void {
if (isArgumentTypeCompatible(expectedType, argument.value) !== false) return;

const { start, end } = argument.value.position;
context.report({
message: getArgumentTypeMismatchMessage(argument.name, expectedType, argument.value),
startIndex: start,
endIndex: end,
suggest: generateTypeMismatchSuggestions(expectedType, start, end),
});
}

function reportLiquidDocTypeMismatch(
context: Context<SourceCodeType.LiquidHtml>,
node: LiquidDocParamNode,
Expand Down
Loading
Loading