diff --git a/.changeset/hand-written-liquid-html-parser.md b/.changeset/hand-written-liquid-html-parser.md new file mode 100644 index 000000000..0ee06def3 --- /dev/null +++ b/.changeset/hand-written-liquid-html-parser.md @@ -0,0 +1,16 @@ +--- +'@shopify/liquid-html-parser': minor +'@shopify/theme-language-server-common': minor +'@shopify/theme-check-common': minor +'@shopify/prettier-plugin-liquid': minor +'@shopify/theme-graph': minor +--- + +Adopt the hand-written `liquid-html-parser` for performance + +Replace the parser-combinator-based Liquid/HTML parser with a hand-written +recursive-descent parser. The new parser is significantly faster, adds +resilient parsing (it recovers from malformed input instead of bailing), and is +adapted to the theme-tools source model. `theme-language-server-common`, +`theme-check-common`, `prettier-plugin-liquid`, and `theme-graph` are updated to +consume the new parser. diff --git a/packages/liquid-html-parser/.gitignore b/packages/liquid-html-parser/.gitignore index 02a8b6d42..3a0374528 100644 --- a/packages/liquid-html-parser/.gitignore +++ b/packages/liquid-html-parser/.gitignore @@ -4,9 +4,12 @@ pnpm-debug.log* .DS_Store dawn TODO -grammar/liquid-html.ohm.js -standalone.js -standalone.js.LICENSE.txt **/actual.liquid coverage .nyc_output + +# Generated parser fixtures (goldens + downloaded themes) — regenerated by scripts/setup-fixtures.ts +fixtures/golden-html-ast/ +fixtures/golden-liquid-ast/ +fixtures/theme/ +fixtures/theme-bundle.ts diff --git a/packages/liquid-html-parser/fixtures/error-corpus.ts b/packages/liquid-html-parser/fixtures/error-corpus.ts new file mode 100644 index 000000000..db465c168 --- /dev/null +++ b/packages/liquid-html-parser/fixtures/error-corpus.ts @@ -0,0 +1,75 @@ +/* + * Adversarial error corpus for the tolerant-parser overhead benchmark. + * + * Every `source` here is *invalid* Liquid/HTML: the strict `toLiquidHtmlAST` + * throws on each one, so these sources exist only to exercise the tolerant + * path (`toTolerantLiquidHtmlAST`), which recovers instead of throwing and + * surfaces one `LiquidErrorNode` per region it gives up on. + * + * The bench arm that consumes this corpus measures resync/recovery cost, not + * throughput on clean input. + * + * Shape is identical to `THEME_FILES` in `theme-bundle.ts` + * (`Array<{ path; source }>`) so the bench loop stays uniform across corpora. + * + * Contract (verified in C2): each `source` passed to + * `toTolerantLiquidHtmlAST` returns a `DocumentNode` without throwing and + * contains at least one `LiquidErrorNode`. + */ + +/* + * A single orphan close tag followed by a valid variable output. The close + * has no matching open, so the tolerant parser emits one error node, then + * resynchronizes on the next construct-open boundary and recovers the output. + * + * Repeating this unit forces the resync loop to fire once per unit — the + * "error every few tokens" density stress. + */ +const FREQUENT_ERROR_UNIT = "{% endfor %}{{ x }}{% endif %}{{ y }}"; + +/* + * A well-formed Liquid+HTML fragment the strict parser accepts as-is. + * + * Repeated many times it builds a large clean body; a single orphan close + * tag appended near EOF then costs exactly one recovery, proving tail + * recovery does not rescan the whole document. + */ +const CLEAN_UNIT = + '
{{ product.title }}' + + "{% if product.available %}{{ product.price }}{% endif %}" + + "
\n"; + +export const ERROR_FILES: Array<{ path: string; source: string }> = [ + { + /* Seeded verbatim from tolerant.test.ts:150 — one orphan close tag. */ + path: "error-corpus/single-error.liquid", + source: "{% endfor %}", + }, + { + /* + * Seeded verbatim from tolerant.test.ts:178 — two orphan closes with a + * valid output recovered between them (interleaved resync). + */ + path: "error-corpus/interleaved-resync.liquid", + source: "{% endfor %}{{ good }}{% endif %}", + }, + { + /* + * Net-new: an orphan close roughly every few tokens across a moderately + * long source, bounding worst-case resync frequency. + * + * 60 units yields 120 error nodes interleaved with 120 recovered + * outputs. + */ + path: "error-corpus/pathological-frequent.liquid", + source: FREQUENT_ERROR_UNIT.repeat(60), + }, + { + /* + * Net-new: a large valid body (150 clean units) with a single orphan + * close tag just before EOF — "parse a lot, then recover once". + */ + path: "error-corpus/large-clean-error-near-eof.liquid", + source: CLEAN_UNIT.repeat(150) + "{% endfor %}", + }, +]; diff --git a/packages/liquid-html-parser/package.json b/packages/liquid-html-parser/package.json index e052b1b57..aba6395a9 100644 --- a/packages/liquid-html-parser/package.json +++ b/packages/liquid-html-parser/package.json @@ -20,7 +20,6 @@ "@shopify:registry": "https://registry.npmjs.org" }, "files": [ - "grammar/*", "dist/**/*.js", "dist/**/*.ts" ], @@ -28,11 +27,11 @@ "build": "pnpm build:ts", "build:ci": "pnpm build", "build:ts": "tsc -p tsconfig.build.json", - "type-check": "tsc --noEmit" + "type-check": "tsc --noEmit", + "test": "vitest --root ../.. --run src/**/*.test.ts" }, "dependencies": { - "line-column": "^1.0.2", - "ohm-js": "^17.0.0" + "line-column": "^1.0.2" }, "devDependencies": { "@types/line-column": "^1.0.0", diff --git a/packages/liquid-html-parser/src/ast.test.ts b/packages/liquid-html-parser/src/ast.test.ts index 3fc517c4b..996d97a6d 100644 --- a/packages/liquid-html-parser/src/ast.test.ts +++ b/packages/liquid-html-parser/src/ast.test.ts @@ -1345,6 +1345,26 @@ describe('Unit: Stage 2 (AST)', () => { expect(useChild.type).to.eql('TextNode'); }); + it('should depth-balance nested same-name raw tags and close at the outer tag (issue 156)', () => { + const expectPath = makeExpectPath('toLiquidHtmlAST - nested raw close balancing'); + + // Nested elements: the outer must close at the LAST + // , keeping the inner … as raw body text rather than + // closing early at the first (which previously threw / mis-parsed). + ast = toLiquidHtmlAST('abc'); + expectPath(ast, 'children').to.have.lengthOf(1); + expectPath(ast, 'children.0.type').to.eql('HtmlRawNode'); + expectPath(ast, 'children.0.name').to.eql('svg'); + expectPath(ast, 'children.0.body.value').to.eql('abc'); + + // Same balancing for a non-svg raw tag (c'); + expectPath(ast, 'children').to.have.lengthOf(1); + expectPath(ast, 'children.0.type').to.eql('HtmlRawNode'); + expectPath(ast, 'children.0.name').to.eql('script'); + expectPath(ast, 'children.0.body.value').to.eql('ac'); + }); + it(`should parse a basic text node into a TextNode`, () => { for (const { toAST, expectPath, expectPosition } of testCases) { ast = toAST('Hello world!'); diff --git a/packages/liquid-html-parser/src/ast.ts b/packages/liquid-html-parser/src/ast.ts index d95c2df13..cc1051e66 100644 --- a/packages/liquid-html-parser/src/ast.ts +++ b/packages/liquid-html-parser/src/ast.ts @@ -49,7 +49,8 @@ export type LiquidHtmlNode = | LiquidDocParamNode | LiquidDocExampleNode | LiquidDocPromptNode - | LiquidDocDescriptionNode; + | LiquidDocDescriptionNode + | LiquidErrorNode; /** The root node of all LiquidHTML ASTs. */ export interface DocumentNode extends ASTNode { @@ -885,6 +886,18 @@ export interface LiquidDocPromptNode extends ASTNode { + /** The message of the caught parse error. */ + message: string; + /** The token-type name at the point the parse failed, when available. */ + found?: string; +} + export interface ASTNode { /** * The type of the node, as a string. diff --git a/packages/liquid-html-parser/src/document/base.ts b/packages/liquid-html-parser/src/document/base.ts index e362275a7..6d9125017 100644 --- a/packages/liquid-html-parser/src/document/base.ts +++ b/packages/liquid-html-parser/src/document/base.ts @@ -54,6 +54,17 @@ export class ParserBase { return this.source; } + /** + * Whether this document parse is tolerant. The strict/default parser returns + * false; `TolerantDocumentParser` overrides it to true. Markup-construction + * sites consult this to enable the markup parser's tolerant recovery axis + * (`enableTolerant()`), leaving strict (and theme-check) parses untouched. + * Disjoint from the render-tree lax axis. + */ + isTolerant(): boolean { + return false; + } + tokenAt(index: number): Token { return this.tokens[index]; } diff --git a/packages/liquid-html-parser/src/document/factories.ts b/packages/liquid-html-parser/src/document/factories.ts index 38aba5ff7..1640e6f5b 100644 --- a/packages/liquid-html-parser/src/document/factories.ts +++ b/packages/liquid-html-parser/src/document/factories.ts @@ -105,12 +105,15 @@ export function makeLiquidTagBaseCase( blockEndPosition?: Position, delimiterWhitespace?: { start: LiquidOpenWhitespace; end: LiquidCloseWhitespace }, reason?: string, + // `#`-comment lines in `{% liquid %}` keep their inner indentation verbatim; + // every other base-case tag trims (the default). + preserveMarkup: boolean = false, ): LiquidTagBaseCase { const posEnd = blockEndPosition ? blockEndPosition.end : envelope.blockStartPosition.end; return { type: NodeTypes.LiquidTag, name: envelope.tagName, - markup: envelope.markupString.trim(), + markup: preserveMarkup ? envelope.markupString : envelope.markupString.trim(), children, whitespaceStart: envelope.whitespaceStart, whitespaceEnd: envelope.whitespaceEnd, diff --git a/packages/liquid-html-parser/src/document/html.ts b/packages/liquid-html-parser/src/document/html.ts index 18ad20c67..a296519f4 100644 --- a/packages/liquid-html-parser/src/document/html.ts +++ b/packages/liquid-html-parser/src/document/html.ts @@ -217,11 +217,15 @@ export function parseHtmlComment(parser: HtmlParserDelegate): HtmlComment { const openToken = parser.consume(TokenType.HtmlCommentOpen); const bodyStart = openToken.end; - while (!parser.isAtEnd() && !parser.check(TokenType.HtmlCommentClose)) { - parser.advance(); - } - - if (parser.isAtEnd()) { + // Find the closing `-->` by scanning the source rather than walking tokens. + // A conditional comment body (``) contains + // `` as an HtmlTagClose, so no + // HtmlCommentClose token is ever emitted and the token walk would run to EOF. + // A source scan pins us to the real comment close, so the (unchanged) + // HtmlComment node feeds getConditionalComment correctly in the printer. + const closeIdx = source.indexOf('-->', bodyStart); + if (closeIdx === -1) { throw new LiquidHTMLASTParsingError( `Attempting to end parsing before HtmlComment '` const body = source.slice(bodyStart, bodyEnd).trim(); - return makeHtmlComment(body, openToken.start, closeToken.end, source); + parser.seekToSourceOffset(commentEnd); + + return makeHtmlComment(body, openToken.start, commentEnd, source); } // htmlDoctype := "" @@ -499,7 +505,11 @@ function parseAttributeList(parser: HtmlParserDelegate): AttributeNode[] { const attrEnd = closeQuote.end; const attributePosition: Position = { start: valueStart, end: valueEnd }; - if (quoteChar === '"') { + // Double straight quote and double curly quotes (“ ”) map to a + // double-quoted attr; single straight quote and single curly quotes + // (‘ ’) map to a single-quoted attr. The printer normalizes the curly + // variants to straight quotes. + if (quoteChar === '"' || quoteChar === '“' || quoteChar === '”') { attrs.push( makeAttrDoubleQuoted(name, value, attributePosition, attrStart, attrEnd, source), ); @@ -738,13 +748,39 @@ export function scanForHtmlCloseTag(parser: ParserBase, tagName: string): number const tokenCount = parser.tokenCount(); const pos = parser.getPosition(); + // Depth-balance nested same-name elements so the OUTER close tag is + // returned, not the first inner one (e.g. `………`). A + // nested open tag is an `HtmlTagOpen` followed by a `Text` token whose first + // word matches the tag name — the tokenizer folds the tag name and any + // trailing attributes into a single text token, so we take the first word. + // The scan begins after the outer open tag has been consumed, so the outer + // open is never counted. Mirrors `scanForEndTagNested`. + let depth = 0; for (let i = pos; i < tokenCount; i++) { - if (parser.tokenAt(i).type !== TokenType.HtmlCloseTagOpen) continue; + const token = parser.tokenAt(i); + + if (token.type === TokenType.HtmlTagOpen) { + const textIdx = i + 1; + if (textIdx >= tokenCount) continue; + if (parser.tokenAt(textIdx).type !== TokenType.Text) continue; + const trimmed = source + .slice(parser.tokenAt(textIdx).start, parser.tokenAt(textIdx).end) + .trimStart(); + const firstWs = trimmed.search(/\s/); + const name = firstWs === -1 ? trimmed.trim() : trimmed.slice(0, firstWs); + if (name.toLowerCase() === lowerName) depth++; + continue; + } + + if (token.type !== TokenType.HtmlCloseTagOpen) continue; const textIdx = i + 1; if (textIdx >= tokenCount) continue; if (parser.tokenAt(textIdx).type !== TokenType.Text) continue; const text = source.slice(parser.tokenAt(textIdx).start, parser.tokenAt(textIdx).end); - if (text.trim().toLowerCase() === lowerName) return i; + if (text.trim().toLowerCase() === lowerName) { + if (depth === 0) return i; + depth--; + } } return -1; } @@ -768,6 +804,7 @@ function scriptKindFromAttributes(attributes: AttributeNode[]): RawMarkupKinds { const typeValue = extractPlainAttributeValue(attributes, 'type'); if (typeValue === null) return RawMarkupKinds.javascript; if (typeValue === 'text/html') return RawMarkupKinds.html; + if (typeValue === 'text/markdown') return RawMarkupKinds.markdown; if ( typeValue.endsWith('json') || typeValue.endsWith('importmap') || diff --git a/packages/liquid-html-parser/src/document/liquid-blocks.ts b/packages/liquid-html-parser/src/document/liquid-blocks.ts index 470c4ac37..ba08d651e 100644 --- a/packages/liquid-html-parser/src/document/liquid-blocks.ts +++ b/packages/liquid-html-parser/src/document/liquid-blocks.ts @@ -68,6 +68,7 @@ export function parseBlockTag( const markupStringStart = closeToken.start - envelope.markupString.length; const tokens = tokenizeMarkup(envelope.markupString, markupStringStart); const markupParser = new MarkupParser(tokens, parser.getSource()); + if (parser.isTolerant()) markupParser.enableTolerant(); markup = def.parse(envelope.tagName, markupParser, parser); if (!markupParser.isAtEnd()) { markup = undefined; @@ -341,6 +342,7 @@ export function parseBranchMarkup( try { const tokens = tokenizeMarkup(envelope.markupString, markupStringStart); const markupParser = new MarkupParser(tokens, parser.getSource()); + if (parser.isTolerant()) markupParser.enableTolerant(); const result = elsifBranchParse(branchName, markupParser); if (!markupParser.isAtEnd()) return envelope.markupString.trim(); return result; @@ -352,6 +354,7 @@ export function parseBranchMarkup( try { const tokens = tokenizeMarkup(envelope.markupString, markupStringStart); const markupParser = new MarkupParser(tokens, parser.getSource()); + if (parser.isTolerant()) markupParser.enableTolerant(); const result = whenBranchParse(branchName, markupParser); if (!markupParser.isAtEnd()) return envelope.markupString.trim(); return result; diff --git a/packages/liquid-html-parser/src/document/liquid-hybrid.ts b/packages/liquid-html-parser/src/document/liquid-hybrid.ts index 4262f2dc3..f855efa19 100644 --- a/packages/liquid-html-parser/src/document/liquid-hybrid.ts +++ b/packages/liquid-html-parser/src/document/liquid-hybrid.ts @@ -31,6 +31,7 @@ export function parseHybridTag( const markupStringStart = closeToken.start - envelope.markupString.length; const tokens = tokenizeMarkup(envelope.markupString, markupStringStart); const markupParser = new MarkupParser(tokens, parser.getSource()); + if (parser.isTolerant()) markupParser.enableTolerant(); markup = def.parse(envelope.tagName, markupParser, parser); if (!markupParser.isAtEnd()) { markup = undefined; diff --git a/packages/liquid-html-parser/src/document/liquid-lines.test.ts b/packages/liquid-html-parser/src/document/liquid-lines.test.ts index 0741ef38a..e5c1729cb 100644 --- a/packages/liquid-html-parser/src/document/liquid-lines.test.ts +++ b/packages/liquid-html-parser/src/document/liquid-lines.test.ts @@ -112,6 +112,14 @@ describe('Unit: liquid-lines', () => { expectPath(ast, 'children.0.markup.1.name').to.eql('echo'); }); + it('should preserve inner indentation after `#`, stripping only one separator space (Category J)', () => { + // `# fancy` must keep four spaces of the ASCII-art indent — only a + // single separator space after `#` is stripped (ohm `"#" space?`). + const ast = toLiquidHtmlAST('{% liquid\n# fancy\necho "hi"\n%}'); + expectPath(ast, 'children.0.markup.0.name').to.eql('#'); + expectPath(ast, 'children.0.markup.0.markup').to.eql(' fancy'); + }); + it('should skip empty lines', () => { const ast = toLiquidHtmlAST('{% liquid\n\necho "hi"\n\n%}'); expectPath(ast, 'children.0.markup').to.have.lengthOf(1); @@ -226,4 +234,33 @@ describe('Unit: liquid-lines', () => { expectPath(ast, `${branch}.blockEndPosition.end`).to.eql(source.indexOf('endif')); }); }); + + describe('nested comment/doc balancing (Category B)', () => { + it('should balance nested comments so the outer endcomment closes the block', () => { + // The inner `comment`/`endcomment` pair must not close the outer + // block early; the outer comment ends at the final `endcomment`. + const source = '{% liquid\ncomment\nouter\ncomment\ninner\nendcomment\nendcomment\n%}'; + const ast = toLiquidHtmlAST(source); + expectPath(ast, 'children.0.markup').to.have.lengthOf(1); + expectPath(ast, 'children.0.markup.0.type').to.eql('LiquidRawTag'); + expectPath(ast, 'children.0.markup.0.name').to.eql('comment'); + expectPath(ast, 'children.0.markup.0.body.value').to.eql( + 'outer\ncomment\ninner\nendcomment\n', + ); + }); + + it('should carve out a nested raw so a literal endcomment inside it does not close early', () => { + // The `endcomment` line sits inside a nested `raw`…`endraw` block, so it + // must be ignored by the depth scan; the outer comment closes at the + // real trailing `endcomment`. + const source = '{% liquid\ncomment\nbefore\nraw\nendcomment\nendraw\nafter\nendcomment\n%}'; + const ast = toLiquidHtmlAST(source); + expectPath(ast, 'children.0.markup').to.have.lengthOf(1); + expectPath(ast, 'children.0.markup.0.type').to.eql('LiquidRawTag'); + expectPath(ast, 'children.0.markup.0.name').to.eql('comment'); + expectPath(ast, 'children.0.markup.0.body.value').to.eql( + 'before\nraw\nendcomment\nendraw\nafter\n', + ); + }); + }); }); diff --git a/packages/liquid-html-parser/src/document/liquid-lines.ts b/packages/liquid-html-parser/src/document/liquid-lines.ts index 92499f105..f00cf46a1 100644 --- a/packages/liquid-html-parser/src/document/liquid-lines.ts +++ b/packages/liquid-html-parser/src/document/liquid-lines.ts @@ -64,7 +64,9 @@ export function parseLiquidStatement( const envelope = envelopeFromLine(line, parser.getSource()); if (tagName === '#') { - return makeLiquidTagBaseCase(envelope); + // Preserve the inline comment's inner indentation (`preserveMarkup`); only + // the single separator space after `#` was stripped upstream. + return makeLiquidTagBaseCase(envelope, undefined, undefined, undefined, undefined, true); } if (tagName.startsWith('end')) { @@ -93,6 +95,7 @@ export function parseLiquidStatement( markupOffset, envelope.markupEnd, ); + if (parser.isTolerant()) markupParser.enableTolerant(); const markup = def.parse(tagName, markupParser, parser); if (!markupParser.isAtEnd()) { @@ -140,6 +143,7 @@ export function parseLineBlockTag( try { const tokens = tokenizeMarkup(markupString, markupOffset); const markupParser = new MarkupParser(tokens, parser.getSource()); + if (parser.isTolerant()) markupParser.enableTolerant(); markup = def.parse(envelope.tagName, markupParser, parser); if (!markupParser.isAtEnd()) { @@ -196,11 +200,41 @@ export function parseLineRawTag( ? ctx.lines[ctx.index - 1].lineEnd + 1 : envelope.blockStartPosition.end; + // `comment`/`doc` bodies balance nested opens of the same tag before + // matching their end line, mirroring the document-path scan in + // liquid-raw.ts (Ruby comment.rb v5.13.0 `comment_tag_depth`). A nested + // `raw` block is carved out: `raw` is first-match and does not nest, so an + // `endcomment`/`comment` word sitting on a line inside a raw body must not + // affect the depth. Every other raw tag (`raw`, `javascript`, `schema`, + // `style`) keeps the original first-match scan and stays byte-identical. + const balanced = tagName === 'comment' || tagName === 'doc'; let endLineIndex = -1; - for (let i = ctx.index; i < ctx.lines.length; i++) { - if (ctx.lines[i].tagName === endTagName) { - endLineIndex = i; - break; + if (balanced) { + let depth = 0; + for (let i = ctx.index; i < ctx.lines.length; i++) { + const name = ctx.lines[i].tagName; + if (name === 'raw') { + // Skip past the nested raw block; its body lines never affect depth. + i++; + while (i < ctx.lines.length && ctx.lines[i].tagName !== 'endraw') i++; + continue; + } + if (name === tagName) { + depth++; + } else if (name === endTagName) { + if (depth === 0) { + endLineIndex = i; + break; + } + depth--; + } + } + } else { + for (let i = ctx.index; i < ctx.lines.length; i++) { + if (ctx.lines[i].tagName === endTagName) { + endLineIndex = i; + break; + } } } @@ -262,6 +296,7 @@ export function parseLineHybridTag( try { const tokens = tokenizeMarkup(markupString, markupOffset); const markupParser = new MarkupParser(tokens, parser.getSource()); + if (parser.isTolerant()) markupParser.enableTolerant(); markup = def.parse(envelope.tagName, markupParser, parser); if (!markupParser.isAtEnd()) { @@ -385,6 +420,7 @@ export function parseLineBranchedBody( line.tagName as BranchName, branchEnvelope, parser.getSource(), + parser.isTolerant(), ); currentBranch = makeLiquidBranchNamed(branchEnvelope, branchMarkup); currentChildren = []; @@ -423,12 +459,14 @@ export function parseLineBranchMarkup( branchName: BranchName, envelope: LiquidTagEnvelope, source: string, + tolerant: boolean = false, ): unknown { switch (branchName) { case 'elsif': { try { const tokens = tokenizeMarkup(envelope.markupString, envelope.markupOffset); const markupParser = new MarkupParser(tokens, source); + if (tolerant) markupParser.enableTolerant(); const result = elsifBranchParse(branchName, markupParser); if (!markupParser.isAtEnd()) return envelope.markupString.trim(); @@ -441,6 +479,7 @@ export function parseLineBranchMarkup( try { const tokens = tokenizeMarkup(envelope.markupString, envelope.markupOffset); const markupParser = new MarkupParser(tokens, source); + if (tolerant) markupParser.enableTolerant(); const result = whenBranchParse(branchName, markupParser); if (!markupParser.isAtEnd()) return envelope.markupString.trim(); diff --git a/packages/liquid-html-parser/src/document/liquid-raw.ts b/packages/liquid-html-parser/src/document/liquid-raw.ts index dd136716a..56690aad5 100644 --- a/packages/liquid-html-parser/src/document/liquid-raw.ts +++ b/packages/liquid-html-parser/src/document/liquid-raw.ts @@ -188,6 +188,14 @@ export function scanForEndTag(parser: ParserBase, endTagName: string): EndTagSca const source = parser.getSource(); const searchStart = parser.tokenAt(parser.getPosition()).start; + // `comment` and `doc` bodies balance nested opens of the same tag before + // matching their end tag, mirroring Ruby comment.rb v5.13.0's + // `comment_tag_depth`. `raw` (and every other raw tag) keeps first-match + // semantics — Ruby's raw does not balance. + if (endTagName === 'endcomment' || endTagName === 'enddoc') { + return scanForBalancedEndTag(source, searchStart, endTagName); + } + const pattern = new RegExp(`\\{%(-?)\\s*${endTagName}\\s*(-?)%\\}`); const match = pattern.exec(source.slice(searchStart)); if (!match) return null; @@ -200,6 +208,56 @@ export function scanForEndTag(parser: ParserBase, endTagName: string): EndTagSca return { tagStart, tagEnd, wsStart, wsEnd }; } +/** + * Depth-balancing end-tag scan for `comment`/`doc`. Counts nested opens of the + * same tag so a `{% comment %}` inside the body is paired with its own + * `{% endcomment %}` rather than closing the outer block early. + * + * `searchStart` is already positioned past the outer open tag, so depth starts + * at 0 and every `{% comment %}` encountered is a nested open. A nested + * `{% raw %}`…`{% endraw %}` is carved out — its literal contents (which may + * contain `{% comment %}`/`{% endcomment %}` text) do not affect the depth — + * mirroring Ruby's `parse_raw_tag_body`. + */ +function scanForBalancedEndTag( + source: string, + searchStart: number, + endTagName: string, +): EndTagScanResult | null { + const openTagName = endTagName.slice(3); // "endcomment" -> "comment" + const scanner = new RegExp( + `\\{%(-?)\\s*(${openTagName}|${endTagName}|raw|endraw)\\b\\s*(-?)%\\}`, + 'g', + ); + scanner.lastIndex = searchStart; + + let depth = 0; + let inRaw = false; + let match: RegExpExecArray | null; + while ((match = scanner.exec(source)) !== null) { + const name = match[2]; + if (inRaw) { + if (name === 'endraw') inRaw = false; + continue; + } + if (name === 'raw') { + inRaw = true; + } else if (name === openTagName) { + depth++; + } else if (name === endTagName) { + if (depth === 0) { + const tagStart = match.index; + const tagEnd = tagStart + match[0].length; + const wsStart: LiquidOpenWhitespace = match[1] === '-' ? '-' : ''; + const wsEnd: LiquidCloseWhitespace = match[3] === '-' ? '-' : ''; + return { tagStart, tagEnd, wsStart, wsEnd }; + } + depth--; + } + } + return null; +} + export function rawMarkupKindForTag(tagName: string, bodySource: string = ''): RawMarkupKinds { switch (tagName) { case 'javascript': diff --git a/packages/liquid-html-parser/src/document/liquid-tags.ts b/packages/liquid-html-parser/src/document/liquid-tags.ts index e23cfff21..604761a90 100644 --- a/packages/liquid-html-parser/src/document/liquid-tags.ts +++ b/packages/liquid-html-parser/src/document/liquid-tags.ts @@ -98,6 +98,7 @@ function parseStandaloneTag( markupStringStart, markupStringEnd, ); + if (parser.isTolerant()) markupParser.enableTolerant(); const markup = def.parse(envelope.tagName, markupParser, parser); if (!markupParser.isAtEnd()) { return makeLiquidTagBaseCase( diff --git a/packages/liquid-html-parser/src/document/liquid-variable-output.ts b/packages/liquid-html-parser/src/document/liquid-variable-output.ts index d7af5452a..e3ad2f7f2 100644 --- a/packages/liquid-html-parser/src/document/liquid-variable-output.ts +++ b/packages/liquid-html-parser/src/document/liquid-variable-output.ts @@ -17,6 +17,7 @@ export function parseLiquidVariableOutput(parser: ParserBase): LiquidVariableOut try { const tokens = tokenizeMarkup(rawMarkup, openToken.end); const markupParser = new MarkupParser(tokens, source); + if (parser.isTolerant()) markupParser.enableTolerant(); const liquidVariable = markupParser.liquidVariable(); if (!markupParser.isAtEnd()) { diff --git a/packages/liquid-html-parser/src/document/tokenizer.ts b/packages/liquid-html-parser/src/document/tokenizer.ts index f32b2ac1d..dcef56ec4 100644 --- a/packages/liquid-html-parser/src/document/tokenizer.ts +++ b/packages/liquid-html-parser/src/document/tokenizer.ts @@ -250,7 +250,18 @@ export function tokenize(source: string, options: TokenizeOptions = {}): Token[] continue; } - if (ch(0) === '"' || ch(0) === "'") { + // Accept straight quotes and curly (smart) quotes as attribute-value + // openers. Curly quotes get normalized to straight quotes downstream; + // recognizing them here (HTML scope only — Liquid tokenization is + // untouched) stops the value from splitting at interior spaces. + if ( + ch(0) === '"' || + ch(0) === "'" || + ch(0) === '“' || + ch(0) === '”' || + ch(0) === '‘' || + ch(0) === '’' + ) { quoteChar = ch(0); emit(TokenType.HtmlQuoteOpen, 1); pushMode(Mode.QuotedValue); @@ -265,7 +276,10 @@ export function tokenize(source: string, options: TokenizeOptions = {}): Token[] case Mode.QuotedValue: { if (scanLiquidOpen()) continue; - if (ch(0) === quoteChar) { + // Curly quotes are directional, so a value opened with a left curly + // quote closes on its right partner (and vice-versa); straight quotes + // close on themselves. + if (ch(0) === closingQuoteFor(quoteChar) || ch(0) === quoteChar) { emit(TokenType.HtmlQuoteClose, 1); popMode(); continue; @@ -293,3 +307,21 @@ enum Mode { LiquidTag = 'LiquidTag', LiquidVariableOutput = 'LiquidVariableOutput', } + +// Curly (smart) quotes come in directional pairs: a value opened with a left +// curly quote closes on its right partner and vice-versa. Straight quotes are +// their own partner, so they close on an identical character. +function closingQuoteFor(open: string): string { + switch (open) { + case '“': + return '”'; + case '”': + return '“'; + case '‘': + return '’'; + case '’': + return '‘'; + default: + return open; + } +} diff --git a/packages/liquid-html-parser/src/document/tolerant-parser.ts b/packages/liquid-html-parser/src/document/tolerant-parser.ts new file mode 100644 index 000000000..f942cd61f --- /dev/null +++ b/packages/liquid-html-parser/src/document/tolerant-parser.ts @@ -0,0 +1,111 @@ +import { DocumentParser } from './parser'; +import { TokenType } from './tokenizer'; +import { NodeTypes } from '../types'; +import { LiquidHTMLASTParsingError } from '../errors'; +import type { LiquidErrorNode, LiquidHtmlNode } from '../ast'; + +/* + * Token types that open a new top-level construct. Panic-mode recovery + * resynchronizes onto one of these so the next `super.parseNode()` call + * resumes on a real node boundary rather than mid-construct. EndOfInput is a + * member so recovery always has a boundary to stop on. + */ +export const RESYNC_TOKENS: ReadonlySet = new Set([ + TokenType.LiquidTagOpen, + TokenType.LiquidVariableOutputOpen, + TokenType.HtmlTagOpen, + TokenType.HtmlCloseTagOpen, + TokenType.HtmlCommentOpen, + TokenType.HtmlDoctypeOpen, + TokenType.EndOfInput, +]); + +/** Whether a token type opens a construct we can safely resume parsing on. */ +export function isResyncToken(type: TokenType): boolean { + return RESYNC_TOKENS.has(type); +} + +/* + * The readable name of a token type. TokenType is a string enum, so its value + * is already the name; this indirection keeps the call sites self-documenting. + */ +export function tokenTypeName(type: TokenType): string { + return type; +} + +/* + * Builds a LiquidErrorNode leaf covering a skipped region. Kept local to the + * tolerant path — deliberately not in the frozen factories.ts — so the + * default parse has no way to construct it. + */ +export function makeLiquidErrorNode( + start: number, + end: number, + source: string, + message: string, + found?: string, +): LiquidErrorNode { + return { + type: NodeTypes.LiquidErrorNode, + position: { start, end }, + source, + message, + found, + }; +} + +/* + * Opt-in tolerant parser. It behaves exactly like DocumentParser except that + * a structural parse failure — which the default parser throws on, aborting the + * whole parse — is caught here and turned into a LiquidErrorNode so parsing can + * continue. The strict/default DocumentParser is a different class reached by a + * different entry point and is left byte-identical. + */ +export class TolerantDocumentParser extends DocumentParser { + /** + * Marks this parse as tolerant so the markup-construction sites enable the + * markup parser's tolerant recovery axis. Disjoint from lax. + */ + isTolerant(): boolean { + return true; + } + + /* + * Wraps the polymorphic node parse. On a LiquidHTMLASTParsingError it emits a + * LiquidErrorNode covering the skipped region and resynchronizes onto the next + * construct-open boundary, so parsing continues and one document can surface + * several errors interleaved with the constructs it did recover. A forced + * >=1-token advance before the resync scan makes every recovery strictly + * advance the cursor, which guarantees the parseDocument loop terminates. + * Foreign (non-parse) errors are rethrown untouched. + */ + parseNode(): LiquidHtmlNode { + const startTok = this.peek(); + const startPos = this.getPosition(); + try { + return super.parseNode(); + } catch (e) { + if (!(e instanceof LiquidHTMLASTParsingError)) throw e; + const source = this.getSource(); + const found = tokenTypeName(this.peek().type); + /* + * Guarantee at least one token of progress before scanning. A failed + * parse can throw without having advanced the cursor (consume throws + * before its own increment), and if the offending token is itself a + * resync token the scan below would match it immediately and never move, + * re-trapping parseDocument in an infinite loop. Forcing one advance + * breaks that stall. + */ + if (this.getPosition() <= startPos) this.advance(); + /* + * Skip to the next construct-open boundary so the following + * super.parseNode() resumes on a real node start rather than mid- + * construct. isResyncToken includes EndOfInput, so this also stops + * cleanly at the end of the source. + */ + while (!this.isAtEnd() && !isResyncToken(this.peek().type)) this.advance(); + const end = this.peek().start; + return makeLiquidErrorNode(startTok.start, end, source, e.message, found); + } + } +} diff --git a/packages/liquid-html-parser/src/errors.ts b/packages/liquid-html-parser/src/errors.ts index bed0af4c8..21086fe84 100644 --- a/packages/liquid-html-parser/src/errors.ts +++ b/packages/liquid-html-parser/src/errors.ts @@ -25,20 +25,44 @@ export class LiquidHTMLASTParsingError extends SyntaxError { this.unclosed = unclosed ?? null; const lc = lineColumn(source); - const start = lc.fromIndex(startIndex); - const end = lc.fromIndex(Math.min(endIndex, source.length - 1)); + + /* + * A parse can fail at a position that is out of the source's range: the + * end-of-input token sits one past the last character when a closing + * delimiter like "%}" is never found, and some failures report a -1 + * sentinel position. line-column returns null for any out-of-range + * index, so dereferencing it while building `loc` below would throw a + * TypeError ("Cannot read properties of null") that masks the real + * syntax error. + * + * We have to guard this now because the tolerant parser is the first + * caller to drive malformed input through this throw site. The `loc` + * is built here at error-construction time, before the tolerant + * parser's recovery catch runs, so a null dereference would crash + * before the throw can become a recovered LiquidErrorNode, defeating + * the tolerant parser entirely. + * + * Clamping the indices into the valid range covers the out-of-range + * and -1 cases. The `?? 1` fallback on each line/column below stays + * load-bearing for empty or degenerate source, where fromIndex(0) on + * "" is still null even after clamping. Same root cause, both needed, + * so we always produce a real location. + */ + const lastIndex = Math.max(0, source.length - 1); + const start = lc.fromIndex(Math.min(Math.max(startIndex, 0), lastIndex)); + const end = lc.fromIndex(Math.min(Math.max(endIndex, 0), lastIndex)); // Plugging ourselves into @babel/code-frame since this is how // the babel parser can print where the parsing error occured. // https://github.com/prettier/prettier/blob/cd4a57b113177c105a7ceb94e71f3a5a53535b81/src/main/parser.js this.loc = { start: { - line: start!.line, - column: start!.col, + line: start?.line ?? 1, + column: start?.col ?? 1, }, end: { - line: end!.line, - column: end!.col, + line: end?.line ?? 1, + column: end?.col ?? 1, }, }; } diff --git a/packages/liquid-html-parser/src/index.ts b/packages/liquid-html-parser/src/index.ts index 5fd75e961..749d2c404 100644 --- a/packages/liquid-html-parser/src/index.ts +++ b/packages/liquid-html-parser/src/index.ts @@ -1,6 +1,7 @@ export * from './ast'; export * from './types'; export * from './errors'; +export { findErrorNodeAtOffset, toTolerantLiquidAST, toTolerantLiquidHtmlAST } from './tolerant'; export { TAGS_WITHOUT_MARKUP, RAW_TAGS, VOID_ELEMENTS, BLOCKS } from './grammar'; export { getConditionalComment } from './conditional-comment'; export { tokenize, TokenType } from './document/tokenizer'; diff --git a/packages/liquid-html-parser/src/liquid-doc/parser.ts b/packages/liquid-html-parser/src/liquid-doc/parser.ts index 364a418a9..b0bb4bec0 100644 --- a/packages/liquid-html-parser/src/liquid-doc/parser.ts +++ b/packages/liquid-html-parser/src/liquid-doc/parser.ts @@ -119,6 +119,18 @@ export class LiquidDocParser { const annotationToken = this.consume(LiquidDocTokenType.Annotation); const name = annotationToken.value; + // `@descriptionText` — no space between `@description` and its content. The + // level-1 tokenizer greedily folds the leading text into the annotation + // name (`descriptionText`), so recognize the `description` prefix here and + // treat the glued remainder as the start of the inline description content. + // This matches the pre-swap parser, which printed `@description Text`. + if ( + name !== LiquidDocAnnotation.Description && + name.startsWith(LiquidDocAnnotation.Description) + ) { + return this.parseGluedDescription(annotationToken); + } + if (!isKnownAnnotation(name)) { return this.parseUnsupportedAnnotation(annotationToken); } @@ -282,6 +294,36 @@ export class LiquidDocParser { ); } + // gluedDescription := "@description" gluedContent (no separating space) + // + // The glued suffix and any same-line text that follows are one inline + // description. Content is read straight from source (not by concatenating + // token values) so the original spacing between the suffix and the trailing + // text is preserved — the suffix's own token boundary is mid-word. + private parseGluedDescription(annotationToken: LiquidDocToken): LiquidDocDescriptionNode { + const start = annotationToken.start; + // Source offset immediately past the literal `@description`; the glued + // remainder (e.g. `This` in `@descriptionThis`) begins here. + const contentStart = start + 1 + LiquidDocAnnotation.Description.length; + + const inlineToken = this.accept(LiquidDocTokenType.TextLine); + const { value: rest, endPos } = this.collectContentLines(); + const hasRest = rest.trim().length > 0; + + let end: number; + if (hasRest) end = endPos; + else if (inlineToken) end = inlineToken.end; + else end = annotationToken.end; + + const content = makeTextNode( + this.source.slice(contentStart, end), + contentStart, + end, + this.source, + ); + return makeLiquidDocDescriptionNode(content, false, true, start, end, this.source); + } + // example := "@example" content private parseExample(annotationToken: LiquidDocToken): LiquidDocExampleNode { const start = annotationToken.start; diff --git a/packages/liquid-html-parser/src/markup/parser.ts b/packages/liquid-html-parser/src/markup/parser.ts index a5a1c5961..de78f751b 100644 --- a/packages/liquid-html-parser/src/markup/parser.ts +++ b/packages/liquid-html-parser/src/markup/parser.ts @@ -52,6 +52,14 @@ export class MarkupParser { * `liquid-render-tree/src/lax-recover.ts`. */ private lax: boolean; + /** + * Tolerant recovery axis, DISJOINT from `lax`. Enabled exclusively by the + * tolerant document parser (`TolerantDocumentParser`) so the formatter can + * build a best-effort tag/markup node instead of discarding markup. It never + * affects strict parsing (`toLiquidHtmlAST` / `theme-check`) and it does NOT + * change lax (Ruby-render-parity) behavior. + */ + private tolerant: boolean; /** * True while parsing a condition (`if`/`unless`/`elsif`). In lax mode this * permits stripping meaningless grouping parens (`(false || true)`), which is @@ -69,6 +77,7 @@ export class MarkupParser { this.markupStart = markupStart ?? tokens[0]?.start ?? 0; this.markupEnd = markupEnd ?? this.computeLastTokenEnd(); this.lax = false; + this.tolerant = false; this.inCondition = false; } @@ -86,6 +95,21 @@ export class MarkupParser { return this.lax; } + /** Enables tolerant recovery for subsequent parse calls. Returns `this` for + * chaining. Only the tolerant document parser calls this. Disjoint from + * lax — enabling one does not enable the other. */ + enableTolerant(): this { + this.tolerant = true; + return this; + } + + /** True when tolerant recovery is enabled. Tag/markup parse callbacks consult + * this to build a best-effort node instead of discarding markup, without + * affecting strict or lax parsing. */ + isTolerant(): boolean { + return this.tolerant; + } + /** Returns the raw source text from the current token up to (but excluding) * the token whose type is `stop` (or end of markup), trimmed. Advances the * cursor past everything consumed. Used by lax recovery to capture an @@ -1173,7 +1197,7 @@ export class MarkupParser { const result: LiquidFilter[] = []; while (this.consumeOptional(MarkupTokenType.Pipe)) { // Lax: a `|` not followed by a filter name (e.g. trailing `|`) is dropped. - if (this.lax && !this.look(MarkupTokenType.Id)) { + if ((this.lax || this.tolerant) && !this.look(MarkupTokenType.Id)) { this.skipToNextPipe(); continue; } @@ -1199,7 +1223,7 @@ export class MarkupParser { if (this.consumeOptional(MarkupTokenType.Colon)) { // Lax: a colon with no following argument (`upcase:`) is tolerated; only // parse arguments when something argument-like actually follows. - if (!this.lax || this.atArgumentStart()) { + if (!(this.lax || this.tolerant) || this.atArgumentStart()) { args = this.arguments(); } if (args.length > 0) { @@ -1259,7 +1283,7 @@ export class MarkupParser { while (this.consumeOptional(MarkupTokenType.Comma)) { // Lax: a trailing or empty comma (`append: "x",`) ends the argument list // rather than forcing another argument parse. - if (this.lax && !this.atArgumentStart()) { + if ((this.lax || this.tolerant) && !this.atArgumentStart()) { break; } args.push(this.argument()); diff --git a/packages/liquid-html-parser/src/parser-oracle.test.ts b/packages/liquid-html-parser/src/parser-oracle.test.ts index b9d7c981b..7e8d36d4a 100644 --- a/packages/liquid-html-parser/src/parser-oracle.test.ts +++ b/packages/liquid-html-parser/src/parser-oracle.test.ts @@ -1,11 +1,9 @@ import { describe, expect, it } from 'vitest'; import { readFileSync, readdirSync, existsSync } from 'node:fs'; -import { dirname, resolve, join } from 'node:path'; -import { fileURLToPath } from 'node:url'; +import { resolve, join } from 'node:path'; import { toLiquidHtmlAST, toLiquidAST } from './ast'; import { nonTraversableProperties } from './types'; -const __dirname = dirname(fileURLToPath(import.meta.url)); const FIXTURES_DIR = resolve(__dirname, '..', 'fixtures', 'theme'); const GOLDEN_HTML_AST_DIR = resolve(__dirname, '..', 'fixtures', 'golden-html-ast'); const GOLDEN_LIQUID_AST_DIR = resolve(__dirname, '..', 'fixtures', 'golden-liquid-ast'); diff --git a/packages/liquid-html-parser/src/tags/assign.ts b/packages/liquid-html-parser/src/tags/assign.ts index 10ca63e8a..7c6f68502 100644 --- a/packages/liquid-html-parser/src/tags/assign.ts +++ b/packages/liquid-html-parser/src/tags/assign.ts @@ -42,7 +42,7 @@ export const assignTag: TagDefinitionTag = { name, value, position: { start, end: markup.peek().start }, - source: '', + source: value.source, }; }, }; diff --git a/packages/liquid-html-parser/src/tags/block.ts b/packages/liquid-html-parser/src/tags/block.ts index 471bf7897..d57ed52e9 100644 --- a/packages/liquid-html-parser/src/tags/block.ts +++ b/packages/liquid-html-parser/src/tags/block.ts @@ -37,7 +37,7 @@ export const blockTag: TagDefinitionBlock = { name, args, position: { start: name.position.start, end: markup.peek().start }, - source: '', + source: name.source, }; }, }; diff --git a/packages/liquid-html-parser/src/tags/capture.ts b/packages/liquid-html-parser/src/tags/capture.ts index ea9b69b79..1c877aef4 100644 --- a/packages/liquid-html-parser/src/tags/capture.ts +++ b/packages/liquid-html-parser/src/tags/capture.ts @@ -48,7 +48,7 @@ export const captureTag: TagDefinitionBlock = { name: match[1], lookups: [], position: { start: bodyStart, end: bodyEnd }, - source: '', + source, }; } // No VariableSignature run (`''`, empty, spaces only) → unrecoverable in diff --git a/packages/liquid-html-parser/src/tags/content-for.ts b/packages/liquid-html-parser/src/tags/content-for.ts index 5c9c06727..b9832d2d7 100644 --- a/packages/liquid-html-parser/src/tags/content-for.ts +++ b/packages/liquid-html-parser/src/tags/content-for.ts @@ -13,8 +13,14 @@ export const contentForTag: TagDefinitionTag = { } const args: LiquidNamedArgument[] = []; - if (markup.consumeOptional(MarkupTokenType.Comma)) { - args.push(...markup.namedArguments()); + markup.consumeOptional(MarkupTokenType.Comma); + while (markup.look(MarkupTokenType.Id)) { + try { + args.push(markup.namedArgument()); + } catch { + break; + } + markup.consumeOptional(MarkupTokenType.Comma); } return { @@ -22,7 +28,7 @@ export const contentForTag: TagDefinitionTag = { contentForType, args, position: { start: contentForType.position.start, end: markup.peek().start }, - source: '', + source: contentForType.source, }; }, }; diff --git a/packages/liquid-html-parser/src/tags/cycle.ts b/packages/liquid-html-parser/src/tags/cycle.ts index 994ff7a8f..6dc8e5ebc 100644 --- a/packages/liquid-html-parser/src/tags/cycle.ts +++ b/packages/liquid-html-parser/src/tags/cycle.ts @@ -34,7 +34,7 @@ export const cycleTag: TagDefinitionTag = { groupName, args, position: { start: first.position.start, end: markup.peek().start }, - source: '', + source: first.source, }; }, }; diff --git a/packages/liquid-html-parser/src/tags/for.ts b/packages/liquid-html-parser/src/tags/for.ts index b59dd8f24..32d8cf65f 100644 --- a/packages/liquid-html-parser/src/tags/for.ts +++ b/packages/liquid-html-parser/src/tags/for.ts @@ -42,7 +42,7 @@ function parseForMarkup(_name: string, markup: MarkupParser, _parser: Parser): F reversed, args, position: { start: nameToken.start, end: markup.peek().start }, - source: '', + source: collection.source, }; } diff --git a/packages/liquid-html-parser/src/tags/liquid.ts b/packages/liquid-html-parser/src/tags/liquid.ts index c2b1e8554..a0d2c4592 100644 --- a/packages/liquid-html-parser/src/tags/liquid.ts +++ b/packages/liquid-html-parser/src/tags/liquid.ts @@ -78,7 +78,11 @@ function parseLines(body: string, bodyStart: number): LiquidLine[] { if (firstLineTrimmed.startsWith('#')) { tagName = '#'; - markup = trimmed.slice(1).trimStart(); + // Strip at most one separator whitespace after `#`, mirroring the + // pre-swap ohm grammar `"#" space?`. Trimming all leading whitespace + // would collapse intentional indentation in `#`-comment art such as + // `# fancy`, which the printer re-pads with a single space. + markup = trimmed.slice(1).replace(/^[ \t]/, ''); nameOffset = startIndex + leadingWs; markupOffset = startIndex + leadingWs + 1 + (trimmed.length - 1 - markup.length); } else { diff --git a/packages/liquid-html-parser/src/tags/paginate.ts b/packages/liquid-html-parser/src/tags/paginate.ts index 06f910ff1..9d8422bcb 100644 --- a/packages/liquid-html-parser/src/tags/paginate.ts +++ b/packages/liquid-html-parser/src/tags/paginate.ts @@ -19,6 +19,12 @@ export const paginateTag: TagDefinitionBlock = { const args: LiquidNamedArgument[] = []; if (markup.consumeOptional(MarkupTokenType.Comma)) { args.push(...markup.namedArguments()); + } else if (markup.isTolerant() && markup.peek().type === MarkupTokenType.Id) { + // Tolerant: paginate accepts whitespace-separated named attrs with no + // leading comma (`... by N window_size: 50`). Consume them so the tag + // builds as PaginateMarkup instead of leaving leftover tokens. Strict is + // unchanged. + args.push(...markup.namedArguments()); } return { @@ -27,7 +33,7 @@ export const paginateTag: TagDefinitionBlock = { pageSize, args, position: { start: collection.position.start, end: markup.peek().start }, - source: '', + source: collection.source, }; }, }; diff --git a/packages/liquid-html-parser/src/tags/render.ts b/packages/liquid-html-parser/src/tags/render.ts index ca4b83be3..f53ee2178 100644 --- a/packages/liquid-html-parser/src/tags/render.ts +++ b/packages/liquid-html-parser/src/tags/render.ts @@ -58,7 +58,7 @@ function parseRenderMarkup( kind, name, position: { start: kwStart, end: name.position.end }, - source: '', + source: name.source, }; } @@ -70,7 +70,7 @@ function parseRenderMarkup( type: NodeTypes.RenderAliasExpression, value: aliasToken.value, position: { start: asStart, end: aliasToken.end }, - source: '', + source: snippet.source, }; } @@ -96,7 +96,7 @@ function parseRenderMarkup( alias, args, position: { start: snippet.position.start, end }, - source: '', + source: snippet.source, }; } diff --git a/packages/liquid-html-parser/src/tags/section.ts b/packages/liquid-html-parser/src/tags/section.ts index 3f5828c0f..3df088ca2 100644 --- a/packages/liquid-html-parser/src/tags/section.ts +++ b/packages/liquid-html-parser/src/tags/section.ts @@ -22,7 +22,7 @@ export const sectionTag: TagDefinitionHybrid = { name, args, position: { start: name.position.start, end: markup.peek().start }, - source: '', + source: name.source, }; }, }; diff --git a/packages/liquid-html-parser/src/tolerant.test.ts b/packages/liquid-html-parser/src/tolerant.test.ts new file mode 100644 index 000000000..b4da977b3 --- /dev/null +++ b/packages/liquid-html-parser/src/tolerant.test.ts @@ -0,0 +1,267 @@ +import { describe, expect, it } from 'vitest'; +import { existsSync, readdirSync, readFileSync } from 'node:fs'; +import { join, resolve } from 'node:path'; +import { isLiquidHtmlNode, toLiquidAST, toLiquidHtmlAST, walk } from './ast'; +import type { DocumentNode, LiquidErrorNode, LiquidHtmlNode } from './ast'; +import { LiquidHTMLASTParsingError } from './errors'; +import { findErrorNodeAtOffset, toTolerantLiquidAST, toTolerantLiquidHtmlAST } from './tolerant'; +import { NodeTypes } from './types'; + +/* + * Drop the `source`/`_source` strings before comparing two ASTs. Both parses + * receive the identical source argument, so those fields are equal by + * construction; stripping them keeps the structural comparison fast and loses + * nothing. Mirrors the strip step in parser-oracle.test.ts. + */ +function structural(ast: unknown): unknown { + return JSON.parse( + JSON.stringify(ast, (key, value) => + key === 'source' || key === '_source' ? undefined : value, + ), + ); +} + +function errorNodesOf(ast: DocumentNode): LiquidErrorNode[] { + const found: LiquidErrorNode[] = []; + walk(ast, (node) => { + if (node.type === NodeTypes.LiquidErrorNode) { + found.push(node as LiquidErrorNode); + } + }); + return found; +} + +/* + * Default-path neutrality proof (Gate-S1 / golden-neutrality gate). + * + * Tolerant mode is a different class reached by a different function; on + * well-formed input its overridden `parseNode` never catches, so it must be + * byte-identical to the strict default path. These are the two proofs of that: + * a curated set of clean sources, and the whole on-disk clean fixture corpus. + */ +describe('tolerant mode is inert on clean input', () => { + const cleanSources = [ + '', + 'plain text only', + '{{ product.title }}', + '{% assign x = 1 %}', + '{% if x %}a{% else %}b{% endif %}', + '{% for item in collection %}{{ item.title }}{% endfor %}', + '
{{ x }}
', + '{% render \'snippet\' %}', + '{% liquid\n assign y = 2\n echo y\n%}', + '{% comment %}hi{% endcomment %}', + 'text {{ a }} more {% if b %}{{ c }}{% endif %} end', + ]; + + for (const source of cleanSources) { + it(`toTolerantLiquidHtmlAST deep-equals toLiquidHtmlAST for ${JSON.stringify(source)}`, () => { + expect(structural(toTolerantLiquidHtmlAST(source))).toEqual( + structural(toLiquidHtmlAST(source)), + ); + }); + + it(`toTolerantLiquidAST deep-equals toLiquidAST for ${JSON.stringify(source)}`, () => { + expect(structural(toTolerantLiquidAST(source))).toEqual(structural(toLiquidAST(source))); + }); + } +}); + +const FIXTURES_DIR = resolve(__dirname, '..', 'fixtures', 'theme'); +const GOLDEN_HTML_AST_DIR = resolve(__dirname, '..', 'fixtures', 'golden-html-ast'); +const GOLDEN_LIQUID_AST_DIR = resolve(__dirname, '..', 'fixtures', 'golden-liquid-ast'); +const THEMES = ['base-theme', 'dawn', 'horizon']; + +/** Recursively find all .liquid files under a directory. */ +function findLiquidFiles(dir: string, prefix = ''): { path: string; fullPath: string }[] { + const results: { path: string; fullPath: string }[] = []; + if (!existsSync(dir)) return results; + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const relPath = prefix ? `${prefix}/${entry.name}` : entry.name; + if (entry.isDirectory()) { + results.push(...findLiquidFiles(join(dir, entry.name), relPath)); + } else if (entry.name.endsWith('.liquid')) { + results.push({ path: relPath, fullPath: join(dir, entry.name) }); + } + } + return results; +} + +function goldenFileName(relativePath: string): string { + return relativePath.replace(/\//g, '-') + '.json'; +} + +const hasFixtures = existsSync(FIXTURES_DIR) && readdirSync(FIXTURES_DIR).length > 0; + +/* + * The clean corpus is exactly the files the oracle has a golden for: a golden + * exists only for a file the strict parser accepts, so those files are the + * proven-clean set. Tolerant must reproduce the strict AST for each. + */ +describe.skipIf(!hasFixtures)('tolerant mode is inert over the clean fixture corpus', () => { + for (const theme of THEMES) { + const themeDir = join(FIXTURES_DIR, theme); + const files = findLiquidFiles(themeDir); + const themeMissing = files.length === 0; + + describe.skipIf(themeMissing)(`toTolerantLiquidHtmlAST === toLiquidHtmlAST - ${theme}`, () => { + for (const { path, fullPath } of files) { + const goldenPath = join(GOLDEN_HTML_AST_DIR, theme, goldenFileName(path)); + if (!existsSync(goldenPath)) continue; + + it(`matches strict for ${theme}/${path}`, () => { + const source = readFileSync(fullPath, 'utf-8'); + expect(structural(toTolerantLiquidHtmlAST(source))).toEqual( + structural(toLiquidHtmlAST(source)), + ); + }); + } + }); + + describe.skipIf(themeMissing)(`toTolerantLiquidAST === toLiquidAST - ${theme}`, () => { + for (const { path, fullPath } of files) { + const goldenPath = join(GOLDEN_LIQUID_AST_DIR, theme, goldenFileName(path)); + if (!existsSync(goldenPath)) continue; + + it(`matches strict for ${theme}/${path}`, () => { + const source = readFileSync(fullPath, 'utf-8'); + expect(structural(toTolerantLiquidAST(source))).toEqual(structural(toLiquidAST(source))); + }); + } + }); + } +}); + +/* + * Positive-path tolerant behavior. These consolidate the throwaway probes + * from B2 (error nodes emitted), B3 (panic-mode resync) and B5 (locator) into + * committed tests. + */ +describe('tolerant mode surfaces parse errors as nodes', () => { + it('strict parse throws on a close tag with no matching open', () => { + expect(() => toLiquidHtmlAST('{% endfor %}')).toThrow(LiquidHTMLASTParsingError); + }); + + it('toTolerantLiquidHtmlAST emits a LiquidErrorNode instead of throwing', () => { + const source = '{% endfor %}'; + const ast = toTolerantLiquidHtmlAST(source); + + expect(ast.type).toBe(NodeTypes.Document); + expect(ast.children).toHaveLength(1); + + const node = ast.children[0] as LiquidErrorNode; + expect(node.type).toBe(NodeTypes.LiquidErrorNode); + expect(node.position).toEqual({ start: 0, end: 12 }); + expect(node.found).toBe('EndOfInput'); + expect(typeof node.message).toBe('string'); + expect(source.slice(node.position.start, node.position.end)).toBe('{% endfor %}'); + }); + + it('toTolerantLiquidAST also recovers instead of throwing', () => { + const ast = toTolerantLiquidAST('{% endfor %}'); + + expect(ast.type).toBe(NodeTypes.Document); + expect(errorNodesOf(ast)).toHaveLength(1); + }); +}); + +describe('tolerant mode resynchronizes after an error (panic mode)', () => { + it('recovers valid nodes between multiple errors and terminates', () => { + const ast = toTolerantLiquidHtmlAST('{% endfor %}{{ good }}{% endif %}'); + + expect(ast.children.map((child) => child.type)).toEqual([ + NodeTypes.LiquidErrorNode, + NodeTypes.LiquidVariableOutput, + NodeTypes.LiquidErrorNode, + ]); + + const errors = errorNodesOf(ast); + expect(errors.length).toBeGreaterThanOrEqual(2); + + // The construct between the two errors is recovered as a real node. + const recovered = ast.children[1]; + expect(recovered.type).toBe(NodeTypes.LiquidVariableOutput); + expect(recovered.position).toEqual({ start: 12, end: 22 }); + + // Errors resynced on the next construct-open boundary. + expect(ast.children[0].position).toEqual({ start: 0, end: 12 }); + expect(ast.children[2].position).toEqual({ start: 22, end: 33 }); + }); +}); + +describe('findErrorNodeAtOffset', () => { + const source = '{% endfor %} {{ good }} {% endif %}'; + const ast = toTolerantLiquidHtmlAST(source); + + it('produces two error nodes spanning up to the next construct boundary', () => { + const errors = errorNodesOf(ast); + expect(errors).toHaveLength(2); + expect(errors[0].position).toEqual({ start: 0, end: 13 }); + expect(errors[1].position).toEqual({ start: 24, end: 35 }); + }); + + it('error nodes are first-class, walkable citizens', () => { + const errors = errorNodesOf(ast); + expect(isLiquidHtmlNode(errors[0])).toBe(true); + }); + + it('returns the error node the offset sits inside, with its ancestry', () => { + const result = findErrorNodeAtOffset(ast, 5); + expect(result).not.toBeNull(); + expect(result!.node.type).toBe(NodeTypes.LiquidErrorNode); + expect(result!.node.position).toEqual({ start: 0, end: 13 }); + expect(result!.ancestors[0].type).toBe(NodeTypes.Document); + }); + + it('resolves the second error span for an offset inside it', () => { + const result = findErrorNodeAtOffset(ast, 30); + expect(result).not.toBeNull(); + expect(result!.node.position).toEqual({ start: 24, end: 35 }); + }); + + it('returns null for an offset outside every error span', () => { + // Offset 17 is inside the recovered {{ good }}, not an error region. + expect(findErrorNodeAtOffset(ast, 17)).toBeNull(); + }); + + it('returns the deepest error node when spans nest', () => { + const src = 'x'.repeat(20); + const inner = { + type: NodeTypes.LiquidErrorNode, + position: { start: 5, end: 10 }, + source: src, + message: 'inner', + } as LiquidErrorNode; + const container = { + type: NodeTypes.HtmlElement, + position: { start: 2, end: 15 }, + source: src, + children: [inner], + } as unknown as LiquidHtmlNode; + const outer = { + type: NodeTypes.LiquidErrorNode, + position: { start: 0, end: 20 }, + source: src, + message: 'outer', + } as LiquidErrorNode; + const root = { + type: NodeTypes.Document, + name: '#document', + position: { start: 0, end: 20 }, + source: src, + _source: src, + children: [outer, container], + } as unknown as LiquidHtmlNode; + + const deep = findErrorNodeAtOffset(root, 7); + expect(deep!.node.message).toBe('inner'); + expect(deep!.ancestors.map((node) => node.type)).toEqual([ + NodeTypes.Document, + NodeTypes.HtmlElement, + ]); + + const shallow = findErrorNodeAtOffset(root, 1); + expect(shallow!.node.message).toBe('outer'); + expect(shallow!.ancestors.map((node) => node.type)).toEqual([NodeTypes.Document]); + }); +}); diff --git a/packages/liquid-html-parser/src/tolerant.ts b/packages/liquid-html-parser/src/tolerant.ts new file mode 100644 index 000000000..27de69e64 --- /dev/null +++ b/packages/liquid-html-parser/src/tolerant.ts @@ -0,0 +1,138 @@ +/** + * Opt-in tolerant entry points for the Liquid+HTML parser. + * + * These mirror `toLiquidAST` / `toLiquidHtmlAST` from `./ast` exactly, except + * they construct a `TolerantDocumentParser` instead of the base + * `DocumentParser`. A structural parse failure that would abort the default + * parse is instead caught and surfaced as a `LiquidErrorNode`, so the returned + * `DocumentNode` can carry several errors interleaved with the constructs the + * parser did recover. The default path is a different class reached by a + * different function and stays byte-identical. + */ + +import type { ASTBuildOptions, DocumentNode, LiquidErrorNode, LiquidHtmlNode } from './ast'; +import { walk } from './ast'; +import { TolerantDocumentParser } from './document/tolerant-parser'; +import { tokenize } from './document/tokenizer'; +import { Environment } from './environment'; +import { NodeTypes } from './types'; + +/* + * Tolerant variant of `toLiquidAST` (Liquid-only, `parseHtml: false`). + * Defaults `allowUnclosedDocumentNode: true` so an unterminated document is + * recovered rather than thrown on — the tolerant contract. + */ +export function toTolerantLiquidAST( + source: string, + options: ASTBuildOptions = { + allowUnclosedDocumentNode: true, + mode: 'tolerant', + }, +): DocumentNode { + const env = options.environment ?? Environment.default(); + const tokens = tokenize(source); + const parser = new TolerantDocumentParser( + tokens, + source, + env, + false, + options.allowUnclosedDocumentNode, + ); + return parser.parseDocument(); +} + +/* + * Tolerant variant of `toLiquidHtmlAST` (Liquid+HTML, `parseHtml: true`). + * Defaults `allowUnclosedDocumentNode: true` so an unterminated document is + * recovered rather than thrown on — the tolerant contract. + */ +export function toTolerantLiquidHtmlAST( + source: string, + options: ASTBuildOptions = { + allowUnclosedDocumentNode: true, + mode: 'tolerant', + }, +): DocumentNode { + const env = options.environment ?? Environment.default(); + const tokens = tokenize(source); + const parser = new TolerantDocumentParser( + tokens, + source, + env, + true, + options.allowUnclosedDocumentNode, + ); + return parser.parseDocument(); +} + +/* + * Locate the error node the caret is sitting in. + * + * A tolerant parse can leave several `LiquidErrorNode`s scattered through + * the tree, one per region the parser gave up on. Completion needs the one + * the caret is actually inside, so we return the *deepest* error node whose + * span contains `offset` — the most specific recovery point — paired with + * its ancestry. + * + * The result shape (`{ node, ancestors }`) mirrors `findCurrentNode` in the + * language server so a later phase can swap the completion context source + * with minimal churn. `null` means the caret is not inside any error region + * and the existing (non-error) path should handle it. + */ +export function findErrorNodeAtOffset( + ast: LiquidHtmlNode, + offset: number, +): { node: LiquidErrorNode; ancestors: LiquidHtmlNode[] } | null { + /* + * `walk` only hands each visited node its immediate parent, and it visits + * in post-order (parents after children), so the map is only complete once + * the traversal ends. We therefore record every parent link first, gather + * the error nodes that contain the offset, and resolve depth/ancestry from + * the finished map afterwards. + */ + const parentOf = new Map(); + const candidates: LiquidErrorNode[] = []; + + walk(ast, (node, parent) => { + parentOf.set(node, parent); + if ( + node.type === NodeTypes.LiquidErrorNode && + offset >= node.position.start && + offset <= node.position.end + ) { + candidates.push(node); + } + }); + + if (candidates.length === 0) { + return null; + } + + /* Ancestry, root→parent, by climbing the parent map from the node up. */ + const ancestorsOf = (node: LiquidHtmlNode): LiquidHtmlNode[] => { + const chain: LiquidHtmlNode[] = []; + let cursor = parentOf.get(node); + while (cursor !== undefined) { + chain.push(cursor); + cursor = parentOf.get(cursor); + } + return chain.reverse(); + }; + + /* + * Deepest wins: when error spans nest, the innermost node has the longest + * ancestor chain. First candidate wins ties, which is stable given walk's + * deterministic traversal order. + */ + let best = candidates[0]; + let bestDepth = ancestorsOf(best).length; + for (let i = 1; i < candidates.length; i++) { + const depth = ancestorsOf(candidates[i]).length; + if (depth > bestDepth) { + best = candidates[i]; + bestDepth = depth; + } + } + + return { node: best, ancestors: ancestorsOf(best) }; +} diff --git a/packages/liquid-html-parser/src/types.ts b/packages/liquid-html-parser/src/types.ts index 84c0a517e..9a984b538 100644 --- a/packages/liquid-html-parser/src/types.ts +++ b/packages/liquid-html-parser/src/types.ts @@ -52,6 +52,7 @@ export enum NodeTypes { LiquidDocParamNode = 'LiquidDocParamNode', LiquidDocExampleNode = 'LiquidDocExampleNode', LiquidDocPromptNode = 'LiquidDocPromptNode', + LiquidErrorNode = 'LiquidErrorNode', } // These are officially supported with special node types diff --git a/packages/liquid-html-parser/tsconfig.json b/packages/liquid-html-parser/tsconfig.json index f1f831c96..601fa06fc 100644 --- a/packages/liquid-html-parser/tsconfig.json +++ b/packages/liquid-html-parser/tsconfig.json @@ -1,7 +1,7 @@ { "extends": "../../tsconfig.json", "include": ["./src/**/*.ts", "./src/**/*.json"], - "exclude": ["./dist"], + "exclude": ["./dist", "src/parser.bench.ts"], "compilerOptions": { "outDir": "dist", "rootDir": "src", diff --git a/packages/prettier-plugin-liquid/src/parser.ts b/packages/prettier-plugin-liquid/src/parser.ts index 143889a7a..51bcb66d8 100644 --- a/packages/prettier-plugin-liquid/src/parser.ts +++ b/packages/prettier-plugin-liquid/src/parser.ts @@ -1,9 +1,9 @@ -import { toLiquidHtmlAST, LiquidHtmlNode } from '@shopify/liquid-html-parser'; +import { toTolerantLiquidHtmlAST, LiquidHtmlNode } from '@shopify/liquid-html-parser'; import { locEnd, locStart } from './utils'; export function parse(text: string): LiquidHtmlNode { - return toLiquidHtmlAST(text); + return toTolerantLiquidHtmlAST(text); } export const liquidHtmlAstFormat = 'liquid-html-ast'; diff --git a/packages/prettier-plugin-liquid/src/printer/preprocess/augment-with-css-properties.ts b/packages/prettier-plugin-liquid/src/printer/preprocess/augment-with-css-properties.ts index f5d204b09..9aac64c54 100644 --- a/packages/prettier-plugin-liquid/src/printer/preprocess/augment-with-css-properties.ts +++ b/packages/prettier-plugin-liquid/src/printer/preprocess/augment-with-css-properties.ts @@ -121,6 +121,8 @@ function getCssDisplay(node: AugmentedNode, options: LiquidParserO case NodeTypes.Range: case NodeTypes.VariableLookup: case NodeTypes.AssignMarkup: + case NodeTypes.BlockMarkup: + case NodeTypes.SectionMarkup: case NodeTypes.CycleMarkup: case NodeTypes.ContentForMarkup: case NodeTypes.ForMarkup: @@ -134,6 +136,7 @@ function getCssDisplay(node: AugmentedNode, options: LiquidParserO case NodeTypes.LiquidDocExampleNode: case NodeTypes.LiquidDocDescriptionNode: case NodeTypes.LiquidDocPromptNode: + case NodeTypes.LiquidErrorNode: return 'should not be relevant'; default: @@ -232,6 +235,8 @@ function getNodeCssStyleWhiteSpace( case NodeTypes.Range: case NodeTypes.VariableLookup: case NodeTypes.AssignMarkup: + case NodeTypes.BlockMarkup: + case NodeTypes.SectionMarkup: case NodeTypes.CycleMarkup: case NodeTypes.ContentForMarkup: case NodeTypes.ForMarkup: @@ -245,6 +250,7 @@ function getNodeCssStyleWhiteSpace( case NodeTypes.LiquidDocExampleNode: case NodeTypes.LiquidDocDescriptionNode: case NodeTypes.LiquidDocPromptNode: + case NodeTypes.LiquidErrorNode: return 'should not be relevant'; default: diff --git a/packages/prettier-plugin-liquid/src/printer/print/liquid.ts b/packages/prettier-plugin-liquid/src/printer/print/liquid.ts index 7e8628311..3e750ea66 100644 --- a/packages/prettier-plugin-liquid/src/printer/print/liquid.ts +++ b/packages/prettier-plugin-liquid/src/printer/print/liquid.ts @@ -7,6 +7,7 @@ import { LiquidDocExampleNode, LiquidDocDescriptionNode, LiquidDocPromptNode, + TAGS_WITHOUT_MARKUP, } from '@shopify/liquid-html-parser'; import { Doc, doc } from 'prettier'; @@ -192,13 +193,28 @@ function printNamedLiquidBlockStart( case NamedTags.increment: case NamedTags.decrement: case NamedTags.layout: - case NamedTags.section: { + case NamedTags.section: + /* + * `block` prints like `section` (both carry a name plus optional + * named arguments) and `partial` prints like `sections` (a bare + * string markup). Both delegate markup rendering to `printNode`. + */ + case NamedTags.block: + case NamedTags.partial: { return tag(' '); } case NamedTags.sections: { return tag(' '); } + /* + * `ifchanged` has no markup (`markup: null`), so we print just the + * tag name — there is nothing to render between the name and `%}`. + */ + case NamedTags.ifchanged: { + return wrapper([...prefix, node.name, ...suffix(' ')]); + } + case NamedTags.form: { const trailingWhitespace = node.markup.length > 1 ? line : ' '; return tagWithArrayMarkup(trailingWhitespace); @@ -342,16 +358,16 @@ export function printLiquidBlockStart( } const markup = node.markup; - return group([ - '{%', - whitespaceStart, - ' ', - node.name, - markup ? ` ${markup}` : '', - ' ', - whitespaceEnd, - '%}', - ]); + /* + * A few tags — `break`, `continue`, `else`, and friends listed in + * TAGS_WITHOUT_MARKUP — accept no markup at all. When one of them is + * written with stray arguments, e.g. `{% break huh?? %}`, we drop the + * markup rather than echo the invalid text back out. This restores the + * pre-port behaviour, where the parser blanked the markup before the + * printer ever saw it. + */ + const printedMarkup = markup && !TAGS_WITHOUT_MARKUP.includes(node.name) ? ` ${markup}` : ''; + return group(['{%', whitespaceStart, ' ', node.name, printedMarkup, ' ', whitespaceEnd, '%}']); } export function printLiquidBlockEnd( @@ -426,7 +442,15 @@ export function printLiquidTag( let body: Doc = []; - if (isBranchedTag(node)) { + /* + * `tablerow` is not a branched tag, but like `for` it wraps its body in a + * single default `LiquidBranch`. Routing it through `printChildren` (the + * non-branched path below) would indent that branch twice and prepend a + * blank line; the branched-path `path.map` prints the default branch with + * `for`'s single-indent, no-leading-blank shape. This is a printer-only + * special-case — `tablerow` is deliberately kept out of `isBranchedTag`. + */ + if (isBranchedTag(node) || node.name === 'tablerow') { body = cleanDoc( path.map( (p) => @@ -480,7 +504,13 @@ export function printLiquidRawTag( ' ', node.name, ' ', - node.markup ? `${node.markup} ` : '', + /* + * Argument-less raw tags such as `style` keep no markup. When one + * carries stray arguments, e.g. `{% style what %}`, we strip them + * instead of printing the invalid text; other raw tags keep their + * markup as before. + */ + node.markup && !TAGS_WITHOUT_MARKUP.includes(node.name) ? `${node.markup} ` : '', node.whitespaceEnd, '%}', ]); @@ -530,16 +560,30 @@ export function printLiquidDocParam( _args: LiquidPrinterArgs, ): Doc { const node = path.getValue(); + + /* + * A malformed `@param` line (e.g. an unclosed `[missingTail`) parses to a + * degenerate node with an empty `paramName`. Synthesizing the parts below + * would emit a spurious `@param - ...`, so emit the raw source span + * verbatim instead to preserve a faithful round-trip. `@param` is kept as + * the first part so `printLiquidDoc`'s tag grouping still treats this as a + * `@param` node and does not insert a blank line before the next param. + */ + if (node.paramName.value === '') { + const raw = node.source.slice(node.position.start, node.position.end); + return ['@param', raw.slice('@param'.length)]; + } + const parts: Doc[] = ['@param']; if (node.paramType?.value) { - parts.push(' ', `{${node.paramType.value}}`); + parts.push(' ', `{${node.paramType.value.trim()}}`); } if (node.required) { - parts.push(' ', node.paramName.value); + parts.push(' ', node.paramName.value.trim()); } else { - parts.push(' ', `[${node.paramName.value}]`); + parts.push(' ', `[${node.paramName.value.trim()}]`); } if (node.paramDescription?.value) { diff --git a/packages/prettier-plugin-liquid/src/printer/print/tag.ts b/packages/prettier-plugin-liquid/src/printer/print/tag.ts index 3c28506ee..675ce5bda 100644 --- a/packages/prettier-plugin-liquid/src/printer/print/tag.ts +++ b/packages/prettier-plugin-liquid/src/printer/print/tag.ts @@ -437,11 +437,18 @@ function getCompoundName( .map((part) => { if (part.type === NodeTypes.TextNode) { return part.value; - } else if (typeof part.markup === 'string') { - return `{{ ${part.markup.trim()} }}`; - } else { - return `{{ ${part.markup.rawSource} }}`; } + if (part.type === NodeTypes.LiquidVariableOutput) { + return typeof part.markup === 'string' + ? `{{ ${part.markup.trim()} }}` + : `{{ ${part.markup.rawSource} }}`; + } + /* + * Remaining compound-name arms are LiquidTag | LiquidRawTag + * (e.g. `<{% if c %}a{% endif %}>`). Neither carries a `.rawSource`, + * so reproduce the original source span verbatim. + */ + return part.source.slice(part.position.start, part.position.end); }) .join(''); } diff --git a/packages/prettier-plugin-liquid/src/printer/printer-liquid-html.ts b/packages/prettier-plugin-liquid/src/printer/printer-liquid-html.ts index 57f4f91bf..1f61701e0 100644 --- a/packages/prettier-plugin-liquid/src/printer/printer-liquid-html.ts +++ b/packages/prettier-plugin-liquid/src/printer/printer-liquid-html.ts @@ -194,6 +194,18 @@ function printNode( args: LiquidPrinterArgs = {}, ): Doc { const node = path.getValue(); + + if ((node as any).type === 'BlockArrayLiteral') { + return [ + '[', + join( + [',', line], + (path as any).map((p: any) => print(p), 'elements'), + ), + ']', + ]; + } + switch (node.type) { case NodeTypes.Document: { return [printChildren(path as AstPath, options, print, args), hardline]; @@ -343,7 +355,7 @@ function printNode( whitespace, join( [',', whitespace], - path.map((p) => print(p), 'args'), + path.map((p: any) => print(p), 'args'), ), ); @@ -362,7 +374,7 @@ function printNode( line, join( line, - path.map((p) => print(p), 'args'), + path.map((p: any) => print(p), 'args'), ), ]); } @@ -384,7 +396,7 @@ function printNode( line, join( [',', line], - path.map((p) => print(p), 'args'), + path.map((p: any) => print(p), 'args'), ), ]); } @@ -401,7 +413,29 @@ function printNode( line, join( [',', line], - path.map((p) => print(p), 'args'), + path.map((p: any) => print(p), 'args'), + ), + ); + } + + return doc; + } + + /* + * `block` and `section` markup share the same shape: a name (a + * LiquidString) followed by optional named arguments. We print them + * the same way `content_for` prints its markup. + */ + case NodeTypes.BlockMarkup: + case NodeTypes.SectionMarkup: { + const doc: Doc = [path.call((p: any) => print(p), 'name')]; + if (node.args.length > 0) { + doc.push( + ',', + line, + join( + [',', line], + path.map((p: any) => print(p), 'args'), ), ); } @@ -428,7 +462,7 @@ function printNode( line, join( [',', line], - path.map((p) => print(p), 'args'), + path.map((p: any) => print(p), 'args'), ), ); } @@ -479,7 +513,7 @@ function printNode( let args: Doc[] = []; if (node.args.length > 0) { - const printed = path.map((p) => print(p), 'args'); + const printed = path.map((p: any) => print(p), 'args'); const shouldPrintFirstArgumentSameLine = node.args[0].type !== NodeTypes.NamedArgument; if (shouldPrintFirstArgumentSameLine) { @@ -590,6 +624,10 @@ function printNode( return printLiquidDocPrompt(path as AstPath, options, print, args); } + case NodeTypes.LiquidErrorNode: { + return node.source.slice(node.position.start, node.position.end); + } + default: { return assertNever(node); } diff --git a/packages/prettier-plugin-liquid/src/printer/utils/node.ts b/packages/prettier-plugin-liquid/src/printer/utils/node.ts index 42c723bc1..5341de3e8 100644 --- a/packages/prettier-plugin-liquid/src/printer/utils/node.ts +++ b/packages/prettier-plugin-liquid/src/printer/utils/node.ts @@ -1,4 +1,10 @@ -import { NodeTypes, LiquidNodeTypes, HtmlNodeTypes, Position } from '@shopify/liquid-html-parser'; +import { + NodeTypes, + LiquidNodeTypes, + HtmlNodeTypes, + Position, + CompoundNameSegment, +} from '@shopify/liquid-html-parser'; import { HtmlSelfClosingElement, LiquidHtmlNode, @@ -330,10 +336,7 @@ export function getLastDescendant(node: LiquidHtmlNode): LiquidHtmlNode { return node.lastChild ? getLastDescendant(node.lastChild) : node; } -function isTagNameIncluded( - collection: string[], - name: (TextNode | LiquidVariableOutput)[], -): boolean { +function isTagNameIncluded(collection: string[], name: CompoundNameSegment[]): boolean { if (name.length !== 1 || name[0].type !== NodeTypes.TextNode) return false; return collection.includes(name[0].value); } diff --git a/packages/prettier-plugin-liquid/src/test/liquid-tag-ifchanged/fixed.liquid b/packages/prettier-plugin-liquid/src/test/liquid-tag-ifchanged/fixed.liquid new file mode 100644 index 000000000..a6b10d22f --- /dev/null +++ b/packages/prettier-plugin-liquid/src/test/liquid-tag-ifchanged/fixed.liquid @@ -0,0 +1,5 @@ +It should format ifchanged block tags +{% ifchanged %}content{% endifchanged %} + +It should normalize whitespace control on ifchanged tags +{%- ifchanged -%}content{%- endifchanged -%} diff --git a/packages/prettier-plugin-liquid/src/test/liquid-tag-ifchanged/index.liquid b/packages/prettier-plugin-liquid/src/test/liquid-tag-ifchanged/index.liquid new file mode 100644 index 000000000..f8130f74a --- /dev/null +++ b/packages/prettier-plugin-liquid/src/test/liquid-tag-ifchanged/index.liquid @@ -0,0 +1,5 @@ +It should format ifchanged block tags +{% ifchanged %}content{% endifchanged %} + +It should normalize whitespace control on ifchanged tags +{%- ifchanged -%}content{%- endifchanged -%} diff --git a/packages/prettier-plugin-liquid/src/test/liquid-tag-ifchanged/index.spec.ts b/packages/prettier-plugin-liquid/src/test/liquid-tag-ifchanged/index.spec.ts new file mode 100644 index 000000000..0a3e8d617 --- /dev/null +++ b/packages/prettier-plugin-liquid/src/test/liquid-tag-ifchanged/index.spec.ts @@ -0,0 +1,7 @@ +import { test } from 'vitest'; +import { assertFormattedEqualsFixed } from '../test-helpers'; +import * as path from 'path'; + +test('Unit: liquid-tag-ifchanged', async () => { + await assertFormattedEqualsFixed(__dirname); +}); diff --git a/packages/prettier-plugin-liquid/vitest.config.mjs b/packages/prettier-plugin-liquid/vitest.config.mjs index 8c27203ac..488dbfd86 100644 --- a/packages/prettier-plugin-liquid/vitest.config.mjs +++ b/packages/prettier-plugin-liquid/vitest.config.mjs @@ -9,6 +9,5 @@ export default defineConfig({ maxWorkers: 1, isolate: true, globalSetup: ['./src/test/test-setup.js'], - setupFiles: ['../liquid-html-parser/build/shims.js'], }, }); diff --git a/packages/theme-check-common/src/checks/cdn-preconnect/index.ts b/packages/theme-check-common/src/checks/cdn-preconnect/index.ts index 0bb04972a..12b4b60e1 100644 --- a/packages/theme-check-common/src/checks/cdn-preconnect/index.ts +++ b/packages/theme-check-common/src/checks/cdn-preconnect/index.ts @@ -1,5 +1,6 @@ +import { HtmlSelfClosingElement, HtmlVoidElement } from '@shopify/liquid-html-parser'; import { Severity, SourceCodeType, LiquidCheckDefinition } from '../../types'; -import { isAttr, isValuedHtmlAttribute, valueIncludes } from '../utils'; +import { getHtmlNodeName, isAttr, isValuedHtmlAttribute, valueIncludes } from '../utils'; export const CdnPreconnect: LiquidCheckDefinition = { meta: { @@ -17,26 +18,37 @@ export const CdnPreconnect: LiquidCheckDefinition = { }, create(context) { - return { - async HtmlVoidElement(node) { - if (node.name !== 'link') return; + function checkNode(node: HtmlVoidElement | HtmlSelfClosingElement) { + if (getHtmlNodeName(node) !== 'link') return; + + const isPreconnect = node.attributes + .filter(isValuedHtmlAttribute) + .some((attr) => isAttr(attr, 'rel') && valueIncludes(attr, 'preconnect')); + if (!isPreconnect) return; - const isPreconnect = node.attributes - .filter(isValuedHtmlAttribute) - .some((attr) => isAttr(attr, 'rel') && valueIncludes(attr, 'preconnect')); - if (!isPreconnect) return; + const isShopifyCdn = node.attributes + .filter(isValuedHtmlAttribute) + .some((attr) => isAttr(attr, 'href') && valueIncludes(attr, '.+cdn.shopify.com.+')); + if (!isShopifyCdn) return; - const isShopifyCdn = node.attributes - .filter(isValuedHtmlAttribute) - .some((attr) => isAttr(attr, 'href') && valueIncludes(attr, '.+cdn.shopify.com.+')); - if (!isShopifyCdn) return; + context.report({ + message: + 'Preconnecting to cdn.shopify.com is unnecessary and can lead to worse performance', + startIndex: node.position.start, + endIndex: node.position.end, + }); + } - context.report({ - message: - 'Preconnecting to cdn.shopify.com is unnecessary and can lead to worse performance', - startIndex: node.position.start, - endIndex: node.position.end, - }); + return { + async HtmlVoidElement(node) { + checkNode(node); + }, + // The ported parser emits `HtmlSelfClosingElement` for self-closed + // void tags such as ``, whereas the previous parser emitted + // `HtmlVoidElement` regardless of the trailing slash. Visit both so the + // check still fires on self-closing markup. + async HtmlSelfClosingElement(node) { + checkNode(node); }, }; }, diff --git a/packages/theme-check-common/src/checks/deprecate-lazysizes/index.ts b/packages/theme-check-common/src/checks/deprecate-lazysizes/index.ts index 783582d32..2986faa48 100644 --- a/packages/theme-check-common/src/checks/deprecate-lazysizes/index.ts +++ b/packages/theme-check-common/src/checks/deprecate-lazysizes/index.ts @@ -1,6 +1,8 @@ +import { HtmlSelfClosingElement, HtmlVoidElement } from '@shopify/liquid-html-parser'; import { Severity, SourceCodeType, LiquidCheckDefinition } from '../../types'; import { ValuedHtmlAttribute, + getHtmlNodeName, isAttr, isValuedHtmlAttribute, isHtmlAttribute, @@ -28,30 +30,41 @@ export const DeprecateLazysizes: LiquidCheckDefinition = { }, create(context) { + function checkNode(node: HtmlVoidElement | HtmlSelfClosingElement) { + if (getHtmlNodeName(node) !== 'img') return; + + const attributes = node.attributes.filter(isHtmlAttribute); + const hasSrc = attributes.some((attr) => isAttr(attr, 'src')); + const hasNativeLoading = attributes.some((attr) => isAttr(attr, 'loading')); + if (hasSrc && hasNativeLoading) return; + + const hasLazyloadClass = node.attributes + .filter(isValuedHtmlAttribute) + .some((attr) => isAttr(attr, 'class') && valueIncludes(attr, 'lazyload')); + if (!hasLazyloadClass) return; + + const hasLazysizesAttribute = node.attributes + .filter(isValuedHtmlAttribute) + .some(showsLazysizesUsage); + if (!hasLazysizesAttribute) return; + + context.report({ + message: 'Use the native loading="lazy" attribute instead of lazysizes', + startIndex: node.position.start, + endIndex: node.position.end, + }); + } + return { async HtmlVoidElement(node) { - if (node.name !== 'img') return; - - const attributes = node.attributes.filter(isHtmlAttribute); - const hasSrc = attributes.some((attr) => isAttr(attr, 'src')); - const hasNativeLoading = attributes.some((attr) => isAttr(attr, 'loading')); - if (hasSrc && hasNativeLoading) return; - - const hasLazyloadClass = node.attributes - .filter(isValuedHtmlAttribute) - .some((attr) => isAttr(attr, 'class') && valueIncludes(attr, 'lazyload')); - if (!hasLazyloadClass) return; - - const hasLazysizesAttribute = node.attributes - .filter(isValuedHtmlAttribute) - .some(showsLazysizesUsage); - if (!hasLazysizesAttribute) return; - - context.report({ - message: 'Use the native loading="lazy" attribute instead of lazysizes', - startIndex: node.position.start, - endIndex: node.position.end, - }); + checkNode(node); + }, + // The ported parser emits `HtmlSelfClosingElement` for self-closed + // void tags such as ``, whereas the previous parser emitted + // `HtmlVoidElement` regardless of the trailing slash. Visit both so the + // check still fires on self-closing markup. + async HtmlSelfClosingElement(node) { + checkNode(node); }, }; }, diff --git a/packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidBooleanExpression.spec.ts b/packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidBooleanExpression.spec.ts index 88af441fc..fc0afd32a 100644 --- a/packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidBooleanExpression.spec.ts +++ b/packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidBooleanExpression.spec.ts @@ -28,7 +28,7 @@ describe('detectTrailingAssignValue', async () => { [`{% assign foo = something == else %}`, '{% assign foo = something %}'], [`{% echo foo != bar %}`, '{% echo foo %}'], [`{{ this > that }}`, '{{ this }}'], - [`{{ bool and cond }}`, '{{ bool}}'], + [`{{ bool and cond }}`, '{{ bool }}'], ]; for (const [sourceCode, expected] of testCases) { diff --git a/packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidConditionalNode.ts b/packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidConditionalNode.ts index a1a62ac83..ed20aa5ef 100644 --- a/packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidConditionalNode.ts +++ b/packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidConditionalNode.ts @@ -70,6 +70,16 @@ function isOperatorToken(token: Token): boolean { function checkInvalidStartingToken(tokens: Token[]): ExpressionIssue | null { const firstToken = tokens[0]; + // `contains` is the only word-operator in the comparison pattern that is + // also a valid identifier (the symbolic operators == != >= <= > < never + // are). When it stands alone it is a bare variable named `contains`, not + // the comparison operator, so it must not be flagged as an invalid + // starting token. This mirrors the previous parser, which produced a + // structured VariableLookup here; the ported parser falls back to string + // markup instead. + if (tokens.length === 1 && firstToken.value === 'contains') { + return null; + } if (firstToken.type === 'invalid' || firstToken.type === 'comparison') { return { message: `Conditional cannot start with '${firstToken.value}'. Use a variable or value instead`, diff --git a/packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidFilterName.ts b/packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidFilterName.ts index ee98a086d..66458117c 100644 --- a/packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidFilterName.ts +++ b/packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidFilterName.ts @@ -1,4 +1,10 @@ -import { LiquidVariableOutput, LiquidTag, NodeTypes, NamedTags } from '@shopify/liquid-html-parser'; +import { + LiquidVariableOutput, + LiquidTag, + LiquidVariable, + NodeTypes, + NamedTags, +} from '@shopify/liquid-html-parser'; import { Problem, SourceCodeType, Context, FilterEntry } from '../../..'; import { INVALID_SYNTAX_MESSAGE } from './utils'; @@ -11,30 +17,90 @@ export async function detectInvalidFilterName( } if (node.type === NodeTypes.LiquidVariableOutput) { - if (typeof node.markup !== 'string') { - return []; + // When the markup parses cleanly the parser hands us a structured + // `LiquidVariable`; only a bail-out leaves the raw markup as a string. + // Both carry the same offenses, they just live in different places. + if (typeof node.markup === 'string') { + return detectInvalidFilterNameInMarkup(node, node.markup, filters); } - return detectInvalidFilterNameInMarkup(node, node.markup, filters); + return detectInvalidFilterNameInFilters(node, node.markup, filters); } if (node.type === NodeTypes.LiquidTag) { - if (node.name === NamedTags.echo && typeof node.markup !== 'string') { - return []; - } - if (node.name === NamedTags.assign && typeof node.markup !== 'string') { - return []; + if (node.name === NamedTags.echo) { + if (typeof node.markup === 'string') { + return detectInvalidFilterNameInMarkup(node, node.markup, filters); + } + if (node.markup.type === NodeTypes.LiquidVariable) { + return detectInvalidFilterNameInFilters(node, node.markup, filters); + } } - if ( - typeof node.markup === 'string' && - (node.name === NamedTags.echo || node.name === NamedTags.assign) - ) { - return detectInvalidFilterNameInMarkup(node, node.markup, filters); + + if (node.name === NamedTags.assign) { + if (typeof node.markup === 'string') { + return detectInvalidFilterNameInMarkup(node, node.markup, filters); + } + if (node.markup.type === NodeTypes.AssignMarkup) { + return detectInvalidFilterNameInFilters(node, node.markup.value, filters); + } } } return []; } +// When the tokenizer meets an invalid trailing character (`@`, `!`, `#`, ...) +// it silently drops the byte, so the filter parses cleanly and the markup ends +// up as a structured `LiquidVariable` — the raw-markup regex above never sees +// it. The dropped byte still lives in the document `source` right after the +// filter name, so we recover the offense from the source span instead. +async function detectInvalidFilterNameInFilters( + node: LiquidVariableOutput | LiquidTag, + variable: LiquidVariable, + filters: FilterEntry[], +): Promise[]> { + const knownFilters = filters; + const source = node.source; + const markupEnd = variable.position.end; + const problems: Problem[] = []; + + for (const filter of variable.filters) { + if (!knownFilters.some((known) => known.name === filter.name)) { + continue; + } + + // `filter.position.start` sits before this filter's pipe, so the first + // occurrence of the name from there is this filter's name. + const nameStartInSource = source.indexOf(filter.name, filter.position.start); + if (nameStartInSource === -1) { + continue; + } + + const trailingStartInSource = nameStartInSource + filter.name.length; + + // Capture the run of characters wedged between the filter name and its + // next valid boundary (whitespace, `:` before arguments, `|` before the + // next filter, or the end of the markup). Anything there is junk. + const trailing = source.slice(trailingStartInSource, markupEnd).match(/^([^\s:|]+)/)?.[1]; + if (!trailing) { + continue; + } + + const trailingEndInSource = trailingStartInSource + trailing.length; + + problems.push({ + message: `${INVALID_SYNTAX_MESSAGE} Filter '${filter.name}' has trailing characters '${trailing}' that should be removed.`, + startIndex: trailingStartInSource, + endIndex: trailingEndInSource, + fix: (corrector) => { + corrector.replace(trailingStartInSource, trailingEndInSource, ''); + }, + }); + } + + return problems; +} + async function detectInvalidFilterNameInMarkup( node: LiquidVariableOutput | LiquidTag, markup: string, diff --git a/packages/theme-check-common/src/checks/liquid-html-syntax-error/index.spec.ts b/packages/theme-check-common/src/checks/liquid-html-syntax-error/index.spec.ts index 566b273c8..0858f1b35 100644 --- a/packages/theme-check-common/src/checks/liquid-html-syntax-error/index.spec.ts +++ b/packages/theme-check-common/src/checks/liquid-html-syntax-error/index.spec.ts @@ -54,7 +54,7 @@ describe('Module: LiquidHTMLSyntaxError', () => { const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); expect(offenses).to.have.length(1); expect(offenses[0].message).to.equal( - `Attempting to close LiquidTag 'if' before HtmlElement 'a' was closed`, + `Attempting to close LiquidTag 'endif' before HtmlElement 'a' was closed`, ); }); @@ -65,7 +65,7 @@ describe('Module: LiquidHTMLSyntaxError', () => { const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); expect(offenses).to.have.length(1); - expect(offenses[0].message).to.equal(`SyntaxError: expected "%}"`); + expect(offenses[0].message).to.equal(`Expected LiquidTagClose but got EndOfInput`); }); it('should report unexpected tokens (2)', async () => { @@ -75,7 +75,7 @@ describe('Module: LiquidHTMLSyntaxError', () => { const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); expect(offenses).to.have.length(1); - expect(offenses[0].message).to.equal(`SyntaxError: expected ">", not """`); + expect(offenses[0].message).to.equal(`Expected HtmlTagClose but got EndOfInput`); }); it('should report unexpected tokens (3)', async () => { @@ -85,9 +85,7 @@ describe('Module: LiquidHTMLSyntaxError', () => { const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); expect(offenses).to.have.length(1); - expect(offenses[0].message).to.equal( - `SyntaxError: expected "#", a letter, "when", "sections", "section", "render", "liquid", "layout", "increment", "include", "elsif", "else", "echo", "decrement", "content_for", "cycle", "continue", "break", "assign", "tablerow", "unless", "if", "ifchanged", "for", "case", "capture", "paginate", "form", "end", "style", "stylesheet", "schema", "javascript", "raw", "comment", or "doc"`, - ); + expect(offenses[0].message).to.equal(`Expected LiquidTagClose but got EndOfInput`); }); it('should not report syntax error in valid Liquid code', async () => { @@ -114,16 +112,16 @@ describe('Module: LiquidHTMLSyntaxError', () => { source = `
`; offenses = await runLiquidCheck(LiquidHTMLSyntaxError, source); highlights = highlightedOffenses({ 'file.liquid': source }, offenses); - expect(highlights).to.include(''); + expect(highlights).to.include(''); source = ``; offenses = await runLiquidCheck(LiquidHTMLSyntaxError, source); highlights = highlightedOffenses({ 'file.liquid': source }, offenses); - expect(highlights).to.include('{% endif %}'); + expect(highlights).to.include(''); source = ``; offenses = await runLiquidCheck(LiquidHTMLSyntaxError, source); highlights = highlightedOffenses({ 'file.liquid': source }, offenses); - expect(highlights).to.include('"'); + expect(highlights).to.include('>'); }); }); diff --git a/packages/theme-check-common/src/checks/missing-template/index.ts b/packages/theme-check-common/src/checks/missing-template/index.ts index f1cda6a07..d2b3c4b67 100644 --- a/packages/theme-check-common/src/checks/missing-template/index.ts +++ b/packages/theme-check-common/src/checks/missing-template/index.ts @@ -58,7 +58,12 @@ export const MissingTemplate: LiquidCheckDefinition = { return { async RenderMarkup(node) { - if (node.snippet.type === NodeTypes.VariableLookup) return; + if ( + node.snippet.type === NodeTypes.VariableLookup || + node.snippet.type === NodeTypes.Range + ) { + return; + } const snippet = node.snippet; const relativePath = `snippets/${snippet.value}.liquid`; @@ -71,9 +76,14 @@ export const MissingTemplate: LiquidCheckDefinition = { if (node.name !== NamedTags.section) return; const markup = node.markup; - const relativePath = `sections/${markup.value}.liquid`; + // The ported parser wraps the section name in a `SectionMarkup` node + // whose name lives at `markup.name` (a String node); the previous + // parser exposed the name directly as `markup.value`. Read the name and + // report against the String node so the offense covers the quoted + // string rather than the whole markup. + const relativePath = `sections/${markup.name.value}.liquid`; - await maybeReportMissing(relativePath, markup); + await maybeReportMissing(relativePath, markup.name); }, }; }, diff --git a/packages/theme-check-common/src/checks/remote-asset/index.ts b/packages/theme-check-common/src/checks/remote-asset/index.ts index 31ff07da3..3baefd36d 100644 --- a/packages/theme-check-common/src/checks/remote-asset/index.ts +++ b/packages/theme-check-common/src/checks/remote-asset/index.ts @@ -1,5 +1,6 @@ import { HtmlRawNode, + HtmlSelfClosingElement, HtmlVoidElement, TextNode, LiquidVariable, @@ -14,7 +15,13 @@ import { LiquidHtmlNode, SchemaProp, } from '../../types'; -import { isAttr, isValuedHtmlAttribute, isNodeOfType, ValuedHtmlAttribute } from '../utils'; +import { + getHtmlNodeName, + isAttr, + isValuedHtmlAttribute, + isNodeOfType, + ValuedHtmlAttribute, +} from '../utils'; import { last } from '../../utils'; const RESOURCE_TAGS = ['img', 'link', 'source', 'script']; @@ -51,6 +58,10 @@ function isDataUri(url: string): boolean { return /^data:/i.test(url); } +function cleanUrlText(url: string): string { + return url.replace(/[“”‘’]/g, ''); +} + /** * Checks if the attribute value starts with a variable lookup. * When a value starts with a VariableLookup (e.g., {{ source.url }}, {{ image }}), @@ -89,8 +100,8 @@ function valueIsDefinitelyNotShopifyHosted( allowedDomains: string[] = [], ): boolean { return attr.value.some((node) => { - if (node.type === NodeTypes.TextNode && /^(https?:)?\/\//.test(node.value)) { - if (!isUrlHostedbyShopify(node.value, allowedDomains)) { + if (node.type === NodeTypes.TextNode && /^(https?:)?\/\//.test(cleanUrlText(node.value))) { + if (!isUrlHostedbyShopify(cleanUrlText(node.value), allowedDomains)) { return true; } } @@ -175,8 +186,9 @@ export const RemoteAsset: LiquidCheckDefinition = { create(context) { const allowedDomains = normaliseAllowedDomains(context.settings.allowedDomains || []); - function checkHtmlNode(node: HtmlVoidElement | HtmlRawNode) { - if (!RESOURCE_TAGS.includes(node.name)) return; + function checkHtmlNode(node: HtmlVoidElement | HtmlSelfClosingElement | HtmlRawNode) { + const nodeName = getHtmlNodeName(node); + if (!nodeName || !RESOURCE_TAGS.includes(nodeName)) return; const urlAttribute: ValuedHtmlAttribute | undefined = node.attributes .filter(isValuedHtmlAttribute) @@ -187,14 +199,14 @@ export const RemoteAsset: LiquidCheckDefinition = { const firstTextNode = urlAttribute.value.find( (node): node is TextNode => node.type === NodeTypes.TextNode, ); - if (firstTextNode && isHashUrl(firstTextNode.value)) return; - if (firstTextNode && isDataUri(firstTextNode.value)) return; + if (firstTextNode && isHashUrl(cleanUrlText(firstTextNode.value))) return; + if (firstTextNode && isDataUri(cleanUrlText(firstTextNode.value))) return; if (startsWithVariableLookup(urlAttribute)) return; const isShopifyUrl = urlAttribute.value .filter((node): node is TextNode => node.type === NodeTypes.TextNode) - .some((textNode) => isUrlHostedbyShopify(textNode.value, allowedDomains)); + .some((textNode) => isUrlHostedbyShopify(cleanUrlText(textNode.value), allowedDomains)); if (isShopifyUrl) return; @@ -265,6 +277,13 @@ export const RemoteAsset: LiquidCheckDefinition = { async HtmlVoidElement(node) { checkHtmlNode(node); }, + // The ported parser emits `HtmlSelfClosingElement` for self-closed + // void tags such as `` and ``, whereas the previous + // parser emitted `HtmlVoidElement` regardless of the trailing slash. + // Visit both so the check still fires on self-closing markup. + async HtmlSelfClosingElement(node) { + checkHtmlNode(node); + }, async HtmlRawNode(node) { checkHtmlNode(node); }, diff --git a/packages/theme-check-common/src/checks/unrecognized-render-snippet-arguments/index.ts b/packages/theme-check-common/src/checks/unrecognized-render-snippet-arguments/index.ts index e7588831d..16eef77e5 100644 --- a/packages/theme-check-common/src/checks/unrecognized-render-snippet-arguments/index.ts +++ b/packages/theme-check-common/src/checks/unrecognized-render-snippet-arguments/index.ts @@ -34,7 +34,7 @@ export const UnrecognizedRenderSnippetArguments: LiquidCheckDefinition = { const variable = node.variable; if (alias && !liquidDocParameters.has(alias.value) && variable) { - const startIndex = variable.position.start + 1; + const startIndex = variable.position.start; context.report({ message: `Unknown argument '${alias.value}' in render tag for snippet '${snippetName}'.`, @@ -45,7 +45,7 @@ export const UnrecognizedRenderSnippetArguments: LiquidCheckDefinition = { message: `Remove '${alias.value}'`, fix: (fixer: any) => { if (variable) { - return fixer.remove(variable.position.start, alias.position.end); + return fixer.remove(variable.position.start - 1, alias.position.end); } }, }, diff --git a/packages/theme-check-common/src/checks/utils.ts b/packages/theme-check-common/src/checks/utils.ts index 73a89500c..bb7a483c1 100644 --- a/packages/theme-check-common/src/checks/utils.ts +++ b/packages/theme-check-common/src/checks/utils.ts @@ -2,6 +2,9 @@ import { Position, NodeTypes, HtmlElement, + HtmlSelfClosingElement, + HtmlVoidElement, + HtmlRawNode, TextNode, AttrEmpty, AttrSingleQuoted, @@ -31,6 +34,25 @@ export function isLiquidBranch(node: LiquidHtmlNode): node is LiquidBranch { return isNodeOfType(NodeTypes.LiquidBranch, node); } +/** + * Returns the static tag name of an HTML node as a string. + * + * Void and raw nodes (``, `