Skip to content

Updated the undefined-object theme check to skip over inline-snippets #1071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
+175 −7
Draft

Updated the undefined-object theme check to skip over inline-snippets #1071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6189068
Updated the undefined-object theme check to skip over inline-snippets
shoplando Oct 9, 2025
3274d57
update the check so that it's scope aware
shoplando Oct 22, 2025
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
5 changes: 5 additions & 0 deletions .changeset/sweet-steaks-greet.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/theme-check-common': minor
---

Updated the `Undefined-object` theme check to skip over the new `snippet` tags
106 changes: 106 additions & 0 deletions packages/theme-check-common/src/checks/undefined-object/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,4 +459,110 @@ describe('Module: UndefinedObject', () => {
assert(offenses.length == 0);
expect(offenses).to.be.empty;
});

it('should not report an offense for inline snippet names in snippet and render tags', async () => {
const sourceCode = `
{% snippet my_inline_snippet %}
{% echo 'hello' %}
{% endsnippet %}

{% render my_inline_snippet %}
`;

const offenses = await runLiquidCheck(UndefinedObject, sourceCode);

expect(offenses).toHaveLength(0);
});

describe('Scope-aware checking for inline snippets', () => {
it('No doc tags anywhere - should skip all checks', async () => {
const sourceCode = `
{{ undefined_in_parent }}

{% snippet inline_one %}
{{ undefined_in_inline }}
{% endsnippet %}
`;

const filePath = 'snippets/file.liquid';
const offenses = await runLiquidCheck(UndefinedObject, sourceCode, filePath);

expect(offenses).toHaveLength(0);
});

it('Doc tag in parent only - should check parent, skip inline snippets', async () => {
const sourceCode = `
{% doc %}
@param {string} parent_param
{% enddoc %}

{{ parent_param }}
{{ undefined_in_parent }}

{% snippet inline_one %}
{{ undefined_in_inline }}
{% endsnippet %}
`;

const filePath = 'snippets/file.liquid';
const offenses = await runLiquidCheck(UndefinedObject, sourceCode, filePath);

expect(offenses).toHaveLength(1);
expect(offenses[0].message).toBe("Unknown object 'undefined_in_parent' used.");
});

it('Doc tag in inline snippet only - should skip parent, check inline snippet', async () => {
const sourceCode = `
{{ undefined_in_parent }}

{% snippet inline_one %}
{% doc %}
@param {string} inline_param
{% enddoc %}

{{ inline_param }}
{{ undefined_in_inline }}
{% endsnippet %}
`;

const filePath = 'snippets/file.liquid';
const offenses = await runLiquidCheck(UndefinedObject, sourceCode, filePath);

expect(offenses).toHaveLength(1);
expect(offenses[0].message).toBe("Unknown object 'undefined_in_inline' used.");
});

it('Doc tags in both - should check both, but skip inline snippets without docs', async () => {
const sourceCode = `
{% doc %}
@param {string} parent_param
{% enddoc %}

{{ parent_param }}
{{ undefined_in_parent }}

{% snippet inline_one %}
{% doc %}
@param {string} inline_param
{% enddoc %}

{{ inline_param }}
{{ undefined_in_inline }}
{% endsnippet %}

{% snippet inline_two %}
{{ anything }}
{% endsnippet %}
`;

const filePath = 'snippets/file.liquid';
const offenses = await runLiquidCheck(UndefinedObject, sourceCode, filePath);

expect(offenses).toHaveLength(2);
expect(offenses.map((o) => o.message)).toEqual([
"Unknown object 'undefined_in_parent' used.",
"Unknown object 'undefined_in_inline' used.",
]);
});
});
});
60 changes: 53 additions & 7 deletions packages/theme-check-common/src/checks/undefined-object/index.ts
View file
Open in desktop
Copy link
Contributor

@aswamy aswamy Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So i think you have the right logic, but i think you need to modify the logic on line 48:

