Skip to content

Commit fff687a

Browse files
aswamyaswamy
authored
Merge pull request #1045 from Shopify/check-to-fix-loop-errors
Add check for InvalidLoopArguments
2 parents 0380e43 + 0c0f6c0 commit fff687a

File tree

10 files changed

+429
-28
lines changed

10 files changed

+429
-28
lines changed

‎.changeset/dry-hounds-smoke.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/theme-check-common': patch
3+
---
4+
5+
Add check for invalid loop arguments for `for` and `tablerow` tags

‎packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidEchoValue.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ describe('detectInvalidEchoValue', async () => {
9393
expect(toLiquidAST).toHaveBeenCalledTimes(2);
9494
expect(toLiquidAST).toHaveBeenCalledWith(`{% echo one %}`, {
9595
allowUnclosedDocumentNode: false,
96-
mode: 'strict',
96+
mode: 'tolerant',
9797
});
9898

9999
const fixed = applyFix(sourceCode, offenses[0]);

‎packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidLoopArguments.spec.ts‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
import { expect, describe, it } from 'vitest';
2+
import { applyFix, runLiquidCheck } from '../../../test';
3+
import { LiquidHTMLSyntaxError } from '../index';
4+
5+
describe('detectInvalidLoopArguments', async () => {
6+
it('should not report when args are valid', async () => {
7+
const testCases = [
8+
`{% for i in array reversed %}{% endfor %}`,
9+
`{% for i in array reversed offset: 1 %}{% endfor %}`,
10+
`{% tablerow x in array limit: 10 %}{% endtablerow %}`,
11+
`{% tablerow x in array cols: 2 limit: 10 %}{% endtablerow %}`,
12+
];
13+
14+
for (const sourceCode of testCases) {
15+
const offenses = await testCheck(sourceCode);
16+
expect(offenses).to.have.length(0);
17+
}
18+
});
19+
20+
it('should report when invalid args are found', async () => {
21+
const testCases = [
22+
[
23+
`{% for i in array limit1: 10 %}{% endfor %}`,
24+
'{% for i in array %}{% endfor %}',
25+
'limit1: 10',
26+
],
27+
[
28+
`{% for i in array !! range: (1..2) %}{% endfor %}`,
29+
'{% for i in array %}{% endfor %}',
30+
'!!, range: (1..2)',
31+
],
32+
[
33+
`{% tablerow i in (1..10) limit1: 10 %}{{ i }}{% endtablerow %}`,
34+
'{% tablerow i in (1..10) %}{{ i }}{% endtablerow %}',
35+
'limit1: 10',
36+
],
37+
];
38+
39+
for (const [sourceCode, expected, invalidArguments] of testCases) {
40+
const offenses = await testCheck(sourceCode);
41+
expect(offenses).to.have.length(1);
42+
expect(offenses[0].message).toEqual(
43+
expect.stringContaining(`Invalid/Unknown arguments: ${invalidArguments}`),
44+
);
45+
46+
const fixed = applyFix(sourceCode, offenses[0]);
47+
expect(fixed).to.equal(expected);
48+
}
49+
});
50+
51+
it('should report when positional args are found after named/invalid args', async () => {
52+
const testCases = [
53+
[
54+
`{% for i in array limit: 10 reversed %}{% endfor %}`,
55+
'{% for i in array limit: 10 %}{% endfor %}',
56+
'reversed',
57+
],
58+
[
59+
`{% for i in array reversed limit: 10 invalid %}{% endfor %}`,
60+
'{% for i in array reversed limit: 10 %}{% endfor %}',
61+
'invalid',
62+
],
63+
[
64+
`{% for i in array !! reversed limit: 10 invalid %}{% endfor %}`,
65+
'{% for i in array limit: 10 %}{% endfor %}',
66+
'!!, reversed, invalid',
67+
],
68+
[
69+
`{% for i in array reversed: (1..2) %}{% endfor %}`,
70+
'{% for i in array %}{% endfor %}',
71+
'reversed: (1..2)',
72+
],
73+
];
74+
75+
for (const [sourceCode, expected, invalidArguments] of testCases) {
76+
const offenses = await testCheck(sourceCode);
77+
expect(offenses).to.have.length(1);
78+
expect(offenses[0].message).toEqual(
79+
expect.stringContaining(`Invalid/Unknown arguments: ${invalidArguments}`),
80+
);
81+
82+
const fixed = applyFix(sourceCode, offenses[0]);
83+
expect(fixed).to.equal(expected);
84+
}
85+
});
86+
87+
it('should report when there are multiple instances of the error', async () => {
88+
const sourceCode = `{% for i in array reversed limit: 10 invalid %}{% endfor %} {% tablerow x in array cols: 2 limit: 10 invalid %}{% endtablerow %}`;
89+
90+
const offenses = await testCheck(sourceCode);
91+
expect(offenses).to.have.length(2);
92+
for (const offense of offenses) {
93+
expect(offense.message).toEqual(
94+
expect.stringContaining(`Invalid/Unknown arguments: invalid`),
95+
);
96+
}
97+
98+
const fixed = applyFix(sourceCode, offenses[0]);
99+
expect(fixed).to.equal(
100+
'{% for i in array reversed limit: 10 %}{% endfor %} {% tablerow x in array cols: 2 limit: 10 invalid %}{% endtablerow %}',
101+
);
102+
103+
const fixed2 = applyFix(sourceCode, offenses[1]);
104+
expect(fixed2).to.equal(
105+
'{% for i in array reversed limit: 10 invalid %}{% endfor %} {% tablerow x in array cols: 2 limit: 10 %}{% endtablerow %}',
106+
);
107+
});
108+
});
109+
110+
function testCheck(sourceCode: string) {
111+
return runLiquidCheck(LiquidHTMLSyntaxError, sourceCode, undefined, {
112+
themeDocset: {
113+
tags: () =>
114+
Promise.resolve([
115+
{
116+
name: 'for',
117+
parameters: [
118+
{
119+
name: 'reversed',
120+
positional: true,
121+
description: '...',
122+
required: false,
123+
types: [],
124+
},
125+
{ name: 'limit', positional: false, description: '...', required: false, types: [] },
126+
{ name: 'offset', positional: false, description: '...', required: false, types: [] },
127+
],
128+
},
129+
{
130+
name: 'tablerow',
131+
parameters: [
132+
{ name: 'cols', positional: false, description: '...', required: false, types: [] },
133+
{ name: 'limit', positional: false, description: '...', required: false, types: [] },
134+
{ name: 'offset', positional: false, description: '...', required: false, types: [] },
135+
],
136+
},
137+
]),
138+
filters: () => Promise.resolve([]),
139+
objects: () => Promise.resolve([]),
140+
liquidDrops: () => Promise.resolve([]),
141+
systemTranslations: () => Promise.resolve({}),
142+
},
143+
});
144+
}

‎packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidLoopArguments.ts‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
import {
2+
ForMarkup,
3+
LiquidTag,
4+
LiquidTagFor,
5+
LiquidTagTablerow,
6+
NodeTypes,
7+
} from '@shopify/liquid-html-parser';
8+
import { Problem, SourceCodeType, TagEntry } from '../../..';
9+
import {
10+
doesFragmentContainUnsupportedParentheses,
11+
getFragmentsInMarkup,
12+
fragmentKeyValuePair,
13+
} from './utils';
14+
import { isLoopLiquidTag } from '../../utils';
15+
16+
export function detectInvalidLoopArguments(
17+
node: LiquidTag,
18+
tags: TagEntry[] = [],
19+
): Problem<SourceCodeType.LiquidHtml> | undefined {
20+
if (node.type === NodeTypes.LiquidTag && !isLoopLiquidTag(node)) {
21+
return;
22+
}
23+
24+
let markup: ForMarkup | string = (node as LiquidTagFor | LiquidTagTablerow).markup;
25+
26+
if (!markup) {
27+
return;
28+
}
29+
30+
if (typeof markup !== 'string') {
31+
markup = markup.source.slice(markup.position.start, markup.position.end);
32+
}
33+
34+
const fragments = getFragmentsInMarkup(markup);
35+
36+
// The first three fragments are: `<variable> in <array>`
37+
// If they haven't started typing the tag, we don't want to report an error.
38+
if (fragments.length <= 3) {
39+
return;
40+
}
41+
42+
const markupIndex = node.source.indexOf(markup, node.position.start);
43+
let startIndex = markupIndex + fragments.at(3)!.index!;
44+
let endIndex = markupIndex + fragments.at(-1)!.index! + fragments.at(-1)!.value.length;
45+
46+
const filteredFragments: string[] = [];
47+
48+
// Positional arguments are ignored if they come after invalid positional args or named args
49+
let noMorePositionalArguments = false;
50+
const invalidFragments: string[] = [];
51+
52+
for (let i = 3; i < fragments.length; i++) {
53+
const fragment = fragments[i];
54+
55+
if (doesFragmentContainUnsupportedParentheses(fragment.value)) {
56+
noMorePositionalArguments = true;
57+
invalidFragments.push(fragment.value);
58+
continue;
59+
}
60+
61+
const keyValuePair = fragmentKeyValuePair(fragment.value);
62+
63+
// Named arg is found
64+
if (keyValuePair) {
65+
noMorePositionalArguments = true;
66+
67+
if (!isSupportedTagArgument(tags, node.name, keyValuePair.key, false)) {
68+
invalidFragments.push(fragment.value);
69+
continue;
70+
}
71+
}
72+
// Unsupported positional arg is found
73+
else if (
74+
noMorePositionalArguments ||
75+
!isSupportedTagArgument(tags, node.name, fragment.value, true)
76+
) {
77+
noMorePositionalArguments = true;
78+
invalidFragments.push(fragment.value);
79+
continue;
80+
}
81+
82+
filteredFragments.push(fragment.value);
83+
}
84+
85+
if (invalidFragments.length === 0) {
86+
return;
87+
}
88+
89+
return {
90+
message: `Arguments must be provided in the format \`${
91+
node.name
92+
} in <array> <positional arguments> <named arguments>\`. Invalid/Unknown arguments: ${invalidFragments.join(
93+
', ',
94+
)}`,
95+
startIndex,
96+
endIndex,
97+
fix: (corrector) => {
98+
corrector.replace(startIndex, endIndex, filteredFragments.join(' '));
99+
},
100+
};
101+
}
102+
103+
function isSupportedTagArgument(
104+
tags: TagEntry[],
105+
tagName: string,
106+
key: string,
107+
positional: boolean,
108+
) {
109+
return (
110+
tags
111+
.find((tag) => tag.name === tagName)
112+
?.parameters?.some(
113+
(parameter) => parameter.name === key && parameter.positional === positional,
114+
) || false
115+
);
116+
}

‎packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidLoopRange.spec.ts‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,22 @@ describe('detectInvalidLoopRange', async () => {
6161
}
6262
});
6363

64+
it('should report when the range contains a real number', async () => {
65+
const sourceCode = `{% for i in (-2.9..2.9) %}{% endfor %}`;
66+
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
67+
expect(offenses).to.have.length(1);
68+
expect(offenses[0].message).to.equal(INVALID_LOOP_RANGE_MESSAGE);
69+
70+
const fixed = applyFix(sourceCode, offenses[0]);
71+
expect(fixed).to.equal(`{% for i in (-2..2) %}{% endfor %}`);
72+
});
73+
74+
it('should report when the range contains a nested variable', async () => {
75+
const sourceCode = `{% for i in (some.var..some.other.var) %}{% endfor %}`;
76+
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
77+
expect(offenses).to.have.length(0);
78+
});
79+
6480
it('should report when there are multiple instances of the error', async () => {
6581
const sourceCode = `{% for i in (1...10) %}{% endfor %} {% tablerow x in (a...b) %}{% endtablerow %}`;
6682

@@ -72,11 +88,11 @@ describe('detectInvalidLoopRange', async () => {
7288
expect(toLiquidAST).toHaveBeenCalledTimes(2);
7389
expect(toLiquidAST).toHaveBeenCalledWith(`{% for i in (1..10) %}{% endfor %}`, {
7490
allowUnclosedDocumentNode: false,
75-
mode: 'strict',
91+
mode: 'tolerant',
7692
});
7793
expect(toLiquidAST).toHaveBeenCalledWith(`{% tablerow x in (a..b) %}{% endtablerow %}`, {
7894
allowUnclosedDocumentNode: false,
79-
mode: 'strict',
95+
mode: 'tolerant',
8096
});
8197

8298
const fixed = applyFix(sourceCode, offenses[0]);

‎packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidLoopRange.ts‎

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,10 @@
11
import { LiquidTag, LiquidTagFor, LiquidTagTablerow, NodeTypes } from '@shopify/liquid-html-parser';
22
import { Problem, SourceCodeType } from '../../..';
3-
import { ensureValidAst } from './utils';
3+
import { ensureValidAst, getRangeMatch } from './utils';
44
import { isLoopLiquidTag } from '../../utils';
55

6-
// Lax parser does NOT complain about leading/trailing spaces inside ranges (e.g. `(1 .. 10 )`) and
7-
// within the parenthesis (e.g. `( 1 .. 10 )`), but fails to render the liquid when using the gem.
8-
// Strict parser does NOT complain, but still renders it.
9-
// To avoid any issues, we will remove extra spaces.
10-
const RANGE_MARKUP_REGEX = /\(\s*(\w+)\s*(\.{2,})\s*(\w+)\s*\)/;
11-
126
export const INVALID_LOOP_RANGE_MESSAGE =
13-
'Ranges must be in the following format: (<start>..<end>)';
7+
'Ranges must be in the format `(<start>..<end>)`. The start and end of the range must be whole numbers or variables.';
148

159
export function detectInvalidLoopRange(
1610
node: LiquidTag,
@@ -41,23 +35,33 @@ function validateMarkup(
4135
node: LiquidTag,
4236
markup: string,
4337
): Problem<SourceCodeType.LiquidHtml> | undefined {
44-
const match = markup.match(RANGE_MARKUP_REGEX);
38+
const match = getRangeMatch(markup);
4539
if (!match || match.index === undefined) {
4640
return;
4741
}
4842

49-
const [, start, , end] = match;
43+
const [fullMatch, start, , end] = match;
44+
45+
let startCleaned = start;
46+
let endCleaned = end;
47+
48+
if (!isNaN(Number(start)) && Number(start) % 1 !== 0) {
49+
startCleaned = `${Math.trunc(Number(start))}`;
50+
}
51+
if (!isNaN(Number(end)) && Number(end) % 1 !== 0) {
52+
endCleaned = `${Math.trunc(Number(end))}`;
53+
}
5054

51-
const expectedRangeMarkup = `(${start}..${end})`;
55+
const expectedRangeMarkup = `(${startCleaned}..${endCleaned})`;
5256

53-
if (markup.slice(match.index) === expectedRangeMarkup) {
57+
if (markup.slice(match.index, match.index + fullMatch.length) === expectedRangeMarkup) {
5458
return;
5559
}
5660

5761
const markupIndex = node.source.indexOf(markup, node.position.start);
5862

5963
const startIndex = markupIndex + match.index;
60-
const endIndex = markupIndex + markup.length;
64+
const endIndex = startIndex + fullMatch.length;
6165

6266
if (
6367
!ensureValidAst(

‎packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/MultipleAssignValues.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ describe('detectMultipleAssignValues', async () => {
5959
expect(toLiquidAST).toHaveBeenCalledTimes(2);
6060
expect(toLiquidAST).toHaveBeenCalledWith(`{% assign foo = '123' %}`, {
6161
allowUnclosedDocumentNode: false,
62-
mode: 'strict',
62+
mode: 'tolerant',
6363
});
6464

6565
const fixed = applyFix(sourceCode, offenses[0]);

0 commit comments

Comments
 (0)