Fix several Theme Check paper cuts

.changeset/fix-assigned-schema-variables.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Fix schema expressions to resolve assigned Liquid variables and recognize their usage.

.changeset/fix-layout-snippet-references.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-graph': patch
+---
+
+Fix orphaned-snippet detection by following layout and `{% block %}` references.

.changeset/fix-liquid-doc-literal-parameters.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Fix LiquidDoc parameter validation by reserving Liquid literal keywords.

.changeset/fix-private-liquid-variables.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Fix Liquid variable-name validation to allow a leading underscore.

.changeset/fix-responsive-lcp-preloads.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Fix responsive LCP preload validation by allowing explicit image links with `fetchpriority="high"`.

packages/theme-check-common/src/checks/asset-preload/index.spec.ts

@@ -43,6 +43,28 @@ describe('Module: AssetPreload', () => {
     expect(highlights).to.eql([`<link href="a.png" rel="preload" as="image">`]);
   });
 
+  it('allows high-priority image preloading', async () => {
+    // The preload_tag filter cannot generate responsive imagesrcset or
+    // imagesizes attributes, so LCP images may require an explicit
+    // high-priority preload link.
+    const sourceCode = `
+      <link href="a.png" rel="preload" as="image" fetchpriority="high">
+    `;
+
+    const offenses = await runLiquidCheck(AssetPreload, sourceCode);
+    expect(offenses).to.have.lengthOf(0);
+  });
+
+  it('reports image preloading when attribute values contain Liquid', async () => {
+    const sourceCode = `
+      <link href="a.png" rel="preload" as="image{{ '-invalid' }}" fetchpriority="high">
+      <link href="b.png" rel="preload" as="image" fetchpriority="high{{ '-invalid' }}">
+    `;
+
+    const offenses = await runLiquidCheck(AssetPreload, sourceCode);
+    expect(offenses).to.have.lengthOf(2);
+  });
+
   it('reports general preloading', async () => {
     const sourceCode = `
       <link href="a.js" rel="preload" as="script">

packages/theme-check-common/src/checks/asset-preload/index.ts

@@ -1,6 +1,12 @@
 import { NodeTypes, TextNode } from '@shopify/liquid-html-parser';
 import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types';
-import { ValuedHtmlAttribute, isAttr, isNodeOfType, isValuedHtmlAttribute } from '../utils';
+import {
+  ValuedHtmlAttribute,
+  hasAttributeValueOf,
+  isAttr,
+  isNodeOfType,
+  isValuedHtmlAttribute,
+} from '../utils';
 
 function isPreload(attr: ValuedHtmlAttribute): boolean {
   return (
@@ -9,6 +15,13 @@ function isPreload(attr: ValuedHtmlAttribute): boolean {
   );
 }
 
+function isHighPriorityImagePreload(attributes: ValuedHtmlAttribute[]): boolean {
+  return (
+    attributes.some((attr) => isAttr(attr, 'as') && hasAttributeValueOf(attr, 'image')) &&
+    attributes.some((attr) => isAttr(attr, 'fetchpriority') && hasAttributeValueOf(attr, 'high'))
+  );
+}
+
 export const AssetPreload: LiquidCheckDefinition = {
   meta: {
     code: 'AssetPreload',
@@ -33,9 +46,13 @@ export const AssetPreload: LiquidCheckDefinition = {
         ) as ValuedHtmlAttribute | undefined;
 
         if (node.name === 'link' && preloadLinkAttr) {
-          const asAttr: ValuedHtmlAttribute | undefined = node.attributes
-            .filter(isValuedHtmlAttribute)
-            .find((attr) => isAttr(attr, 'as'));
+          const valuedAttributes = node.attributes.filter(isValuedHtmlAttribute);
+
+          if (isHighPriorityImagePreload(valuedAttributes)) return;
+
+          const asAttr: ValuedHtmlAttribute | undefined = valuedAttributes.find((attr) =>
+            isAttr(attr, 'as'),
+          );
 
           const assetType = asAttr?.value.find((node): node is TextNode =>
             isNodeOfType(NodeTypes.TextNode, node),

packages/theme-check-common/src/checks/reserved-doc-param-names/index.spec.ts

@@ -3,60 +3,49 @@ import { ReservedDocParamNames } from './index';
 import { runLiquidCheck } from '../../test';
 
 describe('Module: ReservedDocParamNames', () => {
-  describe('block file', () => {
-    it(`should not report an error when no doc params share names with reserved content_for tag params`, async () => {
+  it.each(['blocks/file.liquid', 'snippets/file.liquid'])(
+    'reports Liquid literal parameter names in %s',
+    async (fileName) => {
       const sourceCode = `
-        {% doc %}
-          @param param1 - Example param
-        {% enddoc %}
-      `;
-
-      const offenses = await runLiquidCheck(
-        ReservedDocParamNames,
-        sourceCode,
-        'blocks/file.liquid',
-      );
-
-      expect(offenses).to.be.empty;
-    });
-
-    it('should report an error when a doc param shares names with reserved content_for tag params', async () => {
+          {% doc %}
+            @param nil - Example param
+            @param null - Example param
+            @param true - Example param
+            @param false - Example param
+            @param blank - Example param
+            @param empty - Example param
+          {% enddoc %}
+        `;
+
+      const offenses = await runLiquidCheck(ReservedDocParamNames, sourceCode, fileName);
+
+      expect(offenses).to.have.length(6);
+      expect(offenses.map(({ message }) => message)).toEqual([
+        "The parameter name 'nil' is reserved because Liquid parses it as a literal.",
+        "The parameter name 'null' is reserved because Liquid parses it as a literal.",
+        "The parameter name 'true' is reserved because Liquid parses it as a literal.",
+        "The parameter name 'false' is reserved because Liquid parses it as a literal.",
+        "The parameter name 'blank' is reserved because Liquid parses it as a literal.",
+        "The parameter name 'empty' is reserved because Liquid parses it as a literal.",
+      ]);
+    },
+  );
+
+  it.each(['blocks/file.liquid', 'snippets/file.liquid'])(
+    'allows non-literal parameter names in %s',
+    async (fileName) => {
       const sourceCode = `
-        {% doc %}
-          @param param1 - Example param
-          @param id - Example param
-        {% enddoc %}
-      `;
-
-      const offenses = await runLiquidCheck(
-        ReservedDocParamNames,
-        sourceCode,
-        'blocks/file.liquid',
-      );
+          {% doc %}
+            @param param1 - Example param
+            @param class - Example param
+            @param attributes - Example param
+            @param id - Example param
+          {% enddoc %}
+        `;
 
-      expect(offenses).to.have.length(1);
-      expect(offenses[0].message).to.contain(
-        `The parameter name is not supported because it's a reserved argument for 'content_for' tags.`,
-      );
-    });
-  });
+      const offenses = await runLiquidCheck(ReservedDocParamNames, sourceCode, fileName);
 
