From dee787b4a4fc1b07ca6a24b35f2d0ce3c9864378 Mon Sep 17 00:00:00 2001 From: Maciej Krajowski-Kukiel Date: Mon, 20 Apr 2026 11:55:59 +0200 Subject: [PATCH] recognize syntax errors for assign and function properly --- UPSTREAM-PROPOSALS.md | 791 ----------------- .../grammar/liquid-html.ohm | 72 +- .../src/stage-1-cst.spec.ts | 814 +++++++++++++++++- .../liquid-html-parser/src/stage-1-cst.ts | 41 +- .../src/stage-2-ast.spec.ts | 86 +- .../liquid-html-parser/src/stage-2-ast.ts | 30 +- packages/liquid-html-parser/src/types.ts | 1 - .../checks/InvalidAssignSyntax.spec.ts | 422 +++++++++ .../checks/InvalidAssignSyntax.ts | 63 ++ .../checks/InvalidOutputPush.spec.ts | 104 +++ .../checks/InvalidOutputPush.ts | 39 + .../checks/InvalidTagSyntax.spec.ts | 88 +- .../checks/InvalidTagSyntax.ts | 2 +- .../checks/liquid-html-syntax-error/index.ts | 19 + .../src/checks/undefined-object/index.spec.ts | 101 +++ .../src/TypeSystem.spec.ts | 4 +- .../src/TypeSystem.ts | 14 +- .../params/LiquidCompletionParams.ts | 3 +- .../preprocess/augment-with-css-properties.ts | 2 - .../src/printer/print/liquid.ts | 6 +- .../src/printer/printer-liquid-html.ts | 8 - 21 files changed, 1785 insertions(+), 925 deletions(-) delete mode 100644 UPSTREAM-PROPOSALS.md create mode 100644 packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidAssignSyntax.spec.ts create mode 100644 packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidAssignSyntax.ts create mode 100644 packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidOutputPush.spec.ts create mode 100644 packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidOutputPush.ts diff --git a/UPSTREAM-PROPOSALS.md b/UPSTREAM-PROPOSALS.md deleted file mode 100644 index 13b637b3..00000000 --- a/UPSTREAM-PROPOSALS.md +++ /dev/null @@ -1,791 +0,0 @@ -# Upstream Feature Proposals — pos-cli check and pos-cli LSP - -Features identified during plugin development that belong at the platform tooling layer -rather than in the agent plugin. Each proposal includes rationale for placement, full -implementation detail, and expected impact. - -**Why layer placement matters:** features that detect code quality problems should live in -`pos-cli check` or the LSP so they fire universally — in CI, in editors, in the agent plugin -via auto-diagnostics — without requiring the plugin to re-implement analysis logic. The plugin -gets them for free via `pos-cli check` output. Features that exist only in the plugin are -invisible outside agent sessions. - ---- - -## pos-cli check proposals - -### 1. N+1 GraphQL query detection - -**Check code:** `NestedGraphQLQuery` (or `GraphQLInLoop`) -**Severity:** WARNING -**Type:** `SourceCodeType.LiquidHtml` - -#### The problem - -A `{% graphql %}` tag inside a `{% for %}` or `{% tablerow %}` loop executes one database -request per loop iteration. With 100 records in the outer loop, that is 100 sequential -GraphQL requests instead of one batch query — a catastrophic performance footprint that is -completely invisible at the template level and produces no error at runtime. The page simply -loads slowly (or times out under load). - -This is the single most damaging performance pattern in platformOS templates and it is not -currently detected by any tool. - -#### Why it belongs in pos-cli check - -This is a pure static analysis problem: detect a graphql tag whose AST ancestor chain -contains a `for` or `tablerow` tag. It requires no runtime information, no network calls, -and no session state. It should fire in CI, in the editor diagnostics panel, and during -`pos-cli check` runs — not only when an agent happens to be active. Putting it in the -plugin means it only fires in agent sessions and is invisible everywhere else. - -#### Implementation - -The check uses entry and exit visitors to maintain a for-loop nesting depth counter. When -a `graphql` tag is encountered at any nesting depth > 0, an offense is reported. - -```js -import { SourceCodeType, Severity } from '@platformos/platformos-check-common'; -import { NamedTags, NodeTypes } from '@platformos/liquid-html-parser'; - -export const NestedGraphQLQuery = { - meta: { - code: 'NestedGraphQLQuery', - name: 'GraphQL query inside a loop', - docs: { - description: 'A {% graphql %} tag inside a {% for %} loop executes one ' + - 'database request per iteration (N+1 pattern). Move the query before ' + - 'the loop and pass results as a variable.', - recommended: true, - url: 'https://documentation.platformos.com/best-practices/performance/graphql-in-loops', - }, - type: SourceCodeType.LiquidHtml, - severity: Severity.WARNING, - schema: {}, - targets: [], - }, - create(context) { - const loopStack = []; // tracks open for/tablerow nodes - - return { - async LiquidTag(node) { - if (node.name === NamedTags.for || node.name === NamedTags.tablerow) { - loopStack.push(node.name); - return; - } - - if (node.name !== NamedTags.graphql) return; - if (loopStack.length === 0) return; - - const outerLoop = loopStack[loopStack.length - 1]; - const markup = node.markup; - const resultVar = markup.type === NodeTypes.GraphQLMarkup - ? markup.name // the 'result' in {% graphql result = 'path' %} - : null; - - context.report({ - message: - `N+1 pattern: {% graphql ${resultVar ? resultVar + ' = ' : ''}... %} ` + - `is inside a {% ${outerLoop} %} loop. ` + - `This executes one database request per iteration. ` + - `Move the query before the loop and pass data as a variable.`, - startIndex: node.position.start, - endIndex: node.position.end, - }); - }, - - async 'LiquidTag:exit'(node) { - if (node.name === NamedTags.for || node.name === NamedTags.tablerow) { - loopStack.pop(); - } - }, - }; - }, -}; -``` - -#### Edge cases to handle - -- **Nested loops:** the `loopStack` correctly handles `{% for %}` inside `{% for %}` — - depth > 1 is even more dangerous (exponential requests) and should still warn. -- **Dynamic graphql (inline):** `GraphQLInlineMarkup` (inline queries without a file - reference) should also be detected — the markup type check handles both. -- **`background` tag:** a `{% background %}` tag inside a loop is less harmful (async) - but still worth a separate INFO-level note. Could be a separate check or a branch here. -- **`cache` wrapping graphql:** if the `graphql` tag is inside both a `{% for %}` and a - `{% cache %}` block, the severity could be downgraded to INFO since caching mitigates - the repeated requests. Requires checking ancestors for `NamedTags.cache`. - -#### Expected output - -``` -WARNING NestedGraphQLQuery app/views/pages/products.liquid:14 - N+1 pattern: {% graphql result = 'products/related' %} is inside a {% for %} - loop. This executes one database request per iteration. Move the query before - the loop and pass data as a variable. -``` - ---- - -### 2. Render parameter validation (`@param` contracts) - -**Check code:** `UndeclaredRenderParameter` + `MissingRequiredParameter` -**Severity:** WARNING (unknown param) / ERROR (missing required param) -**Type:** `SourceCodeType.LiquidHtml` - -#### The problem - -LiquidDoc supports `@param` annotations that declare a partial's expected inputs: - -```liquid -{% doc %} - @param {string} title - The card heading (required) - @param {string} subtitle - Secondary text (optional) - @param {object} cta - Call-to-action object with url and label -{% enddoc %} -``` - -Currently there is no tool that validates whether a `{% render %}` call actually provides -the required parameters. A caller can omit `title` entirely and get a silent blank value -at runtime. A caller can pass `ttle: "typo"` and the typo is silently ignored. These -are two of the most common causes of "blank partial" bugs in platformOS templates. - -#### Why it belongs in pos-cli check - -This is cross-file static analysis: read the callee's declaration (`@param` nodes in the -partial), inspect the caller's argument list (the `{% render %}` tag's named arguments), -and compare. The check engine already provides `context.fs.readFile()` for reading -dependency files. This pattern is identical to how `MissingTemplate` works — it reads -the referenced file to verify it exists. Here we go one step further and read it to -verify the interface is respected. - -As a check offense it fires in CI (preventing broken renders from reaching staging), -in the editor (inline annotations on the render tag), and in agent sessions without -any plugin-level logic. - -#### Implementation - -```js -import { SourceCodeType, Severity } from '@platformos/platformos-check-common'; -import { NamedTags, NodeTypes, toLiquidHtmlAST, walk } from '@platformos/liquid-html-parser'; -import path from 'node:path'; - -function resolvePartialPath(fileUri, partialName) { - // partialName: "sections/hero" - // resolved: /app/views/partials/sections/hero.liquid - const projectRoot = fileUri.replace(/\/app\/.*$/, ''); - return path.join(projectRoot, 'app', 'views', 'partials', partialName + '.liquid'); -} - -function extractParams(ast) { - // Returns { name, required, type, description }[] - // @param without a default or "optional" marker → required: true - const params = []; - walk(ast, (node) => { - if (node.type === NodeTypes.LiquidDocParamNode) { - params.push({ - name: node.name, - type: node.paramType?.value ?? 'any', - description: node.description?.value ?? '', - // Convention: params without "(optional)" in description are required. - // This matches the emerging LiquidDoc convention — adjust per pos-cli's - // own @param semantics when they are formalised. - required: !node.description?.value?.toLowerCase().includes('optional'), - }); - } - }); - return params; -} - -export const RenderParameterValidation = { - meta: { - code: 'RenderParameterValidation', - name: 'Render call violates @param contract', - docs: { - description: 'Validates that {% render %} calls provide all required ' + - '@param arguments declared by the target partial and do not pass ' + - 'undeclared arguments.', - recommended: true, - }, - type: SourceCodeType.LiquidHtml, - severity: Severity.WARNING, - schema: {}, - targets: [], - }, - create(context) { - return { - async LiquidTag(node) { - if (node.name !== NamedTags.render) return; - - const markup = node.markup; - - // Skip dynamic partials: {% render variable %} — can't resolve statically - if (!markup.partial || markup.partial.type !== NodeTypes.String) return; - - const partialName = markup.partial.value; - const partialPath = resolvePartialPath(context.file.uri, partialName); - - if (!await context.fileExists(partialPath)) return; // MissingTemplate handles this - - const partialSource = await context.fs.readFile(partialPath); - let partialAST; - try { - partialAST = toLiquidHtmlAST(partialSource, { mode: 'tolerant' }); - } catch { - return; // malformed partial — other checks will catch it - } - - const declaredParams = extractParams(partialAST); - - // If the partial declares no @param at all, skip validation. - // Unannotated partials have an implicit "accept anything" interface. - if (declaredParams.length === 0) return; - - const providedArgs = new Map( - (markup.args ?? []).map(arg => [arg.name, arg]) - ); - const declaredNames = new Set(declaredParams.map(p => p.name)); - - // Check 1: missing required params - for (const param of declaredParams) { - if (param.required && !providedArgs.has(param.name)) { - context.report({ - message: - `Missing required @param '${param.name}' ` + - `(${param.type}) for partial '${partialName}'. ` + - (param.description ? `Description: ${param.description}` : ''), - startIndex: node.position.start, - endIndex: node.position.end, - }); - } - } - - // Check 2: unknown params (caller passes something the partial doesn't declare) - for (const [argName, argNode] of providedArgs) { - if (!declaredNames.has(argName)) { - context.report({ - message: - `Unknown parameter '${argName}' passed to '${partialName}'. ` + - `Declared @params: ${[...declaredNames].join(', ')}. ` + - `Either add @param ${argName} to the partial's LiquidDoc or remove this argument.`, - startIndex: argNode.position?.start ?? node.position.start, - endIndex: argNode.position?.end ?? node.position.end, - }); - } - } - }, - }; - }, -}; -``` - -#### Adoption path - -For this check to be useful, the codebase needs to have `@param` annotations. It cannot -penalise render calls to unannotated partials (hence the early return when `declaredParams.length === 0`). The check is opt-in per partial: annotate a partial's interface -and callers are immediately validated. This creates a natural incremental adoption path — -annotate the most-called partials first. - -A companion check `UnannotatedPartial` (INFO severity) could flag partials with no LiquidDoc -block at all, encouraging annotation coverage over time. - ---- - -### 3. Dead translation keys - -**Check code:** `UnusedTranslationKey` -**Severity:** INFO (or WARNING — configurable) -**Type:** cross-file, runs at `onCodePathEnd` - -#### The problem - -`TranslationKeyExists` already catches keys that are *used but not defined*. The inverse -problem — keys that are *defined but never used* — goes completely undetected. Translation -files accumulate dead keys every time a template is renamed, a UI element is removed, or -a feature is sunset. These dead keys: - -- Bloat translation files, making them harder to maintain -- Create confusion when translators work on entries that are never displayed -- Make it impossible to know which translations actually need to be kept in sync - across languages - -#### Why it belongs in pos-cli check - -This requires building two sets across the entire project: -1. All translation keys *used* in templates (`{{ 'key' | t }}` expressions) -2. All translation keys *defined* in `.yml` files - -The difference (defined ∖ used) is the dead set. The check engine's cross-file lifecycle -(`onCodePathStart` / `onCodePathEnd`) is exactly designed for this pattern. The existing -`TranslationKeyExists` check already builds set 1; the machinery for set 2 would require -reading YAML files via `context.fs`. - -#### Implementation sketch - -```js -export const UnusedTranslationKey = { - meta: { - code: 'UnusedTranslationKey', - name: 'Translation key defined but never used', - type: SourceCodeType.LiquidHtml, // processes liquid files to build usage set - severity: Severity.INFO, - schema: { - translationFiles: { - type: 'string', - default: 'app/translations/*.yml', - description: 'Glob for translation files to analyse', - }, - }, - targets: [], - }, - create(context) { - const usedKeys = new Set(); - - return { - async LiquidFilter(node) { - // Detect: {{ 'some.key' | t }} — the filter named 't' applied to a string - if (node.name !== 't') return; - const variable = node.parent; // the LiquidVariable containing this filter - if (variable?.expression?.type === NodeTypes.String) { - usedKeys.add(variable.expression.value); - } - }, - - async onCodePathEnd() { - // After all .liquid files are processed, load and flatten all .yml files - const projectRoot = context.file.uri.replace(/\/app\/.*$/, ''); - const translationDir = path.join(projectRoot, 'app', 'translations'); - - const ymlFiles = await context.fs.readDirectory(translationDir); - for (const ymlFile of ymlFiles.filter(f => f.endsWith('.yml'))) { - const raw = await context.fs.readFile(ymlFile); - const parsed = yaml.parse(raw); // js-yaml - const definedKeys = flattenYamlKeys(parsed); // { key, value, line }[] - - for (const { key, line } of definedKeys) { - if (!usedKeys.has(key)) { - context.report({ - message: `Translation key '${key}' is defined but never used in any template.`, - uri: ymlFile, - startIndex: line, // approximate — YAML parser gives line, not index - endIndex: line, - }); - } - } - } - }, - }; - }, -}; - -function flattenYamlKeys(obj, prefix = '') { - const keys = []; - for (const [k, v] of Object.entries(obj)) { - const full = prefix ? `${prefix}.${k}` : k; - if (typeof v === 'object' && v !== null) { - keys.push(...flattenYamlKeys(v, full)); - } else { - keys.push({ key: full, value: v }); - } - } - return keys; -} -``` - -#### Caveats - -- **Dynamic keys:** `{{ some_variable | t }}` — the key is not a string literal and - cannot be statically resolved. The check must conservatively treat all non-literal - `| t` usages as "might use any key" and exclude them from the dead-key analysis - (or emit an INFO note that dynamic key usage prevents complete analysis). -- **Interpolated keys:** `{{ 'user.greeting' | t: name: current_user.name }}` — - the key IS a literal and can be tracked normally. -- **Multi-language files:** the check should union keys across all language files - (`en.yml`, `pl.yml`, etc.) — a key defined only in one language is not dead, - just untranslated. - ---- - -### 4. `TranslationKeyExists` — nearest-key suggestion - -**Modification to:** existing `TranslationKeyExists` check -**Change:** add `suggest[]` entries with closest matching keys - -#### The problem - -When `TranslationKeyExists` fires today, the offense message is: - -``` -Translation key 'app.hero.titel' does not exist. -``` - -The developer (or agent) then has to manually open the translation file, search for -similar keys, and figure out the correct spelling. In the case above the key is obviously -`app.hero.title` — a one-character typo. The check already has all the information needed -to surface this suggestion. - -#### Why it belongs in pos-cli check - -The `Offense` type already has a `suggest[]` field specifically for this purpose. A -suggestion is additional information attached to an offense that editors and tools can -surface inline. This is zero-cost to add — the check already reads all translation keys -to verify existence; finding the nearest match is just a few more lines. - -#### Implementation - -```js -// Levenshtein distance — simple O(nm) implementation, keys are short strings -function levenshtein(a, b) { - const dp = Array.from({ length: a.length + 1 }, (_, i) => - Array.from({ length: b.length + 1 }, (_, j) => (i === 0 ? j : j === 0 ? i : 0)) - ); - for (let i = 1; i <= a.length; i++) { - for (let j = 1; j <= b.length; j++) { - dp[i][j] = a[i-1] === b[j-1] - ? dp[i-1][j-1] - : 1 + Math.min(dp[i-1][j], dp[i][j-1], dp[i-1][j-1]); - } - } - return dp[a.length][b.length]; -} - -function findNearestKeys(missingKey, allKeys, maxDistance = 3, maxResults = 3) { - return allKeys - .map(key => ({ key, distance: levenshtein(missingKey, key) })) - .filter(({ distance }) => distance <= maxDistance) - .sort((a, b) => a.distance - b.distance) - .slice(0, maxResults) - .map(({ key }) => key); -} - -// In the existing TranslationKeyExists check, replace the bare context.report() with: -const nearest = findNearestKeys(missingKey, [...allDefinedKeys]); - -context.report({ - message: `Translation key '${missingKey}' does not exist.`, - startIndex: node.position.start, - endIndex: node.position.end, - suggest: nearest.map(key => ({ - message: `Did you mean '${key}'? (value: "${allDefinedKeys.get(key)}")`, - fix: { - startIndex: node.position.start, - endIndex: node.position.end, - newText: `'${key}'`, - }, - })), -}); -``` - -The `fix` on each suggestion means editors and `pos-cli check --fix` can apply the -correction automatically when the user picks a suggestion. - -#### Segment-based fallback - -For keys that have no close Levenshtein match (completely wrong key, not a typo), a -segment-based search is useful: split the missing key by `.` and find defined keys that -share at least one segment. For example, `app.header.titre` has no close match by edit -distance but shares `app.header` with several defined keys. This fallback catches -namespace errors vs typo errors. - ---- - -### 5. `HardcodedRoutes` — autofix - -**Modification to:** existing `HardcodedRoutes` check -**Change:** add `fix` to offenses so `pos-cli check --fix` can correct them - -#### The problem - -`HardcodedRoutes` fires when a literal path like `/products` or `/` appears in a template -context where `{{ routes.products_url }}` (or `{{ '/' | route_url }}`) should be used. -This is already detected. What is missing is an automatic fix. - -The check already knows: -- The offending string (e.g. `"/products"` or just `"/"`) -- Its exact position in the source -- The platformOS routes object keys (via `context.platformosDocset`) - -Everything needed to construct and apply the replacement is already present. - -#### Why it belongs in pos-cli check - -The `Offense.fix` field + `pos-cli check --fix` is the standard platformOS mechanism -for auto-correctable offenses. Adding autofix here means: -- Editors with pos-cli LSP integration can offer a one-click fix -- CI can run `pos-cli check --fix` to auto-correct before failing the build -- `pos-cli check --fix` in a batch migration can update an entire legacy codebase - -Implementing "suggest the route_url replacement" in the plugin is a workaround for a -missing feature in the tool that owns the detection. - -#### Implementation sketch - -```js -// In the existing HardcodedRoutes check, determine the fix based on the offense type: - -// Case 1: literal href="/products" — the path matches a known route slug -const matchingRouteKey = findRouteKey(literalPath, availableRoutes); -if (matchingRouteKey) { - fix = { - startIndex: literalValueStart, // the position of the string content (not the quotes) - endIndex: literalValueEnd, - newText: `{{ routes.${matchingRouteKey} }}`, - }; -} - -// Case 2: literal href="/" — root URL -if (literalPath === '/') { - fix = { - startIndex: literalValueStart, - endIndex: literalValueEnd, - newText: `{{ routes.root_url }}`, - }; -} - -// Case 3: path with no matching route key — suggest route_url filter (less specific) -if (!fix) { - fix = { - startIndex: literalValueStart, - endIndex: literalValueEnd, - newText: `{{ '${literalPath}' | route_url }}`, - }; -} - -context.report({ - message: `Hardcoded route '${literalPath}' — use {{ routes.${matchingRouteKey ?? '...'} }}`, - startIndex: node.position.start, - endIndex: node.position.end, - fix, -}); -``` - -The `StringCorrector` / `applyFixToString` infrastructure in `platformos-check-common` -handles the actual text substitution when `--fix` is invoked. - ---- - -## pos-cli LSP proposals - -### 6. GraphQL result shape in hover - -**LSP method:** `textDocument/hover` on graphql result variables -**Affected files:** `.liquid` files containing `{% graphql result = 'path/to/query' %}` - -#### The problem - -Today, hovering over the result variable `g` in: - -```liquid -{% graphql g = 'records/users' %} -{% for user in g.records.results %} -``` - -returns nothing useful. The agent (and developer) must either: -1. Open the `.graphql` file and mentally trace `query GetUsers { records { results { ... } } }` - back to the access path -2. Guess — and `g.users.results` vs `g.records.results` is the single most common source - of `UnknownProperty` errors in platformOS templates - -The correct access path depends on the query's root field name, which is determined by -the GraphQL schema and the specific query — information the LSP already has. - -#### What the hover should return - -```markdown -**`g`** ← `records/users` (GetUsers) - -**Access pattern:** -{{ g.records.results }} — array of Record objects -{{ g.records.total_entries }} — total count (for pagination) -{{ g.records.total_pages }} - -**Each record has:** -id · created_at · updated_at · properties_object · table -``` - -This eliminates a whole class of runtime errors by making the correct access path -explicit at the point of use. - -#### Implementation approach - -When the LSP receives `hover` on a variable that is the result of a `{% graphql %}` tag: - -1. **Resolve the query file:** parse the `GraphQLMarkup` node to get the query path - (e.g. `records/users`) and resolve to `app/graphql/records/users.graphql` -2. **Parse the query:** use the GraphQL parser to identify the operation name and - root selection field (`records`, `users`, `pages`, etc.) -3. **Look up the return type:** cross-reference the root selection field against the - schema to find its return type (`RecordConnection`, `UserConnection`, etc.) -4. **Build the access path:** from the connection type, derive: - - `g..results` — the items array - - `g..total_entries` — pagination count - - The shape of each item (fields selected in the query) -5. **Return as hover markdown** - -The LSP already performs steps 1–3 for GraphQL diagnostics and completions. Step 4 is -new but straightforward given the type information already resolved. - -#### Liquid variable tracking - -The LSP needs to track that `g` in the Liquid context is bound to the result of the -graphql tag, then recognise `g` in downstream `{{ g.something }}` expressions as the -same binding. This requires a lightweight Liquid variable binding tracker — the LSP -likely already has this for its existing `UndefinedObject` and `UnknownProperty` -diagnostics. - ---- - -### 7. GraphQL type field listing in hover and completions - -**LSP methods:** `textDocument/hover` on type names, `textDocument/completion` inside -selection sets -**Affected files:** `.graphql` files - -#### The problem - -When writing a GraphQL query, the agent (and developer) does not know which fields are -available on a given type without either: -- Running the query and inspecting the result -- Reading the schema file (`app/graphql/schema.graphql`) manually -- Guessing, leading to `UnknownProperty` errors - -Standard GraphQL LSP implementations provide field-level completions and hover out of -the box. The pos-cli LSP has schema awareness (it validates queries against the schema) -but may not surface this at the hover and completion level. - -#### What this should provide - -**Hover on a type name** (`Record`, `User`, `OrderItem`) in a query: -```markdown -**Record** - -Fields: -- `id` — ID -- `table` — String — the table/model name -- `created_at` — String — ISO 8601 timestamp -- `updated_at` — String -- `properties` — JSON — raw properties hash -- `properties_object` — Object — typed property access -- `related_records` — [Record] — associated records -``` - -**Completion inside a selection set** (cursor after `{`): -``` -id -table -created_at -updated_at -properties -properties_object -related_records { ... } -``` - -#### Why this belongs in the LSP - -The LSP already has the schema. GraphQL field completion is standard LSP behavior -(`CompletionItemKind.Field`). The implementation is a standard GraphQL LSP feature — -libraries like `graphql-language-service` implement this and could be integrated or -referenced. The pos-cli LSP could either implement this natively or delegate to -`graphql-language-service-interface` which is schema-aware. - -Implementing this in the plugin would require the plugin to parse the schema, -resolve types, and format completions — all work the LSP already does internally -but does not expose at the hover/completion level. - ---- - -### 8. Circular render detection via `appGraph` - -**LSP feature:** `textDocument/publishDiagnostics` on cyclic render references -**Trigger:** on file save / `didChange` for `.liquid` files involved in a cycle - -#### The problem - -If partial A renders partial B, which renders partial C, which renders partial A, the -result is an infinite loop at runtime — the page request never completes and eventually -times out or crashes. The linter does not detect this. The plugin's `ProjectIndex` knows -each partial's `renders[]` list but cycle detection is not implemented there. - -This is a correctness bug, not a style issue. A render cycle will break any page that -includes any partial in the cycle. - -#### Why it belongs in the LSP - -The LSP already builds and maintains the `appGraph` — the full render dependency graph -for the project. `appGraph/dependencies` gives the transitive dependency tree from any -file. Cycle detection over an already-built graph is a DFS with a visited set — a -trivial algorithm on top of an existing data structure. - -When the LSP detects a cycle, it can publish a diagnostic via -`textDocument/publishDiagnostics` on the specific `{% render %}` tag that closes the -cycle, pointing exactly at the offending line. - -#### Implementation approach - -After rebuilding the `appGraph` (on file save or project index refresh): - -```js -function detectCycles(graph) { - // graph: Map> (file → files it renders) - const cycles = []; - const visited = new Set(); - const stack = []; - - function dfs(node) { - if (stack.includes(node)) { - // Found a cycle — extract the cycle path - const cycleStart = stack.indexOf(node); - cycles.push(stack.slice(cycleStart).concat(node)); - return; - } - if (visited.has(node)) return; - visited.add(node); - stack.push(node); - for (const neighbour of (graph.get(node) ?? [])) { - dfs(neighbour); - } - stack.pop(); - } - - for (const node of graph.keys()) { - if (!visited.has(node)) dfs(node); - } - return cycles; -} -``` - -For each detected cycle, the LSP: -1. Identifies the `{% render %}` tag in the closing file that references back to a - file earlier in the cycle -2. Publishes a diagnostic (ERROR severity) on that specific tag: - ``` - Circular render detected: sections/hero → atoms/icon → sections/hero - This will cause an infinite loop at runtime. - ``` -3. Also publishes the same diagnostic on the opening file's render tag, so both ends - of the cycle are highlighted in the editor - -#### Incremental update - -When a file is saved, only re-run cycle detection for the connected component containing -that file — not the entire graph. The `appGraph` already supports incremental updates; -cycle detection can follow the same invalidation scope. - ---- - -## Summary - -| # | Feature | Target | Severity | Complexity | -|---|---------|--------|----------|------------| -| 1 | N+1 GraphQL query detection | pos-cli check | WARNING | Low — ~60 lines | -| 2 | Render `@param` validation | pos-cli check | ERROR/WARNING | Medium — ~120 lines + cross-file reads | -| 3 | Dead translation keys | pos-cli check | INFO | Medium — needs YAML + cross-file accumulation | -| 4 | `TranslationKeyExists` nearest-key suggest | pos-cli check | (modify existing) | Low — ~30 lines added to existing check | -| 5 | `HardcodedRoutes` autofix | pos-cli check | (modify existing) | Low — fix field on existing offense | -| 6 | GraphQL result shape in hover | pos-cli LSP | — | Medium — query→type resolution | -| 7 | GraphQL type field listing | pos-cli LSP | — | Medium — schema traversal for hover/completions | -| 8 | Circular render detection | pos-cli LSP | ERROR diagnostic | Low — DFS on existing appGraph | - -**Priority recommendation:** #1 (N+1), #4 (suggest), #5 (autofix), #8 (cycles) are the -highest ratio of value to effort. #2 (@param validation) has the most transformative -long-term impact but requires the codebase to adopt `@param` annotations first. diff --git a/packages/liquid-html-parser/grammar/liquid-html.ohm b/packages/liquid-html-parser/grammar/liquid-html.ohm index 77c1bc6c..6562a891 100644 --- a/packages/liquid-html-parser/grammar/liquid-html.ohm +++ b/packages/liquid-html-parser/grammar/liquid-html.ohm @@ -130,16 +130,14 @@ Liquid <: Helpers { // Assign-specific sub-rules assignTarget = variableSegment lookup* + // Only one operator per assign: either `=` (regular assignment) or `<<` (push). + // The two forms are mutually exclusive — `{% assign a << "item" %}` pushes onto `a`, + // and `{% assign a = value %}` assigns to `a`. A compound shape like + // `{% assign a = b << c %}` is INVALID per the platformOS runtime. assignOperator = "<<" | "=" - // The RHS of an assign: either an explicit push expression (source << value) or a regular value - liquidAssignValue = liquidAssignPushExpr | liquidAssignVariable - - // Explicit push: assign a = source << value (equivalent to assign a << value when source is a) - liquidAssignPushExpr = liquidAssignPushSource space* "<<" space* liquidAssignVariable - - // The source expression for a push (no &delim lookahead since "<<" follows) - liquidAssignPushSource = liquidAssignExpression liquidFilter* space* + // The RHS of an assign: a regular value (expression + optional filters). + liquidAssignValue = liquidAssignVariable // Assign variable: like liquidVariable but expression allows JSON literals liquidAssignVariable = liquidAssignExpression liquidFilter* space* &delim @@ -148,7 +146,7 @@ Liquid <: Helpers { | liquidJsonArrayLiteral | liquidComplexExpression - // JSON literal rules (assign-only, recursive) + // JSON literal rules (assign, return, and named argument values; recursive) liquidJsonHashLiteral = "{" space* listOf, liquidJsonSep> space* "}" liquidJsonKeyValue = @@ -415,8 +413,17 @@ Liquid <: Helpers { booleanExpressionCondition = comparison | liquidExpression liquidString = liquidSingleQuotedString | liquidDoubleQuotedString - liquidSingleQuotedString = "'" anyExceptStar<("'"| delim)> "'" - liquidDoubleQuotedString = "\"" anyExceptStar<("\""| delim)> "\"" + // Strings support backslash-X escape pairs (any character after a backslash is + // consumed as part of the string). This matches the platformOS runtime, which parses + // hash/array literal string values with Ruby's JSON library — so an escaped quote or + // an escaped backslash is valid inside a double-quoted string. + liquidSingleQuotedString = "'" liquidSingleQuotedStringBody "'" + liquidDoubleQuotedString = "\"" liquidDoubleQuotedStringBody "\"" + liquidSingleQuotedStringBody = (liquidStringEscape | liquidSingleQuotedStringChar)* + liquidDoubleQuotedStringBody = (liquidStringEscape | liquidDoubleQuotedStringChar)* + liquidSingleQuotedStringChar = ~("'" | delim) any + liquidDoubleQuotedStringChar = ~("\"" | delim) any + liquidStringEscape = "\\" any liquidNumber = liquidFloat | liquidInteger liquidInteger = "-"? digit+ @@ -449,9 +456,18 @@ Liquid <: Helpers { arguments = nonemptyOrderedListOf, namedArgument, argumentSeparator> argumentSeparator = space* "," space* argumentSeparatorOptionalComma = space* ","? space* - positionalArgument = liquidExpression ~(space* ":") + // liquidExpressionOrJsonLiteral: used in argument value positions where JSON hash/array + // literals are accepted (named args, filter positional args). Does NOT extend liquidExpression + // itself so that {{ ["x"] }} drop expressions are still parsed as VariableLookup (empty lookup). + liquidExpressionOrJsonLiteral = liquidJsonHashLiteral | liquidJsonArrayLiteral | liquidExpression + positionalArgument = liquidExpressionOrJsonLiteral ~(space* ":") namedArgument = variableSegment space* ":" space* namedArgumentValue - namedArgumentValue = hashPairValue | liquidExpression + namedArgumentValue = hashPairValue | liquidExpressionOrJsonLiteral + // hashPairValue handles nested key:value pairs within a named argument value, e.g. + // {% function res = 'path', filter: type: "string" %} + // where `type: "string"` is the hashPairValue. Only fires when the value starts with + // `identifier:` — otherwise namedArgumentValue falls through to liquidExpressionOrJsonLiteral. + // Produces a NamedArgument node so linters can inspect nested argument pairs. hashPairValue = variableSegment space* ":" space* liquidExpression tagArguments = listOf, argumentSeparatorOptionalComma> graphqlArguments = listOf @@ -574,10 +590,11 @@ LiquidStatement <: Liquid { // Override string rules so that '#' inside string content is never mistaken for a // delimTag inline-comment marker. In LiquidStatement, delimTag includes `# ...` which - // would otherwise cause anyExceptStar to stop inside strings like '##label##'. - // Strings in liquid blocks end at the matching quote or at a newline (no multi-line strings). - liquidSingleQuotedString := "'" anyExceptStar<("'" | newline | end)> "'" - liquidDoubleQuotedString := "\"" anyExceptStar<("\"" | newline | end)> "\"" + // would otherwise cause the body rule to stop inside strings like '##label##'. + // Strings in liquid blocks end at the matching quote or at a newline (no multi-line + // strings), but still support `\X` escape pairs. + liquidSingleQuotedStringChar := ~("'" | newline | end) any + liquidDoubleQuotedStringChar := ~("\"" | newline | end) any // trailing whitespace, newline, + optional leading `;` and `# comment` before the next tag statementSep = space* ";"? space* ("#" (~newline any)*)? newline (space | newline)* @@ -585,6 +602,27 @@ LiquidStatement <: Liquid { // delimTag is used as &delim lookahead in expression rules; it must succeed before `;` and `#` liquidStatementEnd = ";"? space* ("#" (~newline any)*)? (newline | end) delimTag := liquidStatementEnd + + // Allow newlines inside hash/array literals within {% liquid %} blocks. + // The base Liquid grammar restricts `space` to horizontal whitespace only, but hash/array + // literals written across multiple lines need to match newlines between `{`/`[` and entries, + // between entries (around commas), and between the last entry and the closing `}`/`]`. + liquidJsonSep := (space | newline)* "," (space | newline)* + liquidJsonHashLiteral := "{" (space | newline)* listOf, liquidJsonSep> (space | newline)* "}" + liquidJsonArrayLiteral := "[" (space | newline)* listOf, liquidJsonSep> (space | newline)* "]" + + // Only the function tag supports multi-line named arguments in {% liquid %} blocks. + // render, include, and other tags use the base argumentSeparatorOptionalComma (horizontal- + // only), which prevents their argument lists from spanning lines. Function uses a dedicated + // separator that allows newlines ONLY after a comma, preventing the next statement from + // being swallowed as an argument when no comma is present. + // + // Both alternatives have 3 components (space, comma, space) to match each other's arity, + // ensuring consistent visitor child indexing. + functionArgumentSeparatorOptionalComma = space* "," (space | newline)* | space* ","? space* + functionTagArguments = listOf, functionArgumentSeparatorOptionalComma> + functionRenderArguments = (functionArgumentSeparatorOptionalComma functionTagArguments) (space* ",")? space* + liquidTagFunctionMarkup := liquidVariableLookup space* "=" space* partialExpression functionRenderArguments liquidFilter* space* } LiquidDoc <: Helpers { diff --git a/packages/liquid-html-parser/src/stage-1-cst.spec.ts b/packages/liquid-html-parser/src/stage-1-cst.spec.ts index 96f6ba0b..49839840 100644 --- a/packages/liquid-html-parser/src/stage-1-cst.spec.ts +++ b/packages/liquid-html-parser/src/stage-1-cst.spec.ts @@ -361,6 +361,35 @@ describe('Unit: Stage 1 (CST)', () => { expectPath(cst, '0.markup.expression.lookups').to.eql([]); } }); + + it('should NOT parse array syntax in output tag as JsonArrayLiteral', () => { + // liquidExpressionOrJsonLiteral is intentionally NOT used in output expressions; + // {{ ["x"] }} must remain a VariableLookup subscript, not a JsonArrayLiteral. + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{{ ["x"] }}`); + expectPath(cst, '0.type').to.equal('LiquidVariableOutput'); + expectPath(cst, '0.markup.type').to.equal('LiquidVariable'); + expectPath(cst, '0.markup.expression.type').to.equal('VariableLookup'); + expectPath(cst, '0.markup.expression.name').to.equal(null); + // lookups[0] is a String node — the key of the subscript + expectPath(cst, '0.markup.expression.lookups').to.have.lengthOf(1); + expectPath(cst, '0.markup.expression.lookups.0.type').to.equal('String'); + expectPath(cst, '0.markup.expression.lookups.0.value').to.equal('x'); + } + }); + + it('should NOT parse number-subscript output as JsonArrayLiteral', () => { + // Same invariant as the string-subscript regression; a Number subscript must + // still produce a VariableLookup with an empty name, not a JsonArrayLiteral. + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{{ [0] }}`); + expectPath(cst, '0.markup.expression.type').to.equal('VariableLookup'); + expectPath(cst, '0.markup.expression.name').to.equal(null); + expectPath(cst, '0.markup.expression.lookups').to.have.lengthOf(1); + expectPath(cst, '0.markup.expression.lookups.0.type').to.equal('Number'); + expectPath(cst, '0.markup.expression.lookups.0.value').to.equal('0'); + } + }); }); describe('Case: LiquidTag', () => { @@ -594,20 +623,23 @@ describe('Unit: Stage 1 (CST)', () => { cst = toCST(`{% assign x << 'foo' | upcase %}`); expectPath(cst, '0.markup.operator').to.equal('<<'); expectPath(cst, '0.markup.value.filters').to.have.lengthOf(1); + } + }); - // explicit push form: assign a = source << value - cst = toCST(`{% assign x = roles << 'admin' %}`); - expectPath(cst, '0.markup.operator').to.equal('='); - expectPath(cst, '0.markup.name').to.equal('x'); - expectPath(cst, '0.markup.value.type').to.equal('AssignPushRhs'); - expectPath(cst, '0.markup.value.pushSource.expression.name').to.equal('roles'); - expectPath(cst, '0.markup.value.pushValue.expression.value').to.equal('admin'); - - // explicit push with dot notation source - cst = toCST(`{% assign x = current_profile.roles << 'authenticated' %}`); - expectPath(cst, '0.markup.operator').to.equal('='); - expectPath(cst, '0.markup.value.type').to.equal('AssignPushRhs'); - expectPath(cst, '0.markup.value.pushSource.expression.name').to.equal('current_profile'); + it('should reject compound operator `= ... <<` in strict mode (only one operator allowed)', () => { + // `{% assign a = b << c %}` is NOT valid per the platformOS runtime — an + // assign uses exactly one of `=` (regular assignment) or `<<` (push), not both. + const invalidCases = [ + `{% assign x = roles << 'admin' %}`, + `{% assign x = current_profile.roles << 'authenticated' %}`, + ]; + for (const source of invalidCases) { + try { + toLiquidHtmlCST(source, { mode: 'strict' }); + expect(true, `expected ${source} to throw`).to.be.false; + } catch (e: any) { + expect(e.name, source).to.equal('LiquidHTMLParsingError'); + } } }); @@ -674,6 +706,466 @@ describe('Unit: Stage 1 (CST)', () => { } }); + it('should parse multi-line hash literals in {% liquid %} blocks', () => { + for (const { toCST, expectPath } of testCases) { + // multi-line hash with multiple entries + cst = toCST( + `{% liquid\n assign data = {\n "title": object.title,\n "body": object.body\n }\n%}`, + ); + expectPath(cst, '0.markup.0.name').to.equal('assign'); + expectPath(cst, '0.markup.0.markup.value.expression.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.0.markup.value.expression.entries').to.have.lengthOf(2); + expectPath(cst, '0.markup.0.markup.value.expression.entries.0.key.value').to.equal( + 'title', + ); + expectPath(cst, '0.markup.0.markup.value.expression.entries.1.key.value').to.equal( + 'body', + ); + + // empty multi-line hash + cst = toCST(`{% liquid\n assign data = {\n }\n%}`); + expectPath(cst, '0.markup.0.markup.value.expression.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.0.markup.value.expression.entries').to.have.lengthOf(0); + + // statement following a multi-line hash is parsed correctly + cst = toCST( + `{% liquid\n assign data = {\n "title": object.title\n }\n assign other = 'hi'\n%}`, + ); + expectPath(cst, '0.markup.0.markup.value.expression.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.1.name').to.equal('assign'); + expectPath(cst, '0.markup.1.markup.name').to.equal('other'); + + // hash that starts on the same line as assign, with subsequent entries on the next line + // the comma is mid-line; the continuation is inside the hash literal, not an arg separator + cst = toCST(`{% liquid\n assign hash = { key: val,\n "key2": "value2" }\n%}`); + expectPath(cst, '0.markup.0.name').to.equal('assign'); + expectPath(cst, '0.markup.0.markup.value.expression.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.0.markup.value.expression.entries').to.have.lengthOf(2); + expectPath(cst, '0.markup.0.markup.value.expression.entries.0.key.type').to.equal( + 'VariableLookup', + ); + expectPath(cst, '0.markup.0.markup.value.expression.entries.0.key.name').to.equal('key'); + expectPath(cst, '0.markup.0.markup.value.expression.entries.1.key.type').to.equal( + 'String', + ); + expectPath(cst, '0.markup.0.markup.value.expression.entries.1.key.value').to.equal( + 'key2', + ); + expectPath(cst, '0.markup.0.markup.value.expression.entries.1.value.type').to.equal( + 'String', + ); + expectPath(cst, '0.markup.0.markup.value.expression.entries.1.value.value').to.equal( + 'value2', + ); + } + }); + + it('should parse multi-line array literals in {% liquid %} blocks', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% liquid\n assign items = [\n "a",\n "b"\n ]\n%}`); + expectPath(cst, '0.markup.0.name').to.equal('assign'); + expectPath(cst, '0.markup.0.markup.value.expression.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.0.markup.value.expression.elements').to.have.lengthOf(2); + expectPath(cst, '0.markup.0.markup.value.expression.elements.0.type').to.equal('String'); + expectPath(cst, '0.markup.0.markup.value.expression.elements.0.value').to.equal('a'); + expectPath(cst, '0.markup.0.markup.value.expression.elements.1.type').to.equal('String'); + expectPath(cst, '0.markup.0.markup.value.expression.elements.1.value').to.equal('b'); + } + }); + + it('should parse multi-line nested hash with variable bare keys in {% liquid %} blocks', () => { + for (const { toCST, expectPath } of testCases) { + // release notes example: { outer_key: { inner_key: "Alice" } } + // Both outer_key and inner_key are bare keys (VariableLookup), evaluated at runtime + cst = toCST( + `{% liquid\n assign hash = {\n outer_key: {\n inner_key: "Alice"\n }\n }\n%}`, + ); + expectPath(cst, '0.markup.0.name').to.equal('assign'); + expectPath(cst, '0.markup.0.markup.value.expression.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.0.markup.value.expression.entries').to.have.lengthOf(1); + // outer_key is a bare key — parsed as VariableLookup so linters can detect undefined vars + expectPath(cst, '0.markup.0.markup.value.expression.entries.0.key.type').to.equal( + 'VariableLookup', + ); + expectPath(cst, '0.markup.0.markup.value.expression.entries.0.key.name').to.equal( + 'outer_key', + ); + // value is a nested hash + expectPath(cst, '0.markup.0.markup.value.expression.entries.0.value.type').to.equal( + 'JsonHashLiteral', + ); + expectPath( + cst, + '0.markup.0.markup.value.expression.entries.0.value.entries', + ).to.have.lengthOf(1); + // inner_key is also a bare key — parsed as VariableLookup + expectPath( + cst, + '0.markup.0.markup.value.expression.entries.0.value.entries.0.key.type', + ).to.equal('VariableLookup'); + expectPath( + cst, + '0.markup.0.markup.value.expression.entries.0.value.entries.0.key.name', + ).to.equal('inner_key'); + expectPath( + cst, + '0.markup.0.markup.value.expression.entries.0.value.entries.0.value.type', + ).to.equal('String'); + expectPath( + cst, + '0.markup.0.markup.value.expression.entries.0.value.entries.0.value.value', + ).to.equal('Alice'); + } + }); + + it('should parse assign tag with JSON literals as filter arguments', () => { + for (const { toCST, expectPath } of testCases) { + // empty array as positional filter argument: value | default: [] + cst = toCST(`{% assign arr = value | default: [] %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('VariableLookup'); + expectPath(cst, '0.markup.value.filters').to.have.lengthOf(1); + expectPath(cst, '0.markup.value.filters.0.name').to.equal('default'); + expectPath(cst, '0.markup.value.filters.0.args').to.have.lengthOf(1); + expectPath(cst, '0.markup.value.filters.0.args.0.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.value.filters.0.args.0.elements').to.have.lengthOf(0); + + // empty hash as positional filter argument: value | default: {} + cst = toCST(`{% assign hash = value | default: {} %}`); + expectPath(cst, '0.markup.value.filters.0.name').to.equal('default'); + expectPath(cst, '0.markup.value.filters.0.args.0.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.value.filters.0.args.0.entries').to.have.lengthOf(0); + + // populated array as filter argument + cst = toCST(`{% assign arr = value | default: ["a", "b"] %}`); + expectPath(cst, '0.markup.value.filters.0.args.0.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.value.filters.0.args.0.elements').to.have.lengthOf(2); + expectPath(cst, '0.markup.value.filters.0.args.0.elements.0.type').to.equal('String'); + expectPath(cst, '0.markup.value.filters.0.args.0.elements.0.value').to.equal('a'); + } + }); + + it('should parse assign tag with string interpolation inside hash string values', () => { + for (const { toCST, expectPath } of testCases) { + // {{ ... }} inside a quoted string value is parsed as plain string content (runtime-evaluated) + cst = toCST(`{% assign x = { "email": "{{ email | downcase }}", "name": "Alice" } %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.value.expression.entries').to.have.lengthOf(2); + expectPath(cst, '0.markup.value.expression.entries.0.key.value').to.equal('email'); + expectPath(cst, '0.markup.value.expression.entries.0.value.type').to.equal('String'); + expectPath(cst, '0.markup.value.expression.entries.0.value.value').to.equal( + '{{ email | downcase }}', + ); + expectPath(cst, '0.markup.value.expression.entries.1.key.value').to.equal('name'); + expectPath(cst, '0.markup.value.expression.entries.1.value.value').to.equal('Alice'); + } + }); + + it('should treat {{ ... }} inside a plain assign string RHS as literal content', () => { + // `{{ ... }}` inside a quoted string is preserved verbatim as the String value. + // The runtime (Liquify) may re-render the result, but at parse time it's a String. + for (const { toCST, expectPath } of testCases) { + // double-quoted with a single interpolation + cst = toCST(`{% assign x = "hello {{ name }}" %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('String'); + expectPath(cst, '0.markup.value.expression.single').to.equal(false); + expectPath(cst, '0.markup.value.expression.value').to.equal('hello {{ name }}'); + + // single-quoted with an interpolation + cst = toCST(`{% assign x = 'hello {{ name }}' %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('String'); + expectPath(cst, '0.markup.value.expression.single').to.equal(true); + expectPath(cst, '0.markup.value.expression.value').to.equal('hello {{ name }}'); + + // multiple interpolations with a filter chain inside one + cst = toCST(`{% assign x = "{{ a }}-{{ b | upcase }}" %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('String'); + expectPath(cst, '0.markup.value.expression.value').to.equal('{{ a }}-{{ b | upcase }}'); + } + }); + + it('should treat {{ ... }} inside an array-element string as literal content', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% assign x = ["{{ a }}", "{{ b }}"] %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.value.expression.elements').to.have.lengthOf(2); + expectPath(cst, '0.markup.value.expression.elements.0.type').to.equal('String'); + expectPath(cst, '0.markup.value.expression.elements.0.value').to.equal('{{ a }}'); + expectPath(cst, '0.markup.value.expression.elements.1.value').to.equal('{{ b }}'); + } + }); + + it('should treat {{ ... }} inside a filter-argument string as literal content', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% assign x = y | append: "{{ suffix }}" %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('VariableLookup'); + expectPath(cst, '0.markup.value.filters').to.have.lengthOf(1); + expectPath(cst, '0.markup.value.filters.0.name').to.equal('append'); + expectPath(cst, '0.markup.value.filters.0.args.0.type').to.equal('String'); + expectPath(cst, '0.markup.value.filters.0.args.0.value').to.equal('{{ suffix }}'); + } + }); + + it('should truncate at the first `%}` inside an assign string literal', () => { + // KNOWN LIMITATION: the `delim` lookahead (= `"-%}" | "%}"`) fires on the first + // `%}` it sees, even inside a quoted string. `{% assign x = "{% echo 'hi' %}" %}` + // therefore parses as TWO top-level nodes: a truncated assign whose markup + // stops before the inner `%}`, plus orphan text (`" %}`) containing the unmatched + // closing quote and the real end of the tag. This is a pre-existing parser limit; + // the test locks in the current behavior so any intentional fix is visible. + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% assign x = "{% echo 'hi' %}" %}`); + // Two top-level nodes, not one. + expectPath(cst, 'length').to.equal(2); + + // [0] truncated assign: base-case fallback with string markup that ends + // before the inner `%}` — the open quote is still unmatched in the markup. + expectPath(cst, '0.type').to.equal('LiquidTag'); + expectPath(cst, '0.name').to.equal('assign'); + expectPath(cst, '0.markup').to.equal(`x = "{% echo 'hi'`); + + // [1] orphan text: the unmatched closing quote and the real tag-close. + expectPath(cst, '1.type').to.equal('TextNode'); + expectPath(cst, '1.value').to.equal(`" %}`); + } + }); + + it('should reject invalid assign syntax in strict mode', () => { + const testCases = [ + // missing RHS entirely + `{% assign x = %}`, + // unclosed hash literal + `{% assign x = { "key": "value" %}`, + // unclosed array literal + `{% assign x = ["a" %}`, + // missing value in hash entry + `{% assign x = { "key": } %}`, + // missing colon in hash entry + `{% assign x = { "key" "value" } %}`, + ]; + for (const source of testCases) { + try { + toLiquidHtmlCST(source, { mode: 'strict' }); + expect(true, `expected ${source} to throw`).to.be.false; + } catch (e: any) { + expect(e.name, source).to.equal('LiquidHTMLParsingError'); + expect(e.loc, `expected ${source} to have location info`).not.to.be.undefined; + } + } + }); + + it('should parse assign with array element types beyond strings', () => { + for (const { toCST, expectPath } of testCases) { + // variable lookups inside an array + cst = toCST(`{% assign x = [item1, item2] %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.value.expression.elements').to.have.lengthOf(2); + expectPath(cst, '0.markup.value.expression.elements.0.type').to.equal('VariableLookup'); + expectPath(cst, '0.markup.value.expression.elements.0.name').to.equal('item1'); + expectPath(cst, '0.markup.value.expression.elements.1.type').to.equal('VariableLookup'); + expectPath(cst, '0.markup.value.expression.elements.1.name').to.equal('item2'); + + // number literals inside an array + cst = toCST(`{% assign x = [1, 2, 3] %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.value.expression.elements').to.have.lengthOf(3); + expectPath(cst, '0.markup.value.expression.elements.0.type').to.equal('Number'); + expectPath(cst, '0.markup.value.expression.elements.0.value').to.equal('1'); + + // liquid literals (true, false, nil) inside an array + cst = toCST(`{% assign x = [true, false, nil] %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.value.expression.elements').to.have.lengthOf(3); + expectPath(cst, '0.markup.value.expression.elements.0.type').to.equal('LiquidLiteral'); + expectPath(cst, '0.markup.value.expression.elements.0.value').to.equal(true); + expectPath(cst, '0.markup.value.expression.elements.1.value').to.equal(false); + expectPath(cst, '0.markup.value.expression.elements.2.value').to.equal(null); + } + }); + + it('should parse assign hash with mixed string and bare keys', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% assign x = { "str_key": 1, bare_key: 2 } %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.value.expression.entries').to.have.lengthOf(2); + expectPath(cst, '0.markup.value.expression.entries.0.key.type').to.equal('String'); + expectPath(cst, '0.markup.value.expression.entries.0.key.value').to.equal('str_key'); + expectPath(cst, '0.markup.value.expression.entries.1.key.type').to.equal( + 'VariableLookup', + ); + expectPath(cst, '0.markup.value.expression.entries.1.key.name').to.equal('bare_key'); + } + }); + + it('should chain filters after a JSON-literal filter argument', () => { + for (const { toCST, expectPath } of testCases) { + // | default: [] | join: "," — two filters; the empty array is the arg to the first + cst = toCST(`{% assign x = val | default: [] | join: "," %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('VariableLookup'); + expectPath(cst, '0.markup.value.filters').to.have.lengthOf(2); + expectPath(cst, '0.markup.value.filters.0.name').to.equal('default'); + expectPath(cst, '0.markup.value.filters.0.args').to.have.lengthOf(1); + expectPath(cst, '0.markup.value.filters.0.args.0.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.value.filters.1.name').to.equal('join'); + expectPath(cst, '0.markup.value.filters.1.args.0.type').to.equal('String'); + expectPath(cst, '0.markup.value.filters.1.args.0.value').to.equal(','); + } + }); + + it('should parse assign with whitespace-strip delimiters and a JSON value', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{%- assign x = { "key": "val" } -%}`); + expectPath(cst, '0.type').to.equal('LiquidTag'); + expectPath(cst, '0.name').to.equal('assign'); + expectPath(cst, '0.whitespaceStart').to.equal('-'); + expectPath(cst, '0.whitespaceEnd').to.equal('-'); + expectPath(cst, '0.markup.value.expression.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.value.expression.entries').to.have.lengthOf(1); + } + }); + + it('should parse multi-line array in {% liquid %} block followed by another statement', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% liquid\n assign items = [\n "a"\n ]\n assign other = 'hi'\n%}`); + expectPath(cst, '0.markup.0.name').to.equal('assign'); + expectPath(cst, '0.markup.0.markup.value.expression.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.0.markup.value.expression.elements').to.have.lengthOf(1); + // the closing ] must not swallow the next statement + expectPath(cst, '0.markup.1.name').to.equal('assign'); + expectPath(cst, '0.markup.1.markup.name').to.equal('other'); + } + }); + + it('should parse backslash-X escape sequences in string literals', () => { + // Escape support matches the platformOS runtime (Ruby JSON's parser accepts + // these inside hash/array string values). The CST preserves the raw escape + // sequence in `value` — consumers can decode as needed. + for (const { toCST, expectPath } of testCases) { + // escaped double-quote inside double-quoted string + cst = toCST(`{% assign x = "He said \\"hello\\"" %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('String'); + expectPath(cst, '0.markup.value.expression.single').to.equal(false); + expectPath(cst, '0.markup.value.expression.value').to.equal('He said \\"hello\\"'); + + // escaped backslash inside double-quoted string + cst = toCST(`{% assign x = "a\\\\b" %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('String'); + expectPath(cst, '0.markup.value.expression.value').to.equal('a\\\\b'); + + // escaped single-quote inside single-quoted string + cst = toCST(`{% assign x = 'can\\'t' %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('String'); + expectPath(cst, '0.markup.value.expression.single').to.equal(true); + expectPath(cst, '0.markup.value.expression.value').to.equal("can\\'t"); + + // escaped double-quote inside a JSON hash literal value (the motivating case) + cst = toCST(`{% assign data = { "quote": "He said \\"hello\\"" } %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.value.expression.entries').to.have.lengthOf(1); + expectPath(cst, '0.markup.value.expression.entries.0.key.value').to.equal('quote'); + expectPath(cst, '0.markup.value.expression.entries.0.value.type').to.equal('String'); + expectPath(cst, '0.markup.value.expression.entries.0.value.value').to.equal( + 'He said \\"hello\\"', + ); + } + }); + + it('should preserve plain strings (no escape) unchanged', () => { + // Regression guard: the escape-aware body rule must not alter simple strings. + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% assign x = "plain text" %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('String'); + expectPath(cst, '0.markup.value.expression.value').to.equal('plain text'); + + cst = toCST(`{% assign x = 'no escapes here' %}`); + expectPath(cst, '0.markup.value.expression.type').to.equal('String'); + expectPath(cst, '0.markup.value.expression.value').to.equal('no escapes here'); + } + }); + + it('should reject trailing commas in assign JSON literals in strict mode', () => { + const trailingCommaTests = [ + `{% assign x = { "key": "val", } %}`, + `{% assign x = ["a", ] %}`, + ]; + for (const source of trailingCommaTests) { + try { + toLiquidHtmlCST(source, { mode: 'strict' }); + expect(true, `expected ${source} to throw`).to.be.false; + } catch (e: any) { + expect(e.name, source).to.equal('LiquidHTMLParsingError'); + expect(e.loc, `expected ${source} to have location info`).not.to.be.undefined; + } + } + }); + + it('should reject mismatched / nested-unclosed JSON in assign (strict mode)', () => { + const invalidCases = [ + // mismatched closing bracket + `{% assign x = { "k": "v" ] %}`, + // nested unclosed array inside valid outer array + `{% assign x = [ "a", [1,2 ] %}`, + // nested unclosed hash inside hash + `{% assign x = { "a": { "b": "c" } %}`, + ]; + for (const source of invalidCases) { + try { + toLiquidHtmlCST(source, { mode: 'strict' }); + expect(true, `expected ${source} to throw`).to.be.false; + } catch (e: any) { + expect(e.name, source).to.equal('LiquidHTMLParsingError'); + expect(e.loc, `expected ${source} to have location info`).not.to.be.undefined; + } + } + }); + + it('should parse primitive RHS types (strings, numbers, literals, ranges)', () => { + // Lock in that each primitive RHS type produces the expected expression AST node. + const primitives: Array<[string, string, any]> = [ + [`{% assign x = 'str' %}`, 'String', 'str'], + [`{% assign x = "str" %}`, 'String', 'str'], + [`{% assign x = 42 %}`, 'Number', '42'], + [`{% assign x = -1.5 %}`, 'Number', '-1.5'], + [`{% assign x = true %}`, 'LiquidLiteral', true], + [`{% assign x = false %}`, 'LiquidLiteral', false], + [`{% assign x = nil %}`, 'LiquidLiteral', null], + ]; + for (const [source, expectedType, expectedValue] of primitives) { + for (const { toCST, expectPath } of testCases) { + cst = toCST(source); + expectPath(cst, '0.markup.value.expression.type').to.equal(expectedType); + expectPath(cst, '0.markup.value.expression.value').to.equal(expectedValue); + } + } + }); + + it('should parse valid identifier variants as assign target', () => { + // Underscore prefix, snake_case, hyphen, trailing digit — all accepted by the grammar. + const validIdentifiers = [ + [`{% assign _private = 1 %}`, '_private'], + [`{% assign snake_case = 1 %}`, 'snake_case'], + [`{% assign with-dash = 1 %}`, 'with-dash'], + [`{% assign x1 = 1 %}`, 'x1'], + ]; + for (const [source, expectedName] of validIdentifiers) { + for (const { toCST, expectPath } of testCases) { + cst = toCST(source); + expectPath(cst, '0.markup.target.name').to.equal(expectedName); + } + } + }); + + it('should parse bare push syntax (`a << b`) in assign', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% assign my_val << "item" %}`); + expectPath(cst, '0.markup.target.name').to.equal('my_val'); + expectPath(cst, '0.markup.operator').to.equal('<<'); + + cst = toCST(`{% assign my_val << "item" | upcase %}`); + expectPath(cst, '0.markup.operator').to.equal('<<'); + expectPath(cst, '0.markup.value.filters').to.have.lengthOf(1); + } + }); + it('should parse simple assign with backward compatible name field', () => { for (const { toCST, expectPath } of testCases) { cst = toCST(`{% assign x = 'hi' %}`); @@ -844,6 +1336,25 @@ describe('Unit: Stage 1 (CST)', () => { ); }); + it('should reject multi-line arguments for render in {% liquid %} blocks (strict mode)', () => { + // render does not support multi-line argument lists; only function does. + // A newline after the comma in render arguments must be treated as a statement separator, + // not an argument continuation. + const invalidCases = [ + `{% liquid\n render 'partial',\n arg1: "val"\n%}`, + `{% liquid\n render 'partial',\n arg1: "val",\n arg2: "val2"\n%}`, + ]; + for (const source of invalidCases) { + try { + toLiquidHtmlCST(source, { mode: 'strict' }); + expect(true, `expected ${source} to throw`).to.be.false; + } catch (e: any) { + expect(e.name, source).to.equal('LiquidHTMLParsingError'); + expect(e.loc, `expected ${source} to have location info`).not.to.be.undefined; + } + } + }); + it('should parse the include tag', () => { [ { @@ -1030,6 +1541,180 @@ describe('Unit: Stage 1 (CST)', () => { } }); + it('should parse multi-line function arguments in {% liquid %} blocks', () => { + for (const { toCST, expectPath } of testCases) { + // arguments on separate lines with commas + cst = toCST( + `{% liquid\n function result = 'partial/path',\n arg1: val1,\n arg2: val2\n%}`, + ); + expectPath(cst, '0.markup.0.name').to.equal('function'); + expectPath(cst, '0.markup.0.markup.functionArguments').to.have.lengthOf(2); + expectPath(cst, '0.markup.0.markup.functionArguments.0.name').to.equal('arg1'); + expectPath(cst, '0.markup.0.markup.functionArguments.1.name').to.equal('arg2'); + + // statement following multi-line function is parsed correctly + cst = toCST( + `{% liquid\n function result = 'partial/path',\n arg1: val1\n assign x = 'hi'\n%}`, + ); + expectPath(cst, '0.markup.0.name').to.equal('function'); + expectPath(cst, '0.markup.0.markup.functionArguments').to.have.lengthOf(1); + expectPath(cst, '0.markup.1.name').to.equal('assign'); + } + }); + + it('should parse function tag with JSON literals as named argument values', () => { + for (const { toCST, expectPath } of testCases) { + // empty array and empty hash as named argument values + cst = toCST(`{% function result = 'my/function', array: [], hash: {} %}`); + expectPath(cst, '0.markup.functionArguments').to.have.lengthOf(2); + expectPath(cst, '0.markup.functionArguments.0.name').to.equal('array'); + expectPath(cst, '0.markup.functionArguments.0.value.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.functionArguments.0.value.elements').to.have.lengthOf(0); + expectPath(cst, '0.markup.functionArguments.1.name').to.equal('hash'); + expectPath(cst, '0.markup.functionArguments.1.value.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.functionArguments.1.value.entries').to.have.lengthOf(0); + + // populated array as named argument value + cst = toCST(`{% function result = 'my/function', items: ["a", "b"] %}`); + expectPath(cst, '0.markup.functionArguments').to.have.lengthOf(1); + expectPath(cst, '0.markup.functionArguments.0.name').to.equal('items'); + expectPath(cst, '0.markup.functionArguments.0.value.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.functionArguments.0.value.elements').to.have.lengthOf(2); + expectPath(cst, '0.markup.functionArguments.0.value.elements.0.type').to.equal('String'); + expectPath(cst, '0.markup.functionArguments.0.value.elements.0.value').to.equal('a'); + + // populated hash as named argument value — keys are variable lookups (bare keys) + cst = toCST(`{% function result = 'my/function', config: { key: "val" } %}`); + expectPath(cst, '0.markup.functionArguments').to.have.lengthOf(1); + expectPath(cst, '0.markup.functionArguments.0.name').to.equal('config'); + expectPath(cst, '0.markup.functionArguments.0.value.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.functionArguments.0.value.entries').to.have.lengthOf(1); + expectPath(cst, '0.markup.functionArguments.0.value.entries.0.key.type').to.equal( + 'VariableLookup', + ); + expectPath(cst, '0.markup.functionArguments.0.value.entries.0.key.name').to.equal('key'); + expectPath(cst, '0.markup.functionArguments.0.value.entries.0.value.value').to.equal( + 'val', + ); + + // multi-line function with JSON array argument in {% liquid %} block + cst = toCST( + `{% liquid\n function result = 'my/function',\n array: [],\n hash: {}\n%}`, + ); + expectPath(cst, '0.markup.0.name').to.equal('function'); + expectPath(cst, '0.markup.0.markup.functionArguments').to.have.lengthOf(2); + expectPath(cst, '0.markup.0.markup.functionArguments.0.name').to.equal('array'); + expectPath(cst, '0.markup.0.markup.functionArguments.0.value.type').to.equal( + 'JsonArrayLiteral', + ); + expectPath(cst, '0.markup.0.markup.functionArguments.1.name').to.equal('hash'); + expectPath(cst, '0.markup.0.markup.functionArguments.1.value.type').to.equal( + 'JsonHashLiteral', + ); + } + }); + + it('should reject invalid function syntax in strict mode', () => { + const invalidCases = [ + // missing partial (no = expression) + `{% function result %}`, + // missing = operator + `{% function result 'partial' %}`, + ]; + for (const source of invalidCases) { + try { + toLiquidHtmlCST(source, { mode: 'strict' }); + expect(true, `expected ${source} to throw`).to.be.false; + } catch (e: any) { + expect(e.name, source).to.equal('LiquidHTMLParsingError'); + expect(e.loc, `expected ${source} to have location info`).not.to.be.undefined; + } + } + }); + + it('should parse function with nested multi-line hash argument in {% liquid %} block', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST( + `{% liquid\n function res = 'path',\n config: {\n key: "val"\n }\n%}`, + ); + expectPath(cst, '0.markup.0.name').to.equal('function'); + expectPath(cst, '0.markup.0.markup.functionArguments').to.have.lengthOf(1); + expectPath(cst, '0.markup.0.markup.functionArguments.0.name').to.equal('config'); + expectPath(cst, '0.markup.0.markup.functionArguments.0.value.type').to.equal( + 'JsonHashLiteral', + ); + expectPath(cst, '0.markup.0.markup.functionArguments.0.value.entries').to.have.lengthOf( + 1, + ); + expectPath( + cst, + '0.markup.0.markup.functionArguments.0.value.entries.0.key.type', + ).to.equal('VariableLookup'); + expectPath( + cst, + '0.markup.0.markup.functionArguments.0.value.entries.0.key.name', + ).to.equal('key'); + } + }); + + it('should parse function with JSON array containing variable lookups as named arg', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% function result = 'my/function', items: [item1, item2] %}`); + expectPath(cst, '0.markup.functionArguments').to.have.lengthOf(1); + expectPath(cst, '0.markup.functionArguments.0.name').to.equal('items'); + expectPath(cst, '0.markup.functionArguments.0.value.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.functionArguments.0.value.elements').to.have.lengthOf(2); + expectPath(cst, '0.markup.functionArguments.0.value.elements.0.type').to.equal( + 'VariableLookup', + ); + expectPath(cst, '0.markup.functionArguments.0.value.elements.0.name').to.equal('item1'); + } + }); + + it('should parse function tag with nested key:value pair as named argument value (hashPairValue)', () => { + for (const { toCST, expectPath } of testCases) { + // `filter: type: "string"` — `filter` is the named arg, `type: "string"` is a hashPairValue + // which produces a nested NamedArgument node for linter inspection. + cst = toCST(`{% function result = 'my/function', filter: type: "string" %}`); + expectPath(cst, '0.markup.functionArguments').to.have.lengthOf(1); + expectPath(cst, '0.markup.functionArguments.0.name').to.equal('filter'); + expectPath(cst, '0.markup.functionArguments.0.value.type').to.equal('NamedArgument'); + expectPath(cst, '0.markup.functionArguments.0.value.name').to.equal('type'); + expectPath(cst, '0.markup.functionArguments.0.value.value.type').to.equal('String'); + expectPath(cst, '0.markup.functionArguments.0.value.value.value').to.equal('string'); + } + }); + + it('should NOT swallow the next statement when function has no trailing comma', () => { + for (const { toCST, expectPath } of testCases) { + // no comma after partial path — next line must be a separate statement, not an argument + cst = toCST(`{% liquid\n function res = 'path'\n assign x = 'hi'\n%}`); + expectPath(cst, '0.markup.0.name').to.equal('function'); + expectPath(cst, '0.markup.0.markup.functionArguments').to.have.lengthOf(0); + expectPath(cst, '0.markup.1.name').to.equal('assign'); + } + }); + + it('should reject invalid function JSON argument syntax in strict mode', () => { + const invalidCases = [ + // missing partial after = + `{% function res = %}`, + // unclosed hash in named argument value + `{% function res = 'path', config: { "key": "val" %}`, + // unclosed array in named argument value + `{% function res = 'path', items: ["a", "b" %}`, + ]; + for (const source of invalidCases) { + try { + toLiquidHtmlCST(source, { mode: 'strict' }); + expect(true, `expected ${source} to throw`).to.be.false; + } catch (e: any) { + expect(e.name, source).to.equal('LiquidHTMLParsingError'); + expect(e.loc, `expected ${source} to have location info`).not.to.be.undefined; + } + } + }); + it('should parse the graphql tag', () => { [ { @@ -2057,6 +2742,109 @@ describe('Unit: Stage 1 (CST)', () => { } }); + it('should parse multi-line return hash literal in {% liquid %} blocks', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST( + `{% liquid\n return {\n "payload": payload,\n "type": 'POST'\n }\n%}`, + ); + expectPath(cst, '0.markup.0.name').to.equal('return'); + expectPath(cst, '0.markup.0.markup.expression.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.0.markup.expression.entries').to.have.lengthOf(2); + expectPath(cst, '0.markup.0.markup.expression.entries.0.key.value').to.equal('payload'); + } + }); + + it('should parse the return tag with {{ ... }} interpolations inside a string', () => { + // This pattern is common for function partials: + // {% return "{{ arg1 }} my {{ arg2 }}" %} + // The interpolations are preserved as literal String content at parse time; + // the runtime re-renders the returned string. + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% return "{{ arg1 }} my {{ arg2 }}" %}`); + expectPath(cst, '0.name').to.equal('return'); + expectPath(cst, '0.markup.expression.type').to.equal('String'); + expectPath(cst, '0.markup.expression.value').to.equal('{{ arg1 }} my {{ arg2 }}'); + + // single interpolation with a filter + cst = toCST(`{% return "{{ name | upcase }}" %}`); + expectPath(cst, '0.markup.expression.value').to.equal('{{ name | upcase }}'); + + // hyphenated variable inside interpolation + cst = toCST(`{% return "{{ arg1 }} my {{ arg2-with-hyphens }}" %}`); + expectPath(cst, '0.markup.expression.value').to.equal( + '{{ arg1 }} my {{ arg2-with-hyphens }}', + ); + } + }); + + it('should parse the return tag with JSON array literals', () => { + for (const { toCST, expectPath } of testCases) { + // empty array + cst = toCST(`{% return [] %}`); + expectPath(cst, '0.name').to.equal('return'); + expectPath(cst, '0.markup.expression.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.expression.elements').to.have.lengthOf(0); + + // populated array + cst = toCST(`{% return ["a", "b"] %}`); + expectPath(cst, '0.markup.expression.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.expression.elements').to.have.lengthOf(2); + expectPath(cst, '0.markup.expression.elements.0.type').to.equal('String'); + expectPath(cst, '0.markup.expression.elements.0.value').to.equal('a'); + } + }); + + it('should parse the return tag with a filter applied to a hash literal', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% return { "key": "val" } | json_escape %}`); + expectPath(cst, '0.name').to.equal('return'); + expectPath(cst, '0.markup.expression.type').to.equal('JsonHashLiteral'); + expectPath(cst, '0.markup.expression.entries').to.have.lengthOf(1); + expectPath(cst, '0.markup.filters').to.have.lengthOf(1); + expectPath(cst, '0.markup.filters.0.name').to.equal('json_escape'); + } + }); + + it('should parse multi-line array return in {% liquid %} block', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% liquid\n return [\n "a",\n "b"\n ]\n%}`); + expectPath(cst, '0.markup.0.name').to.equal('return'); + expectPath(cst, '0.markup.0.markup.expression.type').to.equal('JsonArrayLiteral'); + expectPath(cst, '0.markup.0.markup.expression.elements').to.have.lengthOf(2); + expectPath(cst, '0.markup.0.markup.expression.elements.0.type').to.equal('String'); + expectPath(cst, '0.markup.0.markup.expression.elements.0.value').to.equal('a'); + } + }); + + it('should not swallow next statement after multi-line return hash in {% liquid %} block', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% liquid\n return {\n "key": val\n }\n assign x = 1\n%}`); + expectPath(cst, '0.markup.0.name').to.equal('return'); + expectPath(cst, '0.markup.0.markup.expression.type').to.equal('JsonHashLiteral'); + // the assign must be a separate statement, not swallowed into the return + expectPath(cst, '0.markup.1.name').to.equal('assign'); + expectPath(cst, '0.markup.1.markup.name').to.equal('x'); + } + }); + + it('should reject invalid return syntax in strict mode', () => { + const invalidCases = [ + // unclosed hash + `{% return { "key": "val" %}`, + // unclosed array + `{% return ["a" %}`, + ]; + for (const source of invalidCases) { + try { + toLiquidHtmlCST(source, { mode: 'strict' }); + expect(true, `expected ${source} to throw`).to.be.false; + } catch (e: any) { + expect(e.name, source).to.equal('LiquidHTMLParsingError'); + expect(e.loc, `expected ${source} to have location info`).not.to.be.undefined; + } + } + }); + it('should parse the inline background tag without a job ID (all named args)', () => { for (const { toCST, expectPath } of testCases) { cst = toCST(`{% background delay: 10, message_id: event.message_id %}`); diff --git a/packages/liquid-html-parser/src/stage-1-cst.ts b/packages/liquid-html-parser/src/stage-1-cst.ts index e380e1cc..7d11702b 100644 --- a/packages/liquid-html-parser/src/stage-1-cst.ts +++ b/packages/liquid-html-parser/src/stage-1-cst.ts @@ -78,7 +78,6 @@ export enum ConcreteNodeTypes { Condition = 'Condition', AssignMarkup = 'AssignMarkup', - AssignPushRhs = 'AssignPushRhs', HashAssignMarkup = 'HashAssignMarkup', ContentForMarkup = 'ContentForMarkup', CycleMarkup = 'CycleMarkup', @@ -415,16 +414,12 @@ export interface ConcreteLiquidTagAssign extends ConcreteLiquidTagNode< NamedTags.assign, ConcreteLiquidTagAssignMarkup > {} -export interface ConcreteLiquidTagAssignPushRhs extends ConcreteBasicNode { - pushSource: ConcreteLiquidVariable; - pushValue: ConcreteLiquidVariable; -} export interface ConcreteLiquidTagAssignMarkup extends ConcreteBasicNode { name: string; target: ConcreteLiquidVariableLookup; operator: string; - value: ConcreteLiquidVariable | ConcreteLiquidTagAssignPushRhs; + value: ConcreteLiquidVariable; } export interface ConcreteLiquidTagHashAssign extends ConcreteLiquidTagNode< @@ -1104,26 +1099,6 @@ function toCST( }, assignOperator: (node: Node) => node.sourceString, liquidAssignValue: 0, - liquidAssignPushExpr: { - type: ConcreteNodeTypes.AssignPushRhs, - // tokens: [pushSource, space*, "<<", space*, pushValue] - pushSource: 0, - pushValue: 4, - locStart, - locEnd, - source, - }, - liquidAssignPushSource: { - type: ConcreteNodeTypes.LiquidVariable, - expression: 0, - filters: 1, - // No &delim lookahead at end (unlike liquidAssignVariable), so last token is space* - rawSource: (tokens: Node[]) => - source.slice(locStart(tokens), tokens[tokens.length - 1].source.endIdx).trimEnd(), - locStart, - locEnd, - source, - }, liquidAssignVariable: { type: ConcreteNodeTypes.LiquidVariable, expression: 0, @@ -1378,6 +1353,8 @@ function toCST( liquidTagRollback: 0, renderArguments: 1, + functionRenderArguments: 1, + functionTagArguments: 0, completionModeRenderArguments: function ( _0, namedArguments, @@ -1486,6 +1463,9 @@ function toCST( tagArguments: 0, graphqlArguments: 0, graphqlRenderArguments: 1, + // Pass-through: delegates to whichever alternative matched (liquidJsonHashLiteral, + // liquidJsonArrayLiteral, or liquidExpression). Index 0 = first (and only) child. + liquidExpressionOrJsonLiteral: 0, positionalArgument: 0, namedArgument: { type: ConcreteNodeTypes.NamedArgument, @@ -1496,6 +1476,9 @@ function toCST( source, }, namedArgumentValue: 0, + // Handles nested key:value pairs within a named argument value, e.g. `filter: type: "string"`. + // Only fires when the namedArgumentValue starts with `identifier:`. + // Produces a NamedArgument so linters can inspect the nested pair. hashPairValue: { type: ConcreteNodeTypes.NamedArgument, name: 0, @@ -1570,7 +1553,9 @@ function toCST( liquidDoubleQuotedString: { type: ConcreteNodeTypes.String, single: () => false, - value: 1, + // body is an iteration node (strings now support `\X` escape pairs); use + // `.sourceString` to get the raw text between quotes. + value: (tokens: Node[]) => tokens[1].sourceString, locStart, locEnd, source, @@ -1578,7 +1563,7 @@ function toCST( liquidSingleQuotedString: { type: ConcreteNodeTypes.String, single: () => true, - value: 1, + value: (tokens: Node[]) => tokens[1].sourceString, locStart, locEnd, source, diff --git a/packages/liquid-html-parser/src/stage-2-ast.spec.ts b/packages/liquid-html-parser/src/stage-2-ast.spec.ts index fd182f24..0c988d6a 100644 --- a/packages/liquid-html-parser/src/stage-2-ast.spec.ts +++ b/packages/liquid-html-parser/src/stage-2-ast.spec.ts @@ -474,21 +474,6 @@ describe('Unit: Stage 2 (AST)', () => { expectPath(ast, 'children.0.markup.name').to.eql('x'); expectPath(ast, 'children.0.markup.lookups').to.have.lengthOf(0); - // explicit push form: assign a = source << value - ast = toAST(`{% assign x = roles << 'admin' %}`); - expectPath(ast, 'children.0.markup.operator').to.eql('='); - expectPath(ast, 'children.0.markup.name').to.eql('x'); - expectPath(ast, 'children.0.markup.value.type').to.eql('AssignPushRhs'); - expectPath(ast, 'children.0.markup.value.pushSource.expression.name').to.eql('roles'); - expectPath(ast, 'children.0.markup.value.pushValue.expression.value').to.eql('admin'); - - // explicit push with dot notation source (e.g. assign x = current_profile.roles << 'auth') - ast = toAST(`{% assign x = current_profile.roles << 'authenticated' %}`); - expectPath(ast, 'children.0.markup.value.type').to.eql('AssignPushRhs'); - expectPath(ast, 'children.0.markup.value.pushSource.expression.name').to.eql( - 'current_profile', - ); - // simple assign backward compatibility ast = toAST(`{% assign x = 'hi' %}`); expectPath(ast, 'children.0.markup.name').to.eql('x'); @@ -897,6 +882,40 @@ describe('Unit: Stage 2 (AST)', () => { } }); + it('should parse function tags with JSON literals as named argument values', () => { + for (const { toAST, expectPath } of testCases) { + // empty array and empty hash as named argument values + ast = toAST(`{% function result = 'my/function', array: [], hash: {} %}`); + expectPath(ast, 'children.0.markup.args').to.have.lengthOf(2); + expectPath(ast, 'children.0.markup.args.0.type').to.equal('NamedArgument'); + expectPath(ast, 'children.0.markup.args.0.name').to.equal('array'); + expectPath(ast, 'children.0.markup.args.0.value.type').to.equal('JsonArrayLiteral'); + expectPath(ast, 'children.0.markup.args.0.value.elements').to.have.lengthOf(0); + expectPath(ast, 'children.0.markup.args.1.name').to.equal('hash'); + expectPath(ast, 'children.0.markup.args.1.value.type').to.equal('JsonHashLiteral'); + expectPath(ast, 'children.0.markup.args.1.value.entries').to.have.lengthOf(0); + + // populated array as named argument value + ast = toAST(`{% function result = 'my/function', items: ["a", "b"] %}`); + expectPath(ast, 'children.0.markup.args').to.have.lengthOf(1); + expectPath(ast, 'children.0.markup.args.0.name').to.equal('items'); + expectPath(ast, 'children.0.markup.args.0.value.type').to.equal('JsonArrayLiteral'); + expectPath(ast, 'children.0.markup.args.0.value.elements').to.have.lengthOf(2); + expectPath(ast, 'children.0.markup.args.0.value.elements.0.type').to.equal('String'); + + // populated hash as named argument value — bare key is a VariableLookup + ast = toAST(`{% function result = 'my/function', config: { key: "val" } %}`); + expectPath(ast, 'children.0.markup.args').to.have.lengthOf(1); + expectPath(ast, 'children.0.markup.args.0.name').to.equal('config'); + expectPath(ast, 'children.0.markup.args.0.value.type').to.equal('JsonHashLiteral'); + expectPath(ast, 'children.0.markup.args.0.value.entries').to.have.lengthOf(1); + expectPath(ast, 'children.0.markup.args.0.value.entries.0.key.type').to.equal( + 'VariableLookup', + ); + expectPath(ast, 'children.0.markup.args.0.value.entries.0.key.name').to.equal('key'); + } + }); + it('should parse graphql tags', () => { [ { @@ -1436,6 +1455,43 @@ describe('Unit: Stage 2 (AST)', () => { } }); + it('should parse the return tag with a JSON array literal', () => { + for (const { toAST, expectPath, expectPosition } of testCases) { + // empty array + ast = toAST(`{% return [] %}`); + expectPath(ast, 'children.0.type').to.equal('LiquidTag'); + expectPath(ast, 'children.0.name').to.equal('return'); + expectPath(ast, 'children.0.markup.type').to.equal('LiquidVariable'); + expectPath(ast, 'children.0.markup.expression.type').to.equal('JsonArrayLiteral'); + expectPath(ast, 'children.0.markup.expression.elements').to.have.lengthOf(0); + expectPosition(ast, 'children.0'); + + // populated array + ast = toAST(`{% return ["a", "b"] %}`); + expectPath(ast, 'children.0.markup.expression.type').to.equal('JsonArrayLiteral'); + expectPath(ast, 'children.0.markup.expression.elements').to.have.lengthOf(2); + expectPath(ast, 'children.0.markup.expression.elements.0.type').to.equal('String'); + expectPath(ast, 'children.0.markup.expression.elements.0.value').to.equal('a'); + } + }); + + it('should parse return in {% liquid %} block with multi-line hash and correct position', () => { + for (const { toAST, expectPath, expectPosition } of testCases) { + ast = toAST( + `{% liquid\n return {\n "payload": payload,\n "type": 'POST'\n }\n%}`, + ); + expectPath(ast, 'children.0.type').to.equal('LiquidTag'); + expectPath(ast, 'children.0.name').to.equal('liquid'); + expectPath(ast, 'children.0.markup.0.name').to.equal('return'); + expectPath(ast, 'children.0.markup.0.markup.expression.type').to.equal('JsonHashLiteral'); + expectPath(ast, 'children.0.markup.0.markup.expression.entries').to.have.lengthOf(2); + expectPath(ast, 'children.0.markup.0.markup.expression.entries.0.key.value').to.equal( + 'payload', + ); + expectPosition(ast, 'children.0'); + } + }); + it('should parse the yield tag', () => { for (const { toAST, expectPath, expectPosition } of testCases) { ast = toAST(`{% yield 'name' %}`); diff --git a/packages/liquid-html-parser/src/stage-2-ast.ts b/packages/liquid-html-parser/src/stage-2-ast.ts index 2881c0a0..01f1a916 100644 --- a/packages/liquid-html-parser/src/stage-2-ast.ts +++ b/packages/liquid-html-parser/src/stage-2-ast.ts @@ -58,7 +58,6 @@ import { ConcreteLiquidTagNamed, ConcreteLiquidTag, ConcreteLiquidTagAssignMarkup, - ConcreteLiquidTagAssignPushRhs, ConcreteLiquidTagRenderMarkup, ConcreteLiquidTagFunctionMarkup, ConcreteLiquidTagGraphQLMarkup, @@ -118,7 +117,6 @@ export type LiquidHtmlNode = | LiquidFilter | LiquidNamedArgument | AssignMarkup - | AssignPushRhs | HashAssignMarkup | ContentForMarkup | CycleMarkup @@ -325,16 +323,8 @@ export interface AssignMarkup extends ASTNode { /** '=' for assignment, '<<' for array append */ operator: '=' | '<<'; - /** the value of the variable that is being assigned, or a push expression */ - value: LiquidVariable | AssignPushRhs; -} - -/** The RHS of an explicit push assign: `assign a = source << value` */ -export interface AssignPushRhs extends ASTNode { - /** the source array to push into */ - pushSource: LiquidVariable; - /** the value being pushed */ - pushValue: LiquidVariable; + /** the value of the variable that is being assigned */ + value: LiquidVariable; } export interface LiquidTagHashAssign extends LiquidTagNode< @@ -2159,26 +2149,12 @@ function toUnnamedLiquidBranch(parentNode: LiquidHtmlNode): LiquidBranchUnnamed } function toAssignMarkup(node: ConcreteLiquidTagAssignMarkup): AssignMarkup { - const value = - node.value.type === ConcreteNodeTypes.AssignPushRhs - ? toAssignPushRhs(node.value) - : toLiquidVariable(node.value); return { type: NodeTypes.AssignMarkup, name: node.name, lookups: node.target.lookups.map(toExpression), operator: node.operator as '=' | '<<', - value, - position: position(node), - source: node.source, - }; -} - -function toAssignPushRhs(node: ConcreteLiquidTagAssignPushRhs): AssignPushRhs { - return { - type: NodeTypes.AssignPushRhs, - pushSource: toLiquidVariable(node.pushSource), - pushValue: toLiquidVariable(node.pushValue), + value: toLiquidVariable(node.value), position: position(node), source: node.source, }; diff --git a/packages/liquid-html-parser/src/types.ts b/packages/liquid-html-parser/src/types.ts index 38ec06a4..6c5ab746 100644 --- a/packages/liquid-html-parser/src/types.ts +++ b/packages/liquid-html-parser/src/types.ts @@ -39,7 +39,6 @@ export enum NodeTypes { LogicalExpression = 'LogicalExpression', AssignMarkup = 'AssignMarkup', - AssignPushRhs = 'AssignPushRhs', HashAssignMarkup = 'HashAssignMarkup', ContentForMarkup = 'ContentForMarkup', CycleMarkup = 'CycleMarkup', 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 new file mode 100644 index 00000000..9ba876de --- /dev/null +++ b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidAssignSyntax.spec.ts @@ -0,0 +1,422 @@ +import { describe, it, expect } from 'vitest'; +import { runLiquidCheck } from '../../../test'; +import { LiquidHTMLSyntaxError } from '../index'; + +describe('detectInvalidAssignSyntax', () => { + describe('structurally-broken assign tags', () => { + const brokenCases: Array<[string, string]> = [ + ['missing `=` with quoted value', `{% assign x "var" %}`], + ['missing `=` with bare identifier', `{% assign x abc %}`], + ['missing target', `{% assign = 'val' %}`], + ['completely empty', `{% assign %}`], + ['target only, no operator', `{% assign x %}`], + ['empty RHS after `=`', `{% assign x = %}`], + ]; + + for (const [label, sourceCode] of brokenCases) { + it(`should report: ${label} — ${sourceCode}`, async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); + expect(syntaxOffenses).toHaveLength(1); + expect(syntaxOffenses[0].message).toContain("Invalid syntax for tag 'assign'"); + }); + } + }); + + describe('invalid targets', () => { + // Literal delimiters at the start of the target are never a valid assign target. + // Digit-starting names (e.g. `23_hours_ago`) are accepted by the platformOS runtime + // even though our parser's `variableSegment` grammar rule falls back on them — + // to avoid false positives, we do not flag digit-starting targets here. + const invalidTargetCases: Array<[string, string]> = [ + ['target is a single-quoted string', `{% assign 'str' = 'v' %}`], + ['target is a double-quoted string', `{% assign "str" = 'v' %}`], + ['target is an array literal', `{% assign [] = 'v' %}`], + ['target is a hash literal', `{% assign {} = 'v' %}`], + ]; + + for (const [label, sourceCode] of invalidTargetCases) { + 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); + }); + } + + it('should NOT flag a valid dotted target (parser accepts it)', async () => { + const sourceCode = `{% assign foo.bar = 'v' %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('should NOT flag a valid indexed target', async () => { + const sourceCode = `{% assign foo[0] = 'v' %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('should NOT flag a digit-starting target (valid per platformOS runtime)', async () => { + // Liquify accepts `23_hours_ago` as a valid name; our parser falls back to base + // case but this check must not over-report vs. runtime. + const sourceCode = `{% assign 23_hours_ago = 'some value' %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => + o.message.includes("Invalid syntax for tag 'assign'"), + ); + expect(syntaxOffenses).toHaveLength(0); + }); + }); + + describe('operator variants', () => { + it('should report `:=` (not a recognized operator) as an assign syntax error', async () => { + const sourceCode = `{% assign x := 'v' %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => + o.message.includes("Invalid syntax for tag 'assign'"), + ); + expect(syntaxOffenses).toHaveLength(1); + }); + + it('should have some offense for `==` (MultipleAssignValues handles it)', async () => { + const sourceCode = `{% assign x == 'v' %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses.length).toBeGreaterThan(0); + }); + + it('should have some offense for `=+`', async () => { + const sourceCode = `{% assign x =+ 'v' %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses.length).toBeGreaterThan(0); + }); + }); + + describe('valid syntax — should NOT report', () => { + const validCases: Array<[string, string]> = [ + // primitives + ['single-quoted string', `{% assign x = 'str' %}`], + ['double-quoted string', `{% assign x = "str" %}`], + ['integer', `{% assign x = 42 %}`], + ['negative float', `{% assign x = -1.5 %}`], + ['true', `{% assign x = true %}`], + ['false', `{% assign x = false %}`], + ['nil', `{% assign x = nil %}`], + ['null', `{% assign x = null %}`], + ['blank', `{% assign x = blank %}`], + ['empty', `{% assign x = empty %}`], + ['range', `{% assign x = (1..10) %}`], + + // lookups + ['variable', `{% assign x = other %}`], + ['dot lookup', `{% assign x = other.prop %}`], + ['string index lookup', `{% assign x = other["prop"] %}`], + ['variable index lookup', `{% assign x = other[key] %}`], + ['deep dot chain', `{% assign x = a.b.c.d %}`], + + // filters + ['single filter', `{% assign x = y | upcase %}`], + ['filter with arg', `{% assign x = y | default: 'z' %}`], + ['chained filters', `{% assign x = y | default: 'z' | upcase %}`], + ['filter with positional args', `{% assign x = y | append: "a" %}`], + ['filter with JSON array arg', `{% assign x = y | concat: [1,2] %}`], + ['filter with JSON hash arg', `{% assign x = y | merge: {a:1} %}`], + ['chained filter after JSON arg', `{% assign x = y | default: [] | join: "," %}`], + + // JSON hash + ['empty hash', `{% assign x = {} %}`], + ['hash with string key', `{% assign x = { "a": 1 } %}`], + ['hash with bare key', `{% assign x = { a: 1 } %}`], + ['hash with multiple string keys', `{% assign x = { "a": 1, "b": 2 } %}`], + ['hash with mixed keys', `{% assign x = { "a": 1, b: 2 } %}`], + ['hash with nested hash', `{% assign x = { "a": { "nested": true } } %}`], + ['hash with nested array', `{% assign x = { "a": [1,2,3] } %}`], + [ + 'hash with string-interpolated value', + `{% assign x = { "email": "{{ email | downcase }}" } %}`, + ], + + // JSON array + ['empty array', `{% assign x = [] %}`], + ['array of numbers', `{% assign x = [1, 2, 3] %}`], + ['array of strings', `{% assign x = ["a", "b"] %}`], + ['array of literals', `{% assign x = [true, false, nil] %}`], + ['array of variables', `{% assign x = [a, b, c] %}`], + ['nested arrays', `{% assign x = [[1,2],[3,4]] %}`], + ['array of hashes', `{% assign x = [{"a":1}, {"b":2}] %}`], + + // push syntax — only the bare form `a << value` is valid; `a = b << c` + // is a compound operator and is NOT accepted by the platformOS runtime. + ['bare push with string value', `{% assign my_val << "item" %}`], + ['bare push with variable value', `{% assign my_val << val %}`], + ['bare push with filter', `{% assign my_val << val | upcase %}`], + + // whitespace-strip + ['both trim', `{%- assign x = 'v' -%}`], + ['both trim with hash', `{%- assign x = {} -%}`], + ['left trim with array', `{%- assign x = [] %}`], + ['right trim only', `{% assign x = 'v' -%}`], + + // identifiers + ['underscore prefix', `{% assign _private = 1 %}`], + ['snake_case', `{% assign snake_case = 1 %}`], + ['hyphen in identifier', `{% assign my-var = "hello" %}`], + ['multiple hyphens', `{% assign my-complex-var-name = "test" %}`], + ['digit in middle', `{% assign x1 = 1 %}`], + ]; + + for (const [label, sourceCode] of validCases) { + it(`should not report: ${label} — ${sourceCode}`, async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + } + }); + + describe('interaction with other sub-checks', () => { + it('should not fire when MultipleAssignValues already reports trailing garbage', async () => { + const sourceCode = `{% assign foo = '123' 555 text %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); + expect(syntaxOffenses).toHaveLength(0); + const supportOffenses = offenses.filter((o) => o.message === 'Syntax is not supported'); + expect(supportOffenses).toHaveLength(1); + }); + + it('should not fire on assign with invalid filter name', async () => { + const sourceCode = `{% assign x = "v" | upcase@ %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => + o.message.includes("Invalid syntax for tag 'assign'"), + ); + expect(syntaxOffenses).toHaveLength(0); + }); + + it('should not fire on assign with pipe syntax issue', async () => { + const sourceCode = `{% assign x = "v" || upcase %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => + o.message.includes("Invalid syntax for tag 'assign'"), + ); + expect(syntaxOffenses).toHaveLength(0); + }); + + it('should not fire for non-assign tags', async () => { + const sourceCode = `{% echo x %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => + o.message.includes("Invalid syntax for tag 'assign'"), + ); + expect(syntaxOffenses).toHaveLength(0); + }); + }); + + describe('inside {% liquid %} blocks', () => { + it('should accept a simple assign statement', async () => { + const sourceCode = `{% liquid + assign x = 1 +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('should accept a multi-line hash literal', async () => { + const sourceCode = `{% liquid + assign h = { + "a": 1, + "b": 2 + } +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('should accept a multi-line array literal', async () => { + const sourceCode = `{% liquid + assign a = [ + "a", + "b" + ] +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('should accept nested multi-line hash', async () => { + const sourceCode = `{% liquid + assign h = { + outer: { + inner: "v" + } + } +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('should not let an empty hash swallow the next statement', async () => { + const sourceCode = `{% liquid + assign h = {} + assign a = 'x' +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('should parse subsequent tag cleanly after array', async () => { + const sourceCode = `{% liquid + assign a = [1] + render 'p' +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('should report target-only assign', async () => { + const sourceCode = `{% liquid + assign x +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => + o.message.includes("Invalid syntax for tag 'assign'"), + ); + expect(syntaxOffenses).toHaveLength(1); + }); + + it('should report assign with empty RHS', async () => { + const sourceCode = `{% liquid + assign x = +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => + o.message.includes("Invalid syntax for tag 'assign'"), + ); + expect(syntaxOffenses).toHaveLength(1); + }); + + it('should report assign without `=`', async () => { + const sourceCode = `{% liquid + assign x "v" +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => + o.message.includes("Invalid syntax for tag 'assign'"), + ); + expect(syntaxOffenses).toHaveLength(1); + }); + + it('should report garbage between hash entries as a parse error', async () => { + const sourceCode = `{% liquid + assign x = { + "a": 1 + extra garbage + } +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses.length).toBeGreaterThan(0); + }); + + it('should report an unclosed hash before `%}` in tolerant mode', async () => { + const sourceCode = `{% liquid + assign x = { "a": 1 +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses.length).toBeGreaterThan(0); + }); + }); + + describe('whitespace-trimming delimiters', () => { + it('should report with trim delimiters', async () => { + const sourceCode = `{%- assign x "var" -%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => + o.message.includes("Invalid syntax for tag 'assign'"), + ); + expect(syntaxOffenses).toHaveLength(1); + }); + }); + + // These assertions mirror scenarios from the platformOS runtime test suite + // (desksnearme/test/lib/liquify/tags/assign_tag_test.rb) to make sure our + // lint-level acceptance aligns with runtime acceptance for common patterns. + describe('runtime-alignment scenarios', () => { + it('accepts JSON literal as default filter argument (matches Liquify behavior)', async () => { + const sourceCode = `{% assign my_val = null | default: [] %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('accepts empty hash as default filter argument', async () => { + const sourceCode = `{% assign my_hash = null | default: {} %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('accepts hash_merge with two hash arguments', async () => { + const sourceCode = `{% assign merged = { "a": 1 } | hash_merge: h2 | hash_merge: h3 %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('accepts JSON array with mixed primitive types', async () => { + const sourceCode = `{% assign arr = ["string", 42, true, null, { "nested": "object" }] %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('accepts JSON hash with escaped double-quote in string value', async () => { + const sourceCode = `{% assign data = { "quote": "He said \\"hello\\"" } %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('accepts escaped backslash in double-quoted string', async () => { + const sourceCode = `{% assign x = "path\\\\to\\\\file" %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('accepts escaped single-quote in single-quoted string', async () => { + const sourceCode = `{% assign x = 'can\\'t' %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('accepts deeply nested JSON structures', async () => { + const sourceCode = `{% assign data = { + "level1": { + "level2": { + "level3": { + "level4": { + "value": "deep", + "array": [1, 2, 3] + } + } + } + } + } %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('accepts JSON array of hashes then map/join filters', async () => { + const sourceCode = `{% assign names = users | map: "name" | join: ", " %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('accepts push syntax — `{% assign v << "x" %}`', async () => { + const sourceCode = `{% assign my_val << "item" %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses).toHaveLength(0); + }); + + it('rejects compound operator — `{% assign a = b << c %}` is invalid', async () => { + // Per platformOS runtime: an assign uses exactly one of `=` or `<<`, never both. + // Our parser falls back to base case; check should flag via MAV or IAS. + const sourceCode = `{% assign a = b << c %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + expect(offenses.length).toBeGreaterThan(0); + }); + }); +}); 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 new file mode 100644 index 00000000..e353d4c3 --- /dev/null +++ b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidAssignSyntax.ts @@ -0,0 +1,63 @@ +import { LiquidTag } from '@platformos/liquid-html-parser'; +import { Problem, SourceCodeType } from '../../..'; + +/** + * Detects structurally-invalid `assign` tags that neither `MultipleAssignValues` nor + * `InvalidPipeSyntax`/`InvalidFilterName` catch: + * + * {% assign %} — empty markup + * {% assign x %} — target only, no operator + * {% assign x "var" %} — missing `=` + * {% assign = 'v' %} — missing target + * {% assign x = %} — empty RHS + * {% assign 'str' = 'v' %} — target is a literal + * {% assign x := 'v' %} — operator is not `=` + * + * When the strict grammar rule `liquidTagAssignMarkup` fails, the parser falls back to + * the base case and stores markup as a raw string. That string almost always still + * contains `=`-plus-RHS with a filter or pipe issue (handled elsewhere). This check + * targets the cases where the `target = value` skeleton itself is broken, so it + * complements rather than duplicates the other sub-checks. + */ +export function detectInvalidAssignSyntax( + node: LiquidTag, +): Problem | undefined { + if (node.name !== 'assign') return; + if (typeof node.markup !== 'string') return; + + const markup = node.markup.trim(); + + const eqIndex = markup.indexOf('='); + const hasEquals = eqIndex !== -1; + const lhs = hasEquals ? markup.slice(0, eqIndex).trim() : markup; + const rhs = hasEquals ? markup.slice(eqIndex + 1).trim() : ''; + + const isStructurallyBroken = + markup === '' || !hasEquals || lhs === '' || rhs === '' || !isValidAssignTarget(lhs); + + if (!isStructurallyBroken) return; + + return { + message: `Invalid syntax for tag 'assign'. Expected syntax: {% assign = %}`, + startIndex: node.position.start, + endIndex: node.position.end, + }; +} + +/** + * Rejects an LHS that is obviously not an assign target. + * + * NOTE: the parser's `variableSegment` rule is stricter than the platformOS runtime — + * Liquify (see assign_tag_test.rb "allow variable names to start with digit") accepts + * `23_hours_ago` as a valid name, but our grammar requires `(letter | "_")` at the + * start and falls back to the base case for digit-starting names. To avoid + * false-positive lint errors on code that runs fine, this shape check only rejects + * LHS forms that are never valid: literal delimiters at the start (`'`, `"`, `[`, `{`) + * and stray operator characters (`:` or a second `=`) that indicate the operator + * itself is malformed (e.g. `:=`). + */ +function isValidAssignTarget(lhs: string): boolean { + if (/^['"[{]/.test(lhs)) return false; + if (/[:=]/.test(lhs)) return false; + return true; +} diff --git a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidOutputPush.spec.ts b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidOutputPush.spec.ts new file mode 100644 index 00000000..981b3761 --- /dev/null +++ b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidOutputPush.spec.ts @@ -0,0 +1,104 @@ +import { describe, it, expect } from 'vitest'; +import { runLiquidCheck } from '../../../test'; +import { LiquidHTMLSyntaxError } from '../index'; + +// Runtime-aligned via pos-cli sync against a live staging instance: +// {{ arr << "el" }} → sync rejects (parse error in output position) +// {% echo arr << "el" %} → same +// {% assign arr << "el" %} → sync accepts (only valid position for `<<`) + +describe('detectInvalidOutputPush', () => { + describe('{{ }} output position', () => { + const invalidCases: Array<[string, string]> = [ + ['string value', `{{ arr << "el" }}`], + ['single-quoted value', `{{ arr << 'el' }}`], + ['variable value', `{{ arr << other }}`], + ['with filter chain', `{{ arr << "el" | upcase }}`], + ['whitespace-trim delimiters', `{{- arr << "el" -}}`], + ]; + + for (const [label, sourceCode] of invalidCases) { + it(`should report: ${label} — ${sourceCode}`, async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const pushOffenses = offenses.filter((o) => o.message.includes("'<<' (push) operator")); + expect(pushOffenses).toHaveLength(1); + }); + } + + it('should not fire on plain output without `<<`', async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, `{{ arr }}`); + expect(offenses).toHaveLength(0); + }); + + it('should not fire on output with filters', async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, `{{ arr | upcase }}`); + expect(offenses).toHaveLength(0); + }); + + it('should not fire when `<<` is inside a quoted string', async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, `{{ "a << b" }}`); + expect(offenses).toHaveLength(0); + }); + + it('should not fire when `<<` is inside a single-quoted string', async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, `{{ 'a << b' }}`); + expect(offenses).toHaveLength(0); + }); + + it('should not fire when `<<` appears only inside a filter-argument string', async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, `{{ a | append: " << " }}`); + expect(offenses).toHaveLength(0); + }); + + it('should suppress the generic InvalidEchoValue duplicate', async () => { + // Without the dedicated check, InvalidEchoValue would also report "Syntax is not supported". + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, `{{ arr << "el" }}`); + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toContain("'<<' (push) operator"); + }); + }); + + describe('{% echo %} tag', () => { + it('should report push in echo tag', async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, `{% echo arr << "el" %}`); + const pushOffenses = offenses.filter((o) => o.message.includes("'<<' (push) operator")); + expect(pushOffenses).toHaveLength(1); + }); + + it('should not fire on valid echo', async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, `{% echo arr %}`); + expect(offenses).toHaveLength(0); + }); + }); + + describe('assign tag — `<<` remains valid here', () => { + it('should NOT fire on bare push in assign', async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, `{% assign arr << "el" %}`); + const pushOffenses = offenses.filter((o) => o.message.includes("'<<' (push) operator")); + expect(pushOffenses).toHaveLength(0); + }); + + it('should NOT fire on bare push with variable', async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, `{% assign arr << v %}`); + const pushOffenses = offenses.filter((o) => o.message.includes("'<<' (push) operator")); + expect(pushOffenses).toHaveLength(0); + }); + + it('should NOT fire on bare push with filter', async () => { + const offenses = await runLiquidCheck( + LiquidHTMLSyntaxError, + `{% assign arr << "el" | upcase %}`, + ); + const pushOffenses = offenses.filter((o) => o.message.includes("'<<' (push) operator")); + expect(pushOffenses).toHaveLength(0); + }); + }); + + describe('other tags', () => { + it('should not fire for non-echo tags', async () => { + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, `{% render 'p' %}`); + const pushOffenses = offenses.filter((o) => o.message.includes("'<<' (push) operator")); + expect(pushOffenses).toHaveLength(0); + }); + }); +}); diff --git a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidOutputPush.ts b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidOutputPush.ts new file mode 100644 index 00000000..98770198 --- /dev/null +++ b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidOutputPush.ts @@ -0,0 +1,39 @@ +import { LiquidTag, LiquidVariableOutput, NodeTypes } from '@platformos/liquid-html-parser'; +import { Problem, SourceCodeType } from '../../..'; + +const PUSH_OPERATOR_MESSAGE = + "The '<<' (push) operator is only valid inside '{% assign target << value %}'. Remove it or move the expression into an assign tag."; + +/** + * Detects misuse of the `<<` (array push) operator inside output positions: + * + * {{ arr << "el" }} — invalid + * {% echo arr << "el" %} — invalid + * + * `<<` is only accepted as the top-level operator in an assign tag: + * + * {% assign arr << "el" %} — valid (pushes "el" onto arr) + * + * Runtime rejects output-position push with a hard syntax error; this check + * mirrors that with a clearer, actionable message than the generic + * `InvalidEchoValue` fallback ("Syntax is not supported"). + */ +export function detectInvalidOutputPush( + node: LiquidTag | LiquidVariableOutput, +): Problem | undefined { + if (node.type === NodeTypes.LiquidTag && node.name !== 'echo') return; + + const markup = node.markup; + if (typeof markup !== 'string' || !markup) return; + + // Strip quoted strings so literal `<<` inside a string doesn't trigger the check + // (e.g. `{{ "a << b" }}` is a harmless string with no push operator). + const stripped = markup.replace(/'[^']*'|"[^"]*"/g, ''); + if (!/< { const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); expect(syntaxOffenses).toHaveLength(0); }); + + it('should not report valid function with JSON array argument', async () => { + const sourceCode = `{% function res = 'path/to/function', items: ["a", "b"] %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); + expect(syntaxOffenses).toHaveLength(0); + }); + + it('should not report valid function with JSON hash argument', async () => { + const sourceCode = `{% function res = 'path/to/function', config: { key: "val" } %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); + expect(syntaxOffenses).toHaveLength(0); + }); + + it('should not report valid multi-line function with JSON arguments inside liquid block', async () => { + const sourceCode = `{% liquid + function res = 'path/to/function', + array: [], + hash: {} +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); + expect(syntaxOffenses).toHaveLength(0); + }); + + it('should report function with missing partial after =', async () => { + const sourceCode = `{% function res = %}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); + expect(syntaxOffenses).toHaveLength(1); + expect(syntaxOffenses[0].message).toContain("Invalid syntax for tag 'function'"); + }); + + it('should not report valid function with nested multi-line hash argument', async () => { + const sourceCode = `{% liquid + function res = 'path/to/function', + config: { + key: "val" + } +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); + expect(syntaxOffenses).toHaveLength(0); + }); }); describe('graphql tag', () => { @@ -166,6 +211,43 @@ describe('Module: InvalidTagSyntax', () => { const sourceCode = `{% liquid render 'partial' function res = 'path/to/function' +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); + expect(syntaxOffenses).toHaveLength(0); + }); + + it('should not report valid multi-line return with JSON hash in liquid block', async () => { + const sourceCode = `{% liquid + return { + "key": val + } +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); + expect(syntaxOffenses).toHaveLength(0); + }); + + it('should not report valid multi-line return with JSON array in liquid block', async () => { + const sourceCode = `{% liquid + return [ + "a", + "b" + ] +%}`; + const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); + const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); + expect(syntaxOffenses).toHaveLength(0); + }); + + it('should not report valid multi-line assign with JSON array followed by render', async () => { + // assign is excluded from this check (has dedicated sub-checks), but parsing + // the multi-line liquid block must not corrupt subsequent tags + const sourceCode = `{% liquid + assign x = [ + "a" + ] + render 'partial' %}`; const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); @@ -174,8 +256,10 @@ describe('Module: InvalidTagSyntax', () => { }); describe('should NOT fire on tags with dedicated sub-checks', () => { - it('should not fire InvalidTagSyntax on assign (has MultipleAssignValues)', async () => { - const sourceCode = `{% assign x abc %}`; + it('should not fire InvalidTagSyntax on assign when MultipleAssignValues fires', async () => { + // MultipleAssignValues reports "Syntax is not supported"; InvalidTagSyntax is suppressed + // by the `problems.length === 0` guard in the parent pipeline. + const sourceCode = `{% assign foo = '123' 555 text %}`; const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); const syntaxOffenses = offenses.filter((o) => o.message.includes('Invalid syntax for tag')); expect(syntaxOffenses).toHaveLength(0); diff --git a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidTagSyntax.ts b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidTagSyntax.ts index 0b763b04..31fba68c 100644 --- a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidTagSyntax.ts +++ b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidTagSyntax.ts @@ -12,7 +12,7 @@ const TAGS_WITH_NO_EXPECTED_MARKUP = new Set(TAGS_WITHOUT_MARKUP); * more specific error messages and autofixes. This check should NOT fire on these * to avoid double-reporting or overriding their nuanced decisions. * - * - assign → MultipleAssignValues, InvalidFilterName, InvalidPipeSyntax + * - assign → MultipleAssignValues, InvalidAssignSyntax, InvalidFilterName, InvalidPipeSyntax * - echo → InvalidEchoValue, InvalidFilterName, InvalidPipeSyntax * - if/elsif/unless → InvalidConditionalNode, InvalidConditionalNodeParenthesis * - for/tablerow → InvalidLoopRange, InvalidLoopArguments 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 105a15ce..7eb27d1a 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,6 +1,7 @@ import { Severity, SourceCodeType, LiquidCheckDefinition, Problem } from '../../types'; import { getOffset, isError } from '../../utils'; import { detectMultipleAssignValues } from './checks/MultipleAssignValues'; +import { detectInvalidAssignSyntax } from './checks/InvalidAssignSyntax'; import { detectInvalidBooleanExpressions } from './checks/InvalidBooleanExpressions'; import { detectInvalidEchoValue } from './checks/InvalidEchoValue'; import { detectInvalidConditionalNode } from './checks/InvalidConditionalNode'; @@ -11,6 +12,7 @@ import { detectInvalidFilterName } from './checks/InvalidFilterName'; import { detectInvalidPipeSyntax } from './checks/InvalidPipeSyntax'; import { detectUnknownTag } from './checks/UnknownTag'; import { detectInvalidTagSyntax } from './checks/InvalidTagSyntax'; +import { detectInvalidOutputPush } from './checks/InvalidOutputPush'; import { isWithinRawTagThatDoesNotParseItsContents } from '../utils'; type LineColPosition = { @@ -75,9 +77,18 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = { return; } + // Push-operator misuse in `{% echo … << … %}` — dedicated message. + // Short-circuit so InvalidEchoValue doesn't duplicate with a generic message. + const outputPushProblem = detectInvalidOutputPush(node); + if (outputPushProblem) { + context.report(outputPushProblem); + return; + } + // Run specific sub-checks first — they provide better error messages and autofixes. const problems = [ detectMultipleAssignValues(node), + detectInvalidAssignSyntax(node), detectInvalidEchoValue(node), detectInvalidLoopRange(node), detectInvalidLoopArguments(node, tags), @@ -129,6 +140,14 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = { async LiquidVariableOutput(node, ancestors) { if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return; + // Push-operator misuse in `{{ … << … }}` — dedicated message. + // Short-circuit so InvalidEchoValue doesn't duplicate with a generic message. + const outputPushProblem = detectInvalidOutputPush(node); + if (outputPushProblem) { + context.report(outputPushProblem); + return; + } + const filterProblems = await detectInvalidFilterName(node, (await filtersPromise) ?? []); if (filterProblems.length > 0) { filterProblems.forEach((problem) => context.report(problem)); diff --git a/packages/platformos-check-common/src/checks/undefined-object/index.spec.ts b/packages/platformos-check-common/src/checks/undefined-object/index.spec.ts index 72d5108b..22a94903 100644 --- a/packages/platformos-check-common/src/checks/undefined-object/index.spec.ts +++ b/packages/platformos-check-common/src/checks/undefined-object/index.spec.ts @@ -700,4 +700,105 @@ describe('Module: UndefinedObject', () => { expect(offenses).toHaveLength(1); expect(offenses[0].message).toBe("Unknown object 'error' used."); }); + + describe('JSON literals in assign tag', () => { + it('should report undefined bare key AND bare value in a hash literal', async () => { + // bare keys and bare values are both VariableLookups in platformOS semantics — + // they are evaluated at runtime, so both are subject to undefined-object checks + const sourceCode = ` + {% doc %} + {% enddoc %} + {% assign hash = { bare_key: bare_val } %} + `; + + const offenses = await runLiquidCheck(UndefinedObject, sourceCode); + + expect(offenses.map((o) => o.message).sort()).toEqual([ + "Unknown object 'bare_key' used.", + "Unknown object 'bare_val' used.", + ]); + }); + + it('should NOT report string keys in a hash literal (they are literals, not variables)', async () => { + const sourceCode = ` + {% doc %} + {% enddoc %} + {% assign hash = { "key": "val" } %} + `; + + const offenses = await runLiquidCheck(UndefinedObject, sourceCode); + + expect(offenses).toHaveLength(0); + }); + + it('should report undefined bare value under a string key', async () => { + const sourceCode = ` + {% doc %} + {% enddoc %} + {% assign hash = { "key": undefined_var } %} + `; + + const offenses = await runLiquidCheck(UndefinedObject, sourceCode); + + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toBe("Unknown object 'undefined_var' used."); + }); + + it('should report undefined elements in an array literal', async () => { + const sourceCode = ` + {% doc %} + {% enddoc %} + {% assign arr = [a, undefined_var] %} + `; + + const offenses = await runLiquidCheck(UndefinedObject, sourceCode); + + expect(offenses.map((o) => o.message).sort()).toEqual([ + "Unknown object 'a' used.", + "Unknown object 'undefined_var' used.", + ]); + }); + + it('should NOT report string elements in an array literal', async () => { + const sourceCode = ` + {% doc %} + {% enddoc %} + {% assign arr = ["a", "b", "c"] %} + `; + + const offenses = await runLiquidCheck(UndefinedObject, sourceCode); + + expect(offenses).toHaveLength(0); + }); + + it('should NOT report bare keys/values when they refer to defined variables', async () => { + const sourceCode = ` + {% doc %} + {% enddoc %} + {% assign bare_key = "k" %} + {% assign bare_val = "v" %} + {% assign hash = { bare_key: bare_val } %} + `; + + const offenses = await runLiquidCheck(UndefinedObject, sourceCode); + + expect(offenses).toHaveLength(0); + }); + + it('should report undefined bare keys/values in nested hash literals', async () => { + const sourceCode = ` + {% doc %} + {% enddoc %} + {% assign hash = { outer: { inner: value } } %} + `; + + const offenses = await runLiquidCheck(UndefinedObject, sourceCode); + + expect(offenses.map((o) => o.message).sort()).toEqual([ + "Unknown object 'inner' used.", + "Unknown object 'outer' used.", + "Unknown object 'value' used.", + ]); + }); + }); }); diff --git a/packages/platformos-language-server-common/src/TypeSystem.spec.ts b/packages/platformos-language-server-common/src/TypeSystem.spec.ts index 140e2255..819dc629 100644 --- a/packages/platformos-language-server-common/src/TypeSystem.spec.ts +++ b/packages/platformos-language-server-common/src/TypeSystem.spec.ts @@ -882,8 +882,8 @@ query { } }); - it('should support explicit push form (assign a = source << value)', async () => { - const ast = toLiquidHtmlAST(`{% assign arr = [] %}{% assign arr = arr << "item" %}{{ arr }}`); + it('should support bare push form (assign arr << value)', async () => { + const ast = toLiquidHtmlAST(`{% assign arr = [] %}{% assign arr << "item" %}{{ arr }}`); const variableOutput = ast.children[2]; assert(isLiquidVariableOutput(variableOutput)); const inferredType = await typeSystem.inferType( diff --git a/packages/platformos-language-server-common/src/TypeSystem.ts b/packages/platformos-language-server-common/src/TypeSystem.ts index cc3100b1..f377dd90 100644 --- a/packages/platformos-language-server-common/src/TypeSystem.ts +++ b/packages/platformos-language-server-common/src/TypeSystem.ts @@ -1,6 +1,5 @@ import { AssignMarkup, - AssignPushRhs, ComplexLiquidExpression, FunctionMarkup, LiquidDocParamNode, @@ -382,14 +381,9 @@ async function buildSymbolsTable( // {% assign x = {a: 1, b: "hello"} %} // {% assign x["key"] = value %} // {% assign arr << item %} - // {% assign arr = source << item %} async AssignMarkup(node) { - // For explicit push form (assign a = source << value), treat as array append - const isPushRhs = node.value.type === NodeTypes.AssignPushRhs; - const effectiveValue = isPushRhs - ? (node.value as AssignPushRhs).pushValue - : (node.value as LiquidVariable); - const effectiveOperator = isPushRhs ? '<<' : node.operator; + const effectiveValue = node.value as LiquidVariable; + const effectiveOperator = node.operator; const expression = effectiveValue.expression; let valueShape: PropertyShape | undefined; @@ -970,9 +964,7 @@ function inferType( // The type of the assign markup is the type of the right hand side. // {% assign x = y.property | filter1 | filter2 %} case NodeTypes.AssignMarkup: { - const assignValue = - thing.value.type === NodeTypes.AssignPushRhs ? thing.value.pushValue : thing.value; - return inferType(assignValue, symbolsTable, objectMap, filtersMap); + return inferType(thing.value, symbolsTable, objectMap, filtersMap); } // A variable lookup is expression[.lookup]* diff --git a/packages/platformos-language-server-common/src/completions/params/LiquidCompletionParams.ts b/packages/platformos-language-server-common/src/completions/params/LiquidCompletionParams.ts index 64c42c30..acd59064 100644 --- a/packages/platformos-language-server-common/src/completions/params/LiquidCompletionParams.ts +++ b/packages/platformos-language-server-common/src/completions/params/LiquidCompletionParams.ts @@ -447,8 +447,7 @@ function findCurrentNode( case NodeTypes.ExportMarkup: case NodeTypes.RedirectToMarkup: case NodeTypes.IncludeFormMarkup: - case NodeTypes.SpamProtectionMarkup: - case NodeTypes.AssignPushRhs: { + case NodeTypes.SpamProtectionMarkup: { break; } 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 6e8caa6c..425f80ca 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 @@ -151,7 +151,6 @@ function getCssDisplay(node: AugmentedNode, options: LiquidParserO case NodeTypes.LiquidDocExampleNode: case NodeTypes.LiquidDocDescriptionNode: case NodeTypes.LiquidDocPromptNode: - case NodeTypes.AssignPushRhs: return 'should not be relevant'; default: @@ -280,7 +279,6 @@ function getNodeCssStyleWhiteSpace( case NodeTypes.LiquidDocExampleNode: case NodeTypes.LiquidDocDescriptionNode: case NodeTypes.LiquidDocPromptNode: - case NodeTypes.AssignPushRhs: 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 37846fb0..1e2fc966 100644 --- a/packages/prettier-plugin-liquid/src/printer/print/liquid.ts +++ b/packages/prettier-plugin-liquid/src/printer/print/liquid.ts @@ -158,11 +158,7 @@ function printNamedLiquidBlockStart( } case NamedTags.assign: { - const valueFilters = - node.markup.value.type === NodeTypes.AssignPushRhs - ? node.markup.value.pushValue.filters - : node.markup.value.filters; - const trailingWhitespace = valueFilters.length > 0 ? line : ' '; + const trailingWhitespace = node.markup.value.filters.length > 0 ? line : ' '; return tag(trailingWhitespace); } 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 aac1f821..46ae498f 100644 --- a/packages/prettier-plugin-liquid/src/printer/printer-liquid-html.ts +++ b/packages/prettier-plugin-liquid/src/printer/printer-liquid-html.ts @@ -795,14 +795,6 @@ function printNode( ]); } - case NodeTypes.AssignPushRhs: { - return [ - path.call((p: any) => print(p), 'pushSource'), - ' << ', - path.call((p: any) => print(p), 'pushValue'), - ]; - } - default: { return assertNever(node); }