Fix false reporting of UndefinedObject within visible_if (#1061)

We were visiting those nodes as though they were part of the execution
of the file when in fact those are not parsed by the backend but
stripped away.

I first tried doing that at the parsing layer by putting the body as a
TextNode (the same way `{% raw %}` does it) but quickly discovered that
we do depend on it being parsed for the StaticStylesheetAndJavascriptTags
check.

So instead, I just added a helper that both UndefinedObject and
UnusedAssign use to skip visiting of nodes that are parent'ed by a
"raw-like" tag

Fixes #685

Author: charlespwd

.changeset/yellow-needles-turn.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Fix false-positive reporting of UndefinedObject within {% schema %} tags

packages/liquid-html-parser/src/stage-1-cst.ts

@@ -620,6 +620,8 @@ function toCST<T>(
         const nameNode = tokens[3];
         const rawMarkupStringNode = tokens[9];
         switch (nameNode.sourceString) {
+          // {% schema %} parses its content as a string and should not be visited
+          case 'schema':
           // {% raw %} accepts syntax errors, we shouldn't try to parse that
           case 'raw': {
             return toCST(
@@ -632,7 +634,10 @@ function toCST<T>(
             );
           }
 
-          // {% javascript %}, {% style %}
+          // {% style %} actually parses its child nodes, so they are part of the AST
+          // {% javascript %}, {% stylesheet %} don't, but we want to flag folks that
+          // those are not supported in StaticStylesheetAndJavascriptTags, so we put
+          // them in the AST
           default: {
             return toCST(
               source,

packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidBooleanExpressions.ts

@@ -1,22 +1,15 @@
 import { LiquidBooleanExpression, LiquidHtmlNode, NodeTypes } from '@shopify/liquid-html-parser';
 import { Problem, SourceCodeType } from '../../..';
 import { INVALID_SYNTAX_MESSAGE } from './utils';
+import { isWithinRawTagThatDoesNotParseItsContents } from '../../utils';
 
 export function detectInvalidBooleanExpressions(
   node: LiquidBooleanExpression,
   ancestors: LiquidHtmlNode[],
 ): Problem<SourceCodeType.LiquidHtml> | undefined {
-  const condition = node.condition;
-
-  // We are okay with boolean expressions in schema tags (specifically for visible_if)
-  if (
-    ancestors.some(
-      (ancestor) => ancestor.type === NodeTypes.LiquidRawTag && ancestor.name === 'schema',
-    )
-  ) {
-    return;
-  }
+  if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return;
 
+  const condition = node.condition;
   if (condition.type !== NodeTypes.Comparison && condition.type !== NodeTypes.LogicalExpression) {
     return;
   }

packages/theme-check-common/src/checks/liquid-html-syntax-error/index.spec.ts

@@ -95,6 +95,11 @@ describe('Module: LiquidHTMLSyntaxError', () => {
       {% if some_variable %}
         Hello, world!
       {% endif %}
+      {% schema %}
+      {
+        "settings": [{ "visible_if": "{{ foo.bar == true }}" }]
+      }
+      {% endschema %}
     `;
 
     const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);

packages/theme-check-common/src/checks/liquid-html-syntax-error/index.ts

@@ -9,6 +9,7 @@ import { detectInvalidLoopArguments } from './checks/InvalidLoopArguments';
 import { detectConditionalNodeUnsupportedParenthesis } from './checks/InvalidConditionalNodeParenthesis';
 import { detectInvalidFilterName } from './checks/InvalidFilterName';
 import { detectInvalidPipeSyntax } from './checks/InvalidPipeSyntax';
+import { isWithinRawTagThatDoesNotParseItsContents } from '../utils';
 
 type LineColPosition = {
   line: number;
@@ -50,6 +51,8 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
     if (!isError(ast)) {
       return {
         async BooleanExpression(node, ancestors) {
+          if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return;
+
           const problem = detectInvalidBooleanExpressions(node, ancestors);
 
           if (!problem) {
@@ -58,7 +61,9 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
 
           context.report(problem);
         },
-        async LiquidTag(node) {
+        async LiquidTag(node, ancestors) {
+          if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return;
+
           const problems = [
             detectMultipleAssignValues(node),
             detectInvalidEchoValue(node),
@@ -87,7 +92,10 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
             pipeProblems.forEach((pipeProblem) => context.report(pipeProblem));
           }
         },
-        async LiquidBranch(node) {
+
+        async LiquidBranch(node, ancestors) {
+          if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return;
+
           const problem = detectInvalidConditionalNode(node);
 
           if (!problem) {
@@ -96,7 +104,10 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
 
           context.report(problem);
         },
-        async LiquidVariableOutput(node) {
+
+        async LiquidVariableOutput(node, ancestors) {
+          if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return;
+
           const filterProblems = await detectInvalidFilterName(node, await filtersPromise);
           if (filterProblems.length > 0) {
             filterProblems.forEach((problem) => context.report(problem));

packages/theme-check-common/src/checks/undefined-object/index.spec.ts

@@ -1,4 +1,4 @@
-import { expect, describe, it } from 'vitest';
+import { expect, describe, it, assert } from 'vitest';
 import { UndefinedObject } from './index';
 import { runLiquidCheck, highlightedOffenses } from '../../test';
 import { Offense } from '../../types';
@@ -432,4 +432,31 @@ describe('Module: UndefinedObject', () => {
     expect(offenses).toHaveLength(1);
     expect(offenses[0].message).toBe("Unknown object 'my_var' used.");
   });
+
+  it('should not report an offense when using variables inside visible_if statements in a schema tag', async () => {
+    const sourceCode = `
+    {% schema %}
+    {
+      "settings": [
+        {
+          "type": "text",
+          "label": "foo",
+          "id": "foo",
+        }
+        {
+          "type": "text",
+          "label": "bar",
+          "id": "bar",
+          "visible_if": "{{ block.settings.foo }}",
+        }
+      ]
+    }
+    {% endschema %}
+    `;
+
+    const offenses = await runLiquidCheck(UndefinedObject, sourceCode);
+
+    assert(offenses.length == 0);
+    expect(offenses).to.be.empty;
+  });
 });

packages/theme-check-common/src/checks/undefined-object/index.ts

@@ -16,6 +16,7 @@ import {
 import { LiquidCheckDefinition, Severity, SourceCodeType, ThemeDocset } from '../../types';
 import { isError, last } from '../../utils';
 import { hasLiquidDoc } from '../../liquid-doc/liquidDoc';
+import { isWithinRawTagThatDoesNotParseItsContents } from '../utils';
 
 type Scope = { start?: number; end?: number };
 
@@ -72,7 +73,9 @@ export const UndefinedObject: LiquidCheckDefinition = {
         }
       },
 
-      async LiquidTag(node) {
+      async LiquidTag(node, ancestors) {
+        if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return;
+
         if (isLiquidTagAssign(node)) {
           indexVariableScope(node.markup.name, {
             start: node.blockStartPosition.end,
@@ -134,8 +137,9 @@ export const UndefinedObject: LiquidCheckDefinition = {
       },
 
       async VariableLookup(node, ancestors) {
-        const parent = last(ancestors);
+        if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return;
 
+        const parent = last(ancestors);
         if (isLiquidTag(parent) && isLiquidTagCapture(parent)) return;
 
         variables.push(node);

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

@@ -85,7 +85,7 @@ describe('Module: UnusedAssign', () => {
   });
 
   it('should not report unused assigns for things used in a raw-like tag', async () => {
-    const tags = ['style', 'javascript', 'stylesheet'];
+    const tags = ['style'];
     for (const tag of tags) {
       const sourceCode = `
         {% assign usedVar = 1 %}
@@ -99,6 +99,21 @@ describe('Module: UnusedAssign', () => {
     }
   });
 
+  it('should report unused assigns for things used in raw code that gets stripped away (schema, etc)', async () => {
+    const tags = ['schema', 'javascript', 'stylesheet'];
+    for (const tag of tags) {
+      const sourceCode = `
+        {% assign usedVar = 1 %}
+        {% ${tag} %}
+          {{ usedVar }}
+        {% end${tag} %}
+      `;
+
+      const offenses = await runLiquidCheck(UnusedAssign, sourceCode);
+      expect(offenses).to.not.be.empty;
+    }
+  });
+
   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,6 +6,7 @@ import {
   NodeTypes,
 } from '@shopify/liquid-html-parser';
 import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types';
+import { isWithinRawTagThatDoesNotParseItsContents } from '../utils';
 
 export const UnusedAssign: LiquidCheckDefinition = {
   meta: {
@@ -34,7 +35,8 @@ export const UnusedAssign: LiquidCheckDefinition = {
     }
 
     return {
-      async LiquidTag(node) {
+      async LiquidTag(node, ancestors) {
+        if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return;
         if (isLiquidTagAssign(node)) {
           assignedVariables.set(node.markup.name, node);
         } else if (isLiquidTagCapture(node) && node.markup.name) {
@@ -43,6 +45,7 @@ export const UnusedAssign: LiquidCheckDefinition = {
       },
 
       async VariableLookup(node, ancestors) {
+        if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return;
         const parentNode = ancestors.at(-1);
         if (parentNode && isLiquidTagCapture(parentNode)) {
           return;

packages/theme-check-common/src/checks/utils.ts

@@ -103,3 +103,15 @@ export function isLoopScopedVariable(variableName: string, ancestors: LiquidHtml
 export function isLoopLiquidTag(tag: LiquidTag): tag is LiquidTagFor | LiquidTagTablerow {
   return LoopNamedTags.includes(tag.name as any);
 }
+
+const RawTagsThatDoNotParseTheirContents = ['raw', 'stylesheet', 'javascript', 'schema'];
+
+function isRawTagThatDoesNotParseItsContent(node: LiquidHtmlNode) {
+  return (
+    node.type === NodeTypes.LiquidRawTag && RawTagsThatDoNotParseTheirContents.includes(node.name)
+  );
+}
+
+export function isWithinRawTagThatDoesNotParseItsContents(ancestors: LiquidHtmlNode[]) {
+  return ancestors.some(isRawTagThatDoesNotParseItsContent);
+}