-  describe('snippet file', () => {
-    it('should not report an error when a doc param shares names with reserved content_for tag params', async () => {
-      const sourceCode = `
-        {% doc %}
-          @param param1 - Example param
-          @param id - Example param
-        {% enddoc %}
-      `;
-
-      const offenses = await runLiquidCheck(
-        ReservedDocParamNames,
-        sourceCode,
-        'snippets/file.liquid',
-      );
-
-      expect(offenses).to.have.length(0);
-    });
-  });
+      expect(offenses).to.be.empty;
+    },
+  );
 });

packages/theme-check-common/src/checks/reserved-doc-param-names/index.ts

@@ -1,10 +1,7 @@
-import { TextNode } from '@shopify/liquid-html-parser';
+import { LiquidLiteralValues } from '@shopify/liquid-html-parser';
 import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types';
-import { isBlock } from '../../to-schema';
-import {
-  REQUIRED_CONTENT_FOR_ARGUMENTS,
-  RESERVED_CONTENT_FOR_ARGUMENTS,
-} from '../../tags/content-for';
+
+const RESERVED_DOC_PARAM_NAMES = new Set(Object.keys(LiquidLiteralValues));
 
 export const ReservedDocParamNames: LiquidCheckDefinition = {
   meta: {
@@ -23,35 +20,18 @@ export const ReservedDocParamNames: LiquidCheckDefinition = {
   },
 
   create(context) {
-    if (!isBlock(context.file.uri)) {
-      return {};
-    }
-
-    const defaultParameterNames = [
-      ...REQUIRED_CONTENT_FOR_ARGUMENTS,
-      ...RESERVED_CONTENT_FOR_ARGUMENTS,
-    ];
-
     return {
       async LiquidDocParamNode(node) {
         const paramName = node.paramName.value;
 
-        if (defaultParameterNames.includes(paramName)) {
-          reportWarning(
-            context,
-            `The parameter name is not supported because it's a reserved argument for 'content_for' tags.`,
-            node.paramName,
-          );
-        }
+        if (!RESERVED_DOC_PARAM_NAMES.has(paramName)) return;
+
+        context.report({
+          message: `The parameter name '${paramName}' is reserved because Liquid parses it as a literal.`,
+          startIndex: node.paramName.position.start,
+          endIndex: node.paramName.position.end,
+        });
       },
     };
   },
 };
-
-function reportWarning(context: any, message: string, node: TextNode) {
-  context.report({
-    message,
-    startIndex: node.position.start,
-    endIndex: node.position.end,
-  });
-}

packages/theme-check-common/src/checks/unused-assign/index.spec.ts

@@ -114,6 +114,48 @@ describe('Module: UnusedAssign', () => {
     }
   });
 
