Run all Liquid checks in one walk of the AST

.changeset/await-block-validation.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Wait for nested block validation in `ValidBlockTarget` and referenced block validation in `ValidSettingsKey`. Neither was awaited, so their offenses could arrive after `check()` returned and be lost.

.changeset/single-walk-liquid-checks.md

@@ -0,0 +1,7 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Run all checks on a Liquid file in one walk of its AST, instead of one walk per check.
+
+Each check still runs its methods in the order it did before, each one settled before the next, without waiting for other checks, and a check that throws still stops on that file only. On Dawn and Horizon, `check()` returns the same offenses 2–3× faster. Offenses from different checks may come back in a different order.

packages/theme-check-common/src/checks/valid-block-content-setting-type/index.spec.ts

@@ -98,22 +98,24 @@ describe('ValidBlockContentSettingType', () => {
       ValidBlockArgumentTypes,
       ValidBlockContentSettingType,
     ]);
+    // Checks report in no particular order.
+    offenses.sort((a, b) => a.check.localeCompare(b.check));
 
     expect(offenses).toMatchObject([
       {
-        check: 'ValidBlockContentSettingType',
+        check: 'ValidBlockArgumentTypes',
         message:
-          "Schema setting 'content' has Liquid type 'number', but the built-in 'content' parameter has type 'string'.",
+          "The built-in parameter 'content' has Liquid type 'string', but LiquidDoc declares 'number'. The built-in parameter type is authoritative.",
       },
       {
-        check: 'ValidBlockArgumentTypes',
+        check: 'ValidBlockContentSettingType',
         message:
-          "The built-in parameter 'content' has Liquid type 'string', but LiquidDoc declares 'number'. The built-in parameter type is authoritative.",
+          "Schema setting 'content' has Liquid type 'number', but the built-in 'content' parameter has type 'string'.",
       },
     ]);
     expect(highlightedOffenses({ 'blocks/card.liquid': source }, offenses)).toEqual([
-      '"number"',
       '{number}',
+      '"number"',
     ]);
   });
 });

packages/theme-check-common/src/checks/valid-block-target/index.spec.ts

@@ -1,6 +1,6 @@
 import { expect, describe, it } from 'vitest';
 import { ValidBlockTarget } from './index';
-import { check, MockTheme } from '../../test';
+import { check, MockFileSystem, MockTheme } from '../../test';
 import { Preset, Setting } from '../../types/schemas';
 
 describe('Module: ValidBlockTarget', () => {
@@ -851,6 +851,10 @@ describe('Module: ValidBlockTarget', () => {
         expect(offenses[0].message).to.equal(
           'Block type "image" is not allowed in "group" blocks. Allowed types are: text.',
         );
+
+        // Also when the file system answers slowly
+        const fs = new SlowFileSystem(theme);
+        expect(await check(theme, [ValidBlockTarget], { fs })).to.have.length(1);
       });
 
       describe(`Static Blocks used in a ${path} file`, () => {
@@ -1463,3 +1467,11 @@ describe('Module: ValidBlockTarget', () => {
     });
   });
 });
+
+// Answers like MockFileSystem, a moment later.
+class SlowFileSystem extends MockFileSystem {
+  async stat(uri: string) {
+    await new Promise((resolve) => setTimeout(resolve, 10));
+    return super.stat(uri);
+  }
+}

packages/theme-check-common/src/checks/valid-settings-key/index.spec.ts

@@ -1,5 +1,7 @@
 import { expect, describe, it } from 'vitest';
+import { toSchema, toSourceCode } from '../../index';
 import { check } from '../../test';
