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/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/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 index 465c1452e..f942cd61f 100644 --- a/packages/liquid-html-parser/src/document/tolerant-parser.ts +++ b/packages/liquid-html-parser/src/document/tolerant-parser.ts @@ -62,6 +62,14 @@ export function makeLiquidErrorNode( * 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 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/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 8bd707721..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 { 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 (``, `