+  it('should not report variables used by a schema visible_if expression', async () => {
+    const sourceCode = `
+      {% assign has_logo_image = true %}
+      {% schema %}
+        {
+          "name": "Logo",
+          "settings": [
+            {
+              "type": "range",
+              "id": "height",
+              "min": 16,
+              "max": 80,
+              "step": 4,
+              "default": 32,
+              "visible_if": "{{ has_logo_image == true }}"
+            }
+          ]
+        }
+      {% endschema %}
+    `;
+
+    const offenses = await runLiquidCheck(UnusedAssign, sourceCode, 'blocks/logo.liquid');
+
+    expect(offenses).to.be.empty;
+  });
+
+  it('should still report variables mentioned only in other schema strings', async () => {
+    const sourceCode = `
+      {% assign unused_var = true %}
+      {% schema %}
+        {
+          "name": "unused_var",
+          "settings": []
+        }
+      {% endschema %}
+    `;
+
+    const offenses = await runLiquidCheck(UnusedAssign, sourceCode, 'blocks/example.liquid');
+
+    expect(offenses).to.have.lengthOf(1);
+  });
+
   it('should not report unused assigns for things used in a HTML raw-like tag', async () => {
     const tags = ['style', 'script'];
     for (const tag of tags) {

packages/theme-check-common/src/checks/unused-assign/index.ts

@@ -6,7 +6,9 @@ import {
   NodeTypes,
 } from '@shopify/liquid-html-parser';
 import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types';
+import { getSchema } from '../../to-schema';
 import { isWithinRawTagThatDoesNotParseItsContents } from '../utils';
+import { getVariableLookupsInExpression } from '../valid-visible-if/visible-if-utils';
 
 export const UnusedAssign: LiquidCheckDefinition = {
   meta: {
@@ -53,6 +55,25 @@ export const UnusedAssign: LiquidCheckDefinition = {
         checkVariableUsage(node);
       },
 
+      async LiquidRawTag(node) {
+        if (node.name !== 'schema' || node.body.kind !== 'json') return;
+
+        const schema = await getSchema(context);
+        const validSchema = schema?.validSchema;
+        if (!validSchema || validSchema instanceof Error) return;
+
+        for (const setting of validSchema.settings ?? []) {
+          if (!('visible_if' in setting) || typeof setting.visible_if !== 'string') continue;
+
+          const lookups = getVariableLookupsInExpression(setting.visible_if);
+          if (!lookups || 'warning' in lookups) continue;
+
+          for (const lookup of lookups) {
+            if (lookup.name) usedVariables.add(lookup.name);
+          }
+        }
+      },
+
       async onCodePathEnd() {
         for (const [variable, node] of assignedVariables.entries()) {
           if (!usedVariables.has(variable) && !variable.startsWith('_')) {

packages/theme-check-common/src/checks/valid-visible-if/index.spec.ts

@@ -97,6 +97,43 @@ describe('Module: ValidVisibleIf', () => {
     expect(offenses).toEqual([]);
   });
 
+  it('reports no error for a variable assigned in the Liquid file', async () => {
+    const themeData = structuredClone(baseThemeData);
+
+    themeData['blocks/example.liquid'].settings!.push({
+      id: 'some-other-setting',
+      visible_if: '{{ effective_fill == "image" }}',
+    });
+
+    const theme = makeTheme(themeData);
+    theme['blocks/example.liquid'] = `
+      {% liquid
+        assign effective_fill = 'image'
+      %}
+      ${theme['blocks/example.liquid']}
+    `;
+
+    const offenses = await check(theme, [ValidVisibleIf, ValidVisibleIfSettingsSchema]);
+    expect(offenses).toEqual([]);
+  });
+
+  it('reports no error for a variable assigned after the schema tag', async () => {
+    const themeData = structuredClone(baseThemeData);
+
+    themeData['blocks/example.liquid'].settings!.push({
+      id: 'some-other-setting',
+      visible_if: '{{ effective_fill == "image" }}',
+    });
+
+    const theme = makeTheme(themeData);
+    theme['blocks/example.liquid'] += `
+      {% assign effective_fill = 'image' %}
+    `;
+
+    const offenses = await check(theme, [ValidVisibleIf, ValidVisibleIfSettingsSchema]);
+    expect(offenses).toEqual([]);
+  });
+
   it('reports no error for a valid reference to a section schema (simple lookup)', async () => {
     const themeData = structuredClone(baseThemeData);