+import { ThemeBlockSchema } from '../../types';
 import { ValidSettingsKey } from './index';
 
 describe('Module: ValidSettingsKey', () => {
@@ -225,6 +227,28 @@ describe('Module: ValidSettingsKey', () => {
             `Setting 'non-existent-setting' does not exist in 'blocks/referenced.liquid'.`,
           );
         });
+
+        it(`reports an error when ${label} block setting does not exist in a referenced file that loads slowly`, async () => {
+          const theme = {
+            ...referencedBlock,
+            'sections/example.liquid': toLiquidFile({
+              ...schemaTemplate,
+              ...blockTemplate([{ type: 'referenced', settings: { 'non-existent-setting': 'v' } }]),
+            }),
+          };
+          const uri = 'file:///blocks/referenced.liquid';
+          const source = toSourceCode(uri, theme['blocks/referenced.liquid']);
+
+          const offenses = await check(theme, [ValidSettingsKey], {
+            async getBlockSchema() {
+              await new Promise((resolve) => setTimeout(resolve, 10));
+              return toSchema('theme', uri, source, async () => true) as Promise<
+                ThemeBlockSchema | undefined
+              >;
+            },
+          });
+          expect(offenses).to.have.length(1);
+        });
       });
 
       describe('local blocks', () => {

packages/theme-check-common/src/checks/valid-settings-key/index.ts

@@ -58,20 +58,36 @@ export const ValidSettingsKey: LiquidCheckDefinition = {
           validateSettingsKey(context, offset, settingsNode, validSchema.settings);
 
           // Check if default block settings match the settings defined in the block file's schema
-          validSchema.default.blocks?.forEach((block, i) => {
-            const settingsNode = nodeAtPath(ast, ['default', 'blocks', i, 'settings']);
-
-            validateReferencedBlock(context, offset, settingsNode, rootLevelLocalBlocks, block);
-          });
+          await Promise.all(
+            (validSchema.default.blocks ?? []).map((block, i) => {
+              const settingsNode = nodeAtPath(ast, ['default', 'blocks', i, 'settings']);
+
+              return validateReferencedBlock(
+                context,
+                offset,
+                settingsNode,
+                rootLevelLocalBlocks,
+                block,
+              );
+            }),
+          );
         }
 
         // Check if preset block settings match the settings defined in the block file's schema
         for (const [_depthStr, blocks] of Object.entries(presetLevelBlocks)) {
-          blocks.forEach(({ node: blockNode, path }) => {
-            const settingsNode = nodeAtPath(ast, path.slice(0, -1).concat('settings'));
-
-            validateReferencedBlock(context, offset, settingsNode, rootLevelLocalBlocks, blockNode);
-          });
+          await Promise.all(
+            blocks.map(({ node: blockNode, path }) => {
+              const settingsNode = nodeAtPath(ast, path.slice(0, -1).concat('settings'));
+
+              return validateReferencedBlock(
+                context,
+                offset,
+                settingsNode,
+                rootLevelLocalBlocks,
+                blockNode,
+              );
+            }),
+          );
         }
       },
     };

packages/theme-check-common/src/index.spec.ts

@@ -3,8 +3,16 @@ import { MissingBlockArguments } from './checks/missing-block-arguments';
 import { UnrecognizedBlockArguments } from './checks/unrecognized-block-arguments';
 import { ValidBlockArgumentTypes } from './checks/valid-block-argument-types';
 import { check } from './index';