if (relativePath.startsWith('snippets/') && !hasLiquidDoc(ast)) return {};

This might not make too much sense, but here is the logic:

  • There exists real world snippets (especially old ones) that DONT have doc tag
  • The code above ensures that we only have UndefinedObject errors on snippets IF they have doc tag because technically, you can pass ANYTHING into snippets, and without a doc tag we don't really know which one is Defined, and which one isn't
  • But by introducing inline snippets, we have a slight problem: having a doc tag inside the inline snippets don't mean the top-level snippet file has one too...

So we need a robust solution that works for the following situations:

  • What happens when you have no doc tags in the parent snippet AND the inline snippets
  • What happens when you have doc tag in the parent, but not in the inline snippets
  • What happens when you have doc tag in the inline snippets, but not in the parent
  • What happens when you have doc tag in both

I think we need to layout each situation before full implementation of this

Copy link
Contributor Author

@shoplando shoplando Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so basically we would want:

  • no checks at all if both the snippet file and inline snippet don't have any doc tags
  • yes checks when there is a doc tag at its respective scope, for example when there is a doc tag in the main snippet file and none inside the inline snippet:
{%- comment -%} snippets/my-snippet.liquid {%- endcomment -%}

{% doc %}
  @param {string} parent_param
{% enddoc %}

{{ parent_param }}         <- ✅ Should NOT warn
{{ undefined_in_parent }}  <- ❓ Should this warn? YES

{% snippet inline_one %}
  {{ undefined_in_inline }} <- ❓ Should this warn? NO
{% endsnippet %}

