Skip to content

Commit 345a6b6

Browse files
karreirokarreiro
committed
Update liquid-html-syntax-error check
1 parent 2063dd8 commit 345a6b6

42 files changed

Lines changed: 5977 additions & 17 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.changeset/blue-comics-return.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+
Improve LiquidHTMLSyntaxError parity with Ruby Liquid syntax errors

‎packages/theme-check-common/package.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
"@types/line-column": "^1.0.0",
4343
"@types/lodash": "^4.17.20",
4444
"@types/postcss-safe-parser": "^5.0.4",
45-
"@vitest/expect": "2.1.9"
45+
"@vitest/expect": "2.1.9",
46+
"yaml": "^2.8.3"
4647
}
4748
}

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

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
3131
'{% if 1 %}hello{% endif %}',
3232
'{% if 0 %}hello{% endif %}',
3333
"{% if 'string' %}hello{% endif %}",
34-
'{% if contains %}hello{% endif %}',
3534
];
3635

3736
for (const testCase of testCases) {
@@ -370,6 +369,39 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
370369
}
371370
});
372371

372+
it('should report an offense for invalid operators in unless expressions', async () => {
373+
const testCases = [
374+
{
375+
source: '{% unless x && y %}{% endunless %}',
376+
expectedMessage:
377+
"Conditional is invalid. Anything after 'x' will be ignored. Use 'and'/'or' instead of '&&'/'||' for multiple conditions",
378+
},
379+
{
380+
source: '{% unless x || y %}{% endunless %}',
381+
expectedMessage:
382+
"Conditional is invalid. Anything after 'x' will be ignored. Use 'and'/'or' instead of '&&'/'||' for multiple conditions",
383+
},
384+
{
385+
source: '{% unless x startswith "foo" %}{% endunless %}',
386+
expectedMessage:
387+
"Conditional is invalid. Anything after 'x' will be ignored: 'startswith \"foo\"'",
388+
},
389+
{
390+
source: '{% unless x === y %}{% endunless %}',
391+
expectedMessage: "Conditional is invalid. Anything after 'x' will be ignored: '=== y'",
392+
},
393+
];
394+
395+
for (const testCase of testCases) {
396+
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, testCase.source);
397+
expect(offenses).to.have.length(1);
398+
expect(offenses[0].message).to.equal(`Syntax is not supported: ${testCase.expectedMessage}`);
399+
400+
const fixed = applyFix(testCase.source, offenses[0]);
401+
expect(fixed).to.equal('{% unless x %}{% endunless %}');
402+
}
403+
});
404+
373405
it('should NOT report an offense for valid logical operators', async () => {
374406
const validCases = [
375407
'{% if price > 100 and discount < 50 %}hello{% endif %}',

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

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,10 @@ function isOperatorToken(token: Token): boolean {
6868
return token.type === 'logical' || token.type === 'comparison';
6969
}
7070

71+
function isUnsupportedOperatorToken(token: Token): boolean {
72+
return !isOperatorToken(token) && /^(&&|\|\||===|startswith)$/.test(token.value);
73+
}
74+
7175
function checkInvalidStartingToken(tokens: Token[]): ExpressionIssue | null {
7276
const firstToken = tokens[0];
7377
if (firstToken.type === 'invalid' || firstToken.type === 'comparison') {
@@ -146,6 +150,38 @@ function checkLaxParsingIssues(tokens: Token[]): ExpressionIssue | null {
146150
return null;
147151
}
148152

153+
function checkInvalidOperatorsAfterValue(tokens: Token[]): ExpressionIssue | null {
154+
for (let i = 1; i < tokens.length; i++) {
155+
const current = tokens[i];
156+
const previous = tokens[i - 1];
157+
158+
if (!isValueToken(previous) || !isUnsupportedOperatorToken(current)) continue;
159+
160+
const validExpr = tokens
161+
.slice(0, i)
162+
.map((t) => t.value)
163+
.join(' ');
164+
const ignored = tokens
165+
.slice(i)
166+
.map((t) => t.value)
167+
.join(' ');
168+
169+
if (/&&|\|\|/.test(ignored)) {
170+
return {
171+
message: `Conditional is invalid. Anything after '${validExpr}' will be ignored. Use 'and'/'or' instead of '&&'/'||' for multiple conditions`,
172+
fix: validExpr,
173+
};
174+
}
175+
176+
return {
177+
message: `Conditional is invalid. Anything after '${validExpr}' will be ignored: '${ignored}'`,
178+
fix: validExpr,
179+
};
180+
}
181+
182+
return null;
183+
}
184+
149185
function analyzeConditionalExpression(markup: string): ExpressionIssue | null {
150186
const trimmed = markup.trim();
151187
if (!trimmed) return null;
@@ -160,6 +196,7 @@ function analyzeConditionalExpression(markup: string): ExpressionIssue | null {
160196
return (
161197
checkInvalidStartingToken(tokens) ||
162198
checkTrailingTokensAfterComparison(tokens) ||
163-
checkLaxParsingIssues(tokens)
199+
checkLaxParsingIssues(tokens) ||
200+
checkInvalidOperatorsAfterValue(tokens)
164201
);
165202
}

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

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
import type { LiquidTag, AssignMarkup } from './parser-compat';
2+
import type { Context } from './context';
3+
import {
4+
variableHasBareArrayAccess,
5+
hasSkippedCharacters,
6+
hasSkippedPrefixCharacters,
7+
hasRubyAcceptedEmptyFirstFilterArgument,
8+
hasRubyAcceptedEmptyAssignRhs,
9+
hasRubyAcceptedAssignLhsExtraIdentifier,
10+
hasRubyAcceptedAssignLhsUnclosedQuote,
11+
variableHasBareContainsValueExpression,
12+
rawMarkup,
13+
} from './utils';
14+
15+
export function checkAssignTag(node: LiquidTag, context: Context): void {
16+
if (typeof node.markup === 'string') {
17+
if (hasRubyAcceptedEmptyAssignRhs(node.markup)) return;
18+
if (hasRubyAcceptedEmptyFirstFilterArgument(node.markup)) return;
19+
if (hasRubyAcceptedAssignLhsExtraIdentifier(node.markup)) return;
20+
if (hasRubyAcceptedAssignLhsUnclosedQuote(node.markup)) return;
21+
22+
context.report({
23+
message: `Syntax error in 'assign' tag`,
24+
startIndex: node.position.start,
25+
endIndex: node.position.end,
26+
});
27+
return;
28+
}
29+
30+
const markup = node.markup as AssignMarkup;
31+
32+
if (variableHasBareArrayAccess(markup.value)) {
33+
context.report({
34+
message: 'Bare bracket access is not allowed',
35+
startIndex: node.position.start,
36+
endIndex: node.position.end,
37+
});
38+
return;
39+
}
40+
41+
if (variableHasBareContainsValueExpression(markup.value)) {
42+
context.report({
43+
message: `Syntax error in 'assign' tag`,
44+
startIndex: node.position.start,
45+
endIndex: node.position.end,
46+
});
47+
return;
48+
}
49+
50+
if (hasSkippedPrefixCharacters(node.source, node.markupPosition.start, markup.position.start)) {
51+
context.report({
52+
message: `Syntax error in 'assign' tag`,
53+
startIndex: node.position.start,
54+
endIndex: node.position.end,
55+
});
56+
return;
57+
}
58+
59+
const raw = rawMarkup(node);
60+
if (hasRubyAcceptedEmptyAssignRhs(raw)) return;
61+
if (hasRubyAcceptedEmptyFirstFilterArgument(raw)) return;
62+
if (hasRubyAcceptedAssignLhsExtraIdentifier(raw)) return;
63+
if (hasRubyAcceptedAssignLhsUnclosedQuote(raw)) return;
64+
65+
const parsedRaw = node.source.slice(markup.position.start, node.markupPosition.end);
66+
if (hasSkippedCharacters(parsedRaw)) {
67+
context.report({
68+
message: `Syntax error in 'assign' tag`,
69+
startIndex: node.position.start,
70+
endIndex: node.position.end,
71+
});
72+
}
73+
}

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

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import type { LiquidTag } from './parser-compat';
2+
import type { Context } from './context';
3+
4+
const BUILTIN_TAGS = [
5+
'assign',
6+
'break',
7+
'capture',
8+
'case',
9+
'comment',
10+
'continue',
11+
'content_for',
12+
'cycle',
13+
'decrement',
14+
'doc',
15+
'echo',
16+
'elsif',
17+
'for',
18+
'form',
19+
'if',
20+
'ifchanged',
21+
'include',
22+
'increment',
23+
'javascript',
24+
'layout',
25+
'liquid',
26+
'paginate',
27+
'raw',
28+
'render',
29+
'schema',
30+
'section',
31+
'sections',
32+
'style',
33+
'stylesheet',
34+
'tablerow',
35+
'unless',
36+
'when',
37+
];
38+
39+
const KNOWN_LIQUID_TAGS = new Set(['#', ...BUILTIN_TAGS]);
40+
41+
export function checkBaseTag(node: LiquidTag, context: Context): void {
42+
if ('reason' in node && typeof node.reason === 'string') {
43+
context.report({
44+
message: node.reason,
45+
startIndex: node.position.start,
46+
endIndex: node.position.end,
47+
});
48+
return;
49+
}
50+
51+
if (isUnknownTagInsideLiquidBlock(node)) {
52+
context.report({
53+
message: `Unknown tag '${node.name}'`,
54+
startIndex: node.position.start,
55+
endIndex: node.position.end,
56+
});
57+
}
58+
}
59+
60+
function isUnknownTagInsideLiquidBlock(node: LiquidTag): boolean {
61+
// Statements inside `{% liquid %}` do not have their own `{%` delimiter.
62+
return !node.source.startsWith('{%', node.position.start) && !KNOWN_LIQUID_TAGS.has(node.name);
63+
}

0 commit comments

Comments
 (0)