-import { check as runChecks } from './test';
-import { type Setting, type ThemeBlock, ThemeSchemaType, type ThemeBlockSchema } from './types';
+import { getTheme, MockFileSystem, check as runChecks } from './test';
+import {
+  type LiquidCheckDefinition,
+  type Setting,
+  Severity,
+  SourceCodeType,
+  type ThemeBlock,
+  ThemeSchemaType,
+  type ThemeBlockSchema,
+} from './types';
 
 describe('Module: Hello World', () => {
   it('should validate that we can test files', () => {
@@ -57,6 +65,127 @@ describe('check', () => {
   });
 });
 
+describe('check on Liquid files', () => {
+  const files = {
+    'snippets/a.liquid': '<p>{% if x %}{{ x }}{% endif %}</p>',
+    'snippets/b.liquid': "{% render 'a' %}{% render 'a' %}",
+  };
+
+  function run(checks: LiquidCheckDefinition[], onError?: (error: Error) => void) {
+    const config = { context: 'theme' as const, settings: {}, checks, rootUri: 'file:/', onError };
+    return check(getTheme(files), config, { fs: new MockFileSystem(files) });
+  }
+
+  // Logs each method it runs, which takes a moment when `wait` is set.
+  function logger(code: string, log: string[], wait = false) {
+    return liquidCheck(code, ({ file }) => {
+      const step = async (method: string) => {
+        log.push(`${file.uri} ${code} ${method} start`);
+        if (wait) await new Promise((resolve) => setTimeout(resolve, 1));
+        log.push(`${file.uri} ${code} ${method} end`);
+      };
+      return {
+        onCodePathStart: () => step('onCodePathStart'),
+        LiquidTag: (node) => step(node.name),
+        'LiquidTag:exit': (node) => step(`${node.name}:exit`),
+        HtmlElement: () => step('HtmlElement'),
+        onCodePathEnd: () => step('onCodePathEnd'),
+      };
+    });
+  }
+
+  const eventsOf = (log: string[], uri: string, code: string) =>
+    log.filter((event) => event.startsWith(`${uri} ${code} `));
+
+  it("runs each check's methods in the order it runs them alone, each settled before the next", async () => {
+    const [fast, slow, together]: string[][] = [[], [], []];
+    await run([logger('Fast', fast)]);
+    await run([logger('Slow', slow, true)]);
+    await run([logger('Fast', together), logger('Slow', together, true)]);
+
+    for (const uri of ['file:///snippets/a.liquid', 'file:///snippets/b.liquid']) {
+      expect(eventsOf(together, uri, 'Fast')).toEqual(eventsOf(fast, uri, 'Fast'));
+      expect(eventsOf(together, uri, 'Slow')).toEqual(eventsOf(slow, uri, 'Slow'));
+    }
+    expect(eventsOf(slow, 'file:///snippets/b.liquid', 'Slow')).toEqual(
+      [
+        'onCodePathStart start',
+        'onCodePathStart end',
+        'render start',
+        'render end',
+        'render:exit start',
+        'render:exit end',
+        'render start',
+        'render end',
+        'render:exit start',
+        'render:exit end',
+        'onCodePathEnd start',
+        'onCodePathEnd end',
+      ].map((event) => `file:///snippets/b.liquid Slow ${event}`),
+    );
+  });
+
+  it("doesn't make a check wait for a slower one", async () => {
+    const log: string[] = [];
+    await run([logger('Fast', log), logger('Slow', log, true)]);
+
+    const fastDone = log.indexOf('file:///snippets/a.liquid Fast onCodePathEnd end');
+    const slowFirstDone = log.indexOf('file:///snippets/a.liquid Slow onCodePathStart end');
+    expect(fastDone).toBeGreaterThan(-1);
+    expect(fastDone).toBeLessThan(slowFirstDone);
+  });
+
+  it('stops a check that throws on that file only, and reports its error once', async () => {
+    const calls: string[] = [];
+    const errors: Error[] = [];
+    const throws = liquidCheck('Throws', ({ file }) => ({
+      async LiquidTag() {
+        calls.push(file.uri);
+        throw new Error(`Throws failed on ${file.uri}`);
+      },
+      async onCodePathEnd() {
+        calls.push(`${file.uri} end`);
+      },
+    }));
+    const empty = liquidCheck('Empty', () => undefined as any);
+    const reports = liquidCheck('Reports', (context) => ({
+      async LiquidTag(node) {
+        context.report({
+          message: node.name,
+          startIndex: node.position.start,
+          endIndex: node.position.end,
+        });
+      },
+    }));
+
+    const offenses = await run([throws, empty, reports], (error) => errors.push(error));
+
+    expect(calls.sort()).toEqual(['file:///snippets/a.liquid', 'file:///snippets/b.liquid']);
+    expect(errors.map((error) => error.message).sort()).toEqual([
+      expect.stringContaining("(reading 'onCodePathStart')"),
+      expect.stringContaining("(reading 'onCodePathStart')"),
+      'Throws failed on file:///snippets/a.liquid',
+      'Throws failed on file:///snippets/b.liquid',
+    ]);
+    expect(offenses.map((offense) => offense.message).sort()).toEqual(['if', 'render', 'render']);
+  });
+});
+
+function liquidCheck(code: string, create: LiquidCheckDefinition['create']): LiquidCheckDefinition {
+  return {
+    meta: {
+      code,
+      name: code,
+      docs: { description: code },
+      type: SourceCodeType.LiquidHtml,
+      severity: Severity.ERROR,
+      schema: {},
+      targets: [],
+    },
+    create,
+  };
+}
+
 function setting(value: Record<string, unknown>): Setting.InputSetting {
   return value as unknown as Setting.InputSetting;
 }

packages/theme-check-common/src/index.ts

@@ -127,11 +127,12 @@ export async function check(
         const files = filesOfType(type, theme);
         const checkDefs = [DisabledChecksVisitor, ...checksOfType(type, config.checks)];
         for (const file of files) {
+          const checks: LiquidCheck[] = [];
           for (const checkDef of checkDefs) {
             if (isIgnored(file.uri, config, checkDef)) continue;
-            const check = createCheck(checkDef, file, config, offenses, dependencies, validateJSON);
-            pipelines.push(checkLiquidFile(check, file));
+            checks.push(createCheck(checkDef, file, config, offenses, dependencies, validateJSON));
           }
+          pipelines.push(...checkLiquidFile(checks, file));
         }
         break;
       }
@@ -219,9 +220,48 @@ async function checkJSONFile(check: JSONCheck, file: JSONSourceCode): Promise<vo
   if (check.onCodePathEnd) await check.onCodePathEnd(file as typeof file & { ast: JSONNode });
 }
 
-async function checkLiquidFile(check: LiquidCheck, file: LiquidSourceCode): Promise<void> {
-  if (check.onCodePathStart) await check.onCodePathStart(file);
-  if (file.ast instanceof Error) return;
-  if (Object.keys(check).length > 0) await visitLiquid(file.ast, check);
-  if (check.onCodePathEnd) await check.onCodePathEnd(file as typeof file & { ast: LiquidHtmlNode });
+/**
+ * Runs every check on a Liquid file in one walk of its AST, instead of one walk per check. The walk
+ * collects each check's calls; then each check makes its own, in walk order and each settled
+ * before the next, as when it walked the AST alone. Checks don't wait for each other.
+ *
+ * Returns one promise per check. A check that throws skips the rest of this file, and its promise
+ * rejects with the error.
+ */
+function checkLiquidFile(checks: LiquidCheck[], file: LiquidSourceCode): Promise<void>[] {
+  if (checks.length === 0) return [];
+
+  const calls: [method: keyof LiquidCheck, args: unknown[]][][] = checks.map(() => []);
+  const errors = new Map<number, unknown>();
+  const checksWithMethod = new Map<keyof LiquidCheck, number[]>();
+
+  const collect = (method: keyof LiquidCheck, ...args: unknown[]) => {
+    let found = checksWithMethod.get(method);
+    if (!found) {
+      found = [];
+      for (const [i, check] of checks.entries()) {
+        try {
+          if (check[method]) found.push(i);
+        } catch (error) {
+          // e.g. a check whose create() returned nothing
+          if (!errors.has(i)) errors.set(i, error);
+        }
+      }
+      checksWithMethod.set(method, found);
+    }
+    for (const i of found) calls[i].push([method, args]);
+  };
+
+  collect('onCodePathStart', file);
+  if (!(file.ast instanceof Error)) {
+    visitLiquid(file.ast, collect);
+    collect('onCodePathEnd', file);
+  }
+
+  return checks.map(async (check, i) => {
+    if (errors.has(i)) throw errors.get(i);
+    for (const [method, args] of calls[i]) {
+      await (check as Record<string, (...args: unknown[]) => Promise<void>>)[method](...args);
+    }
+  });
 }