Skip to content

Commit 6f32cc2

Browse files
aswamyaswamy
authored
Merge pull request #1033 from Shopify/remove-trailing-assign-values
Remove multiple assign values
2 parents 6a1512d + e1bf12c commit 6f32cc2

File tree

8 files changed

+263
-33
lines changed

8 files changed

+263
-33
lines changed

‎.changeset/nasty-cameras-laugh.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+
Introduce fix for invalid `assign` tag values

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
import { describe, expect, it } from 'vitest';
2+
import { applyFix, runLiquidCheck } from '../../../test';
3+
import { LiquidHTMLSyntaxError } from '..';
4+
5+
describe('detectTrailingAssignValue', async () => {
6+
it('should not report when there are no trailing values', async () => {
7+
const testCases = [`{% assign foo = '123' %}`, `{% assign foo = '123' | upcase %}`];
8+
9+
for (const sourceCode of testCases) {
10+
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
11+
expect(offenses).to.have.length(0);
12+
}
13+
});
14+
15+
it('should report all use of boolean expressions', async () => {
16+
const testCases = [
17+
[`{% assign foo = something == else %}`, '{% assign foo = something %}'],
18+
[`{% echo foo != bar %}`, '{% echo foo %}'],
19+
[`{{ this > that }}`, '{{ this }}'],
20+
[`{{ bool and cond }}`, '{{ bool}}'],
21+
];
22+
23+
for (const [sourceCode, expected] of testCases) {
24+
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
25+
expect(offenses).to.have.length(1);
26+
expect(offenses[0].message).to.equal('Syntax is not supported');
27+
28+
const fixed = applyFix(sourceCode, offenses[0]);
29+
expect(fixed).to.equal(expected);
30+
}
31+
});
32+
});

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
import { NodeTypes } from '@shopify/liquid-html-parser';
2+
import { Check, Context, SourceCodeType } from '../../..';
3+
import { INVALID_SYNTAX_MESSAGE } from './utils';
4+
5+
export function detectInvalidBooleanExpressions(
6+
context: Context<SourceCodeType.LiquidHtml>,
7+
): Check<SourceCodeType.LiquidHtml> {
8+
return {
9+
async BooleanExpression(node) {
10+
const condition = node.condition;
11+
12+
if (
13+
condition.type !== NodeTypes.Comparison &&
14+
condition.type !== NodeTypes.LogicalExpression
15+
) {
16+
return;
17+
}
18+
19+
context.report({
20+
message: INVALID_SYNTAX_MESSAGE,
21+
startIndex: node.position.start,
22+
endIndex: node.position.end,
23+
fix: (corrector) => {
24+
corrector.replace(
25+
node.position.start,
26+
node.position.end,
27+
node.source.slice(condition.left.position.start, condition.left.position.end),
28+
);
29+
},
30+
});
31+
},
32+
};
33+
}

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

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
import { expect, describe, it, vi } from 'vitest';
2+
import { applyFix, runLiquidCheck } from '../../../test';
3+
import { LiquidHTMLSyntaxError } from '../index';
4+
import { toLiquidAST } from '@shopify/liquid-html-parser';
5+
6+
vi.mock('@shopify/liquid-html-parser', async (importOriginal) => {
7+
const original: any = await importOriginal();
8+
9+
return {
10+
...original,
11+
12+
// we will be mocked later on
13+
toLiquidAST: vi.fn().mockImplementation((source, options) => {
14+
return original.toLiquidAST(source, options);
15+
}),
16+
};
17+
});
18+
19+
describe('detectTrailingAssignValue', async () => {
20+
it('should not report when there are no trailing values', async () => {
21+
const testCases = [`{% assign foo = '123' %}`, `{% assign foo = '123' | upcase %}`];
22+
23+
for (const sourceCode of testCases) {
24+
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
25+
expect(offenses).to.have.length(0);
26+
}
27+
});
28+
29+
it('should report when there are multiple values (no filters)', async () => {
30+
const testCases = [
31+
[`{% assign foo = '123' 555 text %}`, "{% assign foo = '123' %}"],
32+
[`{% assign foo = "123" 555 text %}`, '{% assign foo = "123" %}'],
33+
[`{% assign foo = 123 555 text %}`, '{% assign foo = 123 %}'],
34+
[`{% assign foo = true 555 text %}`, '{% assign foo = true %}'],
35+
];
36+
37+
for (const [sourceCode, expected] of testCases) {
38+
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
39+
expect(offenses).to.have.length(1);
40+
expect(offenses[0].message).to.equal('Syntax is not supported');
41+
42+
const fixed = applyFix(sourceCode, offenses[0]);
43+
expect(fixed).to.equal(expected);
44+
}
45+
});
46+
47+
it('should report when there are multiple values (with filters)', async () => {
48+
const testCases = [
49+
[`{% assign foo = '123' 555 text | upcase %}`, "{% assign foo = '123' | upcase %}"],
50+
[`{% assign foo = "123" 555 text | upcase %}`, '{% assign foo = "123" | upcase %}'],
51+
[`{% assign foo = 123 555 text | default: 0 %}`, '{% assign foo = 123 | default: 0 %}'],
52+
[
53+
`{% assign foo = true 555 text | fake-filter: 'yes' %}`,
54+
"{% assign foo = true | fake-filter: 'yes' %}",
55+
],
56+
];
57+
58+
for (const [sourceCode, expected] of testCases) {
59+
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
60+
expect(offenses).to.have.length(1);
61+
expect(offenses[0].message).to.equal('Syntax is not supported');
62+
63+
const fixed = applyFix(sourceCode, offenses[0]);
64+
expect(fixed).to.equal(expected);
65+
}
66+
});
67+
68+
it('should not report when the fixed code produces an invalid AST', async () => {
69+
const sourceCode = `{% assign foo = '123' 555 text %}`;
70+
71+
vi.mocked(toLiquidAST).mockImplementation((_source, _options) => {
72+
throw new SyntaxError('Invalid AST');
73+
});
74+
75+
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
76+
expect(offenses).to.have.length(0);
77+
});
78+
});

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

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
import { Check, Context, SourceCodeType } from '../../..';
2+
import { ensureValidAst, INVALID_SYNTAX_MESSAGE } from './utils';
3+
4+
export function detectMultipleAssignValues(
5+
context: Context<SourceCodeType.LiquidHtml>,
6+
): Check<SourceCodeType.LiquidHtml> {
7+
// Using a regex to match the markup like we do in Shopify/liquid
8+
// https://github.com/Shopify/liquid/blob/9bb7fbf123e6e2bd61e00189b1c83159f375d3f3/lib/liquid/tags/assign.rb#L21
9+
//
10+
// We've broken it up into four groups:
11+
// 1. The variable name
12+
// 2. The assignment operator
13+
// 3. The value section
14+
// 4. The filter section (non-captured)
15+
const ASSIGN_MARKUP_REGEX = /([^=]+)(=\s*)([^|]+)(?:\s*\|\s*.*)?$/m;
16+
17+
return {
18+
async LiquidTag(node) {
19+
if (node.name !== 'assign') {
20+
return;
21+
}
22+
23+
const markup = node.markup;
24+
25+
if (typeof markup !== 'string') {
26+
return;
27+
}
28+
29+
const match = markup.match(ASSIGN_MARKUP_REGEX);
30+
if (!match) {
31+
return;
32+
}
33+
34+
// If we have a markup 'foo = "123" something | upcase: 123', we have the following groups
35+
const [
36+
// 'foo = "123" something | upcase: 123'
37+
_fullMatch,
38+
// 'foo '
39+
assignmentVariable,
40+
// '= '
41+
assignmentOperator,
42+
// '"123" something'
43+
assignmentValue,
44+
] = match;
45+
46+
// Only capture the first item in the value section
47+
const firstValueMatch = assignmentValue.match(/"[^"]*"|'[^']*'|\S+/);
48+
const firstAssignmentValue = firstValueMatch ? firstValueMatch[0] : null;
49+
50+
if (!firstAssignmentValue) {
51+
return;
52+
}
53+
54+
const removalIndices = (source: string) => {
55+
const offset = source.indexOf(markup);
56+
57+
return {
58+
startIndex:
59+
offset +
60+
assignmentVariable.length +
61+
assignmentOperator.length +
62+
firstAssignmentValue.length,
63+
endIndex:
64+
offset +
65+
assignmentVariable.length +
66+
assignmentOperator.length +
67+
assignmentValue.trimEnd().length,
68+
};
69+
};
70+
71+
const tagSource = node.source.slice(node.position.start, node.position.end);
72+
const { startIndex: tagSourceRemovalStartIndex, endIndex: tagSourceRemovalEndIndex } =
73+
removalIndices(tagSource);
74+
75+
if (
76+
!ensureValidAst(
77+
tagSource.slice(0, tagSourceRemovalStartIndex) +
78+
tagSource.slice(tagSourceRemovalEndIndex),
79+
)
80+
) {
81+
// If the new AST is invalid, we don't want to auto-fix it
82+
return;
83+
}
84+
85+
const { startIndex, endIndex } = removalIndices(node.source);
86+
87+
context.report({
88+
message: INVALID_SYNTAX_MESSAGE,
89+
startIndex,
90+
endIndex,
91+
fix: (corrector) => {
92+
corrector.replace(startIndex, endIndex, '');
93+
},
94+
});
95+
},
96+
};
97+
}

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
import { toLiquidAST } from '@shopify/liquid-html-parser';
2+
import { isError } from '../../../utils';
3+
4+
export const INVALID_SYNTAX_MESSAGE = 'Syntax is not supported';
5+
6+
export function ensureValidAst(source: string) {
7+
try {
8+
const ast = toLiquidAST(source, { allowUnclosedDocumentNode: false, mode: 'strict' });
9+
return !isError(ast);
10+
} catch (_error) {
11+
return false;
12+
}
13+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { expect, describe, it } from 'vitest';
1+
import { expect, describe, it, vi } from 'vitest';
22
import { highlightedOffenses, runLiquidCheck } from '../../test';
33
import { Offense } from '../../types';
44
import { LiquidHTMLSyntaxError } from './index';

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

Lines changed: 4 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1-
import { NodeTypes } from '@shopify/liquid-html-parser';
2-
import { Severity, SourceCodeType, LiquidCheckDefinition, Check, Context } from '../../types';
1+
import { Severity, SourceCodeType, LiquidCheckDefinition } from '../../types';
32
import { getOffset, isError } from '../../utils';
3+
import { detectMultipleAssignValues } from './checks/MultipleAssignValues';
4+
import { detectInvalidBooleanExpressions } from './checks/InvalidBooleanExpressions';
45

56
type LineColPosition = {
67
line: number;
@@ -39,6 +40,7 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
3940
if (!isError(ast)) {
4041
return {
4142
...detectInvalidBooleanExpressions(context),
43+
...detectMultipleAssignValues(context),
4244
};
4345
}
4446

@@ -66,33 +68,3 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
6668
};
6769
},
6870
};
69-
70-
function detectInvalidBooleanExpressions(
71-
context: Context<SourceCodeType.LiquidHtml>,
72-
): Check<SourceCodeType.LiquidHtml> {
73-
return {
74-
async BooleanExpression(node) {
75-
const condition = node.condition;
76-
77-
if (
78-
condition.type !== NodeTypes.Comparison &&
79-
condition.type !== NodeTypes.LogicalExpression
80-
) {
81-
return;
82-
}
83-
84-
context.report({
85-
message: 'Syntax is not supported',
86-
startIndex: node.position.start,
87-
endIndex: node.position.end,
88-
fix: (corrector) => {
89-
corrector.replace(
90-
node.position.start,
91-
node.position.end,
92-
node.source.slice(condition.left.position.start, condition.left.position.end),
93-
);
94-
},
95-
});
96-
},
97-
};
98-
}

0 commit comments

Comments
 (0)