Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,55 @@ describe('detectInvalidAssignSyntax', () => {
});
});

describe('trailing garbage after RHS / filters (fallback)', () => {
// These cases have a valid `target = value` skeleton and a valid filter chain,
// but extra non-parseable characters trail the filters. The tolerant parser
// swallows the entire body as string markup, and none of the other sub-checks
// (MultipleAssignValues, InvalidFilterName, InvalidPipeSyntax) detects the
// problem — so the fallback re-parses in strict mode and surfaces it.
const fallbackCases: Array<[string, string]> = [
[
'stray `}` after filter array argument',
`{% assign name = arr | default: [ "hi", k, v] } %}`,
],
['trailing bare word after filter arg', `{% assign x = y | default: "z" trailing %}`],
];

for (const [label, sourceCode] of fallbackCases) {
it(`should report: ${label} — ${sourceCode}`, async () => {
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
const syntaxOffenses = offenses.filter((o) =>
o.message.includes("Invalid syntax for tag 'assign'"),
);
expect(syntaxOffenses).toHaveLength(1);
});
}

// These cases are already flagged by other sub-checks with different messages;
// the fallback must NOT also fire on them (no double-reporting).
const alreadyCoveredCases: Array<[string, string]> = [
[
'stray `}` after unary filter (InvalidFilterName catches it)',
`{% assign x = y | upcase } %}`,
],
[
'stray `}` after RHS with no filter (MultipleAssignValues catches it)',
`{% assign x = "v" } %}`,
],
];

for (const [label, sourceCode] of alreadyCoveredCases) {
it(`should NOT double-report: ${label} — ${sourceCode}`, async () => {
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
expect(offenses.length).toBeGreaterThan(0);
const fallbackOffenses = offenses.filter((o) =>
o.message.includes("Invalid syntax for tag 'assign'"),
);
expect(fallbackOffenses).toHaveLength(0);
});
}
});

