diff --git a/.changeset/block-argument-diagnostic-range.md b/.changeset/block-argument-diagnostic-range.md new file mode 100644 index 000000000..53fe6a69d --- /dev/null +++ b/.changeset/block-argument-diagnostic-range.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme-check-common': patch +--- + +Report unsupported dotted `block` arguments on the first offending argument instead of the entire `block` tag and its body. diff --git a/packages/theme-check-common/src/checks/liquid-syntax-error/block.ts b/packages/theme-check-common/src/checks/liquid-syntax-error/block.ts index 981cbf4af..38ad7ba67 100644 --- a/packages/theme-check-common/src/checks/liquid-syntax-error/block.ts +++ b/packages/theme-check-common/src/checks/liquid-syntax-error/block.ts @@ -1,4 +1,10 @@ -import { NodeTypes, type BlockMarkup, type LiquidTag } from '@shopify/liquid-html-parser'; +import { + NodeTypes, + type BlockMarkup, + type LiquidTag, + type Position, +} from '@shopify/liquid-html-parser'; +import { Problem, SourceCodeType } from '../../types'; import type { Context } from '.'; import { hasBareArrayAccess, @@ -21,27 +27,36 @@ const BLOCK_PARSER_ERROR_MESSAGES = new Set([ ]); export function checkBlockTag(node: LiquidTag, context: Context): void { - const message = blockTagSyntaxError(node); - if (message) report(node, context, message); + const problem = blockTagSyntaxError(node); + if (problem) context.report(problem); } -export function blockTagSyntaxError(node: LiquidTag): string | undefined { - if (typeof node.markup === 'string') return SYNTAX_ERROR; +export function blockTagSyntaxError( + node: LiquidTag, +): Problem | undefined { + if (typeof node.markup === 'string') return syntaxProblem(node.position, SYNTAX_ERROR); const markup = node.markup as BlockMarkup; if (hasInvalidBlockName(markup.name.value)) { - return "Liquid syntax error: in 'block' - Valid syntax: block '[file_name]'"; + return syntaxProblem( + node.position, + "Liquid syntax error: in 'block' - Valid syntax: block '[file_name]'", + ); } /* * Ruby Liquid still accepts the unsupported experimental block.settings. * and block.content caller forms. Theme Check intentionally rejects every * dotted argument except block.name so authors move to plain arguments. + * Report the first one so body-form children stay outside the offense. */ - if (markup.args.some(isDottedArgument)) return DOTTED_ARGUMENT; + const dottedArgument = markup.args.find(isDottedArgument); + if (dottedArgument) return syntaxProblem(dottedArgument.position, DOTTED_ARGUMENT); - if (markup.args.some(isInvalidBlockNameArgument)) return SYNTAX_ERROR; + if (markup.args.some(isInvalidBlockNameArgument)) { + return syntaxProblem(node.position, SYNTAX_ERROR); + } /* * A +BlockArrayLiteral+ value (e.g. +size: [1, 2]+) is a first-class array @@ -54,10 +69,10 @@ export function blockTagSyntaxError(node: LiquidTag): string | undefined { (arg) => arg.value.type !== 'BlockArrayLiteral' && hasBareArrayAccess(arg.value), ) ) { - return BARE_ARRAY_ACCESS; + return syntaxProblem(node.position, BARE_ARRAY_ACCESS); } - if (hasSkippedCharacters(rawMarkup(node))) return SYNTAX_ERROR; + if (hasSkippedCharacters(rawMarkup(node))) return syntaxProblem(node.position, SYNTAX_ERROR); } export function checkBlockParserError(error: Error, context: Context, source: string): void { @@ -79,6 +94,10 @@ export function checkBlockParserError(error: Error, context: Context, source: st }); } +function syntaxProblem(position: Position, message: string): Problem { + return { message, startIndex: position.start, endIndex: position.end }; +} + function hasInvalidBlockName(value: string): boolean { return value.includes('/') || value.includes('.'); } @@ -90,11 +109,3 @@ function isInvalidBlockNameArgument(argument: BlockMarkup['args'][number]): bool function isDottedArgument(argument: BlockMarkup['args'][number]): boolean { return argument.name !== 'block.name' && argument.name.includes('.'); } - -function report(node: LiquidTag, context: Context, message: string): void { - context.report({ - message, - startIndex: node.position.start, - endIndex: node.position.end, - }); -} diff --git a/packages/theme-check-common/src/checks/liquid-syntax-error/index.spec.ts b/packages/theme-check-common/src/checks/liquid-syntax-error/index.spec.ts index f2fbd8184..5c5abee28 100644 --- a/packages/theme-check-common/src/checks/liquid-syntax-error/index.spec.ts +++ b/packages/theme-check-common/src/checks/liquid-syntax-error/index.spec.ts @@ -811,12 +811,13 @@ describe('LiquidSyntaxError', () => { describe('block caller arguments', () => { it.each([ - "{% block 'card', block.settings.heading: 'Heading' %}{% endblock %}", - "{% block 'card', block.content: body %}{% endblock %}", - "{% block 'card', block.settings.heading.label: 'Heading' %}{% endblock %}", - "{% block 'card', block.unknown: 'value' %}{% endblock %}", - "{% block 'card', heading.label: 'Heading' %}{% endblock %}", - ])('rejects unsupported experimental dotted arguments in %s', async (template) => { + "block.settings.heading: 'Heading'", + 'block.content: body', + "block.settings.heading.label: 'Heading'", + "block.unknown: 'value'", + "heading.label: 'Heading'", + ])('rejects the unsupported dotted argument %s', async (argument) => { + const template = `{% block 'card', ${argument} %}{% endblock %}`; const offenses = await runLiquidCheck( LiquidSyntaxError, template, @@ -826,10 +827,47 @@ describe('LiquidSyntaxError', () => { expect(offenses).toMatchObject([ { - message: - "Liquid syntax error: in 'block' - Use plain named arguments, for example: block 'name', heading: value", - start: { index: 0 }, - end: { index: template.length }, + message: DOTTED_ARGUMENT, + start: { index: template.indexOf(argument) }, + end: { index: template.indexOf(argument) + argument.length }, + }, + ]); + }); + + it('reports only the dotted argument in a body-form block', async () => { + const argument = "block.settings.heading: 'Heading'"; + const template = `{% block 'card', ${argument} %}\n Child content\n{% endblock %}`; + const offenses = await runLiquidCheck( + LiquidSyntaxError, + template, + 'templates/test.liquid', + NO_DOCSET, + ); + + expect(offenses).toMatchObject([ + { + message: DOTTED_ARGUMENT, + start: { index: template.indexOf(argument) }, + end: { index: template.indexOf(argument) + argument.length }, + }, + ]); + }); + + it('reports only the first of several dotted arguments', async () => { + const first = "block.settings.heading: 'Heading'"; + const template = `{% block 'card', title: 'Title', ${first}, block.content: body, heading.label: 'Label' %}Child{% endblock %}`; + const offenses = await runLiquidCheck( + LiquidSyntaxError, + template, + 'templates/test.liquid', + NO_DOCSET, + ); + + expect(offenses).toMatchObject([ + { + message: DOTTED_ARGUMENT, + start: { index: template.indexOf(first) }, + end: { index: template.indexOf(first) + first.length }, }, ]); }); @@ -847,8 +885,8 @@ describe('LiquidSyntaxError', () => { it.each([ "{% block 'card', block.settings %}{% endblock %}", - "{% block 'card', block.name: value %}{% endblock %}", - ])('reports other malformed block tags in %s', async (template) => { + "{% block 'card', block.name: value %}Child content{% endblock %}", + ])('reports other malformed block tags on the whole tag in %s', async (template) => { const offenses = await runLiquidCheck( LiquidSyntaxError, template, @@ -856,7 +894,13 @@ describe('LiquidSyntaxError', () => { NO_DOCSET, ); - expect(offenses).toMatchObject([{ message: "Syntax error in 'block' tag" }]); + expect(offenses).toMatchObject([ + { + message: "Syntax error in 'block' tag", + start: { index: 0 }, + end: { index: template.length }, + }, + ]); }); describe('with the block parameter checks', () => { @@ -902,6 +946,7 @@ describe('LiquidSyntaxError', () => { ["heading.label: 'Hello'", DOTTED_ARGUMENT], ['block.name: value', "Syntax error in 'block' tag"], ["block.name: value, heading.label: 'Hello'", DOTTED_ARGUMENT], + ["block.settings.heading: 'Hello', block.content: body", DOTTED_ARGUMENT], ])('reports only one syntax error for %s', async (argument, message) => { const offenses = await checkBlockCall(`${argument}, count: 'many', count: 'more'`);