From 0bdba300a171df61406c7acfafe21c1c783fac22 Mon Sep 17 00:00:00 2001 From: Maciej Krajowski-Kukiel Date: Mon, 20 Apr 2026 16:43:27 +0200 Subject: [PATCH] further improvements to syntax --- .../checks/InvalidAssignSyntax.spec.ts | 49 +++++++++++++++++++ .../checks/InvalidAssignSyntax.ts | 38 +++++++++++++- .../checks/liquid-html-syntax-error/index.ts | 13 ++++- .../src/checks/unused-assign/index.spec.ts | 48 ++++++++++++++++++ .../src/checks/unused-assign/index.ts | 15 ++++++ 5 files changed, 160 insertions(+), 3 deletions(-) diff --git a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidAssignSyntax.spec.ts b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidAssignSyntax.spec.ts index 9ba876d..959bae9 100644 --- a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidAssignSyntax.spec.ts +++ b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidAssignSyntax.spec.ts @@ -92,6 +92,55 @@ describe('detectInvalidAssignSyntax', () => { }); }); + describe('trailing garbage after RHS / filters (fallback)', () => { + // These cases have a valid `target = value` skeleton and a valid filter chain, + // but extra non-parseable characters trail the filters. The tolerant parser + // swallows the entire body as string markup, and none of the other sub-checks + // (MultipleAssignValues, InvalidFilterName, InvalidPipeSyntax) detects the + // problem — so the fallback re-parses in strict mode and surfaces it. + const fallbackCases: Array<[string, string]> = [ + [ + 'stray `}` after filter array argument', + `{% assign name = arr | default: [ "hi", k, v] } %}`, + ], + ['trailing bare word after filter arg', `{% assign x = y | default: "z" trailing %}`], + ]; + + for (const [label, sourceCode] of fallbackCases) { + it(`should report: ${label} — ${sourceCode}`, async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => + o.message.includes("Invalid syntax for tag 'assign'"), + ); + expect(syntaxOffenses).toHaveLength(1); + }); + } + + // These cases are already flagged by other sub-checks with different messages; + // the fallback must NOT also fire on them (no double-reporting). + const alreadyCoveredCases: Array<[string, string]> = [ + [ + 'stray `}` after unary filter (InvalidFilterName catches it)', + `{% assign x = y | upcase } %}`, + ], + [ + 'stray `}` after RHS with no filter (MultipleAssignValues catches it)', + `{% assign x = "v" } %}`, + ], + ]; + + for (const [label, sourceCode] of alreadyCoveredCases) { + it(`should NOT double-report: ${label} — ${sourceCode}`, async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses.length).toBeGreaterThan(0); + const fallbackOffenses = offenses.filter((o) => + o.message.includes("Invalid syntax for tag 'assign'"), + ); + expect(fallbackOffenses).toHaveLength(0); + }); + } + }); + describe('valid syntax — should NOT report', () => { const validCases: Array<[string, string]> = [ // primitives diff --git a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidAssignSyntax.ts b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidAssignSyntax.ts index e353d4c..c5af67d 100644 --- a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidAssignSyntax.ts +++ b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidAssignSyntax.ts @@ -1,6 +1,8 @@ -import { LiquidTag } from '@platformos/liquid-html-parser'; +import { LiquidTag, toLiquidAST } from '@platformos/liquid-html-parser'; import { Problem, SourceCodeType } from '../../..'; +const INVALID_ASSIGN_MESSAGE = `Invalid syntax for tag 'assign'. Expected syntax: {% assign = %}`; + /** * Detects structurally-invalid `assign` tags that neither `MultipleAssignValues` nor * `InvalidPipeSyntax`/`InvalidFilterName` catch: @@ -38,12 +40,44 @@ export function detectInvalidAssignSyntax( if (!isStructurallyBroken) return; return { - message: `Invalid syntax for tag 'assign'. Expected syntax: {% assign = %}`, + message: INVALID_ASSIGN_MESSAGE, startIndex: node.position.start, endIndex: node.position.end, }; } +/** + * Fallback for assign tags where the tolerant parser landed in string markup even + * though the `target = value` skeleton looks fine — meaning the value or filter + * chain has parse-breaking characters (e.g. a stray `}` before `%}`) that no other + * dedicated sub-check (MultipleAssignValues, InvalidFilterName, InvalidPipeSyntax) + * surfaced. Re-parses the tag source in strict mode and reports on failure. + * + * Must run ONLY when no other sub-check already reported on this tag, otherwise + * it double-flags the same problem. The orchestrator enforces that gate. + */ +export function detectInvalidAssignFallback( + node: LiquidTag, +): Problem | undefined { + if (node.name !== 'assign' || typeof node.markup !== 'string') return; + + // Digit-starting targets (e.g. `23_hours_ago`) are accepted by the platformOS + // runtime but rejected by the Ohm grammar's `variableSegment` rule. Skipping + // them here mirrors the intentional tolerance in isValidAssignTarget above. + if (/^\s*\d/.test(node.markup)) return; + + const tagSource = node.source.slice(node.position.start, node.position.end); + try { + toLiquidAST(tagSource, { mode: 'strict', allowUnclosedDocumentNode: true }); + } catch { + return { + message: INVALID_ASSIGN_MESSAGE, + startIndex: node.position.start, + endIndex: node.position.end, + }; + } +} + /** * Rejects an LHS that is obviously not an assign target. * diff --git a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/index.ts b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/index.ts index 7eb27d1..7509567 100644 --- a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/index.ts +++ b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/index.ts @@ -1,7 +1,10 @@ import { Severity, SourceCodeType, LiquidCheckDefinition, Problem } from '../../types'; import { getOffset, isError } from '../../utils'; import { detectMultipleAssignValues } from './checks/MultipleAssignValues'; -import { detectInvalidAssignSyntax } from './checks/InvalidAssignSyntax'; +import { + detectInvalidAssignSyntax, + detectInvalidAssignFallback, +} from './checks/InvalidAssignSyntax'; import { detectInvalidBooleanExpressions } from './checks/InvalidBooleanExpressions'; import { detectInvalidEchoValue } from './checks/InvalidEchoValue'; import { detectInvalidConditionalNode } from './checks/InvalidConditionalNode'; @@ -123,6 +126,14 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = { if (pipeProblems.length > 0) { pipeProblems.forEach((pipeProblem) => context.report(pipeProblem)); } + + // Last-chance check for assign tags whose tolerant parse fell back to + // string markup (e.g. stray `}` before `%}`). Gated on "nothing else + // reported on this tag" to avoid double-flagging. + if (problems.length + filterProblems.length + pipeProblems.length === 0) { + const fallback = detectInvalidAssignFallback(node); + if (fallback) context.report(fallback); + } }, async LiquidBranch(node, ancestors) { diff --git a/packages/platformos-check-common/src/checks/unused-assign/index.spec.ts b/packages/platformos-check-common/src/checks/unused-assign/index.spec.ts index 11ddcad..11dbcf9 100644 --- a/packages/platformos-check-common/src/checks/unused-assign/index.spec.ts +++ b/packages/platformos-check-common/src/checks/unused-assign/index.spec.ts @@ -201,4 +201,52 @@ describe('Module: UnusedAssign', () => { expect(offenses).to.have.lengthOf(1); expect(offenses[0].message).to.equal("The variable 'arr' is assigned but not used"); }); + + it('should not report variables used inside a filter array-literal argument', async () => { + const sourceCode = ` + {% assign k = "key" %} + {% assign v = "value" %} + {% assign name = arr | default: [ "hi", k, v] %} + {{ name }} + `; + + const offenses = await runLiquidCheck(UnusedAssign, sourceCode); + expect(offenses).to.have.lengthOf(0); + }); + + it('should not report variables used inside a filter hash-literal argument', async () => { + const sourceCode = ` + {% assign k = 1 %} + {% assign name = arr | default: { a: k } %} + {{ name }} + `; + + const offenses = await runLiquidCheck(UnusedAssign, sourceCode); + expect(offenses).to.have.lengthOf(0); + }); + + it('should not report variables used inside a named-argument value', async () => { + const sourceCode = ` + {% assign k = 0 %} + {% assign name = arr | slice: start: k %} + {{ name }} + `; + + const offenses = await runLiquidCheck(UnusedAssign, sourceCode); + expect(offenses).to.have.lengthOf(0); + }); + + it('should not report variables referenced in a string-markup fallback assign (parse failure)', async () => { + // The stray `}` before `%}` causes the tolerant parser to fall back to + // string markup. The AST contains no VariableLookup nodes for k/v, so + // without a textual fallback scan the check would wrongly flag them. + const sourceCode = ` + {% assign k = "key" %} + {% assign v = "value" %} + {% assign name = arr | default: [ "hi", k, v] } %} + `; + + const offenses = await runLiquidCheck(UnusedAssign, sourceCode); + expect(offenses).to.have.lengthOf(0); + }); }); diff --git a/packages/platformos-check-common/src/checks/unused-assign/index.ts b/packages/platformos-check-common/src/checks/unused-assign/index.ts index af0ddeb..46373d6 100644 --- a/packages/platformos-check-common/src/checks/unused-assign/index.ts +++ b/packages/platformos-check-common/src/checks/unused-assign/index.ts @@ -31,6 +31,12 @@ export const UnusedAssign: LiquidCheckDefinition = { // effects on the original, so they count as "using" the variable. const referenceAssignedVariables: Set = new Set(); const usedVariables: Set = new Set(); + // Concatenated markup of LiquidTags whose parsing fell back to a raw string + // (e.g. a typo like `{% assign name = x | default: [...] } %}`). The AST has + // no VariableLookup nodes to traverse inside them, so onCodePathEnd scans + // this text for assigned-variable names — avoids compounding the user's + // syntax error with false "unused" warnings. + let fallbackText = ''; function checkVariableUsage(node: any) { if (node.type === NodeTypes.VariableLookup) { @@ -66,6 +72,8 @@ export const UnusedAssign: LiquidCheckDefinition = { } } else if (isLiquidTagCapture(node) && node.markup.name) { assignedVariables.set(node.markup.name, node); + } else if (typeof node.markup === 'string' && node.markup) { + fallbackText += '\n' + node.markup; } }, @@ -79,6 +87,13 @@ export const UnusedAssign: LiquidCheckDefinition = { }, async onCodePathEnd() { + if (fallbackText) { + for (const name of assignedVariables.keys()) { + if (usedVariables.has(name)) continue; + if (new RegExp(`\\b${name}\\b`).test(fallbackText)) usedVariables.add(name); + } + } + for (const [variable, node] of assignedVariables.entries()) { if (!usedVariables.has(variable) && !variable.startsWith('_')) { context.report({