describe('valid syntax — should NOT report', () => {
const validCases: Array<[string, string]> = [
// primitives
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { LiquidTag } from '@platformos/liquid-html-parser';
import { LiquidTag, toLiquidAST } from '@platformos/liquid-html-parser';
import { Problem, SourceCodeType } from '../../..';

const INVALID_ASSIGN_MESSAGE = `Invalid syntax for tag 'assign'. Expected syntax: {% assign <var> = <value> %}`;

/**
* Detects structurally-invalid `assign` tags that neither `MultipleAssignValues` nor
* `InvalidPipeSyntax`/`InvalidFilterName` catch:
Expand Down Expand Up @@ -38,12 +40,44 @@ export function detectInvalidAssignSyntax(
if (!isStructurallyBroken) return;

return {
message: `Invalid syntax for tag 'assign'. Expected syntax: {% assign <var> = <value> %}`,
message: INVALID_ASSIGN_MESSAGE,
startIndex: node.position.start,
endIndex: node.position.end,
};
}

/**
* Fallback for assign tags where the tolerant parser landed in string markup even
* though the `target = value` skeleton looks fine — meaning the value or filter
* chain has parse-breaking characters (e.g. a stray `}` before `%}`) that no other
* dedicated sub-check (MultipleAssignValues, InvalidFilterName, InvalidPipeSyntax)
* surfaced. Re-parses the tag source in strict mode and reports on failure.
*
* Must run ONLY when no other sub-check already reported on this tag, otherwise
* it double-flags the same problem. The orchestrator enforces that gate.
*/
export function detectInvalidAssignFallback(
node: LiquidTag,
): Problem<SourceCodeType.LiquidHtml> | undefined {
if (node.name !== 'assign' || typeof node.markup !== 'string') return;

// Digit-starting targets (e.g. `23_hours_ago`) are accepted by the platformOS
// runtime but rejected by the Ohm grammar's `variableSegment` rule. Skipping
// them here mirrors the intentional tolerance in isValidAssignTarget above.
if (/^\s*\d/.test(node.markup)) return;

const tagSource = node.source.slice(node.position.start, node.position.end);
try {
toLiquidAST(tagSource, { mode: 'strict', allowUnclosedDocumentNode: true });
} catch {
return {
message: INVALID_ASSIGN_MESSAGE,
startIndex: node.position.start,
endIndex: node.position.end,
};
}
}

/**
* Rejects an LHS that is obviously not an assign target.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { Severity, SourceCodeType, LiquidCheckDefinition, Problem } from '../../types';
import { getOffset, isError } from '../../utils';
import { detectMultipleAssignValues } from './checks/MultipleAssignValues';
import { detectInvalidAssignSyntax } from './checks/InvalidAssignSyntax';
import {
detectInvalidAssignSyntax,
detectInvalidAssignFallback,
} from './checks/InvalidAssignSyntax';
import { detectInvalidBooleanExpressions } from './checks/InvalidBooleanExpressions';
import { detectInvalidEchoValue } from './checks/InvalidEchoValue';
import { detectInvalidConditionalNode } from './checks/InvalidConditionalNode';
Expand Down Expand Up @@ -123,6 +126,14 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
if (pipeProblems.length > 0) {
pipeProblems.forEach((pipeProblem) => context.report(pipeProblem));
}

// Last-chance check for assign tags whose tolerant parse fell back to
// string markup (e.g. stray `}` before `%}`). Gated on "nothing else
// reported on this tag" to avoid double-flagging.
if (problems.length + filterProblems.length + pipeProblems.length === 0) {
const fallback = detectInvalidAssignFallback(node);
if (fallback) context.report(fallback);
}
},

async LiquidBranch(node, ancestors) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,4 +201,52 @@ describe('Module: UnusedAssign', () => {
expect(offenses).to.have.lengthOf(1);
expect(offenses[0].message).to.equal("The variable 'arr' is assigned but not used");
});

it('should not report variables used inside a filter array-literal argument', async () => {
const sourceCode = `
{% assign k = "key" %}
{% assign v = "value" %}
{% assign name = arr | default: [ "hi", k, v] %}
{{ name }}
`;

const offenses = await runLiquidCheck(UnusedAssign, sourceCode);
expect(offenses).to.have.lengthOf(0);
});

it('should not report variables used inside a filter hash-literal argument', async () => {
const sourceCode = `
{% assign k = 1 %}
{% assign name = arr | default: { a: k } %}
{{ name }}
`;

const offenses = await runLiquidCheck(UnusedAssign, sourceCode);
expect(offenses).to.have.lengthOf(0);
});

it('should not report variables used inside a named-argument value', async () => {
const sourceCode = `
{% assign k = 0 %}
{% assign name = arr | slice: start: k %}
{{ name }}
`;

const offenses = await runLiquidCheck(UnusedAssign, sourceCode);
expect(offenses).to.have.lengthOf(0);
});

it('should not report variables referenced in a string-markup fallback assign (parse failure)', async () => {
// The stray `}` before `%}` causes the tolerant parser to fall back to
// string markup. The AST contains no VariableLookup nodes for k/v, so
// without a textual fallback scan the check would wrongly flag them.
const sourceCode = `
{% assign k = "key" %}
{% assign v = "value" %}
{% assign name = arr | default: [ "hi", k, v] } %}
`;

const offenses = await runLiquidCheck(UnusedAssign, sourceCode);
expect(offenses).to.have.lengthOf(0);
});
});
15 changes: 15 additions & 0 deletions packages/platformos-check-common/src/checks/unused-assign/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ export const UnusedAssign: LiquidCheckDefinition = {
// effects on the original, so they count as "using" the variable.
const referenceAssignedVariables: Set<string> = new Set();
const usedVariables: Set<string> = new Set();
// Concatenated markup of LiquidTags whose parsing fell back to a raw string
// (e.g. a typo like `{% assign name = x | default: [...] } %}`). The AST has
// no VariableLookup nodes to traverse inside them, so onCodePathEnd scans
// this text for assigned-variable names — avoids compounding the user's
// syntax error with false "unused" warnings.
let fallbackText = '';

function checkVariableUsage(node: any) {
if (node.type === NodeTypes.VariableLookup) {
Expand Down Expand Up @@ -66,6 +72,8 @@ export const UnusedAssign: LiquidCheckDefinition = {
}
} else if (isLiquidTagCapture(node) && node.markup.name) {
assignedVariables.set(node.markup.name, node);
} else if (typeof node.markup === 'string' && node.markup) {
fallbackText += '\n' + node.markup;
}
},

Expand All @@ -79,6 +87,13 @@ export const UnusedAssign: LiquidCheckDefinition = {
},

async onCodePathEnd() {
if (fallbackText) {
for (const name of assignedVariables.keys()) {
if (usedVariables.has(name)) continue;
if (new RegExp(`\\b${name}\\b`).test(fallbackText)) usedVariables.add(name);
}
}

for (const [variable, node] of assignedVariables.entries()) {
if (!usedVariables.has(variable) && !variable.startsWith('_')) {
context.report({
Expand Down