Skip to content

Commit a802894

Browse files
aswamyaswamy
authored
Merge pull request #1052 from Shopify/remove-ast-check-in-syntax-error-check
Remove AST check on fixed code suggestions
2 parents aaa546a + 39a6e63 commit a802894

File tree

10 files changed

+10
-146
lines changed

10 files changed

+10
-146
lines changed

‎.changeset/warm-bobcats-think.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+
Remove AST check on fixed code suggestions

‎packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidConditionalNodeParenthesis.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, vi, beforeEach } from 'vitest';
1+
import { expect, describe, it } from 'vitest';
22
import { applyFix, runLiquidCheck } from '../../../test';
33
import { LiquidHTMLSyntaxError } from '../index';
44

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

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,6 @@
11
import { expect, describe, it, vi, beforeEach } from 'vitest';
22
import { applyFix, runLiquidCheck } from '../../../test';
33
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-
});
184

195
describe('detectInvalidEchoValue', async () => {
206
beforeEach(() => {
@@ -90,12 +76,6 @@ describe('detectInvalidEchoValue', async () => {
9076
expect(offenses[0].message).to.equal('Syntax is not supported');
9177
expect(offenses[1].message).to.equal('Syntax is not supported');
9278

93-
expect(toLiquidAST).toHaveBeenCalledTimes(2);
94-
expect(toLiquidAST).toHaveBeenCalledWith(`{% echo one %}`, {
95-
allowUnclosedDocumentNode: false,
96-
mode: 'tolerant',
97-
});
98-
9979
const fixed = applyFix(sourceCode, offenses[0]);
10080
expect(fixed).to.equal(`{% echo zero %} {% echo one %} {% echo one two %}`);
10181

@@ -119,15 +99,4 @@ describe('detectInvalidEchoValue', async () => {
11999
expect(fixed).to.equal(expected);
120100
}
121101
});
122-
123-
it('should not report when the fixed code produces an invalid AST', async () => {
124-
const sourceCode = `{% echo '123' 555 text %}`;
125-
126-
vi.mocked(toLiquidAST).mockImplementation((_source, _options) => {
127-
throw new SyntaxError('Invalid AST');
128-
});
129-
130-
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
131-
expect(offenses).to.have.length(0);
132-
});
133102
});

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

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { LiquidTag, LiquidVariableOutput, NodeTypes } from '@shopify/liquid-html-parser';
22
import { Problem, SourceCodeType } from '../../..';
3-
import { ensureValidAst, getValuesInMarkup, INVALID_SYNTAX_MESSAGE } from './utils';
3+
import { getValuesInMarkup, INVALID_SYNTAX_MESSAGE } from './utils';
44

55
export function detectInvalidEchoValue(
66
node: LiquidTag | LiquidVariableOutput,
@@ -59,16 +59,6 @@ export function detectInvalidEchoValue(
5959

6060
const { startIndex, endIndex } = removalIndices(node.source, node.position.start);
6161

62-
if (
63-
!ensureValidAst(
64-
node.source.slice(node.position.start, startIndex) +
65-
node.source.slice(endIndex, node.position.end),
66-
)
67-
) {
68-
// If the new AST is invalid, we don't want to auto-fix it
69-
return;
70-
}
71-
7262
return {
7363
message: INVALID_SYNTAX_MESSAGE,
7464
startIndex,

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

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,8 @@
11
import { expect, describe, it, vi, beforeEach } from 'vitest';
22
import { applyFix, runLiquidCheck } from '../../../test';
33
import { LiquidHTMLSyntaxError } from '../index';
4-
import { toLiquidAST } from '@shopify/liquid-html-parser';
54
import { INVALID_LOOP_RANGE_MESSAGE } from './InvalidLoopRange';
65

7-
vi.mock('@shopify/liquid-html-parser', async (importOriginal) => {
8-
const original: any = await importOriginal();
9-
10-
return {
11-
...original,
12-
13-
// we will be mocked later on
14-
toLiquidAST: vi.fn().mockImplementation((source, options) => {
15-
return original.toLiquidAST(source, options);
16-
}),
17-
};
18-
});
19-
206
describe('detectInvalidLoopRange', async () => {
217
beforeEach(() => {
228
vi.clearAllMocks();
@@ -85,16 +71,6 @@ describe('detectInvalidLoopRange', async () => {
8571
expect(offenses[0].message).to.equal(INVALID_LOOP_RANGE_MESSAGE);
8672
expect(offenses[1].message).to.equal(INVALID_LOOP_RANGE_MESSAGE);
8773

88-
expect(toLiquidAST).toHaveBeenCalledTimes(2);
89-
expect(toLiquidAST).toHaveBeenCalledWith(`{% for i in (1..10) %}{% endfor %}`, {
90-
allowUnclosedDocumentNode: false,
91-
mode: 'tolerant',
92-
});
93-
expect(toLiquidAST).toHaveBeenCalledWith(`{% tablerow x in (a..b) %}{% endtablerow %}`, {
94-
allowUnclosedDocumentNode: false,
95-
mode: 'tolerant',
96-
});
97-
9874
const fixed = applyFix(sourceCode, offenses[0]);
9975
expect(fixed).to.equal(
10076
`{% for i in (1..10) %}{% endfor %} {% tablerow x in (a...b) %}{% endtablerow %}`,
@@ -105,15 +81,4 @@ describe('detectInvalidLoopRange', async () => {
10581
`{% for i in (1...10) %}{% endfor %} {% tablerow x in (a..b) %}{% endtablerow %}`,
10682
);
10783
});
108-
109-
it('should not report when the fixed code produces an invalid AST', async () => {
110-
const sourceCode = `{% for i in (1..10) %}{% endfor %}`;
111-
112-
vi.mocked(toLiquidAST).mockImplementation((_source, _options) => {
113-
throw new SyntaxError('Invalid AST');
114-
});
115-
116-
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
117-
expect(offenses).to.have.length(0);
118-
});
11984
});

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

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

66
export const INVALID_LOOP_RANGE_MESSAGE =
@@ -63,17 +63,6 @@ function validateMarkup(
6363
const startIndex = markupIndex + match.index;
6464
const endIndex = startIndex + fullMatch.length;
6565

66-
if (
67-
!ensureValidAst(
68-
node.source.slice(node.position.start, startIndex) +
69-
expectedRangeMarkup +
70-
node.source.slice(endIndex, node.position.end),
71-
)
72-
) {
73-
// If the new AST is invalid, we don't want to auto-fix it
74-
return;
75-
}
76-
7766
return {
7867
message: INVALID_LOOP_RANGE_MESSAGE,
7968
startIndex,

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

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,6 @@
11
import { expect, describe, it, vi, beforeEach } from 'vitest';
22
import { applyFix, runLiquidCheck } from '../../../test';
33
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-
});
184

195
describe('detectMultipleAssignValues', async () => {
206
beforeEach(() => {
@@ -56,12 +42,6 @@ describe('detectMultipleAssignValues', async () => {
5642
expect(offenses[0].message).to.equal('Syntax is not supported');
5743
expect(offenses[1].message).to.equal('Syntax is not supported');
5844

59-
expect(toLiquidAST).toHaveBeenCalledTimes(2);
60-
expect(toLiquidAST).toHaveBeenCalledWith(`{% assign foo = '123' %}`, {
61-
allowUnclosedDocumentNode: false,
62-
mode: 'tolerant',
63-
});
64-
6545
const fixed = applyFix(sourceCode, offenses[0]);
6646
expect(fixed).to.equal(
6747
`{% assign foo = blank %} {% assign foo = '123' %} {% assign foo = '123' 555 text %}`,
@@ -93,15 +73,4 @@ describe('detectMultipleAssignValues', async () => {
9373
expect(fixed).to.equal(expected);
9474
}
9575
});
96-
97-
it('should not report when the fixed code produces an invalid AST', async () => {
98-
const sourceCode = `{% assign foo = '123' 555 text %}`;
99-
100-
vi.mocked(toLiquidAST).mockImplementation((_source, _options) => {
101-
throw new SyntaxError('Invalid AST');
102-
});
103-
104-
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
105-
expect(offenses).to.have.length(0);
106-
});
10776
});

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

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { LiquidTag } from '@shopify/liquid-html-parser';
22
import { Problem, SourceCodeType } from '../../..';
3-
import { ensureValidAst, getValuesInMarkup, INVALID_SYNTAX_MESSAGE } from './utils';
3+
import { getValuesInMarkup, INVALID_SYNTAX_MESSAGE } from './utils';
44

55
export function detectMultipleAssignValues(
66
node: LiquidTag,
@@ -67,16 +67,6 @@ export function detectMultipleAssignValues(
6767

6868
const { startIndex, endIndex } = removalIndices(node.source, node.position.start);
6969

70-
if (
71-
!ensureValidAst(
72-
node.source.slice(node.position.start, startIndex) +
73-
node.source.slice(endIndex, node.position.end),
74-
)
75-
) {
76-
// If the new AST is invalid, we don't want to auto-fix it
77-
return;
78-
}
79-
8070
return {
8171
message: INVALID_SYNTAX_MESSAGE,
8272
startIndex,

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ describe('getValuesInMarkup', () => {
3737
describe('getFragmentsInMarkup', () => {
3838
Object.entries({
3939
space: ' ',
40-
pipe: ' | ',
4140
comma: ', ',
4241
}).forEach(([name, separator]) => {
4342
it(`should return all fragments in markup separated by ${name}`, () => {

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

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,5 @@
1-
import { toLiquidAST } from '@shopify/liquid-html-parser';
2-
import { isError } from '../../../utils';
3-
41
export const INVALID_SYNTAX_MESSAGE = 'Syntax is not supported';
52

6-
export function ensureValidAst(source: string) {
7-
try {
8-
const ast = toLiquidAST(source, { allowUnclosedDocumentNode: false, mode: 'tolerant' });
9-
return !isError(ast);
10-
} catch (_error) {
11-
return false;
12-
}
13-
}
14-
153
export function getValuesInMarkup(markup: string) {
164
return [...markup.matchAll(new RegExp(VALUE_PATTERN, 'g'))].map((match) => ({
175
value: match[0],
@@ -27,7 +15,7 @@ const SINGLE_QUOTED_STRING = `'[^']*'`;
2715
// To avoid any issues, we will remove extra spaces.
2816
const RANGE_MARKUP_COMPONENT_REGEX = `\\s*(-?\\d+(?:\\.\\d+)?|\\w+(?:\\.\\w+)*)\\s*`;
2917
const RANGE_MARKUP_REGEX = `\\(\\s*${RANGE_MARKUP_COMPONENT_REGEX}(\\.{2,})\\s*${RANGE_MARKUP_COMPONENT_REGEX}\\s*\\)`;
30-
const REGULAR_TOKEN = `[^\\s,|]+`; // tokens separated by commas, spaces, or pipes
18+
const REGULAR_TOKEN = `[^\\s,]+`; // tokens separated by commas or spaces
3119

3220
// Quoted strings pattern (combination of double and single quoted)
3321
const QUOTED_STRING = `(${DOUBLE_QUOTED_STRING}|${SINGLE_QUOTED_STRING})`;

0 commit comments

Comments
 (0)