And when the doc tag is inside the inline snippet, the check will be activated only on the inside of the inline snippet, and not outside of it.
I'll implement this right now.

Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
LiquidTagDecrement,
LiquidTagFor,
LiquidTagIncrement,
LiquidTagSnippet,
LiquidTagTablerow,
LiquidVariableLookup,
NamedTags,
Expand All @@ -16,7 +17,7 @@ import {
import { LiquidCheckDefinition, Severity, SourceCodeType, ThemeDocset } from '../../types';
import { isError, last } from '../../utils';
import { hasLiquidDoc } from '../../liquid-doc/liquidDoc';
import { isWithinRawTagThatDoesNotParseItsContents } from '../utils';
import { isWithinRawTagThatDoesNotParseItsContents, findInlineSnippetAncestor } from '../utils';

type Scope = { start?: number; end?: number };

Expand All @@ -38,13 +39,14 @@ export const UndefinedObject: LiquidCheckDefinition = {
create(context) {
const relativePath = context.toRelativePath(context.file.uri);
const ast = context.file.ast;
const isSnippetFile = relativePath.startsWith('snippets/');

if (isError(ast)) return {};

/**
* Skip this check when a snippet does not have the presence of doc tags.
*/
if (relativePath.startsWith('snippets/') && !hasLiquidDoc(ast)) return {};
if (isSnippetFile && !hasLiquidDoc(ast)) return {};

/**
* Skip this check when definitions for global objects are unavailable.
Expand All @@ -56,7 +58,9 @@ export const UndefinedObject: LiquidCheckDefinition = {
const themeDocset = context.themeDocset;
const scopedVariables: Map<string, Scope[]> = new Map();
const fileScopedVariables: Set<string> = new Set();
const variables: LiquidVariableLookup[] = [];
const variables: Array<{ node: LiquidVariableLookup; inlineSnippet: LiquidTag | null }> = [];
const inlineSnippetsWithDocTags: Set<number> = new Set();
let snippetFileHasDocTag = false;

function indexVariableScope(variableName: string | null, scope: Scope) {
if (!variableName) return;
Expand All @@ -66,9 +70,27 @@ export const UndefinedObject: LiquidCheckDefinition = {
}

return {
async LiquidDocParamNode(node: LiquidDocParamNode) {
async LiquidRawTag(node, ancestors) {
if (!isSnippetFile || node.name !== 'doc') return;

const parent = last(ancestors);
if (isLiquidTag(parent) && isLiquidTagSnippet(parent)) {
inlineSnippetsWithDocTags.add(parent.position.start);
} else {
snippetFileHasDocTag = true;
}
},

async LiquidDocParamNode(node: LiquidDocParamNode, ancestors: LiquidHtmlNode[]) {
const paramName = node.paramName?.value;
if (paramName) {
if (!paramName) return;
const snippetAncestor = findInlineSnippetAncestor(ancestors);
if (snippetAncestor) {
indexVariableScope(paramName, {
start: snippetAncestor.blockStartPosition.end,
end: snippetAncestor.blockEndPosition?.start,
});
Comment on lines +89 to +92
Copy link
Contributor

@aswamy aswamy Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL Cool didn't even know we had a variable scope like this

} else {
fileScopedVariables.add(paramName);
}
},
Expand Down Expand Up @@ -134,6 +156,10 @@ export const UndefinedObject: LiquidCheckDefinition = {
end: node.blockEndPosition?.start,
});
}

if (isLiquidTagSnippet(node) && node.markup.name) {
fileScopedVariables.add(node.markup.name);
}
},

async VariableLookup(node, ancestors) {
Expand All @@ -142,17 +168,33 @@ export const UndefinedObject: LiquidCheckDefinition = {
const parent = last(ancestors);
if (isLiquidTag(parent) && isLiquidTagCapture(parent)) return;

variables.push(node);
if (isLiquidTag(parent) && isLiquidTagSnippet(parent)) return;

const inlineSnippet = findInlineSnippetAncestor(ancestors);
variables.push({ node, inlineSnippet });
},

async onCodePathEnd() {
const objects = await globalObjects(themeDocset, relativePath);

objects.forEach((obj) => fileScopedVariables.add(obj.name));

variables.forEach((variable) => {
variables.forEach(({ node: variable, inlineSnippet }) => {
if (!variable.name) return;

// For snippet files: only check variables if they're in a scope with a doc tag.
if (isSnippetFile) {
if (inlineSnippet) {
if (!inlineSnippetsWithDocTags.has(inlineSnippet.position.start)) {
return;
}
} else {
if (!snippetFileHasDocTag) {
return;
}
}
}

const isVariableDefined = isDefined(
variable.name,
variable.position,
Expand Down Expand Up @@ -268,6 +310,10 @@ function isLiquidTagCapture(node: LiquidTag): node is LiquidTagCapture {
return node.name === NamedTags.capture;
}

function isLiquidTagSnippet(node: LiquidTag): node is LiquidTagSnippet {
return node.name === NamedTags.snippet;
}

function isLiquidTagAssign(node: LiquidTag): node is LiquidTagAssign {
return node.name === NamedTags.assign && typeof node.markup !== 'string';
}
Expand Down
11 changes: 11 additions & 0 deletions packages/theme-check-common/src/checks/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
LiquidTagTablerow,
LiquidTag,
LoopNamedTags,
NamedTags,
} from '@shopify/liquid-html-parser';
import { LiquidHtmlNodeOfType as NodeOfType } from '../types';

Expand Down Expand Up @@ -104,6 +105,16 @@ export function isLoopLiquidTag(tag: LiquidTag): tag is LiquidTagFor | LiquidTag
return LoopNamedTags.includes(tag.name as any);
}

export function findInlineSnippetAncestor(ancestors: LiquidHtmlNode[]) {
for (let i = ancestors.length - 1; i >= 0; i--) {
const ancestor = ancestors[i];
if (ancestor.type === NodeTypes.LiquidTag && ancestor.name === NamedTags.snippet) {
return ancestor;
}
}
return null;
}

const RawTagsThatDoNotParseTheirContents = ['raw', 'stylesheet', 'javascript', 'schema'];

function isRawTagThatDoesNotParseItsContent(node: LiquidHtmlNode) {
Expand Down