Run all Liquid checks in one walk of the AST

[clauderic]

What are you adding in this PR?

check() walked each Liquid file's AST once per check. With the recommended config, that's about 75 full walks per file (74 checks plus the disable-comment visitor). No single check was slow: the walks were the cost. This PR walks each file once, making check() 2.3–2.9× faster on Dawn and Horizon with identical offenses.

How it works

• visitLiquid(node, visit) now takes a callback instead of a check. It calls the callback with the method name for each node (type on the way down, then `${type}:exit`), at exactly the points the old per-check walk called methods.
• checkLiquidFile(checks, file) walks the file once and collects each check's calls, in walk order. Then each check makes its own calls, awaiting each one, as the old per-check loop did. Which checks handle each method is looked up once per file.

What each check sees doesn't change

• Order and sequencing: each check still gets its methods in walk order, and each one settles before that check's next one starts.
• Independence: checks don't wait for each other. A slow method, such as one doing file I/O or a remote fetch, delays only its own check, as before.
• Lifecycle: onCodePathStart runs first. If the file doesn't parse, the walk and onCodePathEnd are skipped, as before.
• Errors:
  • A check that throws stops on that file only, and its error reaches config.onError once.
  • checkLiquidFile returns one promise per check, so check()'s error handling is unchanged.
  • A check whose create() returns nothing fails on its own, with the same error as before.
• JSON files: unchanged.

The one observable difference

Offenses from different checks can come back in a different order. That order was never specified: with one pipeline per check, it depended on how many awaits each check happened to make. One test pinned it, ValidBlockContentSettingType "names the built-in parameter as the string authority…". It now sorts by check before asserting.

A bug this exposed: ValidBlockTarget and ValidSettingsKey could lose offenses

Two checks started async work they never awaited:

• validateNestedBlocks (used by ValidBlockTarget) ran Promise.all(...) twice without await.
• ValidSettingsKey called the async validateReferencedBlock from forEach.

Their offenses arrived whenever that work happened to finish, possibly after check() had returned. On main, it usually finished first because the ~75 walks kept check() busy. With a faster check(), two existing tests lost offenses.

This is fixed in its own commit, with its own changeset, and the new tests reproduce the bug on main by making the file system or a block schema answer 10 ms late. A type-aware scan of theme-check-common for un-awaited promises (expression statements whose type is a promise, or an array of them) found no others.

Findings

All numbers come from themeCheckRun in @shopify/theme-check-node (real file system, recommended config), timing only check(). They are medians of runs alternating between main and this branch, on Node 24 and Apple Silicon. Absolute times vary with machine load from session to session; the ratios held.

Whole themes

Theme	main	This PR	Speedup	Heap growth in check()
Dawn	2,725 ms	931 ms	2.9×	137 → 144 MB
Horizon	3,138 ms	1,369 ms	2.3×	239 → 244 MB

• Offenses are identical to main on both themes, compared as sorted full JSON.
• With every file system call delayed 10 ms, it's still 2.6× faster on Dawn and 1.8× on Horizon.

Slow I/O, where the first version of this PR lost

The first commit ran the methods of every check for a node together and waited for all of them before moving on. As @charlespwd pointed out, that made every check wait at every node for the slowest one. A read-only review measured it:

Case	main	First commit (lockstep)	This PR
Horizon sections/main-collection.liquid alone, 20 ms per cold file call	54 ms	161 ms	49 ms
Horizon layout/theme.liquid alone, 20 ms	227 ms	266 ms	219 ms
Dawn sections/main-product.liquid alone, 20 ms	667 ms	891 ms	576 ms
Dawn sections/main-product.liquid alone, 5 ms	270 ms	272 ms	181 ms
5 remote stylesheets and 5 remote scripts, AssetSizeCSS + AssetSizeJavaScript, server answers in 100 ms	528 ms	1,042 ms	528 ms

The single-file cases match the browser language server: it checks open documents only, through a file system that answers over the LSP connection, cached per URI. That's why the PR now collects calls and lets each check make its own.

I also compared chaining each check's calls on its own promise. It was just as fast, but used slightly more memory (Dawn: 153 MB vs 147 MB for collected calls, and 138 MB on main). The lockstep version used the least (110 MB), but it lost the I/O overlap.

How I measured

• Whole themes: a script required @shopify/theme-check-node from a main worktree and from this branch. It replaced require('@shopify/theme-check-common').check with a wrapper that records the duration and the heap growth (after a forced GC) of each call, then ran themeCheckRun(themeRoot) three times per process. For the delayed table, it wrapped each NodeFileSystem method in a setTimeout.
• Single files: each run used getThemeAndConfig, kept one file, and called check() with a ThemeLiquidDocsManager docset and a file system that caches per URI and delays the first call for each URI. The cache was fresh on every run.
• Remote assets: a local HTTP server answered each HEAD after 100 ms.

What's next? Any followup issues?

• JSON files still get one walk per check. They have fewer checks and smaller ASTs, but the same approach applies if it ever shows up in a profile.
• Un-awaited promises inside checks fail silently. A no-floating-promises lint rule would catch the next one.

What did you learn?

The checks themselves are cheap: walking the same AST ~75 times per file was most of check()'s time. And a check that doesn't await its own work only looks correct while something else keeps check() busy.

Tophatting

• pnpm vitest run packages/theme-check-common: all 119 files and 5,096 tests pass.
• New tests in src/index.spec.ts:
  • Order: each check's method order and sequencing match running it alone, pinned exactly for one file.
  • Independence: a fast check finishes a file before a slow check's first method does.
  • Errors: a throwing check, and one whose create() returns nothing, each stop on their own file only and report once (the latter with the stock error message), while another check's offenses are unaffected.
• These tests pass on main too, since they pin existing guarantees. On this branch they fail if:
  • calls aren't awaited;
  • checks share one queue;
  • the lookup guard is dropped;
  • a later lookup error replaces the first.
• New tests in valid-block-target and valid-settings-key fail on main and pass here.
• Full repo vitest run: same results as main. The suites that fail there need builds outside this change.

Before you deploy

• I included a patch bump changeset

🤖 Generated with Claude Code

[charlespwd]

Sick.